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

[Ciprian Marian Costea]

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@xxxxxxxxxxx>
> >
> > 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