[PATCH 0/2] fix the issue when xfer by spi-altera

[PATCH 0/2] fix the issue when xfer by spi-altera

[Xu Yilun]

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

[PATCH 1/2] spi: altera: fix return value for altera_spi_txrx()

[Xu Yilun]

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

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

[Xu Yilun]

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

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

[Mark Brown]

[-- 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 --]

Re: (subset) [PATCH 0/2] fix the issue when xfer by spi-altera

[Mark Brown]

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

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

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

[Mark Brown]

[-- 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 --]

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

[Xu Yilun]

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

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

[Mark Brown]

[-- 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 --]