Re: [PATCH v2 4/5] dmaengine: dw-edma: HDMA: Add sync read before starting the DMA transfer in remote setup

[Serge Semin]

On Mon, Oct 02, 2023 at 03:17:48PM +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>
> ---
> 
> Changes in v2:
> - Move the sync read in a function.
> - Add commments
> ---
>  drivers/dma/dw-edma/dw-hdma-v0-core.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 0cce1880cfdc..26b5020dcc2a 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -221,6 +221,25 @@ static void dw_hdma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
>  	dw_hdma_v0_write_ll_link(chunk, i, control, chunk->ll_region.paddr);
>  }
>  
> +/**
> + * dw_hdma_v0_sync_ll_data() - sync the ll data write
> + * @chunk: dma chunk
> + *

> + * In case of remote HDMA engine setup, the DW PCIe RP/EP internals
> + * configuration registers and Application memory are normally accesse

accessed

> + * over different buses. We need to insure ll data has been written before
> + * toggling the doorbell register.

1. Please replace "We need to insure ..." with "Ensure LL-data reaches
the memory before the doorbell register is toggled by issuing the
dummy-read from the remote LL memory in a hope that the posted MRd TLP
will return only after the last MWr TLP is completed".

2. Please move this comment to being above the if-statement. The
driver doesn't use kdoc at all. Having it for just a single function
doesn't look well, especially seeing it's static and isn't take part
of the kernel API.

> + */
> +static void dw_hdma_v0_sync_ll_data(struct dw_edma_chunk *chunk)
> +{
> +	if (!(chunk->chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))

> +		/* Linux memory barriers don't cater for what's required here.
> +		 * What's required is what's here - a read of the linked
> +		 * list region.
> +		 */

1. The comment isn't that much informative. If it wasn't required then
why would have this been needed in the first place? Anyway just drop it since
you'll move the kdoc detailed comment to being above the if-statement.

2. Note the preferred style for the multi-line comments is:
   /*
    *...
    */
except for the networking subsystem. It's DMA so normal format should
be utilized. I know there are several comments in this driver which
are defined in the net-format. Just ignore them. These are legacy code
which should be eventually fixed to comply with the preferred style.

-Serge(y)

> +		readl(chunk->ll_region.vaddr.io);
> +}
> +
>  static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  {
>  	struct dw_edma_chan *chan = chunk->chan;
> @@ -251,6 +270,9 @@ static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  	/* Set consumer cycle */
>  	SET_CH_32(dw, chan->dir, chan->id, cycle_sync,
>  		  HDMA_V0_CONSUMER_CYCLE_STAT | HDMA_V0_CONSUMER_CYCLE_BIT);
> +
> +	dw_hdma_v0_sync_ll_data(chunk);
> +
>  	/* Doorbell */
>  	SET_CH_32(dw, chan->dir, chan->id, doorbell, HDMA_V0_DOORBELL_START);
>  }
> -- 
> 2.25.1
>