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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <DCPSFZLWJLG7.1B4NISSDKLWBQ@bootlin.com>
Date: Thu, 11 Sep 2025 09:21:34 +0200
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: Théo Lebrun <theo.lebrun@...tlin.com>, "Nicolas Ferre"
 <nicolas.ferre@...rochip.com>, "Jakub Kicinski" <kuba@...nel.org>,
 "Stanimir Varbanov" <svarbanov@...e.de>
Cc: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
 <linux-rpi-kernel@...ts.infradead.org>, "Broadcom internal kernel review
 list" <bcm-kernel-feedback-list@...adcom.com>, "Andrew Lunn"
 <andrew+netdev@...n.ch>, "David S . Miller" <davem@...emloft.net>, "Eric
 Dumazet" <edumazet@...gle.com>, "Paolo Abeni" <pabeni@...hat.com>, "Rob
 Herring" <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>,
 "Conor Dooley" <conor+dt@...nel.org>, "Florian Fainelli"
 <florian.fainelli@...adcom.com>, "Andrea della Porta"
 <andrea.porta@...e.com>, "Claudiu Beznea" <claudiu.beznea@...on.dev>, "Phil
 Elwell" <phil@...pberrypi.com>, "Jonathan Bell" <jonathan@...pberrypi.com>,
 "Dave Stevenson" <dave.stevenson@...pberrypi.com>,
 <stable@...r.kernel.org>, "Andrew Lunn" <andrew@...n.ch>
Subject: Re: [PATCH v2 1/5] net: cadence: macb: Set upper 32bits of DMA ring
 buffer

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@...r.kernel.org # v4.6+
>>>> Fixes: 9ba723b081a2 ("net: macb: remove BUG_ON() and reset the queue to handle RX errors")
>>>> Credits-to: Phil Elwell <phil@...pberrypi.com>
>>>> Credits-to: Jonathan Bell <jonathan@...pberrypi.com>
>>>> Signed-off-by: Stanimir Varbanov <svarbanov@...e.de>
>>>> Reviewed-by: Andrew Lunn <andrew@...n.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