Regression in next with spi return from transfer_one()

Regression in next with spi return from transfer_one()

[Tony Lindgren]

Hi,

Commit 810923f3bf06 ("spi: Deal with slaves that return from
transfer_one() unfinished") causes a regression at least on
droid 4 with SPI PMIC and regulators on the PMIC. During boot
the device just hangs there waiting for rootfs to appear on
MMC. Reverting 810923f3bf06 makes things work again.

Any ideas why this might be?

Regards,

Tony

Re: Regression in next with spi return from transfer_one()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

On Thu, Nov 15, 2018 at 01:14:51PM -0800, Tony Lindgren wrote:

> Commit 810923f3bf06 ("spi: Deal with slaves that return from
> transfer_one() unfinished") causes a regression at least on
> droid 4 with SPI PMIC and regulators on the PMIC. During boot
> the device just hangs there waiting for rootfs to appear on
> MMC. Reverting 810923f3bf06 makes things work again.

> Any ideas why this might be?

Wow, that's not obvious...  as far as I can tell the code in the !slave
case is identical so unless the controller is somehow getting mistakenly
flagged as a slave it looks like it should be something to do with it
being pushed into a function.  Could you try logging what the timeout
ends up getting set to?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Regression in next with spi return from transfer_one()

[Tony Lindgren]

* Mark Brown <broonie@kernel.org> [181115 22:12]:
> On Thu, Nov 15, 2018 at 01:14:51PM -0800, Tony Lindgren wrote:
> 
> > Commit 810923f3bf06 ("spi: Deal with slaves that return from
> > transfer_one() unfinished") causes a regression at least on
> > droid 4 with SPI PMIC and regulators on the PMIC. During boot
> > the device just hangs there waiting for rootfs to appear on
> > MMC. Reverting 810923f3bf06 makes things work again.
> 
> > Any ideas why this might be?
> 
> Wow, that's not obvious...  as far as I can tell the code in the !slave
> case is identical so unless the controller is somehow getting mistakenly
> flagged as a slave it looks like it should be something to do with it
> being pushed into a function.  Could you try logging what the timeout
> ends up getting set to?

It seems to be caused because of the now missing "if (ret > 0) {"
line somehow that was there earlier. New code sets ms to 200 it
seems, then dmesg shows:

SPI transfer timed out

The old code is not updating ms and it's set to 1.

Regards,

Tony

Re: Regression in next with spi return from transfer_one()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

On Thu, Nov 15, 2018 at 03:44:00PM -0800, Tony Lindgren wrote:

> It seems to be caused because of the now missing "if (ret > 0) {"
> line somehow that was there earlier. New code sets ms to 200 it
> seems, then dmesg shows:

Doh, of course :(  Sorry I missed that.

> The old code is not updating ms and it's set to 1.

Right, and not waiting either which should be the issue.  Does the
following work:

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 498d3b9bf3ae..430ad637c643 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1114,9 +1114,11 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
 				goto out;
 			}
 
-			ret = spi_transfer_wait(ctlr, msg, xfer);
-			if (ret < 0)
-				msg->status = ret;
+			if (ret > 0) {
+				ret = spi_transfer_wait(ctlr, msg, xfer);
+				if (ret < 0)
+					msg->status = ret;
+			}
 		} else {
 			if (xfer->len)
 				dev_err(&msg->spi->dev,

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Regression in next with spi return from transfer_one()

[Tony Lindgren]

* Mark Brown <broonie@kernel.org> [181116 00:02]:
> On Thu, Nov 15, 2018 at 03:44:00PM -0800, Tony Lindgren wrote:
> 
> > It seems to be caused because of the now missing "if (ret > 0) {"
> > line somehow that was there earlier. New code sets ms to 200 it
> > seems, then dmesg shows:
> 
> Doh, of course :(  Sorry I missed that.
> 
> > The old code is not updating ms and it's set to 1.
> 
> Right, and not waiting either which should be the issue.  Does the
> following work:

And it's recalculating the timeout every time now too :) Yup that
fix works and the problem makes sense now:

Tested-by: Tony Lindgren <tony@atomide.com>

> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 498d3b9bf3ae..430ad637c643 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1114,9 +1114,11 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
>  				goto out;
>  			}
>  
> -			ret = spi_transfer_wait(ctlr, msg, xfer);
> -			if (ret < 0)
> -				msg->status = ret;
> +			if (ret > 0) {
> +				ret = spi_transfer_wait(ctlr, msg, xfer);
> +				if (ret < 0)
> +					msg->status = ret;
> +			}
>  		} else {
>  			if (xfer->len)
>  				dev_err(&msg->spi->dev,

Re: Regression in next with spi return from transfer_one()

[Lubomir Rintel]

On Thu, 2018-11-15 at 16:07 -0800, Tony Lindgren wrote:
> * Mark Brown <broonie@kernel.org> [181116 00:02]:
> > On Thu, Nov 15, 2018 at 03:44:00PM -0800, Tony Lindgren wrote:
> > 
> > > It seems to be caused because of the now missing "if (ret > 0) {"
> > > line somehow that was there earlier. New code sets ms to 200 it
> > > seems, then dmesg shows:
> > 
> > Doh, of course :(  Sorry I missed that.
> > 
> > > The old code is not updating ms and it's set to 1.
> > 
> > Right, and not waiting either which should be the issue.  Does the
> > following work:
> 
> And it's recalculating the timeout every time now too :) Yup that
> fix works and the problem makes sense now:
> 
> Tested-by: Tony Lindgren <tony@atomide.com>

My bad obviously. Sorry.

I'm grateful that you identified and fixed the problem so quickly.

Thanks,
Lubo

> 
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 498d3b9bf3ae..430ad637c643 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -1114,9 +1114,11 @@ static int spi_transfer_one_message(struct
> > spi_controller *ctlr,
> >  				goto out;
> >  			}
> >  
> > -			ret = spi_transfer_wait(ctlr, msg, xfer);
> > -			if (ret < 0)
> > -				msg->status = ret;
> > +			if (ret > 0) {
> > +				ret = spi_transfer_wait(ctlr, msg,
> > xfer);
> > +				if (ret < 0)
> > +					msg->status = ret;
> > +			}
> >  		} else {
> >  			if (xfer->len)
> >  				dev_err(&msg->spi->dev,
> 
>