Re: [PATCH net 06/20] can: ctucanfd: mark error-active controller status valid

Thank you for your contribution! Sashiko AI review found 1 potential issue(=
s) to consider:

Pre-existing issues:
- [High] The `REG_INT_STAT_BEI` (Bus Error) interrupt is enabled when `CAN_=
CTRLMODE_BERR_REPORTING` is requested, but it is never checked or cleared i=
n the main `ctucan_interrupt()` ISR loop, resulting in an unhandled interru=
pt storm and device shutdown.
--

--- Patch [6]: [PATCH net 06/20] can: ctucanfd: mark error-active controlle=
r status valid ---
commit 528d2823639b052dd9d6f154ca1675ba5d2b7470
Author: Avi Weiss <[email protected]>

can: ctucanfd: mark error-active controller status valid

This commit modifies the error handling path for CAN_STATE_ERROR_ACTIVE
to correctly set the CAN_ERR_CRTL flag in the CAN ID. This ensures
consumers know the controller-status information in cf->data[1] is valid.

Link: https://patch.msgid.link/[email protected]

> diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/c=
tucanfd/ctucanfd_base.c
> index 0ea1ff28dfce8..8f8b1c097ec6d 100644
> --- a/drivers/net/can/ctucanfd/ctucanfd_base.c
> +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
> @@ -869,7 +869,7 @@ static void ctucan_err_interrupt(struct net_device *n=
dev, u32 isr)
>  			break;
>  		case CAN_STATE_ERROR_ACTIVE:
>  			if (skb) {
> -				cf->can_id |=3D CAN_ERR_CNT;
> +				cf->can_id |=3D CAN_ERR_CRTL | CAN_ERR_CNT;
>  				cf->data[1] =3D CAN_ERR_CRTL_ACTIVE;
>  				cf->data[6] =3D bec.txerr;
>  				cf->data[7] =3D bec.rxerr;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this error
handling code I noticed a potential interrupt storm in ctucan_interrupt().

When the interface is configured with CAN_CTRLMODE_BERR_REPORTING, a CAN bus
error will set the REG_INT_STAT_BEI bit. Since any external device on the C=
AN
bus can intentionally or accidentally cause a bus error, this condition is
reachable from the bus.

However, ctucan_interrupt() does not include BEI in the filter condition or
the clear mask:

drivers/net/can/ctucanfd/ctucanfd_base.c:ctucan_interrupt() {
		/* Error interrupts */
		if (FIELD_GET(REG_INT_STAT_EWLI, isr) ||
		    FIELD_GET(REG_INT_STAT_FCSI, isr) ||
		    FIELD_GET(REG_INT_STAT_ALI, isr)) {
			icr =3D isr & (REG_INT_STAT_EWLI | REG_INT_STAT_FCSI | REG_INT_STAT_ALI);

			ctucan_netdev_dbg(ndev, "some ERR interrupt: clearing 0x%08x\n", icr);
			ctucan_write32(priv, CTUCANFD_INT_STAT, icr);
			ctucan_err_interrupt(ndev, isr);
		}
}

Since BEI is never cleared from the status register, will this cause the ISR
to loop until it detects a stuck interrupt and permanently disables all dev=
ice
interrupts, leading to a denial of service?

Should REG_INT_STAT_BEI be included in both the condition and the icr mask
to ensure the interrupt is properly cleared?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260729102802.5051=
[email protected]?part=3D6