drivers: dma: dma_dw_common: Disable channel even if draining times out #68621
Conversation
|
If we're powering off the peripheral before suspending the DMA channel, isn't that a bug that should be fixed seemingly? The rest makes good sense. |
If the peripheral is stopped later then there is a chance of underrun/overrun (tx/rx) since the DMA is not servicing the peripheral. I would debate the existence of the drain instead ;) |
That’s a fair point! That behavior isn’t likely consistent across dmacs either I’d assume? |
drivers/dma/dma_dw_common.c
Outdated
| return -ETIMEDOUT; | ||
| } | ||
| if (!fifo_empty) | ||
| LOG_WRN("%s channel %d drain time out", dev->name, channel); |
There was a problem hiding this comment.
Either you change the format of all logs or you follow the existing convention.
I see e.g. no reason why the logs on line 577 remain the same, that should be a separate patch anyways IMHO.
There was a problem hiding this comment.
@plbossart, I have a separate patch for changing log prints, I did not want to include unrelated patches or changes in this PR.
I have changed these as these prints are for error and now warning and the original ones did not provided clue on the DMA from where the channel was used.
Like:
<inf> dma_dw_common: dw_dma_stop: dw_dma_stop: dma 0 channel drain time out
you would not have guessed which one would it be:
<inf> dma_dw_common: dw_dma_stop: dma@7c000 channel 0 drain time out
<inf> dma_dw_common: dw_dma_stop: dma@7d000 channel 0 drain time out
And for the line 577, I would convert that to LOG_INF(), add similar one to dw_dma_start().
There was a problem hiding this comment.
if there's a separate patch for logs, then let's move the format change in there. This patch should only deal with the subject, i.e. "Disable channel even if draining times out"
ujfalusi
force-pushed
the
peter/pr/dw_dma_disable_on_drain_timeout_01
branch
from
7773c26 to
e048f43
Compare
|
Changes since v1:
|
drivers/dma/dma_dw_common.c
Outdated
| LOG_ERR("%s: dma %d channel drain time out", __func__, channel); | ||
| return -ETIMEDOUT; | ||
| } | ||
| if (!fifo_empty) |
There was a problem hiding this comment.
this needs brackets in zephyr
There was a problem hiding this comment.
I always forget this, sorry.
ujfalusi
force-pushed
the
peter/pr/dw_dma_disable_on_drain_timeout_01
branch
from
e048f43 to
f364cf4
Compare
|
Changes since v2:
|
ujfalusi
force-pushed
the
peter/pr/dw_dma_disable_on_drain_timeout_01
branch
from
f364cf4 to
893ba33
Compare
If the channel suspend with draining fails on stop because of reasons outside of the scope of the DMA driver (the peripheral is powered off before trying to drain for example) we must continue and disable the channel. The channel can be released by the client despite of it remained enabled. A new DMA channel request can pick the channel (as it is released) but re-configuration is going to be skipped and the use of the channel is going to fail. Then we will see the same drain timeout on channel stop again since the channel retained the configuration which resulted the first timeout. The drain timeout was made fatal by an earlier commit which fixed the WAIT_FOR return value handling. Fixes: 6226f9e ("dma: dw: fix the return value check") Signed-off-by: Peter Ujfalusi <[email protected]>
If the channel suspend with draining fails on stop because of reasons outside of the scope of the DMA driver (the peripheral is powered off before trying to drain for example) we must continue and disable the channel.
The channel can be released by the client despite of it remained enabled. A new DMA channel request can pick the channel (as it is released) but re-configuration is going to be skipped and the use of the channel is going to fail. Then we will see the same drain timeout on channel stop again since the channel retained the configuration which resulted the first timeout.
The drain timeout was made fatal by an earlier commit which fixed the WAIT_FOR return value handling.
At the same time improve the warning and error prints for drain and disable timeout by including the DMA device name.
Fixes: 6226f9e ("dma: dw: fix the return value check")