Richard Tresidder
On 26/05/2023 6:16 pm, Ilpo Järvinen wrote:
On Fri, 26 May 2023, Richard Tresidder wrote:
On 25/05/2023 5:38 pm, Ilpo Järvinen wrote:
On Thu, 25 May 2023, Richard Tresidder wrote:
On 25/05/2023 4:43 pm, Ilpo Järvinen wrote:
On Thu, 25 May 2023, Richard Tresidder wrote:
On 24/05/2023 6:06 pm, Ilpo Järvinen wrote:
On Wed, 24 May 2023, Richard Tresidder wrote:
So I just started reverting the patches that had been applied to
the
8250
folder.
Worked backwards from head.
After reverting 57e9af7831dcf211c5c689c2a6f209f4abdf0bce
serial: 8250_dma: Fix DMA Rx rearm race
Things started to work again.
I reset everything and then just reverted that individual patch
and
things
work.
So that looks like the culprit..
Okay, that also worked great, thanks a lot. It gives something
concrete
to
work with to find a fix.
The commit itself looks very much correct, however, my guess is that
when
serial8250_rx_dma_flush() does dmaengine_pause() dma_status somehow
does not become DMA_PAUSED which leads to not properly flushing DMA
Rx
transaction. Can you try the following debug patch (if my guess is
correct, besides triggering the WARN_ON_ONCE, it also works around
the
issue):
[PATCH] DEBUG: 8250_dma: Detect DMA_PAUSED vs DMA_IN_PROGRESS
inconsistency
Detect DMA state not returning DMA_PAUSED after dmaengine_pause()
was
issued.
Not intended for upstream.
Thanks Ilpo
I can confirm that the patch below works. Got the WARN_ON_ONCE
dump so
it's
taking that path.
I think the problem here is that the pl330 DMA controller thats in
these
SOC's
doesn't "really" support pause according to the driver.
For this flush usecase, it is enough to support pause + read residue +
terminate which is supported according to the function comment for
pl330_pause().
Yep agree given the 8250 serial code immediately terminates the dma after
the
pause during the flush anyway..
It doesn't look like it can ever set "DMA_PAUSED"
Why not? It pauses the transfer and is even able to allow reading the
transferred byte count.
What it is claimed to not be able to do is resume. Note that w/o resume
serial8250_tx_dma() XON/XOFF processing will not work but that's
unrelated to this issue (I'll probably need to do another patch to fix
that on 8250 DMA side).
Yep I just meant it doesn't look capable of reporting DMA_PAUSED
Which your patch below probably fixes.
and it kind of does a stop without a resume capability so must be
terminated
after this.
Though I'm not sure of the implications of reporting paused without resume
capability.
Kind of an odd one..
dma_slave_caps has a bool for both pause and resume support (I only found
out about the second flag for resume today) so not unheard of it seems.
I've just tested the pl330 patch and it looks good.
No longer get the WARN_ON_ONCE triggered in the 8250_dma driver so it looks
like
Paused is correctly being set now and we're avoiding that path.
Thanks a lot for testing the fix and also for your invaluable help in
narrowing down the problem. It would have been very hard to find the
culprit otherwise.
Can I add your Tested-by tag before sending the official version of the
fix out?
No problems thanks for turning the patches around so quickly.
Yep you can add a Tested-by Tag
Have a great weekend!
Cheers
Richard Tresidder
Wonder how many others dma drivers are afflicted in the same manner?
A quick grep seems to infer a lot more drivers set the pause function then
have
something in them setting the status to DMA_PAUSED so probably good to keep
the
8250 fix in also for the time being.
There might be others, but then the patch which covered the issue has gone
to stables already months ago and you're the first to report it AFAIK so
perhaps not that wide-spread.
I'll think how to deal with this on 8250_dma side.
