Hi,
we've found a race condition with the PCM on the i.MX6 which results in
an -EIO for the SNDRV_PCM_IOCTL_READI_FRAMES ioctl after an -EPIPE (XRUN).
A possible reproduction may look like the following reduced call graph
during a PCM capture:
us -> ioctl(SNDRV_PCM_IOCTL_READI_FRAMES)
- wait_for_avail()
- schedule_timeout()
-> snd_pcm_update_hw_ptr0()
- snd_pcm_update_state: EPIPE (XRUN)
- sdma_disable_channel_async() # get's scheduled away due to sleep
us <- ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) returns -EPIPE
us -> ioctl(SNDRV_PCM_IOCTL_PREPARE) # as reaction to the EPIPE (XRUN)
us -> ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) # next try to capture frames
- sdma_prep_dma_cyclic()
- sdma_load_context() # not loaded as context_loaded is 1
- wait_for_avail()
- schedule_timeout()
# now the sdma_channel_terminate_work() comes back and sets
# context_loaded = false and frees in vchan_dma_desc_free_list().
us <- ioctl returns -EIO (capture write error (DMA or IRQ trouble?))
What we have found out, based on our understanding:
The dmaengine docu states that a dmaengine_terminate_async() must be
followed by a dmaengine_synchronize().
However, in the pcm_dmaengine.c, only dmaengine_terminate_async() is
called (for performance reasons and because it might be called from an
interrupt handler).
In our tests, we saw that the user-space immediately calls
ioctl(SNDRV_PCM_IOCTL_PREPARE) as a handler for the happened xrun
(previous ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) returns with -EPIPE). In
our case (imx-sdma.c), the terminate really happens asynchronously with
a worker thread which is not awaited/synchronized by the
ioctl(SNDRV_PCM_IOCTL_PREPARE) call.
Since the syscall immediately enters an atomic context
(snd_pcm_stream_lock_irq()), we are not able to flush the work of the
termination worker from within the DMA context. This leads to an
unterminated DMA getting re-initialized and then terminated.
On the i.MX6 platform the problem is (if I got it correctly) that the
sdma_channel_terminate_work() called after the -EPIPE gets scheduled
away (for the 1-2ms sleep [1]). During that time the userspace already
sends in the ioctl(SNDRV_PCM_IOCTL_PREPARE) and
ioctl(SNDRV_PCM_IOCTL_READI_FRAMES).
As none of them are anyhow synchronized to the terminate_worker the
vchan_dma_desc_free_list() [2] and "sdmac->context_loaded = false;" [3]
are executed during the wait_for_avail() [4] of the
ioctl(SNDRV_PCM_IOCTL_READI_FRAMES).
To make sure we identified the problem correctly we've tested to add a
"dmaengine_synchronize()" before the snd_pcm_prepare() in [5]. This
fixed the race condition in all our tests. (Before we were able to
reproduce it in 100% of the test runs).
Based on our understanding, there are two different points to ensure
the termination:
Either ensure that the termination is finished within the previous
SNDRV_PCM_IOCTL_READI_FRAMES call (inside the DMA context) or finishing
it in the SNDRV_PCM_IOCTL_PREPARE call (and all other applicable ioclts)
before entering the atomic context (from the PCM context).
We initially thought about implementing the first approach, basically
splitting up the dma_device terminate_all operation into a sync
(busy-wait) and a async one. This would align the operations with the
DMAengine interface and would enable a sync termination variant from
atomic contexts.
However, we saw that the dma_free_attrs() function has a WARN_ON on irqs
disabled, which would be the case for the sync variant.
Side note: We found this issue on the current v5.4.y LTS branch,
but it also affects v5.8.y.
Any feedback or pointers how we may fix the problem are warmly welcome!
If anything is unclear please just ask :-)
regards;
Richard Leitner
Benjamin Bara
[1]https://elixir.bootlin.com/linux/v5.8/source/drivers/dma/imx-sdma.c#L1066
[2]https://elixir.bootlin.com/linux/v5.8/source/drivers/dma/imx-sdma.c#L1071
[3]https://elixir.bootlin.com/linux/v5.8/source/drivers/dma/imx-sdma.c#L1072
[4]https://elixir.bootlin.com/linux/v5.8/source/sound/core/pcm_lib.c#L1825
[5]https://elixir.bootlin.com/linux/v5.8/source/sound/core/pcm_native.c#L3226
On 2020/08/13 19:23: Richard Leitner <[email protected]> wrote:
> Hi,
> we've found a race condition with the PCM on the i.MX6 which results in an
> -EIO for the SNDRV_PCM_IOCTL_READI_FRAMES ioctl after an -EPIPE (XRUN).
>
> A possible reproduction may look like the following reduced call graph during a
> PCM capture:
>
> us -> ioctl(SNDRV_PCM_IOCTL_READI_FRAMES)
> - wait_for_avail()
> - schedule_timeout()
> -> snd_pcm_update_hw_ptr0()
> - snd_pcm_update_state: EPIPE (XRUN)
> - sdma_disable_channel_async() # get's scheduled away due to sleep us
> <- ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) returns -EPIPE us ->
> ioctl(SNDRV_PCM_IOCTL_PREPARE) # as reaction to the EPIPE (XRUN) us ->
> ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) # next try to capture frames
> - sdma_prep_dma_cyclic()
> - sdma_load_context() # not loaded as context_loaded is 1
> - wait_for_avail()
> - schedule_timeout()
> # now the sdma_channel_terminate_work() comes back and sets #
> context_loaded = false and frees in vchan_dma_desc_free_list().
> us <- ioctl returns -EIO (capture write error (DMA or IRQ trouble?))
Seems the write error caused by context_loaded not set to false before
next transfer start? If yes, please have a try with the 03/04 of the below
patch set, anyway, could you post your failure log?
https://lkml.org/lkml/2020/8/11/111
>
>
> What we have found out, based on our understanding:
> The dmaengine docu states that a dmaengine_terminate_async() must be
> followed by a dmaengine_synchronize().
> However, in the pcm_dmaengine.c, only dmaengine_terminate_async() is
> called (for performance reasons and because it might be called from an
> interrupt handler).
>
> In our tests, we saw that the user-space immediately calls
> ioctl(SNDRV_PCM_IOCTL_PREPARE) as a handler for the happened xrun
> (previous ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) returns with -EPIPE). In
> our case (imx-sdma.c), the terminate really happens asynchronously with a
> worker thread which is not awaited/synchronized by the
> ioctl(SNDRV_PCM_IOCTL_PREPARE) call.
>
> Since the syscall immediately enters an atomic context
> (snd_pcm_stream_lock_irq()), we are not able to flush the work of the
> termination worker from within the DMA context. This leads to an
> unterminated DMA getting re-initialized and then terminated.
>
> On the i.MX6 platform the problem is (if I got it correctly) that the
> sdma_channel_terminate_work() called after the -EPIPE gets scheduled away
> (for the 1-2ms sleep [1]). During that time the userspace already sends in the
> ioctl(SNDRV_PCM_IOCTL_PREPARE) and
> ioctl(SNDRV_PCM_IOCTL_READI_FRAMES).
> As none of them are anyhow synchronized to the terminate_worker the
> vchan_dma_desc_free_list() [2] and "sdmac->context_loaded = false;" [3] are
> executed during the wait_for_avail() [4] of the
> ioctl(SNDRV_PCM_IOCTL_READI_FRAMES).
>
> To make sure we identified the problem correctly we've tested to add a
> "dmaengine_synchronize()" before the snd_pcm_prepare() in [5]. This fixed the
> race condition in all our tests. (Before we were able to reproduce it in 100% of
> the test runs).
>
> Based on our understanding, there are two different points to ensure the
> termination:
> Either ensure that the termination is finished within the previous
> SNDRV_PCM_IOCTL_READI_FRAMES call (inside the DMA context) or finishing
> it in the SNDRV_PCM_IOCTL_PREPARE call (and all other applicable ioclts)
> before entering the atomic context (from the PCM context).
>
> We initially thought about implementing the first approach, basically splitting
> up the dma_device terminate_all operation into a sync
> (busy-wait) and a async one. This would align the operations with the
> DMAengine interface and would enable a sync termination variant from atomic
> contexts.
> However, we saw that the dma_free_attrs() function has a WARN_ON on irqs
> disabled, which would be the case for the sync variant.
> Side note: We found this issue on the current v5.4.y LTS branch, but it also
> affects v5.8.y.
>
> Any feedback or pointers how we may fix the problem are warmly welcome!
> If anything is unclear please just ask :-)
>
> regards;
> Richard Leitner
> Benjamin Bara
>
> [1]https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.
> bootlin.com%2Flinux%2Fv5.8%2Fsource%2Fdrivers%2Fdma%2Fimx-sdma.c%23
> L1066&data=02%7C01%7Cyibin.gong%40nxp.com%7C79ad115b01ef453f7
> e7408d83f7b3c4d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637
> 329145824068928&sdata=D9F%2FRUG27xv9nv8J1KtrLtld2eaI6gsXiWIAIgk
> Avjw%3D&reserved=0
> [2]https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.
> bootlin.com%2Flinux%2Fv5.8%2Fsource%2Fdrivers%2Fdma%2Fimx-sdma.c%23
> L1071&data=02%7C01%7Cyibin.gong%40nxp.com%7C79ad115b01ef453f7
> e7408d83f7b3c4d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637
> 329145824068928&sdata=0EKDVgzOZzL7TpX4ykhqjvpz5ryUHUpWw7frRe
> cksBU%3D&reserved=0
> [3]https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.
> bootlin.com%2Flinux%2Fv5.8%2Fsource%2Fdrivers%2Fdma%2Fimx-sdma.c%23
> L1072&data=02%7C01%7Cyibin.gong%40nxp.com%7C79ad115b01ef453f7
> e7408d83f7b3c4d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637
> 329145824068928&sdata=aIhatvb1ocQqyYCVFEg71LgJlRBoVusbDFPIxnte
> PuY%3D&reserved=0
> [4]https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.
> bootlin.com%2Flinux%2Fv5.8%2Fsource%2Fsound%2Fcore%2Fpcm_lib.c%23L1
> 825&data=02%7C01%7Cyibin.gong%40nxp.com%7C79ad115b01ef453f7e
> 7408d83f7b3c4d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6373
> 29145824073919&sdata=y0Udbd%2FKGaVgqLrcp6fNOlMlFCGHCMfojkpp
> B4HzUuE%3D&reserved=0
> [5]https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.
> bootlin.com%2Flinux%2Fv5.8%2Fsource%2Fsound%2Fcore%2Fpcm_native.c%2
> 3L3226&data=02%7C01%7Cyibin.gong%40nxp.com%7C79ad115b01ef453f
> 7e7408d83f7b3c4d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
> 7329145824073919&sdata=ch3BQ5DDGU5HWXqIZSvUeFnBoRoP%2BMM
> HEpnk8mIfWj8%3D&reserved=0
On Fri, Aug 14, 2020 at 11:18:10AM +0200, Robin Gong wrote:
>
> On 2020/08/13 19:23: Richard Leitner <[email protected]> wrote:
> > Hi,
> > we've found a race condition with the PCM on the i.MX6 which results
> > in an -EIO for the SNDRV_PCM_IOCTL_READI_FRAMES ioctl after an -EPIPE (XRUN).
> >
> > A possible reproduction may look like the following reduced call graph
> > during a PCM capture:
> >
> > us -> ioctl(SNDRV_PCM_IOCTL_READI_FRAMES)
> > - wait_for_avail()
> > - schedule_timeout()
> > -> snd_pcm_update_hw_ptr0()
> > - snd_pcm_update_state: EPIPE (XRUN)
> > - sdma_disable_channel_async() # get's scheduled away due to sleep us
> > <- ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) returns -EPIPE
> > us -> ioctl(SNDRV_PCM_IOCTL_PREPARE) # as reaction to the EPIPE (XRUN)
> > us -> ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) # next try to capture frames
> > - sdma_prep_dma_cyclic()
> > - sdma_load_context() # not loaded as context_loaded is 1
> > - wait_for_avail()
> > - schedule_timeout()
> > # now the sdma_channel_terminate_work() comes back and sets #
> > context_loaded = false and frees in vchan_dma_desc_free_list().
> > us <- ioctl returns -EIO (capture write error (DMA or IRQ trouble?))
>
> Seems the write error caused by context_loaded not set to false before
> next transfer start? If yes, please have a try with the 03/04 of the below
> patch set, anyway, could you post your failure log?
> https://lkml.org/lkml/2020/8/11/111
Thanks for the pointer to those patches. I've tested them on top of the
v5.8 tag during the weekend. It seems those patches are mitigiating
the described EIO issue.
Nonetheless IMHO this patches are not fixing the root cause of the
problem (unsynchronized sdma_disable_channel_async / sdma_prep_dma_cyclic).
Do you (or somebody else) have any suggestions/comments/objections on that?
regards;rl
>
> >
> >
> > What we have found out, based on our understanding:
> > The dmaengine docu states that a dmaengine_terminate_async() must be
> > followed by a dmaengine_synchronize().
> > However, in the pcm_dmaengine.c, only dmaengine_terminate_async() is
> > called (for performance reasons and because it might be called from an
> > interrupt handler).
> >
> > In our tests, we saw that the user-space immediately calls
> > ioctl(SNDRV_PCM_IOCTL_PREPARE) as a handler for the happened xrun
> > (previous ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) returns with -EPIPE). In
> > our case (imx-sdma.c), the terminate really happens asynchronously
> > with a worker thread which is not awaited/synchronized by the
> > ioctl(SNDRV_PCM_IOCTL_PREPARE) call.
> >
> > Since the syscall immediately enters an atomic context
> > (snd_pcm_stream_lock_irq()), we are not able to flush the work of the
> > termination worker from within the DMA context. This leads to an
> > unterminated DMA getting re-initialized and then terminated.
> >
> > On the i.MX6 platform the problem is (if I got it correctly) that the
> > sdma_channel_terminate_work() called after the -EPIPE gets scheduled
> > away (for the 1-2ms sleep [1]). During that time the userspace already
> > sends in the
> > ioctl(SNDRV_PCM_IOCTL_PREPARE) and
> > ioctl(SNDRV_PCM_IOCTL_READI_FRAMES).
> > As none of them are anyhow synchronized to the terminate_worker the
> > vchan_dma_desc_free_list() [2] and "sdmac->context_loaded = false;"
> > [3] are executed during the wait_for_avail() [4] of the
> > ioctl(SNDRV_PCM_IOCTL_READI_FRAMES).
> >
> > To make sure we identified the problem correctly we've tested to add a
> > "dmaengine_synchronize()" before the snd_pcm_prepare() in [5]. This
> > fixed the race condition in all our tests. (Before we were able to
> > reproduce it in 100% of the test runs).
> >
> > Based on our understanding, there are two different points to ensure
> > the
> > termination:
> > Either ensure that the termination is finished within the previous
> > SNDRV_PCM_IOCTL_READI_FRAMES call (inside the DMA context) or
> > finishing it in the SNDRV_PCM_IOCTL_PREPARE call (and all other
> > applicable ioclts) before entering the atomic context (from the PCM context).
> >
> > We initially thought about implementing the first approach, basically
> > splitting up the dma_device terminate_all operation into a sync
> > (busy-wait) and a async one. This would align the operations with the
> > DMAengine interface and would enable a sync termination variant from
> > atomic contexts.
> > However, we saw that the dma_free_attrs() function has a WARN_ON on
> > irqs disabled, which would be the case for the sync variant.
> > Side note: We found this issue on the current v5.4.y LTS branch, but
> > it also affects v5.8.y.
> >
> > Any feedback or pointers how we may fix the problem are warmly welcome!
> > If anything is unclear please just ask :-)
> >
> > regards;
> > Richard Leitner
> > Benjamin Bara
We think this is not an i.MX6-specific problem, but a problem of the DMAengine usage from the PCM.
In case of a XRUN, the DMA channel is never closed but first a SNDRV_PCM_TRIGGER_STOP next a
SNDRV_PCM_TRIGGER_START is triggered.
The SNDRV_PCM_TRIGGER_STOP simply executes a dmaengine_terminate_async() [1]
but does not await the termination by calling dmaengine_synchronize(),
which is required as stated by the docu [2].
Anyways, we are not able to fix it in the pcm_dmaengine layer either at the end of
SNDRV_PCM_TRIGGER_STOP (called from the DMA on complete interrupt handler)
or at the beginning of SNDRV_PCM_TRIGGER_START (called from a PCM ioctl),
since the dmaengine_synchronize() requires a non-atomic context.
Based on my understanding, most of the DMA implementations don't even implement device_synchronize
and if they do, it might not really be necessary since the terminate_all operation is synchron.
With the i.MX6, it looks a bit different:
Since [4], the terminate_all operation really schedules a worker which waits the required ~1ms and
then does the context freeing.
Now, the ioctl(SNDRV_PCM_IOCTL_PREPARE) and the following ioctl(SNDRV_PCM_IOCTL_READI_FRAMES),
which are called from US to handle/recover from a XRUN, are in a race with the terminate_worker.
If the terminate_worker finishes earlier, everything is fine.
Otherwise, the sdma_prep_dma_cyclic() is called, sets up everything and
as soon as it is scheduled out to wait for data, the terminate_worker is scheduled and kills it.
In this case, we wait in [5] until the timeout is reached and return with -EIO.
Based on our understanding, there exist two different fixing approaches:
We thought that the pcm_dmaengine should handle this by either synchronizing the DMA on a trigger or
terminating it synchronously.
However, as we are in an atomic context, we either have to give up the atomic context of the PCM
to finish the termination or we have to design a synchronous terminate variant which is callable
from an atomic context.
For the first option, which is potentially more performant, we have to leave the atomic PCM context
and we are not sure if we are allowed to.
For the second option, we would have to divide the dma_device terminate_all into an atomic sync and
an async one, which would align with the dmaengine API, giving it the option to ensure termination
in an atomic context.
Based on my understanding, most of them are synchronous anyways, for the currently async ones we
would have to implement busy waits.
However, with this approach, we reach the WARN_ON [6] inside of an atomic context,
indicating we might not do the right thing.
Ad Failure Log (at bottom):
I haven't added the ioctl syscalls, but this is basically the output with additional prints to
be able to follow the code execution path.
A XRUN (buffer size is 480 but 960 available) leads to a SNDRV_PCM_TRIGGER_STOP.
This leads to terminate_async, starting the terminate_worker.
Next, the XRUN recovery triggers SNDRV_PCM_TRIGGER_START, calling sdma_prep_dma_cyclic
and then waits for the DMA in wait_for_avail().
Next we see the two freeings, first the old, then the newly added one;
so the terminate_worker is back at work.
Now the DMA is terminated, while we are still waiting on data from it.
What do you think about it? Is any of the provided solutions practicable?
If you need further information or additional logging, feel free to ask.
Best regards
Benjamin
[1] https://elixir.bootlin.com/linux/latest/source/sound/core/pcm_dmaengine.c#L209
[2] https://www.kernel.org/doc/html/latest/driver-api/dmaengine/client.html#further-apis
[3] https://elixir.bootlin.com/linux/latest/source/sound/core/pcm_dmaengine.c#L189
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b8603d2a5795c42f78998e70dc792336e0dc20c9
[5] https://elixir.bootlin.com/linux/v5.8/source/sound/core/pcm_lib.c#L1875
[6] https://elixir.bootlin.com/linux/latest/source/kernel/dma/mapping.c#L306
*Failure Log from latest 5.4 LTS kernel:*
[ 535.201598] imx-sgtl5000 sound: snd_pcm_period_elapsed()
[ 535.201610] imx-sgtl5000 sound: snd_pcm_period_elapsed: calls snd_pcm_update_hw_ptr0()
[ 535.201626] imx-sdma 20ec000.sdma: sdma_tx_status channel: 2
[ 535.201640] snd_pcm_capture_avail: hw_ptr: 960, appl_ptr: 0, avail: 960, boundary: 2013265920
[ 535.201655] imx-sgtl5000 sound: snd_dmaengine_pcm_trigger command: 0
[ 535.201664] imx-sdma 20ec000.sdma: sdma_disable_channel_async channel: 2
[ 535.201672] imx-sdma 20ec000.sdma: sdma_disable_channel channel: 2
[ 535.201752] imx-sgtl5000 sound: wait_for_avail: tout=999, state=4
[ 535.201760] imx-sdma 20ec000.sdma: sdma_channel_terminate_work channel: 2
[ 535.201877] imx-sgtl5000 sound: snd_pcm_do_reset: ioctl SNDRV_PCM_IOCTL1_RESET
[ 535.201888] imx-sgtl5000 sound: snd_pcm_lib_ioctl_reset: calls snd_pcm_update_hw_ptr()
[ 535.201912] imx-sgtl5000 sound: snd_dmaengine_pcm_trigger command: 1
[ 535.201922] imx-sdma 20ec000.sdma: sdma_prep_dma_cyclic channel: 2
[ 535.201931] imx-sdma 20ec000.sdma: sdma_config_write channel: 1
[ 535.201939] imx-sdma 20ec000.sdma: sdma_config_channel channel: 2
[ 535.201948] imx-sdma 20ec000.sdma: sdma_disable_channel channel: 2
[ 535.201959] imx-sdma 20ec000.sdma: sdma_load_context channel: 2
[ 535.201967] imx-sdma 20ec000.sdma: sdma_transfer_init channel: 2
[ 535.201983] imx-sdma 20ec000.sdma: sdma_load_context channel: 2
[ 535.201995] imx-sdma 20ec000.sdma: entry 0: count: 256 dma: 0x4a300000 intr
[ 535.202005] imx-sdma 20ec000.sdma: entry 1: count: 256 dma: 0x4a300100 intr
[ 535.202014] imx-sdma 20ec000.sdma: entry 2: count: 256 dma: 0x4a300200 intr
[ 535.202023] imx-sdma 20ec000.sdma: entry 3: count: 256 dma: 0x4a300300 intr
[ 535.202033] imx-sdma 20ec000.sdma: entry 4: count: 256 dma: 0x4a300400 intr
[ 535.202042] imx-sdma 20ec000.sdma: entry 5: count: 256 dma: 0x4a300500 intr
[ 535.202050] imx-sdma 20ec000.sdma: entry 6: count: 256 dma: 0x4a300600 intr
[ 535.202059] imx-sdma 20ec000.sdma: entry 7: count: 256 dma: 0x4a300700 intr
[ 535.202067] imx-sdma 20ec000.sdma: entry 8: count: 256 dma: 0x4a300800 intr
[ 535.202077] imx-sdma 20ec000.sdma: entry 9: count: 256 dma: 0x4a300900 intr
[ 535.202086] imx-sdma 20ec000.sdma: entry 10: count: 256 dma: 0x4a300a00 intr
[ 535.202094] imx-sdma 20ec000.sdma: entry 11: count: 256 dma: 0x4a300b00 intr
[ 535.202103] imx-sdma 20ec000.sdma: entry 12: count: 256 dma: 0x4a300c00 intr
[ 535.202111] imx-sdma 20ec000.sdma: entry 13: count: 256 dma: 0x4a300d00 intr
[ 535.202120] imx-sdma 20ec000.sdma: entry 14: count: 256 dma: 0x4a300e00 wrap intr
[ 535.202135] imx-sdma 20ec000.sdma: vchan 8aa58994: txd 0a262722[8]: submitted
[ 535.202145] imx-sdma 20ec000.sdma: sdma_issue_pending channel: 2
[ 535.202181] snd_pcm_capture_avail: hw_ptr: 0, appl_ptr: 0, avail: 0, boundary: 2013265920
[ 535.202192] snd_pcm_capture_avail: hw_ptr: 0, appl_ptr: 0, avail: 0, boundary: 2013265920
[ 535.202202] imx-sgtl5000 sound: wait_for_avail: avail=0, state=3, twake=64
[ 535.203182] imx-sdma 20ec000.sdma: txd 19499aa8: freeing
[ 535.203207] imx-sdma 20ec000.sdma: txd 0a262722: freeing
[ 545.766059] imx-sgtl5000 sound: wait_for_avail: tout=0, state=3
[ 545.766075] imx-sgtl5000 sound: capture write error (DMA or IRQ trouble?)
On 2020/08/17 15:29 Benjamin Bara - SKIDATA <[email protected]> wrote:
> We think this is not an i.MX6-specific problem, but a problem of the
> DMAengine usage from the PCM.
> In case of a XRUN, the DMA channel is never closed but first a
> SNDRV_PCM_TRIGGER_STOP next a SNDRV_PCM_TRIGGER_START is triggered.
> The SNDRV_PCM_TRIGGER_STOP simply executes a
> dmaengine_terminate_async() [1] but does not await the termination by calling
> dmaengine_synchronize(), which is required as stated by the docu [2].
> Anyways, we are not able to fix it in the pcm_dmaengine layer either at the
> end of SNDRV_PCM_TRIGGER_STOP (called from the DMA on complete
> interrupt handler) or at the beginning of SNDRV_PCM_TRIGGER_START (called
> from a PCM ioctl), since the dmaengine_synchronize() requires a non-atomic
> context.
>
> Based on my understanding, most of the DMA implementations don't even
> implement device_synchronize and if they do, it might not really be necessary
> since the terminate_all operation is synchron.
>
> With the i.MX6, it looks a bit different:
> Since [4], the terminate_all operation really schedules a worker which waits
> the required ~1ms and then does the context freeing.
> Now, the ioctl(SNDRV_PCM_IOCTL_PREPARE) and the following
> ioctl(SNDRV_PCM_IOCTL_READI_FRAMES),
> which are called from US to handle/recover from a XRUN, are in a race with the
> terminate_worker.
> If the terminate_worker finishes earlier, everything is fine.
> Otherwise, the sdma_prep_dma_cyclic() is called, sets up everything and as
> soon as it is scheduled out to wait for data, the terminate_worker is scheduled
> and kills it.
> In this case, we wait in [5] until the timeout is reached and return with -EIO.
>
> Based on our understanding, there exist two different fixing approaches:
> We thought that the pcm_dmaengine should handle this by either
> synchronizing the DMA on a trigger or terminating it synchronously.
> However, as we are in an atomic context, we either have to give up the atomic
> context of the PCM to finish the termination or we have to design a
> synchronous terminate variant which is callable from an atomic context.
>
> For the first option, which is potentially more performant, we have to leave the
> atomic PCM context and we are not sure if we are allowed to.
> For the second option, we would have to divide the dma_device terminate_all
> into an atomic sync and an async one, which would align with the dmaengine
> API, giving it the option to ensure termination in an atomic context.
> Based on my understanding, most of them are synchronous anyways, for the
> currently async ones we would have to implement busy waits.
> However, with this approach, we reach the WARN_ON [6] inside of an atomic
> context, indicating we might not do the right thing.
>
> Ad Failure Log (at bottom):
> I haven't added the ioctl syscalls, but this is basically the output with additional
> prints to be able to follow the code execution path.
> A XRUN (buffer size is 480 but 960 available) leads to a
> SNDRV_PCM_TRIGGER_STOP.
> This leads to terminate_async, starting the terminate_worker.
> Next, the XRUN recovery triggers SNDRV_PCM_TRIGGER_START, calling
> sdma_prep_dma_cyclic and then waits for the DMA in wait_for_avail().
> Next we see the two freeings, first the old, then the newly added one; so the
> terminate_worker is back at work.
> Now the DMA is terminated, while we are still waiting on data from it.
>
> What do you think about it? Is any of the provided solutions practicable?
> If you need further information or additional logging, feel free to ask.
busy_wait is not good I think, would you please have a try with the attached patch
which is based on https://lkml.org/lkml/2020/8/11/111? The basic idea is
to keep the freed descriptor into another list for freeing in later terminate_worker
instead of freeing directly all in terminate_worker by vchan_get_all_descriptors
which may break next descriptor coming soon
> -----Original Message-----
> From: Robin Gong <[email protected]>
> Sent: Montag, 17. August 2020 11:23
> busy_wait is not good I think, would you please have a try with the attached patch
> which is based on https://lkml.org/lkml/2020/8/11/111? The basic idea is
> to keep the freed descriptor into another list for freeing in later terminate_worker
> instead of freeing directly all in terminate_worker by vchan_get_all_descriptors
> which may break next descriptor coming soon
The idea sounds good, but with this attempt we are still not sure that the 1ms
(the ultimate reason why this is a problem) is awaited between DMA disabling and
re-enabling.
If we are allowed to leave the atomic PCM context on each trigger, synchronize the DMA and then
enter it back again, everything is fine.
This might be the most performant and elegant solution.
However, since we are in an atomic context for a reason, it might not be wanted by the PCM system
that the DMA termination completion of the previous context happens within the next call,
but we are not sure about that.
In this case, a busy wait is not a good solution, but a necessary one,
or at least the only valid solution we are aware of.
Anyhow, based on my understanding, either the start or the stop trigger has to wait the 1ms
(or whats left of it).
On 2020/08/17 19:38 Benjamin Bara - SKIDATA <[email protected]> wrote:
> > -----Original Message-----
> > From: Robin Gong <[email protected]>
> > Sent: Montag, 17. August 2020 11:23
> > busy_wait is not good I think, would you please have a try with the
> > attached patch which is based on
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml
> > .org%2Flkml%2F2020%2F8%2F11%2F111&data=02%7C01%7Cyibin.gong
> %40nxp.
> >
> com%7C96a66f37340648e998f108d842a2057e%7C686ea1d3bc2b4c6fa92cd99
> c5c301
> >
> 635%7C0%7C0%7C637332610926324334&sdata=vn80kNlIY%2BB9v9cOlXJ
> patNkn
> > YAMtVx6v7yhfvAE%2FRM%3D&reserved=0? The basic idea is to keep
> the
> > freed descriptor into another list for freeing in later
> > terminate_worker instead of freeing directly all in terminate_worker
> > by vchan_get_all_descriptors which may break next descriptor coming
> > soon
>
> The idea sounds good, but with this attempt we are still not sure that the 1ms
> (the ultimate reason why this is a problem) is awaited between DMA disabling
> and re-enabling.
The original 1ms delay is for ensuring sdma channel stop indeed, otherwise, sdma may
still access IPs's fifo like uart/sai... during last Water-Mark-Level size transfer. The worst
is some IP such as uart may lead to sdma hang after UCR2_RXEN/ UCR2_TXEN disabled
("Timeout waiting for CH0 ready" would be caught). So I would suggest synchronizing
dma after channel terminated. But for PCM system's limitation, seems no choice but
terminate async. If sdma could access audio fifo without hang after PCM driver terminate
dma channel and rx/tx data buffers are not illegal, maybe 1ms is not a must
because only garbage data harmless touched by sdma and ignored by PCM driver.
Current sdma driver with my patches could ensure below:
-- The last terminated transfer will be stopped before the next quick transfer start.
because load context(sdma_load_context) done by channel0 which is the
lowest priority. In other words, calling successfully dmaengine_prep_* in the
next transfer means new normal transfer without any last terminated transfer
impact.
-- No potential interrupt after terminated could be handled before next transfer
start because 'sdmac->desc' has been set NULL in sdma_terminate_all.
>
> If we are allowed to leave the atomic PCM context on each trigger, synchronize
> the DMA and then enter it back again, everything is fine.
> This might be the most performant and elegant solution.
> However, since we are in an atomic context for a reason, it might not be
> wanted by the PCM system that the DMA termination completion of the
> previous context happens within the next call, but we are not sure about that.
> In this case, a busy wait is not a good solution, but a necessary one, or at least
> the only valid solution we are aware of.
>
> Anyhow, based on my understanding, either the start or the stop trigger has to
> wait the 1ms (or whats left of it).
On 8/17/20 9:28 AM, Benjamin Bara - SKIDATA wrote:
> We think this is not an i.MX6-specific problem, but a problem of the DMAengine usage from the PCM.
> In case of a XRUN, the DMA channel is never closed but first a SNDRV_PCM_TRIGGER_STOP next a
> SNDRV_PCM_TRIGGER_START is triggered.
> The SNDRV_PCM_TRIGGER_STOP simply executes a dmaengine_terminate_async() [1]
> but does not await the termination by calling dmaengine_synchronize(),
> which is required as stated by the docu [2].
> Anyways, we are not able to fix it in the pcm_dmaengine layer either at the end of
> SNDRV_PCM_TRIGGER_STOP (called from the DMA on complete interrupt handler)
> or at the beginning of SNDRV_PCM_TRIGGER_START (called from a PCM ioctl),
> since the dmaengine_synchronize() requires a non-atomic context.
I think this might be an sdma specific problem after all.
dmaengine_terminate_async() will issue a request to stop the DMA. But it
is still safe to issue the next transfer, even without calling
dmaengine_synchronize(). The DMA should start the new transfer at its
earliest convenience in that case.
dmaegine_synchronize() is so that the consumer has a guarantee that the
DMA is finished using the resources (e.g. the memory buffers) associated
with the DMA transfer so it can safely free them.
>
> Based on my understanding, most of the DMA implementations don't even implement device_synchronize
> and if they do, it might not really be necessary since the terminate_all operation is synchron.
There are a lot of buggy DMAengine drivers :) Pretty much all of them
need device_synchronize() to get this right.
>
> With the i.MX6, it looks a bit different:
> Since [4], the terminate_all operation really schedules a worker which waits the required ~1ms and
> then does the context freeing.
> Now, the ioctl(SNDRV_PCM_IOCTL_PREPARE) and the following ioctl(SNDRV_PCM_IOCTL_READI_FRAMES),
> which are called from US to handle/recover from a XRUN, are in a race with the terminate_worker.
> If the terminate_worker finishes earlier, everything is fine.
> Otherwise, the sdma_prep_dma_cyclic() is called, sets up everything and
> as soon as it is scheduled out to wait for data, the terminate_worker is scheduled and kills it.
> In this case, we wait in [5] until the timeout is reached and return with -EIO.
>
> Based on our understanding, there exist two different fixing approaches:
> We thought that the pcm_dmaengine should handle this by either synchronizing the DMA on a trigger or
> terminating it synchronously.
> However, as we are in an atomic context, we either have to give up the atomic context of the PCM
> to finish the termination or we have to design a synchronous terminate variant which is callable
> from an atomic context.
>
> For the first option, which is potentially more performant, we have to leave the atomic PCM context
> and we are not sure if we are allowed to.
> For the second option, we would have to divide the dma_device terminate_all into an atomic sync and
> an async one, which would align with the dmaengine API, giving it the option to ensure termination
> in an atomic context.
> Based on my understanding, most of them are synchronous anyways, for the currently async ones we
> would have to implement busy waits.
> However, with this approach, we reach the WARN_ON [6] inside of an atomic context,
> indicating we might not do the right thing.
I don't know how feasible this is to implement in the SDMA dmaengine
driver. But I think what is should do is to have some flag to indicate
if a terminate is in progress. If a new transfer is issued while
terminate is in progress the transfer should go on a list. Once
terminate finishes it should check the list and start the transfer if
there are any on the list.
- Lars
On 8/19/20 1:08 PM, Lars-Peter Clausen wrote:
> On 8/17/20 9:28 AM, Benjamin Bara - SKIDATA wrote:
>> We think this is not an i.MX6-specific problem, but a problem of the
>> DMAengine usage from the PCM.
>> In case of a XRUN, the DMA channel is never closed but first a
>> SNDRV_PCM_TRIGGER_STOP next a
>> SNDRV_PCM_TRIGGER_START is triggered.
>> The SNDRV_PCM_TRIGGER_STOP simply executes a
>> dmaengine_terminate_async() [1]
>> but does not await the termination by calling dmaengine_synchronize(),
>> which is required as stated by the docu [2].
>> Anyways, we are not able to fix it in the pcm_dmaengine layer either
>> at the end of
>> SNDRV_PCM_TRIGGER_STOP (called from the DMA on complete interrupt
>> handler)
>> or at the beginning of SNDRV_PCM_TRIGGER_START (called from a PCM
>> ioctl),
>> since the dmaengine_synchronize() requires a non-atomic context.
>
> I think this might be an sdma specific problem after all.
> dmaengine_terminate_async() will issue a request to stop the DMA. But
> it is still safe to issue the next transfer, even without calling
> dmaengine_synchronize(). The DMA should start the new transfer at its
> earliest convenience in that case.
>
> dmaegine_synchronize() is so that the consumer has a guarantee that
> the DMA is finished using the resources (e.g. the memory buffers)
> associated with the DMA transfer so it can safely free them.
You can think of dmaengine_terminate_async() and
dmaengine_issue_pending() as adding operations to a command queue. The
DMA is responsible that the operations are executed in the same order
that they were added to the queue and to make sure that their execution
does not conflict.
dmaegine_synchronize() is for external consumers to wait until all
operations in the command queue have been completed.
> -----Original Message-----
> From: Lars-Peter Clausen <[email protected]>
> Sent: Mittwoch, 19. August 2020 13:08
> I think this might be an sdma specific problem after all.
> dmaengine_terminate_async() will issue a request to stop the DMA. But it
> is still safe to issue the next transfer, even without calling
> dmaengine_synchronize(). The DMA should start the new transfer at its
> earliest convenience in that case.
>
> dmaegine_synchronize() is so that the consumer has a guarantee that the
> DMA is finished using the resources (e.g. the memory buffers) associated
> with the DMA transfer so it can safely free them.
Thank you for the clarifications!
> I don't know how feasible this is to implement in the SDMA dmaengine
> driver. But I think what is should do is to have some flag to indicate
> if a terminate is in progress. If a new transfer is issued while
> terminate is in progress the transfer should go on a list. Once
> terminate finishes it should check the list and start the transfer if
> there are any on the list.
IMHO that's nearly what Robin's patches does, so this should be sufficient:
Putting the descriptors to free in an extra termination list and ensuring
that a new transfer is handled after the last termination is done.
@Robin:
Is it possible to tag the commits for the stable-tree
Cc: [email protected]?
Best regards and thank you all!
Benjamin
Richard
On Wed, Aug 19, 2020 at 01:08:29PM +0200, Lars-Peter Clausen wrote:
> > For the first option, which is potentially more performant, we have to leave the atomic PCM context
> > and we are not sure if we are allowed to.
> > For the second option, we would have to divide the dma_device terminate_all into an atomic sync and
> > an async one, which would align with the dmaengine API, giving it the option to ensure termination
> > in an atomic context.
> > Based on my understanding, most of them are synchronous anyways, for the currently async ones we
> > would have to implement busy waits.
> > However, with this approach, we reach the WARN_ON [6] inside of an atomic context,
> > indicating we might not do the right thing.
>
> I don't know how feasible this is to implement in the SDMA dmaengine driver.
> But I think what is should do is to have some flag to indicate if a
> terminate is in progress. If a new transfer is issued while terminate is in
> progress the transfer should go on a list. Once terminate finishes it should
> check the list and start the transfer if there are any on the list.
The list is already there in form of the vchan helpers the driver uses.
I think the big mistake the driver makes is to configure fields in
struct sdma_channel and also the hardware directly in
sdma_prep_memcpy(), sdma_prep_slave_sg() and sdma_prep_dma_cyclic(). All
information should be stored in the struct sdma_desc allocated in the
prep functions and only be used when it's time to fire that specific
descriptor.
More specifically sdma_config_write() may not be called from
sdma_prep_slave_sg() or sdma_prep_dma_cyclic(), but instead must be
called from sdma_start_desc(). sdma_config_ownership() also must be
called later in sdma_start_desc(). 'direction' must be a member of
struct sdma_desc, not of struct sdma_channel.
Overall this sounds like a fair amount of work to do, but should be
feasible and IMO is a step in the right direction.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 2020/08/19 22:26 Benjamin Bara - SKIDATA <[email protected]> wrote:
> > -----Original Message-----
> > From: Lars-Peter Clausen <[email protected]>
> > Sent: Mittwoch, 19. August 2020 13:08
> > I think this might be an sdma specific problem after all.
> > dmaengine_terminate_async() will issue a request to stop the DMA. But
> > it is still safe to issue the next transfer, even without calling
> > dmaengine_synchronize(). The DMA should start the new transfer at its
> > earliest convenience in that case.
> >
> > dmaegine_synchronize() is so that the consumer has a guarantee that
> > the DMA is finished using the resources (e.g. the memory buffers)
> > associated with the DMA transfer so it can safely free them.
>
> Thank you for the clarifications!
>
> > I don't know how feasible this is to implement in the SDMA dmaengine
> > driver. But I think what is should do is to have some flag to indicate
> > if a terminate is in progress. If a new transfer is issued while
> > terminate is in progress the transfer should go on a list. Once
> > terminate finishes it should check the list and start the transfer if
> > there are any on the list.
>
> IMHO that's nearly what Robin's patches does, so this should be sufficient:
> Putting the descriptors to free in an extra termination list and ensuring that a
> new transfer is handled after the last termination is done.
Hello Benjamin,
It seems Lars's list is not the extra termination list in my patch.
Instead, he means the next descriptor should be moved in another pending list
if the last channel terminated not done yet and restart to handle the pending list
after the last channel terminated done if the list is not empty.
I like the idea, but on SDMA that race condition may never happen I think, because
that once the next descriptor got during usleep_range in sdma_channel_terminate_work,
means the last channel stopped indeed. I have mentioned before:
' because load context(sdma_load_context) done by channel0 which is the
lowest priority. In other words, calling successfully dmaengine_prep_* in the
next transfer means new normal transfer without any last terminated transfer impact '
normal data transfer on dma non-channel0, such as channel1,2...etc which has higher
priority than channel0, so if channel0 get to run to load context means the 'potential transfer' on the last terminated channel have been done. So no need to move list since
it's no different with normal transfer although sdma_channel_terminate_work may get
to run later to free the last descriptor(only software impact which my patch fix).
Transfer on sdma channel will be split into many small pieces of transfer
(Water-Mark-Level size). Once dma request event acknowledged and scheduled by sdma
core, this sdma channel will start to transfer WML size of data and then schedule out to
other channel. Now the 'potential transfer' alive on terminated channel only happen in
the below two things come out in the same time:
[1].The channel is running to transfer Water-Mark-Level size (sdma side).
[2].The channel is terminated by sdma_disable_channel at the same time(arm side).
Even if it happen, the 'potential transfer' on sdma is very short, because fetching/filling
fifo in WML size(fifo size or half fifo size) is very quick. After that, this channel is
terminated at HW level. Here 1ms from design team just for big safe I think. So I don't
think that's a big deal if memory buffer is available and descriptor are taken care to
no impact on the next transfer as my patch did.
>
>
> @Robin:
> Is it possible to tag the commits for the stable-tree
> Cc: [email protected]?
Could my patch work in your side? If yes, I will add
Cc: [email protected]
>
> Best regards and thank you all!
> Benjamin
> Richard
On Thu, Aug 20, 2020 at 03:01:44PM +0000, Robin Gong wrote:
> On 2020/08/19 22:26 Benjamin Bara - SKIDATA <[email protected]> wrote:
> >
> > @Robin:
> > Is it possible to tag the commits for the stable-tree
> > Cc: [email protected]?
> Could my patch work in your side? If yes, I will add
> Cc: [email protected]
I've tested the patches 3 & 4 (removing sdmac->context_loaded) of the
series you mentioned and sent Tested-by tags for them [1,2], as they
fix the EIO problems for our use case.
So from our side they are fine for stable.
[1] https://lore.kernel.org/dmaengine/20200817053813.GA551027@pcleri/T/#u
[2] https://lore.kernel.org/dmaengine/20200817053820.GB551027@pcleri/T/#u
regards;rl
On 2020/08/21 12:34 Richard Leitner <[email protected]> wrote:
> On Thu, Aug 20, 2020 at 03:01:44PM +0000, Robin Gong wrote:
> > On 2020/08/19 22:26 Benjamin Bara - SKIDATA <[email protected]>
> wrote:
> > >
> > > @Robin:
> > > Is it possible to tag the commits for the stable-tree
> > > Cc: [email protected]?
> > Could my patch work in your side? If yes, I will add
> > Cc: [email protected]
>
> I've tested the patches 3 & 4 (removing sdmac->context_loaded) of the series
> you mentioned and sent Tested-by tags for them [1,2], as they fix the EIO
> problems for our use case.
>
> So from our side they are fine for stable.
>
Okay, I thought that's just decrease the issue in your side not totally fix, and the patch
I post in https://www.spinics.net/lists/arm-kernel/msg829972.html
could resolve the potential next descriptor wrongly freed by vchan_get_all_descriptors
in sdma_channel_terminate_work. Anyway, I'll add ' Cc: [email protected]' and
your Tested-by tags in 3&4, then resend it again, thanks.
On 2020/08/20 14:52 Sascha Hauer <[email protected]> wrote:
> On Wed, Aug 19, 2020 at 01:08:29PM +0200, Lars-Peter Clausen wrote:
> > > For the first option, which is potentially more performant, we have
> > > to leave the atomic PCM context and we are not sure if we are allowed to.
> > > For the second option, we would have to divide the dma_device
> > > terminate_all into an atomic sync and an async one, which would
> > > align with the dmaengine API, giving it the option to ensure termination in
> an atomic context.
> > > Based on my understanding, most of them are synchronous anyways, for
> > > the currently async ones we would have to implement busy waits.
> > > However, with this approach, we reach the WARN_ON [6] inside of an
> > > atomic context, indicating we might not do the right thing.
> >
> > I don't know how feasible this is to implement in the SDMA dmaengine
> driver.
> > But I think what is should do is to have some flag to indicate if a
> > terminate is in progress. If a new transfer is issued while terminate
> > is in progress the transfer should go on a list. Once terminate
> > finishes it should check the list and start the transfer if there are any on the
> list.
>
> The list is already there in form of the vchan helpers the driver uses.
Seems Lars major concern is on the race condition between next descriptor
and sdma_channel_terminate_work which free the last terminated descriptor,
not the ability of vchan to support multi descriptors. But anyway, I think we
should take care vchan_get_all_descriptors to free descriptors during terminate
phase in case it's done in worker like sdma_channel_terminate_work, since that
may free the next descriptor wrongly. That's what my patch attached in
0001-dmaengine-imx-sdma-add-terminated-list-for-freed-des.patch
https://www.spinics.net/lists/arm-kernel/msg829972.html
>
> I think the big mistake the driver makes is to configure fields in struct
> sdma_channel and also the hardware directly in sdma_prep_memcpy(),
> sdma_prep_slave_sg() and sdma_prep_dma_cyclic(). All information should be
> stored in the struct sdma_desc allocated in the prep functions and only be used
> when it's time to fire that specific descriptor.
Sorry Sascha, seems that's another topic and your intention is to make sure only
software involved in sdma_prep_* and all HW moved into one function inside
sdma_start_desc. I agree that will make code more clean but my concern is
sdma_start_desc is protect by spin_lock which should be short as possible while
some HW touch as context_load may cost some time. Anyway, that's another topic,
maybe we can refine it in the future.
>
> More specifically sdma_config_write() may not be called from
> sdma_prep_slave_sg() or sdma_prep_dma_cyclic(), but instead must be called
> from sdma_start_desc(). sdma_config_ownership() also must be called later
> in sdma_start_desc(). 'direction' must be a member of struct sdma_desc, not of
> struct sdma_channel.
>
> Overall this sounds like a fair amount of work to do, but should be feasible and
> IMO is a step in the right direction.
>
> Sascha
>
> --
On Fri, Aug 21, 2020 at 09:21:37AM +0000, Robin Gong wrote:
> On 2020/08/21 12:34 Richard Leitner <[email protected]> wrote:
> > On Thu, Aug 20, 2020 at 03:01:44PM +0000, Robin Gong wrote:
> > > On 2020/08/19 22:26 Benjamin Bara - SKIDATA <[email protected]>
> > wrote:
> > > >
> > > > @Robin:
> > > > Is it possible to tag the commits for the stable-tree
> > > > Cc: [email protected]?
> > > Could my patch work in your side? If yes, I will add
> > > Cc: [email protected]
> >
> > I've tested the patches 3 & 4 (removing sdmac->context_loaded) of the series
> > you mentioned and sent Tested-by tags for them [1,2], as they fix the EIO
> > problems for our use case.
> >
> > So from our side they are fine for stable.
> >
> Okay, I thought that's just decrease the issue in your side not totally fix, and the patch
As Benjamin mentioned the issue isn't "fixed" for us from the logical/
technical side.
Nonetheless the EIO error won't occur anymore with the patches applied.
Therefore the issue is for me "fixed from a userspace point of view", as
they don't get/see the error anymore.
> I post in https://www.spinics.net/lists/arm-kernel/msg829972.html
> could resolve the potential next descriptor wrongly freed by vchan_get_all_descriptors
> in sdma_channel_terminate_work. Anyway, I'll add ' Cc: [email protected]' and
> your Tested-by tags in 3&4, then resend it again, thanks.
Great. Thank you!
regards;rl
On Fri, Aug 21, 2020 at 09:52:00AM +0000, Robin Gong wrote:
> On 2020/08/20 14:52 Sascha Hauer <[email protected]> wrote:
> > On Wed, Aug 19, 2020 at 01:08:29PM +0200, Lars-Peter Clausen wrote:
> > > > For the first option, which is potentially more performant, we have
> > > > to leave the atomic PCM context and we are not sure if we are allowed to.
> > > > For the second option, we would have to divide the dma_device
> > > > terminate_all into an atomic sync and an async one, which would
> > > > align with the dmaengine API, giving it the option to ensure termination in
> > an atomic context.
> > > > Based on my understanding, most of them are synchronous anyways, for
> > > > the currently async ones we would have to implement busy waits.
> > > > However, with this approach, we reach the WARN_ON [6] inside of an
> > > > atomic context, indicating we might not do the right thing.
> > >
> > > I don't know how feasible this is to implement in the SDMA dmaengine
> > driver.
> > > But I think what is should do is to have some flag to indicate if a
> > > terminate is in progress. If a new transfer is issued while terminate
> > > is in progress the transfer should go on a list. Once terminate
> > > finishes it should check the list and start the transfer if there are any on the
> > list.
> >
> > The list is already there in form of the vchan helpers the driver uses.
> Seems Lars major concern is on the race condition between next descriptor
> and sdma_channel_terminate_work which free the last terminated descriptor,
> not the ability of vchan to support multi descriptors. But anyway, I think we
> should take care vchan_get_all_descriptors to free descriptors during terminate
> phase in case it's done in worker like sdma_channel_terminate_work, since that
> may free the next descriptor wrongly. That's what my patch attached in
> 0001-dmaengine-imx-sdma-add-terminated-list-for-freed-des.patch
> https://www.spinics.net/lists/arm-kernel/msg829972.html
Indeed this should solve the problem of freeing descriptors allocated
between terminate_all and a following prep_slave*.
>
> >
> > I think the big mistake the driver makes is to configure fields in struct
> > sdma_channel and also the hardware directly in sdma_prep_memcpy(),
> > sdma_prep_slave_sg() and sdma_prep_dma_cyclic(). All information should be
> > stored in the struct sdma_desc allocated in the prep functions and only be used
> > when it's time to fire that specific descriptor.
> Sorry Sascha, seems that's another topic and your intention is to make sure only
> software involved in sdma_prep_* and all HW moved into one function inside
> sdma_start_desc. I agree that will make code more clean but my concern is
> sdma_start_desc is protect by spin_lock which should be short as possible while
> some HW touch as context_load may cost some time. Anyway, that's another topic,
> maybe we can refine it in the future.
Yes, you are right. This is another topic.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |