| Message ID | 1609219662-27057-3-git-send-email-yilun.xu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix the issue when xfer by spi-altera | expand |
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
On Wed, Dec 30, 2020 at 10:24:20AM +0800, Xu Yilun wrote: > On Tue, Dec 29, 2020 at 01:13:08PM +0000, 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.
On Wed, Dec 30, 2020 at 01:46:44PM +0000, 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 +0000, 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
On Thu, Dec 31, 2020 at 11:23:37AM +0800, Xu Yilun wrote: > On Wed, Dec 30, 2020 at 01:46:44PM +0000, 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.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 51d7c00..2f3c2c9 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1109,6 +1109,7 @@ static int spi_transfer_wait(struct spi_controller *ctlr, struct spi_statistics *statm = &ctlr->statistics; struct spi_statistics *stats = &msg->spi->statistics; unsigned long long ms; + u32 speed_hz; if (spi_controller_is_slave(ctlr)) { if (wait_for_completion_interruptible(&ctlr->xfer_completion)) { @@ -1116,8 +1117,9 @@ static int spi_transfer_wait(struct spi_controller *ctlr, return -EINTR; } } else { + speed_hz = xfer->speed_hz ? : 1000; ms = 8LL * 1000LL * xfer->len; - do_div(ms, xfer->speed_hz); + do_div(ms, speed_hz); ms += ms + 200; /* some tolerance */ if (ms > UINT_MAX)
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. 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. Signed-off-by: Xu Yilun <yilun.xu@intel.com> --- drivers/spi/spi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)