Re: [PATCH v2 1/5] net: cadence: macb: Set upper 32bits of DMA ring buffer

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

On Wed Sep 10, 2025 at 6:57 PM CEST, Théo Lebrun wrote:
> Hello Nicolas, Jakub, Stanimir,
>
> On Tue Aug 26, 2025 at 11:14 AM CEST, Nicolas Ferre wrote:
>> On 26/08/2025 at 01:53, Jakub Kicinski wrote:
>>> On Fri, 22 Aug 2025 12:34:36 +0300 Stanimir Varbanov wrote:
>>>> In case of rx queue reset and 64bit capable hardware, set the upper
>>>> 32bits of DMA ring buffer address.
>>>>
>>>> Cc: stable@vger.kernel.org # v4.6+
>>>> Fixes: 9ba723b081a2 ("net: macb: remove BUG_ON() and reset the queue to handle RX errors")
>>>> Credits-to: Phil Elwell <phil@raspberrypi.com>
>>>> Credits-to: Jonathan Bell <jonathan@raspberrypi.com>
>>>> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
>>>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>>> 
>>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>>>> index ce95fad8cedd..36717e7e5811 100644
>>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>>> @@ -1634,7 +1634,11 @@ static int macb_rx(struct macb_queue *queue, struct napi_struct *napi,
>>>>                macb_writel(bp, NCR, ctrl & ~MACB_BIT(RE));
>>>>
>>>>                macb_init_rx_ring(queue);
>>>> -             queue_writel(queue, RBQP, queue->rx_ring_dma);
>>>> +             queue_writel(queue, RBQP, lower_32_bits(queue->rx_ring_dma));
>>>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>>>> +             if (bp->hw_dma_cap & HW_DMA_CAP_64B)
>>>> +                     macb_writel(bp, RBQPH, upper_32_bits(queue->rx_ring_dma));
>>>> +#endif
>>>>
>>>>                macb_writel(bp, NCR, ctrl | MACB_BIT(RE));
>>>>
>>> 
>>> Looks like a subset of Théo Lebrun's work:
>>> https://lore.kernel.org/all/20250820-macb-fixes-v4-0-23c399429164@bootlin.com/
>>> let's wait for his patches to get merged instead?
>>
>> Yes, we can certainly wait. As RBOPH changes by Théo are key, they will 
>> probably remove the need for this fix altogether: but I count on you 
>> Stanimir to monitor that (as I don't have a 64 bit capable platform at 
>> hand).
>
> I when looking for where this patch came from.
> Commit in the raspberrypi downstream kernel:
> https://github.com/raspberrypi/linux/commit/e45c98decbb16e58a79c7ec6fbe4374320e814f1
>
> It is somewhat unreadable; the only part that seems related is the:
>
>> net: macb: Several patches for RP1
>> 64-bit RX fix
>
>  - Is there any MACB hardware (not GEM) that uses 64-bit DMA
>    descriptors? What platforms? RPi maybe?
>
>  - Assuming such a platform exists, the next question is why does
>    macb_rx() need to reinit RBQPH/0x04D4. It reinits RBQP/0x0018
>    because it is the buffer pointer and increments as buffers get used.
>
>    To reinit RBQPH would be for the case of the increment overflowing
>    into the upper 32-bits. Sounds like a reasonable fix (for a really
>    rare bug) if that hardware actually exists.
>
>    This wouldn't be needed on GEM because RBQPH is shared across queues.
>    So of course RBQPH would not increment with the buffer pointer.
>
> If this patch is needed (does HW exist?), then my series doesn't address
> it. I can take the patch in a potential V6 if you want. V5 got posted
> today [0].
>
> [0]: https://lore.kernel.org/lkml/20250910-macb-fixes-v5-0-f413a3601ce4@bootlin.com/

Coming back after some sleep: my series does address this.
It updates macb_alloc_consistent() so allocs look like:

   size = bp->num_queues * macb_tx_ring_size_per_queue(bp);
   tx = dma_alloc_coherent(dev, size, &tx_dma, GFP_KERNEL);
   if (!tx || upper_32_bits(tx_dma) != upper_32_bits(tx_dma + size - 1))
      goto out_err;

   // same for rx

In the MACB (!GEM) case, bp->num_queues=1 so we will check that the
start and end of the DMA descriptor ring buffer have the same upper
32-bits.

That implies macb_rx() doesn't have to reinit RBQPH/0x04D4.

Thanks,

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