Re: [PATCH v6 3/6] can: flexcan: split rx/tx masks per mailbox IRQ line

[Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>]

On 8/31/2026 12:29 PM, Ciprian Marian Costea wrote:
> On 8/24/2026 1:16 PM, Marc Kleine-Budde wrote:
>> On 24.08.2026 18:12:41, Bough Chen wrote:
>>> On Mon, Jul 13, 2026 at 09:08:03AM +0000, sashiko-bot@kernel.org wrote:
>>>> Thank you for your contribution! Sashiko AI review found 1 potential 
>>>> issue(s) to consider:
>>>>
>>>> Pre-existing issues:
>>>> - [High] Concurrent execution of multiple flexcan IRQ handlers 
>>>> causes lockless list corruption in the can_rx_offload queue.
>>>> -- 
>>>>
>>>> --- Patch [3]: [PATCH v6 3/6] can: flexcan: split rx/tx masks per 
>>>> mailbox IRQ line ---
>>>> commit 9f143e05fb5957d463ce1d05d4126d168798b0d8
>>>> Author: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>>>
>>>> can: flexcan: split rx/tx masks per mailbox IRQ line
>>>>
>>>> This commit updates flexcan to process split rx/tx masks based on the
>>>> mailbox IRQ line for platforms like S32G2. It introduces MB0 and MB1 
>>>> masks
>>>> and passes them to flexcan_do_mb() to only process the relevant mailbox
>>>> range for each IRQ.
>>>>
>>>>> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/ 
>>>>> can/flexcan/flexcan-core.c
>>>>> --- a/drivers/net/can/flexcan/flexcan-core.c
>>>>> +++ b/drivers/net/can/flexcan/flexcan-core.c
>>>>
>>>> [ ... ]
>>>>
>>>>> @@ -1244,8 +1255,14 @@ static irqreturn_t flexcan_irq_mb(int irq, 
>>>>> void *dev_id)
>>>>>       struct net_device *dev = dev_id;
>>>>>       struct flexcan_priv *priv = netdev_priv(dev);
>>>>>       irqreturn_t handled;
>>>>> +    u64 mb_mask = ~0ULL;
>>>>> +
>>>>> +    if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
>>>>> +        mb_mask = (irq == priv->irq_secondary_mb) ?
>>>>> +              FLEXCAN_SECONDARY_MB_IRQ_MB1_MASK :
>>>>> +              FLEXCAN_SECONDARY_MB_IRQ_MB0_MASK;
>>>>>
>>>>> -    handled = flexcan_do_mb(dev);
>>>>> +    handled = flexcan_do_mb(dev, mb_mask);
>>>>>
>>>>>       if (handled)
>>>>>           can_rx_offload_irq_finish(&priv->offload);
>>>>
>>>> [Severity: High]
>>>> This is a pre-existing issue, but does this concurrent execution risk
>>>> corrupting the lockless rx-offload queues?
>>>>
>>>> Since the primary and secondary mailbox IRQs are separate lines 
>>>> (e.g., on
>>>> S32G2), they can be dispatched concurrently on different CPUs when 
>>>> frames
>>>> arrive simultaneously for both mailbox groups.
>>>>
>>>> If both IRQs execute at the same time:
>>>>
>>>> CPU1
>>>> flexcan_irq_mb()
>>>>     flexcan_do_mb()
>>>>        can_rx_offload_irq_offload_timestamp()
>>>>           __skb_queue_add_sort(&offload->skb_irq_queue, ...)
>>>>
>>>> CPU2
>>>> flexcan_irq_mb()
>>>>     flexcan_do_mb()
>>>>        can_rx_offload_irq_offload_timestamp()
>>>>           __skb_queue_add_sort(&offload->skb_irq_queue, ...)
>>>>
>>>> Or if CPU2 reaches the finish step while CPU1 is still adding:
>>>>
>>>> CPU2
>>>> flexcan_irq_mb()
>>>>     can_rx_offload_irq_finish()
>>>>        skb_queue_splice_tail_init(&offload->skb_irq_queue, ...)
>>>>
>>>> Since __skb_queue_add_sort() and skb_queue_splice_tail_init() operate
>>>> locklessly on offload->skb_irq_queue, could this lead to corrupted 
>>>> linked
>>>> list next/prev pointers or infinite loops during list traversal?
>>>
>>> Hi Ciprian Costea,
>>>
>>> Please double check whether S32 can generate these two mb interrupt at
>>> the same time, if not, no need change, just comment this feature, if
>>> yes, then need to add lock.
>>
>> If the IRQ is delayed for some reason, mailboxes 7 (associated with
>> mb-0) and 8 (associated with mb-1) might be pending at the same time.
>>
>> I assume that both IRQs will fire. One workaround would be to only use
>> the mb-1 IRQ until the rx-offloading has been fixed.
>>
>> regards,
>> Marc
>>
> 
> Hello Haibo, Marc,
> 
> You're both right. On S32G2 the mb-0 and mb-1 lines are independent and
> can fire concurrently if one handler is delayed (Marc's MB7/MB8
> example).
> 
> For V7 I'll implement Marc's suggestion and use only the mb-1 line on
> S32G2: move the RX-offload window into the MB 8–63 group and register
> enable only the secondary MB interrupt.
> This keeps everything on a single hardirq context and avoids the race.
> 
> The underlying limitation is the lockless skb_irq_queue in the rx
> offload core. I'll address that in a separate patchset. Once it's fixed, 
> this workaround can be dropped and the second MB line re-enabled.
> 
> Best regards,
> Ciprian

On a later note, I'll send a proposal to fix the rx-offload concurency
now, to avoid having to workaround this S32N79 patchset just to remove
the temporary fix later.

Best Regards,
Ciprian