Re: [PATCH v4 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Fri, May 22, 2020 at 02:52:35PM +0300, Serge Semin wrote:
> On Fri, May 22, 2020 at 02:13:40PM +0300, Andy Shevchenko wrote:
> > On Fri, May 22, 2020 at 03:07:50AM +0300, Serge Semin wrote:
> > > Since DMA transfers are performed asynchronously with actual SPI
> > > transaction, then even if DMA transfers are finished it doesn't mean
> > > all data is actually pushed to the SPI bus. Some data might still be
> > > in the controller FIFO. This is specifically true for Tx-only
> > > transfers. In this case if the next SPI transfer is recharged while
> > > a tail of the previous one is still in FIFO, we'll loose that tail
> > > data. In order to fix this lets add the wait procedure of the Tx/Rx
> > > SPI transfers completion after the corresponding DMA transactions
> > > are finished.

...

> > > Changelog v4:
> > > - Get back ndelay() method to wait for an SPI transfer completion.
> > >   spi_delay_exec() isn't suitable for the atomic context.
> > 
> > OTOH we may teach spi_delay_exec() to perform atomic sleeps.
> 
> Please, see it's implementation. It does atomic delay when the delay value
> is less than 10us. But selectively gets to the usleep_range() if value is
> greater than that.

Oh, than it means we may do a very long busy loop here which is not good at
all. If we have 10Hz clock, it might take seconds of doing nothing!

...

> > > +	while (dw_spi_dma_tx_busy(dws) && retry--)
> > > +		ndelay(ns);
> > 
> > I might be mistaken, but I think I told that this one misses to keep power
> > management in mind.
> 
> Here we already in nearly atomic context due to the callback executed in the
> tasklet. What power management could be during a tasklet execution? Again we
> can't call sleeping methods in here. What do you suggest in substitution?
> 
> > Have you read Documentation/process/volatile-considered-harmful.rst ?
> 
> That's mentoring tone is redundant. Please, stop it.

I simple gave you pointers to where you may read about power management in busy
loops. Yes, I admit that documentation title and the relation to busy loops is
not obvious.

-- 
With Best Regards,
Andy Shevchenko