* [PATCH 0/2] fix the issue when xfer by spi-altera
@ 2020-12-29 5:27 Xu Yilun
2020-12-29 5:27 ` [PATCH 1/2] spi: altera: fix return value for altera_spi_txrx() Xu Yilun
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Xu Yilun @ 2020-12-29 5:27 UTC (permalink / raw)
To: broonie, linux-spi
Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach,
russell.h.weight, linux-kernel
When doing spi xfer by spi-altera, divide by 0 exception happens in
spi_transfer_wait(), This is because the xfer->speed_hz is always
clamped to 0 by spi->controller->max_speed_hz, the feature is
introduced in:
commit 9326e4f1e5dd ("spi: Limit the spi device max speed to controller's max speed")
The spi-altera doesn't have hardware indication for controller's
max_speed_hz, so its value is uninitialized as 0.
Patch #1 fixes the issue of spi_altera driver. When doing polling
mode xfer, its transfer_one() callback should return 1, to indicate
the xfer is finished. It should return 0 for irq mode xfer. With
this patch the polling mode xfer is OK as it needs no
spi_transfer_wait() anymore.
But the irq mode xfer is still broken. So Patch #2 assumes 1khz xfer
speed if the xfer->speed_hz is not assigned. I try to avoid the
divide by 0 issue and ensures a reasonable tolerant waiting time in
a generic way.
Xu Yilun (2):
spi: altera: fix return value for altera_spi_txrx()
spi: fix the divide by 0 error when calculating xfer waiting time
drivers/spi/spi-altera.c | 26 ++++++++++++++------------
drivers/spi/spi.c | 4 +++-
2 files changed, 17 insertions(+), 13 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] spi: altera: fix return value for altera_spi_txrx() 2020-12-29 5:27 [PATCH 0/2] fix the issue when xfer by spi-altera Xu Yilun @ 2020-12-29 5:27 ` Xu Yilun 2020-12-29 5:27 ` [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time Xu Yilun 2020-12-29 14:33 ` (subset) [PATCH 0/2] fix the issue when xfer by spi-altera Mark Brown 2 siblings, 0 replies; 9+ messages in thread From: Xu Yilun @ 2020-12-29 5:27 UTC (permalink / raw) To: broonie, linux-spi Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, russell.h.weight, linux-kernel This patch fixes the return value for altera_spi_txrx. It should return 1 for interrupt transfer mode, and return 0 for polling transfer mode. The altera_spi_txrx() implements the spi_controller.transfer_one callback. According to the spi-summary.rst, the transfer_one should return 0 when transfer is finished, return 1 when transfer is still in progress. Signed-off-by: Xu Yilun <yilun.xu@intel.com> --- drivers/spi/spi-altera.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/drivers/spi/spi-altera.c b/drivers/spi/spi-altera.c index 809bfff..cbc4c28 100644 --- a/drivers/spi/spi-altera.c +++ b/drivers/spi/spi-altera.c @@ -189,24 +189,26 @@ static int altera_spi_txrx(struct spi_master *master, /* send the first byte */ altera_spi_tx_word(hw); - } else { - while (hw->count < hw->len) { - altera_spi_tx_word(hw); - for (;;) { - altr_spi_readl(hw, ALTERA_SPI_STATUS, &val); - if (val & ALTERA_SPI_STATUS_RRDY_MSK) - break; + return 1; + } + + while (hw->count < hw->len) { + altera_spi_tx_word(hw); - cpu_relax(); - } + for (;;) { + altr_spi_readl(hw, ALTERA_SPI_STATUS, &val); + if (val & ALTERA_SPI_STATUS_RRDY_MSK) + break; - altera_spi_rx_word(hw); + cpu_relax(); } - spi_finalize_current_transfer(master); + + altera_spi_rx_word(hw); } + spi_finalize_current_transfer(master); - return t->len; + return 0; } static irqreturn_t altera_spi_irq(int irq, void *dev) -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time 2020-12-29 5:27 [PATCH 0/2] fix the issue when xfer by spi-altera Xu Yilun 2020-12-29 5:27 ` [PATCH 1/2] spi: altera: fix return value for altera_spi_txrx() Xu Yilun @ 2020-12-29 5:27 ` Xu Yilun 2020-12-29 13:13 ` Mark Brown 2020-12-29 14:33 ` (subset) [PATCH 0/2] fix the issue when xfer by spi-altera Mark Brown 2 siblings, 1 reply; 9+ messages in thread From: Xu Yilun @ 2020-12-29 5:27 UTC (permalink / raw) To: broonie, linux-spi Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, russell.h.weight, linux-kernel 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(-) 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) -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time 2020-12-29 5:27 ` [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time Xu Yilun @ 2020-12-29 13:13 ` Mark Brown 2020-12-30 2:24 ` Xu Yilun 0 siblings, 1 reply; 9+ messages in thread From: Mark Brown @ 2020-12-29 13:13 UTC (permalink / raw) To: Xu Yilun Cc: linux-spi, trix, lgoncalv, hao.wu, matthew.gerlach, russell.h.weight, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1260 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time 2020-12-29 13:13 ` Mark Brown @ 2020-12-30 2:24 ` Xu Yilun 2020-12-30 13:46 ` Mark Brown 0 siblings, 1 reply; 9+ messages in thread From: Xu Yilun @ 2020-12-30 2:24 UTC (permalink / raw) To: Mark Brown Cc: linux-spi, trix, lgoncalv, hao.wu, matthew.gerlach, russell.h.weight, linux-kernel, yilun.xu 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time 2020-12-30 2:24 ` Xu Yilun @ 2020-12-30 13:46 ` Mark Brown 2020-12-31 3:23 ` Xu Yilun 0 siblings, 1 reply; 9+ messages in thread From: Mark Brown @ 2020-12-30 13:46 UTC (permalink / raw) To: Xu Yilun Cc: linux-spi, trix, lgoncalv, hao.wu, matthew.gerlach, russell.h.weight, linux-kernel [-- Attachment #1: Type: text/plain, Size: 616 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time 2020-12-30 13:46 ` Mark Brown @ 2020-12-31 3:23 ` Xu Yilun 2020-12-31 13:36 ` Mark Brown 0 siblings, 1 reply; 9+ messages in thread From: Xu Yilun @ 2020-12-31 3:23 UTC (permalink / raw) To: Mark Brown Cc: linux-spi, trix, lgoncalv, hao.wu, matthew.gerlach, russell.h.weight, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time 2020-12-31 3:23 ` Xu Yilun @ 2020-12-31 13:36 ` Mark Brown 0 siblings, 0 replies; 9+ messages in thread From: Mark Brown @ 2020-12-31 13:36 UTC (permalink / raw) To: Xu Yilun Cc: linux-spi, trix, lgoncalv, hao.wu, matthew.gerlach, russell.h.weight, linux-kernel [-- Attachment #1: Type: text/plain, Size: 865 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: (subset) [PATCH 0/2] fix the issue when xfer by spi-altera 2020-12-29 5:27 [PATCH 0/2] fix the issue when xfer by spi-altera Xu Yilun 2020-12-29 5:27 ` [PATCH 1/2] spi: altera: fix return value for altera_spi_txrx() Xu Yilun 2020-12-29 5:27 ` [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time Xu Yilun @ 2020-12-29 14:33 ` Mark Brown 2 siblings, 0 replies; 9+ messages in thread From: Mark Brown @ 2020-12-29 14:33 UTC (permalink / raw) To: linux-spi, Xu Yilun Cc: matthew.gerlach, linux-kernel, lgoncalv, trix, russell.h.weight, hao.wu On Tue, 29 Dec 2020 13:27:40 +0800, Xu Yilun wrote: > When doing spi xfer by spi-altera, divide by 0 exception happens in > spi_transfer_wait(), This is because the xfer->speed_hz is always > clamped to 0 by spi->controller->max_speed_hz, the feature is > introduced in: > > commit 9326e4f1e5dd ("spi: Limit the spi device max speed to controller's max speed") > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/2] spi: altera: fix return value for altera_spi_txrx() commit: ede090f5a438e97d0586f64067bbb956e30a2a31 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-12-31 13:38 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-29 5:27 [PATCH 0/2] fix the issue when xfer by spi-altera Xu Yilun 2020-12-29 5:27 ` [PATCH 1/2] spi: altera: fix return value for altera_spi_txrx() Xu Yilun 2020-12-29 5:27 ` [PATCH 2/2] spi: fix the divide by 0 error when calculating xfer waiting time Xu Yilun 2020-12-29 13:13 ` Mark Brown 2020-12-30 2:24 ` Xu Yilun 2020-12-30 13:46 ` Mark Brown 2020-12-31 3:23 ` Xu Yilun 2020-12-31 13:36 ` Mark Brown 2020-12-29 14:33 ` (subset) [PATCH 0/2] fix the issue when xfer by spi-altera Mark Brown
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.