Re: [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time
On Thu, Dec 31, 2020 at 11:23:37AM +0800, Xu Yilun wrote: > On Wed, Dec 30, 2020 at 01:46:44PM +, Mark Brown wrote: > > > 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. > > Right, that's the fix. > Seems it still doesn't fix the case that neither controller nor client dev > provides the non-zero max_speed_hz. Do you think the patch is still > necessary? Right, something like this would help if we genuinely have no idea. We probably shouldn't do it at validation stage which would be the other thing since it might cause us to realy hurt performance on systems which happen to have a sensible default in hardware but don't specify a maximum - we might set too low a default speed for the actual transfer. Please fix the coding style issue I mentioned and resubmit. signature.asc Description: PGP signature
Re: [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time
On Wed, Dec 30, 2020 at 01:46:44PM +, Mark Brown wrote: > On Wed, Dec 30, 2020 at 10:24:20AM +0800, Xu Yilun wrote: > > On Tue, Dec 29, 2020 at 01:13:08PM +, Mark Brown wrote: > > > > 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 > > Please check the code in the SPI tree and -next. I see the fix patches in maillist, thanks. > > > 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. > > Right, that's the fix. Seems it still doesn't fix the case that neither controller nor client dev provides the non-zero max_speed_hz. Do you think the patch is still necessary? Thanks, Yilun
Re: [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time
On Wed, Dec 30, 2020 at 10:24:20AM +0800, Xu Yilun wrote: > On Tue, Dec 29, 2020 at 01:13:08PM +, Mark Brown wrote: > > 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 Please check the code in the SPI tree and -next. > 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. Right, that's the fix. signature.asc Description: PGP signature
Re: [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time
On Tue, Dec 29, 2020 at 01:13:08PM +, 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
Re: [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time
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.
> 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
I'd go for at least 100kHz.
> } else {
> + speed_hz = xfer->speed_hz ? : 1000;
Please don't abuse the ternery operator, write normal conditional
statements to make things more legible.
signature.asc
Description: PGP signature
