Re: [PATCH net 1/3] net: macb: never give hardware a NULL RX buffer

[Théo Lebrun]

Hello netdev-bot+sashiko,

On Tue Sep 22, 2026 at 10:37 PM CEST,  wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 2 · Medium: 0 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] gem_init_rx_ring()
>   (drivers/net/ethernet/cadence/macb_main.c:2793) now sets…
> - [High] gem_init_rx_ring() now publishes MACB_BIT(RX_USED) on every RX
>   descriptor of a queue, resets queue->rx_tail/rx_prepared_head and then…
>
> --
>
> net: macb: never give hardware a NULL RX buffer
>
> This makes gem_init_rx_ring() set the RX_USED bit on every RX descriptor
> before calling gem_rx_refill(), so that a refill that fails part way
> through does not leave descriptors owned by hardware while their skb
> pointers are still NULL.  The change affects macb_open(), macb_resume(),
> the HRESP error recovery path and set_ringparam().
>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index b8234ac4b602..751fa9e68099 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -2785,9 +2785,14 @@ static int macb_alloc(struct macb *bp)
>>  
>>  static void gem_init_rx_ring(struct macb_queue *queue)
>>  {
>> +	unsigned int i;
>> +
>>  	queue->rx_tail = 0;
>>  	queue->rx_prepared_head = 0;
>>  
>> +	for (i = 0; i < queue->bp->rx_ring_size; i++)
>> +		macb_rx_desc(queue, i)->addr |= MACB_BIT(RX_USED);
>> +
>
> [Severity: High]
>
> Does RX_USED now carry two different meanings that the RX consume path
> cannot tell apart?
>
> gem_rx() uses RX_USED at queue->rx_tail as the only indication that
> hardware delivered a frame, and it has no bound against
> queue->rx_prepared_head:

Yes that is correct! We can fix that using our cursors
rx_tail/rx_prepared_head that tell us which descriptors were
reallocated properly.

IE in gem_rx() we stop once `queue->rx_tail == queue->rx_prepared_head`.

[...]

> [Severity: High]
>
> Is any exclusion against a running RX poll needed here now that the whole
> ring is published as RX_USED?
>
> macb_hresp_error_task() runs from system_bh_wq, masks queue interrupts,
> clears RE/TE, and then calls mog_init_rings() with no napi_disable(), no
> napi_synchronize() and no lock:

This is a known race, but we won't fix it in this series. And in
particular it is completely unrelated to this first patch.

It is semi-related to the last patch that puts in place the mechanisms
that could be reused to fix macb_hresp_error_task(). But again, not
here, not today.

[...]

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com