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

[Max Staudt]

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



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.


> 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 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.


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

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.


Thanks,
Max