From: Kory Maincent <[email protected]>
When writing the linked list elements and pointer the control need to be
written at the end. If the control is written and the SAR and DAR not
stored we could face a race condition.
Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
Signed-off-by: Kory Maincent <[email protected]>
---
This patch is fixing a commit which is only in dmaengine tree and not
merged mainline.
---
drivers/dma/dw-edma/dw-hdma-v0-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
index 0b77ddbe91b5..f28e1671a753 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
@@ -162,10 +162,10 @@ static void dw_hdma_v0_write_ll_data(struct dw_edma_chunk *chunk, int i,
} else {
struct dw_hdma_v0_lli __iomem *lli = chunk->ll_region.vaddr.io + ofs;
- writel(control, &lli->control);
writel(size, &lli->transfer_size);
writeq(sar, &lli->sar.reg);
writeq(dar, &lli->dar.reg);
+ writel(control, &lli->control);
}
}
@@ -182,8 +182,8 @@ static void dw_hdma_v0_write_ll_link(struct dw_edma_chunk *chunk,
} else {
struct dw_hdma_v0_llp __iomem *llp = chunk->ll_region.vaddr.io + ofs;
- writel(control, &llp->control);
writeq(pointer, &llp->llp.reg);
+ writel(control, &llp->control);
}
}
--
2.25.1
On Fri, Jun 09, 2023 at 10:16:50AM +0200, K?ry Maincent wrote:
> From: Kory Maincent <[email protected]>
>
> When writing the linked list elements and pointer the control need to be
> written at the end. If the control is written and the SAR and DAR not
> stored we could face a race condition.
>
> Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> Signed-off-by: Kory Maincent <[email protected]>
Once again. Is this a hypothetical bug or have you actually
experienced the denoted problem? If you do please describe the
circumstances, give more details. Otherwise it sounds weird. Here is
why.
DW eDMA HW manual states that the control LL DWORD is indeed supposed
to be written after the rest of the descriptor DWORDs are written. But
AFAICS it's only relevant for the LL tree entries recycling. Current
DW eDMA driver design doesn't truly implement that pattern. Instead
the DMA transfer is halted at the end of the chunk. Then the engine is
recharged with the next chunk and execution is started over. So the
runtime recycling isn't implemented (alas) for which the CB flag of
the control DWORD needs to be updated only after the rest of the LLI
fields.
If you described a hypothetical problem then it would be ok to accept
the change for the sake of consistency but I would have dropped the
Fixes tag and updated the patch description with more details of the
race condition you are talking about.
-Serge(y)
> ---
>
> This patch is fixing a commit which is only in dmaengine tree and not
> merged mainline.
> ---
> drivers/dma/dw-edma/dw-hdma-v0-core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 0b77ddbe91b5..f28e1671a753 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -162,10 +162,10 @@ static void dw_hdma_v0_write_ll_data(struct dw_edma_chunk *chunk, int i,
> } else {
> struct dw_hdma_v0_lli __iomem *lli = chunk->ll_region.vaddr.io + ofs;
>
> - writel(control, &lli->control);
> writel(size, &lli->transfer_size);
> writeq(sar, &lli->sar.reg);
> writeq(dar, &lli->dar.reg);
> + writel(control, &lli->control);
> }
> }
>
> @@ -182,8 +182,8 @@ static void dw_hdma_v0_write_ll_link(struct dw_edma_chunk *chunk,
> } else {
> struct dw_hdma_v0_llp __iomem *llp = chunk->ll_region.vaddr.io + ofs;
>
> - writel(control, &llp->control);
> writeq(pointer, &llp->llp.reg);
> + writel(control, &llp->control);
> }
> }
>
> --
> 2.25.1
>
On Mon, 19 Jun 2023 20:15:50 +0300
Serge Semin <[email protected]> wrote:
> On Fri, Jun 09, 2023 at 10:16:50AM +0200, Köry Maincent wrote:
> > From: Kory Maincent <[email protected]>
> >
> > When writing the linked list elements and pointer the control need to be
> > written at the end. If the control is written and the SAR and DAR not
> > stored we could face a race condition.
> >
> > Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> > Signed-off-by: Kory Maincent <[email protected]>
>
> Once again. Is this a hypothetical bug or have you actually
> experienced the denoted problem? If you do please describe the
> circumstances, give more details. Otherwise it sounds weird. Here is
> why.
>
> DW eDMA HW manual states that the control LL DWORD is indeed supposed
> to be written after the rest of the descriptor DWORDs are written. But
> AFAICS it's only relevant for the LL tree entries recycling. Current
> DW eDMA driver design doesn't truly implement that pattern. Instead
> the DMA transfer is halted at the end of the chunk. Then the engine is
> recharged with the next chunk and execution is started over. So the
> runtime recycling isn't implemented (alas) for which the CB flag of
> the control DWORD needs to be updated only after the rest of the LLI
> fields.
This one is only hypothetical. It appears to me that writing the control
after the configuration of sar and dar is more relevant to prevent race issues
and should be the usual coding choice. Also you are right saying that it will
be relevant only for the LL tree entries recycling.
Simple question from non DMA expert: isn't cyclic DMA mode recycle the LL tree
entries?
>
> If you described a hypothetical problem then it would be ok to accept
> the change for the sake of consistency but I would have dropped the
> Fixes tag and updated the patch description with more details of the
> race condition you are talking about.
Alright, I will do that.
Köry
On Mon, Jun 19, 2023 at 08:41:05PM +0200, K?ry Maincent wrote:
> On Mon, 19 Jun 2023 20:15:50 +0300
> Serge Semin <[email protected]> wrote:
>
> > On Fri, Jun 09, 2023 at 10:16:50AM +0200, K?ry Maincent wrote:
> > > From: Kory Maincent <[email protected]>
> > >
> > > When writing the linked list elements and pointer the control need to be
> > > written at the end. If the control is written and the SAR and DAR not
> > > stored we could face a race condition.
> > >
> > > Fixes: e74c39573d35 ("dmaengine: dw-edma: Add support for native HDMA")
> > > Signed-off-by: Kory Maincent <[email protected]>
> >
> > Once again. Is this a hypothetical bug or have you actually
> > experienced the denoted problem? If you do please describe the
> > circumstances, give more details. Otherwise it sounds weird. Here is
> > why.
> >
> > DW eDMA HW manual states that the control LL DWORD is indeed supposed
> > to be written after the rest of the descriptor DWORDs are written. But
> > AFAICS it's only relevant for the LL tree entries recycling. Current
> > DW eDMA driver design doesn't truly implement that pattern. Instead
> > the DMA transfer is halted at the end of the chunk. Then the engine is
> > recharged with the next chunk and execution is started over. So the
> > runtime recycling isn't implemented (alas) for which the CB flag of
> > the control DWORD needs to be updated only after the rest of the LLI
> > fields.
>
> This one is only hypothetical. It appears to me that writing the control
> after the configuration of sar and dar is more relevant to prevent race issues
> and should be the usual coding choice. Also you are right saying that it will
> be relevant only for the LL tree entries recycling.
> Simple question from non DMA expert: isn't cyclic DMA mode recycle the LL tree
> entries?
Ideally the driver should have been designed in the way you say:
define a ring of the Linked List entries and recycle the already used
entries while the already enabled entries are being handled by the
DMA-engine (a similar approach is described in the DW PCIe/eDMA hw
manual). DW eDMA engine CSRs and LLI descriptor provide enough
functionality for that. Alas the driver implementation is more
straightforward:
1. Each DMA-engine config: SG-list, cyclic, interleaved is split up
into the "burst" entries. SG-list entries are directly mapped to the
eDMA "burst" entries. Cyclic iterations are unrolled into the
respective number of eDMA "burst" entries. A similar story with the
interleaved transactions.
2. If there is no enough entries in the Linked-List memory to fully
execute the requested DMA-transfers, then another so called DW eDMA
"chunk" is allocated.
3. DW eDMA engine executes the "chunks" one after another stopping at
the end of each one and recharging the engine with the next "chunk" until
the last one is finished.
It isn't the most effective architecture, but that's how it was
originally developed by Gustavo. Anyway discussing it is a good food
for thoughts for the driver refactoring though.)
-Serge(y)
>
> >
> > If you described a hypothetical problem then it would be ok to accept
> > the change for the sake of consistency but I would have dropped the
> > Fixes tag and updated the patch description with more details of the
> > race condition you are talking about.
>
> Alright, I will do that.
>
> K?ry
On Tue, 20 Jun 2023 15:07:37 +0300
Serge Semin <[email protected]> wrote:
> > This one is only hypothetical. It appears to me that writing the control
> > after the configuration of sar and dar is more relevant to prevent race
> > issues and should be the usual coding choice. Also you are right saying
> > that it will be relevant only for the LL tree entries recycling.
> > Simple question from non DMA expert: isn't cyclic DMA mode recycle the LL
> > tree entries?
>
> Ideally the driver should have been designed in the way you say:
> define a ring of the Linked List entries and recycle the already used
> entries while the already enabled entries are being handled by the
> DMA-engine (a similar approach is described in the DW PCIe/eDMA hw
> manual). DW eDMA engine CSRs and LLI descriptor provide enough
> functionality for that. Alas the driver implementation is more
> straightforward:
> 1. Each DMA-engine config: SG-list, cyclic, interleaved is split up
> into the "burst" entries. SG-list entries are directly mapped to the
> eDMA "burst" entries. Cyclic iterations are unrolled into the
> respective number of eDMA "burst" entries. A similar story with the
> interleaved transactions.
> 2. If there is no enough entries in the Linked-List memory to fully
> execute the requested DMA-transfers, then another so called DW eDMA
> "chunk" is allocated.
> 3. DW eDMA engine executes the "chunks" one after another stopping at
> the end of each one and recharging the engine with the next "chunk" until
> the last one is finished.
>
> It isn't the most effective architecture, but that's how it was
> originally developed by Gustavo. Anyway discussing it is a good food
> for thoughts for the driver refactoring though.)
thanks for enlightening me, then indeed we will never face the issue solved by
this patches as we won't recycle LL tree entries.