REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"

[Marcin Wojtas]

wt., 4 paź 2022 o 09:25 Russell King (Oracle) <linux at armlinux.org.uk>
napisał(a):
>
> On Mon, Oct 03, 2022 at 11:30:31PM +0200, Marcin Wojtas wrote:
> > I think the DMA_FROM_DEVICE is used rather properly in the RX path of
> > the driver - the CPU doesn't access the payload afterward.
>
> Please look at the bigger picture rather than concentrating on what
> happens when a packet is received.
>
> The issue is that the CPU writes to the buffer prior to handing the
> buffer over to the hardware, and then the BM reads from the buffer.
> This is DMA _to_ the device no matter how you look at it.
>
> The BM later writes the received packet to the buffer. This is DMA
> _from_ the device.
>
> So, we have DMA in both directions, hence it really is bidirectional,
> and using DMA_FROM_DEVICE in the RX path _is_ incorrect.
>
> Architectures _can_ and _do_ invalidate the data cache when mapping a
> buffer for DMA_FROM_DEVICE and any writes in the data cache for that
> buffer will be discarded. If the writes don't hit the data cache, then
> they will be unaffected by this.
>
> IMHO, having read the docs on the buffer manager, the use of
> DMA_FROM_DEVICE in mvneta in this path is a long-standing bug - it
> should be bidirectional for the reason I state above - the hardware
> both reads and writes the buffer that is passed to it, expecting to
> read data written by the CPU initially.

Thanks for the explanation and I agree with your reasoning. Therefore
the below should be sufficient if we use HW BM and non-coherent
setting:

--- a/drivers/net/ethernet/marvell/mvneta_bm.c
+++ b/drivers/net/ethernet/marvell/mvneta_bm.c
@@ -103,7 +103,7 @@ int mvneta_bm_construct(struct hwbm_pool
*hwbm_pool, void *buf)
         */
        *(u32 *)buf = (u32)buf;
        phys_addr = dma_map_single(&priv->pdev->dev, buf, bm_pool->buf_size,
-                                  DMA_FROM_DEVICE);
+                                  DMA_BIDIRECTIONAL);
        if (unlikely(dma_mapping_error(&priv->pdev->dev, phys_addr)))
                return -ENOMEM;

Marek - can you please confirm that?

>
> > Another thought - when writing to *buf (memory normal) shouldn't we add a dsb()?
>
> That will make no difference to this - this is not about barriers, it's
> about caches.
>

Sure.


> > I have one overall concern here. On all kinds of A38x-based boards I
> > worked on, by default, the firmware set all devices (e.g. network,
> > AHCI, XHCI) on MBUS as fully IO cache coherent - it should be
> > reflected in the MVNETA_WIN_BASE(w) registers attribute field. Bits
> > [15:8] should be set to 0x1D (or 0x1E if there is a second DRAM CS
> > used). Can you please try adding 'dma-coherent;' property under the
> > 'internal-regs' node?
>
> I did notice that there was no dma-coherent markings in DT, which means
> that the DMA API will assume the device is non-coherent, and cache
> maintenance will be performed. If it is dma-coherent, then cache
> maintenance won't be performed, and DT needs to be updated to indicate
> this.
>
> If firmware is making the devices DMA coherent, and it's under firmware
> control, then shouldn't firmware also be updating the kernel's device
> tree to indicate how it's configured the hardware coherency?
>

Imo there are too many boxes out there and updating firmware in the
field is rather no-go. We already handle this in kernel / DT in big
extent, so I think we should stick to that path.

Thanks,
Marcin