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

[sashiko-bot@kernel.org]

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 in the main `ctucan_interrupt()` ISR loop, resulting in an unhandled interrupt storm and device shutdown.
--

--- Patch [6]: [PATCH net 06/20] can: ctucanfd: mark error-active controller status valid ---
commit 528d2823639b052dd9d6f154ca1675ba5d2b7470
Author: Avi Weiss <thnkslprpt@gmail.com>

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/20260723155543.318414-1-thnkslprpt@gmail.com

> diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/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 *ndev, u32 isr)
>  			break;
>  		case CAN_STATE_ERROR_ACTIVE:
>  			if (skb) {
> -				cf->can_id |= CAN_ERR_CNT;
> +				cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
>  				cf->data[1] = CAN_ERR_CRTL_ACTIVE;
>  				cf->data[6] = bec.txerr;
>  				cf->data[7] = 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 CAN
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 = 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 device
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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729102802.505168-1-mkl@pengutronix.de?part=6