Re: [PATCH] spi: use udelay() for short polling delays

[David Laight]

On Sun, 17 May 2026 16:15:01 -0700
Peter Collingbourne <peter@xxxxxxxxx> wrote:

> On Sun, May 17, 2026 at 7:02 AM David Laight
> <david.laight.linux@xxxxxxxxx> wrote:
> >
> > On Sat, 16 May 2026 23:25:54 -0700
> > Peter Collingbourne <peter@xxxxxxxxx> wrote:
> >  
> > > A short polling delay, such as the delay of 5us
> > > (SPINAND_READ_POLL_DELAY_US) provided by the SPI NAND driver,
> > > can become a 1/HZ (order of ms) delay caused by the usleep_range()
> > > call in read_poll_timeout(), significantly reducing SPI NAND access
> > > performance. Fix it by implementing the polling delay with udelay()
> > > (via read_poll_timeout_atomic()) if it is short enough, matching how
> > > the initial delay is handled.
> > >
> > > Fixes: c955a0cc8a28 ("spi: spi-mem: add automatic poll status functions")
> > > Signed-off-by: Peter Collingbourne <peter@xxxxxxxxx>
> > > ---
> > >  drivers/spi/spi-mem.c | 17 +++++++++++++----
> > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> > > index a09371a075d2..914e35e51cea 100644
> > > --- a/drivers/spi/spi-mem.c
> > > +++ b/drivers/spi/spi-mem.c
> > > @@ -1005,10 +1005,19 @@ int spi_mem_poll_status(struct spi_mem *mem,
> > >                       usleep_range((initial_delay_us >> 2) + 1,
> > >                                    initial_delay_us);
> > >
> > > -             ret = read_poll_timeout(spi_mem_read_status, read_status_ret,
> > > -                                     (read_status_ret || ((status) & mask) == match),
> > > -                                     polling_delay_us, timeout_ms * 1000, false, mem,
> > > -                                     op, &status);
> > > +             if (polling_delay_us < 10)
> > > +                     ret = read_poll_timeout_atomic(
> > > +                             spi_mem_read_status, read_status_ret,
> > > +                             (read_status_ret || ((status)&mask) == match),
> > > +                             polling_delay_us, timeout_ms * 1000, false, mem,
> > > +                             op, &status);
> > > +             else
> > > +                     ret = read_poll_timeout(
> > > +                             spi_mem_read_status, read_status_ret,
> > > +                             (read_status_ret || ((status)&mask) == match),
> > > +                             polling_delay_us, timeout_ms * 1000, false, mem,
> > > +                             op, &status);
> > > +  
> >
> > That looks rather sub-optional.
> > Even if the interval is short you want to drop to sleeps if the device isn't
> > responding.
> > (Or this code is used for erases and you are doing a full device erase.)
> >
> > I doubt you want to spin for more than 1ms.
> >
> > -David  
> 
> In include/linux/mtd/spinand.h we have:
> 
> #define SPINAND_READ_POLL_DELAY_US 5
> #define SPINAND_RESET_POLL_DELAY_US 5
> #define SPINAND_WRITE_POLL_DELAY_US 15
> #define SPINAND_ERASE_POLL_DELAY_US 50
> 
> So we will only delay here for reads and resets.

The timeout_ms values are equally relevant.
If anything goes wrong you don't want to be spinning for a long time
(and the timeout in the polling/atomic case is likely to be significantly
longer than you might expect - it only counts time in delay_us()).

So sort of want something like the following - but it is too hacky!
	int spins = polling_delay_us < 10 ? 10 : 0;
#define usleep_range(min, max) (--spins < 0 ? usleep_range(min, max) : udelay(max))
	ret = read_poll_timeout(....);
#undef usleep_range

-- David

> 
> I would generally expect drivers to arrange for spi_mem_read_status()
> to sleep using wait_for_completion*() where possible, so we shouldn't
> normally be actually spinning here.
> 
> For example, the generic driver:
> 
> spi_mem_read_status()
> -> spi_mem_exec_op()
>   -> spi_sync()
>     -> __spi_sync()
>       -> __spi_transfer_message_noqueue()
>         -> __spi_pump_transfer_message()
>           -> spi_transfer_one_message() [via ctlr->transfer_one_message]
>             -> spi_transfer_wait()
>               -> wait_for_completion_timeout()  
> 
> For spi-mt65xx:
> 
> spi_mem_read_status()
> -> spi_mem_exec_op()
>   -> mtk_spi_mem_exec_op() [via ctlr->mem_ops->exec_op]
>     -> wait_for_completion_timeout()  
> 
> Peter
>