From: Kory Maincent <[email protected]>
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 <[email protected]>
---
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
+ * over different buses. We need to insure ll data has been written before
+ * toggling the doorbell register.
+ */
+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.
+ */
+ 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
On Mon, Oct 02, 2023 at 03:17:48PM +0200, K?ry Maincent wrote:
> From: Kory Maincent <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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
>
From: Kory Maincent <[email protected]>
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 <[email protected]>
---
Changes in v2:
- Move the sync read in a function.
- Add commments
---
drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++++++++++++++++
1 file changed, 17 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..9109dd6c2e76 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
@@ -221,6 +221,20 @@ 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);
}
+static void dw_hdma_v0_sync_ll_data(struct dw_edma_chunk *chunk)
+{
+ /*
+ * In case of remote HDMA engine setup, the DW PCIe RP/EP internals
+ * configuration registers and Application memory are normally accessed
+ * over different buses. 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
+ */
+ if (!(chunk->chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
+ 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 +265,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
On Tue, Oct 03, 2023 at 02:15:42PM +0200, K?ry Maincent wrote:
> From: Kory Maincent <[email protected]>
>
> 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 <[email protected]>
> ---
>
> Changes in v2:
> - Move the sync read in a function.
> - Add commments
Note you need to resubmit the entire series if any of its part has
changed. So please add these patches to your patchset (in place of the
4/5 and 5/5 patches I commented) and resend it as v3.
-Serge(y)
> ---
> drivers/dma/dw-edma/dw-hdma-v0-core.c | 17 +++++++++++++++++
> 1 file changed, 17 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..9109dd6c2e76 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -221,6 +221,20 @@ 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);
> }
>
> +static void dw_hdma_v0_sync_ll_data(struct dw_edma_chunk *chunk)
> +{
> + /*
> + * In case of remote HDMA engine setup, the DW PCIe RP/EP internals
> + * configuration registers and Application memory are normally accessed
> + * over different buses. 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
> + */
> + if (!(chunk->chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> + 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 +265,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
>
On Tue, 3 Oct 2023 18:20:23 +0300
Serge Semin <[email protected]> wrote:
> On Tue, Oct 03, 2023 at 02:15:42PM +0200, Köry Maincent wrote:
> > From: Kory Maincent <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> >
> > Changes in v2:
> > - Move the sync read in a function.
> > - Add commments
>
> Note you need to resubmit the entire series if any of its part has
> changed. So please add these patches to your patchset (in place of the
> 4/5 and 5/5 patches I commented) and resend it as v3.
Alright.
Should I wait for Cai's response for patch 1/5 before sending v3. He seems to
never having woken up in our discussions.
On Tue, Oct 03, 2023 at 05:34:32PM +0200, K?ry Maincent wrote:
> On Tue, 3 Oct 2023 18:20:23 +0300
> Serge Semin <[email protected]> wrote:
>
> > On Tue, Oct 03, 2023 at 02:15:42PM +0200, K?ry Maincent wrote:
> > > From: Kory Maincent <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > >
> > > Changes in v2:
> > > - Move the sync read in a function.
> > > - Add commments
> >
> > Note you need to resubmit the entire series if any of its part has
> > changed. So please add these patches to your patchset (in place of the
> > 4/5 and 5/5 patches I commented) and resend it as v3.
>
> Alright.
> Should I wait for Cai's response for patch 1/5 before sending v3. He seems to
> never having woken up in our discussions.
Ok. Let's wait for Cai for sometime. We are in the middle of the
dev-cycle anyway so no reason to rush.
-Serge(y)