Re: [PATCH net] 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] When Bus Error Reporting is enabled, the Bus Error Interrupt (BEI) is unhandled and uncleared, leading to an infinite interrupt loop and device lockup.
--

--- Patch [1]: [PATCH net] can: ctucanfd: mark error-active controller status valid ---
commit 5feac9571eb44c72dffa41f1990bff2ccac15e3e
Author: Avi Weiss <[email protected]>

can: ctucanfd: mark error-active controller status valid
    
In the CAN_STATE_ERROR_ACTIVE case, cf->data[1] is set to
CAN_ERR_CRTL_ACTIVE, but cf->can_id is not set with CAN_ERR_CRTL in
that path.

> 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 could there be an issue with
handling the Bus Error Interrupt (BEI) in ctucan_interrupt() when Bus Error
Reporting is enabled?

While ctucan_err_interrupt() has code to handle BEI, it appears it might never
be called if only the BEI interrupt is asserted:

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);
        }
        ...
}

Because REG_INT_STAT_BEI is omitted from both the if-condition and the icr mask,
does this leave the BEI status unhandled and uncleared?

If a bus error occurs, would this lead to an infinite interrupt loop and
ultimately lock up the device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1