Re: [PATCH 4/9] dmaengine: dw-edma: HDMA: Add memory barrier before starting the DMA transfer in remote setup

[Köry Maincent]

On Mon, 19 Jun 2023 20:02:01 +0300
Serge Semin <fancer.lancer@xxxxxxxxx> wrote:

> On Fri, Jun 09, 2023 at 10:16:49AM +0200, Köry Maincent wrote:
> > From: Kory Maincent <kory.maincent@xxxxxxxxxxx>
> >   
> 
> > The Linked list element and pointer are not stored in the same memory as
> > the HDMA controller register. If the doorbell register is toggled before
> > the full write of the linked list a race condition error can appears.
> > In remote setup we can only use a readl to the memory to assured the full
> > write has occurred.
> > 
> > Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> > Signed-off-by: Kory Maincent <kory.maincent@xxxxxxxxxxx>  
> 
> Is this a hypothetical bug? Have you actually experienced the
> described problem? If so are you sure that it's supposed to be fixed
> as you suggest?

I do experienced this problem and this patch fixed it.

> 
> I am asking because based on the kernel doc
> (Documentation/memory-barriers.txt):
> 
> *    1. All readX() and writeX() accesses to the same peripheral are ordered
> *       with respect to each other. This ensures that MMIO register accesses
> *       by the same CPU thread to a particular device will arrive in program
> *       order.
> * ...
> * The ordering properties of __iomem pointers obtained with non-default
> * attributes (e.g. those returned by ioremap_wc()) are specific to the
> * underlying architecture and therefore the guarantees listed above cannot
> * generally be relied upon for accesses to these types of mappings.
> 
> the IOs performed by the accessors are supposed to arrive in the
> program order. Thus SET_CH_32(..., HDMA_V0_DOORBELL_START) performed
> after all the previous SET_CH_32(...) are finished looks correct with
> no need in additional barriers. The results of the later operations
> are supposed to be seen by the device (in our case it's a remote DW
> eDMA controller) before the doorbell update from scratch. From that
> perspective your problem looks as if the IO operations preceding the
> doorbell CSR update aren't finished yet. So you are sure that the LL
> memory is mapped with no additional flags like Write-Combine or some
> caching optimizations? Are you sure that the PCIe IOs are correctly
> implemented in your platform?

No, I don't know if there is extra flags or optimizations.

> 
> I do understand that the eDMA CSRs and the LL memory are mapped by
> different BARs in the remote eDMA setup. But they still belong to the
> same device. So the IO accessors semantic described in the kernel doc
> implies no need in additional barrier.

Even if they are on the same device it is two type of memory.
I am not an PCIe expert but I suppose the PCIe controller of the board is
sending to both memory and if one of them (LL memory here) is slower in the
write process then we faced this race issue. We can not find out that the write
to LL memory has finished before the CSRs even if the write command has been
sent earlier.

Köry,