Re: [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time

[Xu Yilun]

On Tue, Dec 29, 2020 at 01:13:08PM +0000, Mark Brown wrote:
> On Tue, Dec 29, 2020 at 01:27:42PM +0800, Xu Yilun wrote:
> > The xfer waiting time is the result of xfer->len / xfer->speed_hz, but
> > when the following patch is merged,
> > 
> > commit 9326e4f1e5dd ("spi: Limit the spi device max speed to controller's max speed")
> > 
> > the xfer->speed_hz may always be clamped to 0 if the controller doesn't
> > provide its max_speed_hz. There may be no hardware indication of the
> > max_speed_hz so the controller driver leaves it, but exception happens
> > when it tries to do irq mode transfer.
> 
> Does this still apply with current code?  There have been some fixes in
> this area which I think should ensure that we don't turn the speed down
> to 0 if the controller doesn't supply a limit IIRC.

Yes, there is chance the speed is set to 0, some related code from 5.11-rc1

  int spi_setup(struct spi_device *spi)
  {
	...

	if (!spi->max_speed_hz ||
	    spi->max_speed_hz > spi->controller->max_speed_hz)
		spi->max_speed_hz = spi->controller->max_speed_hz;

If the controller doesn't supply a limit, spi->max_speed_hz will always
be clamped to 0 here, no matter what the client inputs.

BTW, Could we keep the spi->max_speed_hz if no controller->max_speed_hz?
Always clamp the spi->max_speed_hz to 0 makes no sense.

	...
  }

  static int __spi_validate(struct spi_device *spi, struct spi_message
			    *message)
  {
	...

	if (!xfer->speed_hz)
		xfer->speed_hz = spi->max_speed_hz;

	if (ctlr->max_speed_hz && xfer->speed_hz > ctlr->max_speed_hz)
		xfer->speed_hz = ctlr->max_speed_hz;

If spi->max_speed_hz & controller->max_speed_hz are 0, xfer->speed_hz is
always 0.

	...
  }


I tested it on 5.11-rc1 with spi-altera.

> 
> > This patch makes the assumption of 1khz xfer speed if the xfer->speed_hz
> > is not assigned. This avoids the divide by 0 issue and ensures a
> > reasonable tolerant waiting time.
> 
> It will cause absurdly slow transfers if the controller does actually
> implement speed setting though, if we're going to pick a default value

Maybe I didn't describe clearly, if the controller has a valid limit setting,
the xfer->speed_hz will be set to max_speed_hz and will not fall through to
a default value. The fix code takes function when all the checks in spi_setup &
spi_validate fails to assign the xfer->speed_hz. 

This fix only affects the waiting timeout, it will not slow down the normal
xfer anyway.

> I'd go for at least 100kHz. 

If some controller is actually working at a speed lower than the default
value, xfer will always be unexpectly early terminated.

I'm not sure how slow the controllers in the world could be. If 100kHz
is slow enough to everyone it's OK.
 
> 
> >  	} else {
> > +		speed_hz = xfer->speed_hz ? : 1000;
> 
> Please don't abuse the ternery operator, write normal conditional
> statements to make things more legible.

OK, I'll change it.

Thanks,
Yilun