* Regression in next with spi return from transfer_one() @ 2018-11-15 21:14 Tony Lindgren 2018-11-15 22:12 ` Mark Brown 0 siblings, 1 reply; 6+ messages in thread From: Tony Lindgren @ 2018-11-15 21:14 UTC (permalink / raw) To: Lubomir Rintel, Mark Brown Cc: linux-arm-kernel, linux-omap, Pavel Machek, Geert Uytterhoeven, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Regression in next with spi return from transfer_one() 2018-11-15 21:14 Regression in next with spi return from transfer_one() Tony Lindgren @ 2018-11-15 22:12 ` Mark Brown 2018-11-15 23:44 ` Tony Lindgren 0 siblings, 1 reply; 6+ messages in thread From: Mark Brown @ 2018-11-15 22:12 UTC (permalink / raw) To: Tony Lindgren Cc: Lubomir Rintel, Geert Uytterhoeven, Pavel Machek, linux-kernel, linux-arm-kernel, linux-omap [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Regression in next with spi return from transfer_one() 2018-11-15 22:12 ` Mark Brown @ 2018-11-15 23:44 ` Tony Lindgren 2018-11-16 0:01 ` Mark Brown 0 siblings, 1 reply; 6+ messages in thread From: Tony Lindgren @ 2018-11-15 23:44 UTC (permalink / raw) To: Mark Brown Cc: Lubomir Rintel, Geert Uytterhoeven, Pavel Machek, linux-kernel, linux-arm-kernel, linux-omap * 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Regression in next with spi return from transfer_one() 2018-11-15 23:44 ` Tony Lindgren @ 2018-11-16 0:01 ` Mark Brown 2018-11-16 0:07 ` Tony Lindgren 0 siblings, 1 reply; 6+ messages in thread From: Mark Brown @ 2018-11-16 0:01 UTC (permalink / raw) To: Tony Lindgren Cc: Geert Uytterhoeven, linux-kernel, Lubomir Rintel, Pavel Machek, linux-omap, linux-arm-kernel [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Regression in next with spi return from transfer_one() 2018-11-16 0:01 ` Mark Brown @ 2018-11-16 0:07 ` Tony Lindgren 2018-11-16 15:35 ` Lubomir Rintel 0 siblings, 1 reply; 6+ messages in thread From: Tony Lindgren @ 2018-11-16 0:07 UTC (permalink / raw) To: Mark Brown Cc: Lubomir Rintel, Geert Uytterhoeven, Pavel Machek, linux-kernel, linux-arm-kernel, linux-omap * 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, ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Regression in next with spi return from transfer_one() 2018-11-16 0:07 ` Tony Lindgren @ 2018-11-16 15:35 ` Lubomir Rintel 0 siblings, 0 replies; 6+ messages in thread From: Lubomir Rintel @ 2018-11-16 15:35 UTC (permalink / raw) To: Tony Lindgren, Mark Brown Cc: Geert Uytterhoeven, Pavel Machek, linux-kernel, linux-arm-kernel, linux-omap 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, > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-16 15:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-15 21:14 Regression in next with spi return from transfer_one() Tony Lindgren 2018-11-15 22:12 ` Mark Brown 2018-11-15 23:44 ` Tony Lindgren 2018-11-16 0:01 ` Mark Brown 2018-11-16 0:07 ` Tony Lindgren 2018-11-16 15:35 ` Lubomir Rintel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).