Re: [net-next,v2,5/8] net: macb: add XDP support for gem

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello Paolo,

On Wed Feb 25, 2026 at 7:30 PM CET, Paolo Valerio wrote:
> On 23 Feb 2026 at 04:08:44 PM, Jakub Kicinski <kuba@kernel.org> wrote:
>> This is an AI-generated review of your patch. The human sending this
>> email has considered the AI review valid, or at least plausible.
>>
>> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
>> ---
>> net: macb: add XDP support for gem
>>
>> This patch introduces basic XDP support for the macb/gem driver with
>> XDP_PASS, XDP_DROP, and XDP_REDIRECT verdict support.
>>
>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>>> index 708107e47ae3..26b517ed251c 100644
>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
[...]
>>> @@ -2641,21 +2724,23 @@ static int macb_alloc_consistent(struct macb *bp)
>>>  	return -ENOMEM;
>>>  }
>>>
>>> -static int gem_create_page_pool(struct macb_queue *queue)
>>> +static int gem_create_page_pool(struct macb_queue *queue, int qid)
>>>  {
>>>  	struct page_pool_params pp_params = {
>>>  		.order = 0,
>>>  		.flags = PP_FLAG_DMA_MAP,
>>>  		.pool_size = queue->bp->rx_ring_size,
>>>  		.nid = NUMA_NO_NODE,
>>> -		.dma_dir = DMA_FROM_DEVICE,
>>> +		.dma_dir = rcu_access_pointer(queue->bp->prog)
>>> +				? DMA_BIDIRECTIONAL
>>> +				: DMA_FROM_DEVICE,
>>>  		.dev = &queue->bp->pdev->dev,
>>>  		.netdev = queue->bp->dev,
>>>  		.napi = &queue->napi_rx,
>>>  		.max_len = PAGE_SIZE,
>>>  	};
>>>  	struct page_pool *pool;
>>> -	int err = 0;
>>> +	int err;
>>>
>>>  	/* This can happen in the case of HRESP error.
>>>  	 * Do nothing as page pool is already existing.
>>>  	 */
>>>  	if (queue->page_pool)
>>>  		return err;
>>                        ^^^
>>
>> Does this return an uninitialized value? The patch changed 'int err = 0'
>> to 'int err' without initialization. The early-return path for HRESP error
>> recovery returns err when the page pool already exists.
>>
>
> argh, I missed this patch removing the initialization while rebasing.

I debugged a nasty bug yesterday, which ended up being this typo
combined with the fact that we call gem_init_rings() through the
mog_init_rings function pointer twice at open. Once near the end of
macb_alloc_consistent() and once from macb_open().

This bug is invisible because we supposedly skip the second
gem_create_page_pool() with the queue->page_pool check.

On my system it lead to the "DMA bus error: HRESP not OK" error;
my theory (which I didn't dig into much as the root cause is known):
 - undefined behaviour leading to the compiler skipping the
   `if (queue->page_pool)` condition in gem_create_page_pool()
 - leakage of the old page allocator and the old buffers leading to
   buffer lifetime issues somehow

I still don't understand how we all tolerate -Wno-maybe-uninitialized in
the kernel under GCC. But now that it has been here for a long time
many false positives slipped in, making it harder to re-enable.

https://elixir.bootlin.com/linux/v6.19.3/source/scripts/Makefile.warn#L184-L186

Thanks,

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