Re: [PATCH v6 2/3] can: rx-offload: add a per-IRQ receive context

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

On 9/26/2026 6:26 PM, Max Staudt wrote:
> Thank you for your patch. The general idea sounds useful, and at a quick 
> glance, the implementation seems sound. I'm not able to do a full review 
> - please wait for a maintainer's reply.
> 
> Comments below, from the point of view of a simple CAN driver's 
> maintainer (can327).
> 

Hello Max,

Thank you for taking time in reviewing this patchset.

> 
> 
> On 9/25/26 11:45 PM, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> The IRQ handler fills skb_irq_queue without a lock and the finish helpers
>> then splice it into skb_queue under skb_queue.lock. This breaks when a
>> driver uses the helpers from more than one IRQ line. On NXP S32G2, the
>> flexcan handlers can run at the same time on different CPUs and corrupt
>> skb_irq_queue.
> 
> Please add "Currently, " to the beginning of this paragraph.
> 

Will do in v7.

> 
>> Add struct can_rx_offload_irq, one per IRQ line, which holds
>> skb_irq_queue, skb_queue_len_max and the mailbox range. skb_queue and 
>> napi
>> stay in struct can_rx_offload and are shared by all contexts.
> 
> The IRQ naming gives me a bit of a headache. It may be IRQ in the case 
> of flexcan, but can be something else in other drivers. Also, _irq gives 
> the impression that the struct stores an IRQ, or at least something 
> related to it, but this really does not.
> 
> Can you please rename the new struct and concept to something else? 
> Maybe _queue? And then the old struct can be renamed to _common or 
> _shared or something. The maintainers may have better naming ideas.
> 

I agree. The name does not fit can327 or the USB drivers.
I would rather avoid *_queue, since "queue" is already used for
skb_queue, skb_irq_queue and the can_rx_offload_queue_*() helpers.
What about struct can_rx_offload_source ? The fields and comments would
follow (e.g. source_cnt).

For struct can_rx_offload I would prefer to keep the name. It is
embedded in every driver and used in the mailbox_read() callback, so
renaming it adds a lot of noise for little gain.

Marc, any preference or other view ?

> I think you can keep the name skb_irq_queue inside the struct, my 
> request is about the new struct's name itself. In the same vein, 
> expanding the new struct's explanatory comment would help - please 
> explain that *the purpose* of having this struct is so you can have one 
> per source, *such as* one for each unique IRQ that the same CAN hardware 
> may trigger.
> 

Thanks for this suggestion, I will expand the comment to add more
context regarding the purpose of the new 'can_rx_offload_irq' struct.

> 
>> @@ -351,64 +378,88 @@ 
>> EXPORT_SYMBOL_GPL(can_rx_offload_threaded_irq_finish);
>>   static int can_rx_offload_init_queue(struct net_device *dev,
>>                        struct can_rx_offload *offload,
>> +                     struct can_rx_offload_irq *offload_irq,
>>                        unsigned int weight)
>>   {
>> -    offload->dev = dev;
>> -
>> -    /* Limit queue len to 4x the weight (rounded to next power of 
>> two) */
>> -    offload->skb_queue_len_max = 2 << fls(weight);
>> -    offload->skb_queue_len_max *= 4;
>> -    skb_queue_head_init(&offload->skb_queue);
>> -    __skb_queue_head_init(&offload->skb_irq_queue);
>> +    struct can_rx_offload_irq *pos;
>> +    u32 skb_queue_len_max;
>> +
>> +    offload_irq->offload = offload;
>> +    __skb_queue_head_init(&offload_irq->skb_irq_queue);
>> +
>> +    /* The first registered IRQ initializes the shared state. */
>> +    if (!offload->irq_cnt) {
>> +        offload->dev = dev;
>> +        skb_queue_head_init(&offload->skb_queue);
>> +        INIT_LIST_HEAD(&offload->irqs);
>> +        netif_napi_add_weight(dev, &offload->napi,
>> +                      can_rx_offload_napi_poll, weight);
>> +    } else if (weight > offload->napi.weight) {
>> +        /* All contexts feed the same NAPI, keep the largest weight. */
>> +        offload->napi.weight = weight;
>> +    }
>> +    list_add_tail(&offload_irq->node, &offload->irqs);
>> +    offload->irq_cnt++;
>> -    netif_napi_add_weight(dev, &offload->napi, can_rx_offload_napi_poll,
>> -                  weight);
>> +    /* Limit queue len to 4x the weight (rounded to next power of two).
>> +     * All contexts feed the same skb_queue, so they share its limit.
>> +     */
>> +    skb_queue_len_max = 2 << fls(offload->napi.weight);
>> +    skb_queue_len_max *= 4;
>> +    list_for_each_entry(pos, &offload->irqs, node)
>> +        pos->skb_queue_len_max = skb_queue_len_max;
>>       dev_dbg(dev->dev.parent, "%s: skb_queue_len_max=%d\n",
>> -        __func__, offload->skb_queue_len_max);
>> +        __func__, skb_queue_len_max);
>>       return 0;
>>   }
> 
> This API is confusing now. It's called _init_queue(), but you've really 
> changed it into something that acts like _init_or_add_queue(). Please 
> rename this, and while at it, please add an explanatory comment for the 
> function above it, since it has grown quite complex.

Right. I am thinking about moving the shared setup done on the first
registration into a separate helper, rename the rest into
can_rx_offload_add_source() and document both.

> 
> The comment you added in can_rx_offload_del() is a positive example - 
> having more of this stuff helps! Actually, that specific comment talks 
> about "contexts" - please try to align that wording with whatever you 
> rename the _irq struct to, such as "queue", to keep the code and 
> comments consistent for the next reader.
> 

Will do, the comments will use the new name.

I will wait a bit for Marc's feedback regarding the naming before
sending v7.

> 
> Thanks,
> Max

Best Regards,
Ciprian