| Message ID | 20221108074938.48853-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1,1/2] at_hdmac: check and return DMA_PAUSED status when suitable | expand |
Hi, Andy, On 11/8/22 09:49, Andy Shevchenko wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > device_tx_status() may return DMA_PAUSED status when driver supports it. Yeah, but you haven't described the why. And that's because dma_cookie_status() currently considers just DMA_COMPLETE and DMA_IN_PROGRESS for the status of the cookie, so the controller drivers are forced to query the DMA_PAUSED state themselves. Also, I noticed that Vinod prefers that you use the full paragraph in the commit message, and not a continuation of the patch title, so you may want to reword the commit message. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/dma/at_hdmac.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c > index 8858470246e1..a9d8dd990d6e 100644 > --- a/drivers/dma/at_hdmac.c > +++ b/drivers/dma/at_hdmac.c > @@ -1669,9 +1669,19 @@ atc_tx_status(struct dma_chan *chan, > int ret; > > dma_status = dma_cookie_status(chan, cookie, txstate); > - if (dma_status == DMA_COMPLETE || !txstate) > + if (dma_status == DMA_COMPLETE) > return dma_status; > > + /* > + * There's no point in calculating the residue if there's > + * no txstate to store the value. > + */ > + if (!txstate) { > + if (test_bit(ATC_IS_PAUSED, &atchan->status)) there's a helper function that you can use instead: atc_chan_is_paused() > + return DMA_PAUSED; > + return DMA_ERROR; return dma_status please > + } > + > spin_lock_irqsave(&atchan->vc.lock, flags); > /* Get number of bytes left in the active transactions */ > ret = atc_get_residue(chan, cookie, &residue); > @@ -1684,6 +1694,9 @@ atc_tx_status(struct dma_chan *chan, > dma_set_residue(txstate, residue); > } > > + if (test_bit(ATC_IS_PAUSED, &atchan->status)) The status may change after spin_unlock_irqrestore(). Should the residue be in sync with the dma status? If yes, you should check the status while holding the lock. > + dma_status = DMA_PAUSED; > + > dev_vdbg(chan2dev(chan), "tx_status %d: cookie = %d residue = %u\n", > dma_status, cookie, residue); > > -- > 2.35.1 >
On Tue, Nov 08, 2022 at 11:50:19AM +0000, Tudor.Ambarus@microchip.com wrote: > On 11/8/22 09:49, Andy Shevchenko wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > device_tx_status() may return DMA_PAUSED status when driver supports it. > > Yeah, but you haven't described the why. And that's because dma_cookie_status() > currently considers just DMA_COMPLETE and DMA_IN_PROGRESS for the status of the > cookie, so the controller drivers are forced to query the DMA_PAUSED state > themselves. At last without this change it's inconvenient and requires a lot of additional (unneeded) code to be written by the caller. Moreover, it's racy. If you query status twice in a raw, it may be well changed (for example from PAUSED to IN_PROGRESS or COMPETE). I will add a word summarizing this. > Also, I noticed that Vinod prefers that you use the full paragraph in the commit > message, and not a continuation of the patch title, so you may want to reword > the commit message. Yes, I will fix that. ... > > + if (!txstate) { > > + if (test_bit(ATC_IS_PAUSED, &atchan->status)) > > there's a helper function that you can use instead: atc_chan_is_paused() Will use it. > > + return DMA_PAUSED; > > + return DMA_ERROR; > > return dma_status please Will squash the patch. > > + } ... > > + if (test_bit(ATC_IS_PAUSED, &atchan->status)) > > The status may change after spin_unlock_irqrestore(). Should the residue > be in sync with the dma status? If yes, you should check the status while > holding the lock. You should tell me actually. Because I'm a bit puzzled why we need a spin lock _and_ atomic bit operations together. > > + dma_status = DMA_PAUSED; ... Thank you for the review.
diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c index 8858470246e1..a9d8dd990d6e 100644 --- a/drivers/dma/at_hdmac.c +++ b/drivers/dma/at_hdmac.c @@ -1669,9 +1669,19 @@ atc_tx_status(struct dma_chan *chan, int ret; dma_status = dma_cookie_status(chan, cookie, txstate); - if (dma_status == DMA_COMPLETE || !txstate) + if (dma_status == DMA_COMPLETE) return dma_status; + /* + * There's no point in calculating the residue if there's + * no txstate to store the value. + */ + if (!txstate) { + if (test_bit(ATC_IS_PAUSED, &atchan->status)) + return DMA_PAUSED; + return DMA_ERROR; + } + spin_lock_irqsave(&atchan->vc.lock, flags); /* Get number of bytes left in the active transactions */ ret = atc_get_residue(chan, cookie, &residue); @@ -1684,6 +1694,9 @@ atc_tx_status(struct dma_chan *chan, dma_set_residue(txstate, residue); } + if (test_bit(ATC_IS_PAUSED, &atchan->status)) + dma_status = DMA_PAUSED; + dev_vdbg(chan2dev(chan), "tx_status %d: cookie = %d residue = %u\n", dma_status, cookie, residue);
device_tx_status() may return DMA_PAUSED status when driver supports it. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/dma/at_hdmac.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)