| Message ID | 20180403152905.1524-2-ssuloev@orpaltech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 03, 2018 at 06:29:00PM +0300, Sergey Suloev wrote: > As long as sun4i/sun6i SPI drivers have overriden the default > "wait for completion" procedure then we need to properly > handle -ETIMEDOUT error from transfer_one(). Why is this connected to those drivers specifically?
On 04/03/2018 06:52 PM, Mark Brown wrote: > On Tue, Apr 03, 2018 at 06:29:00PM +0300, Sergey Suloev wrote: >> As long as sun4i/sun6i SPI drivers have overriden the default >> "wait for completion" procedure then we need to properly >> handle -ETIMEDOUT error from transfer_one(). > Why is this connected to those drivers specifically? These 2 drivers have their own "waiting" code and not using the code from SPI core.
On Tue, Apr 03, 2018 at 07:00:55PM +0300, Sergey Suloev wrote: > On 04/03/2018 06:52 PM, Mark Brown wrote: > > On Tue, Apr 03, 2018 at 06:29:00PM +0300, Sergey Suloev wrote: > > > As long as sun4i/sun6i SPI drivers have overriden the default > > > "wait for completion" procedure then we need to properly > > > handle -ETIMEDOUT error from transfer_one(). > > Why is this connected to those drivers specifically? > These 2 drivers have their own "waiting" code and not using the code from > SPI core. Does this not apply to any other driver - why is this something we only have to do when these drivers do it? That's what's setting off alarm bells.
On 04/03/2018 07:18 PM, Mark Brown wrote: > On Tue, Apr 03, 2018 at 07:00:55PM +0300, Sergey Suloev wrote: >> On 04/03/2018 06:52 PM, Mark Brown wrote: >>> On Tue, Apr 03, 2018 at 06:29:00PM +0300, Sergey Suloev wrote: >>>> As long as sun4i/sun6i SPI drivers have overriden the default >>>> "wait for completion" procedure then we need to properly >>>> handle -ETIMEDOUT error from transfer_one(). >>> Why is this connected to those drivers specifically? >> These 2 drivers have their own "waiting" code and not using the code from >> SPI core. > Does this not apply to any other driver - why is this something we only > have to do when these drivers do it? That's what's setting off alarm > bells. sun4i/sun6i drivers have let's say "smart" waiting while SPI core uses a fixed interval to wait. I can't say for every SPI driver in kernel, that's outside of my area of expertise.
On Tue, Apr 03, 2018 at 07:24:11PM +0300, Sergey Suloev wrote: > On 04/03/2018 07:18 PM, Mark Brown wrote: > > On Tue, Apr 03, 2018 at 07:00:55PM +0300, Sergey Suloev wrote: > > > On 04/03/2018 06:52 PM, Mark Brown wrote: > > > > On Tue, Apr 03, 2018 at 06:29:00PM +0300, Sergey Suloev wrote: > > > > > As long as sun4i/sun6i SPI drivers have overriden the default > > > > > "wait for completion" procedure then we need to properly > > > > > handle -ETIMEDOUT error from transfer_one(). > > > > Why is this connected to those drivers specifically? > > > These 2 drivers have their own "waiting" code and not using the code from > > > SPI core. > > Does this not apply to any other driver - why is this something we only > > have to do when these drivers do it? That's what's setting off alarm > > bells. > > sun4i/sun6i drivers have let's say "smart" waiting while SPI core uses a > fixed interval to wait. > > I can't say for every SPI driver in kernel, that's outside of my area of > expertise. I'm not sure what's specific about the sun4i / sun6i case here. Your patch doesn't have anything to do with the delay before the timeout, but the fact that we return -ETIMEDOUT in the first place. And I'm pretty sure that papering over an error returned by a driver is not the right thing to do. Maxime
On Wed, Apr 04, 2018 at 09:08:18AM +0200, Maxime Ripard wrote: > And I'm pretty sure that papering over an error returned by a driver > is not the right thing to do. We've got specific error handling for timeouts - they get accounted for separately in the stats. It *shouldn't* affect actual operation and AFAICT it doesn't. I think the main problem here is that the commit message is very unclear.
On 04/04/2018 10:08 AM, Maxime Ripard wrote: > On Tue, Apr 03, 2018 at 07:24:11PM +0300, Sergey Suloev wrote: >> On 04/03/2018 07:18 PM, Mark Brown wrote: >>> On Tue, Apr 03, 2018 at 07:00:55PM +0300, Sergey Suloev wrote: >>>> On 04/03/2018 06:52 PM, Mark Brown wrote: >>>>> On Tue, Apr 03, 2018 at 06:29:00PM +0300, Sergey Suloev wrote: >>>>>> As long as sun4i/sun6i SPI drivers have overriden the default >>>>>> "wait for completion" procedure then we need to properly >>>>>> handle -ETIMEDOUT error from transfer_one(). >>>>> Why is this connected to those drivers specifically? >>>> These 2 drivers have their own "waiting" code and not using the code from >>>> SPI core. >>> Does this not apply to any other driver - why is this something we only >>> have to do when these drivers do it? That's what's setting off alarm >>> bells. >> sun4i/sun6i drivers have let's say "smart" waiting while SPI core uses a >> fixed interval to wait. >> >> I can't say for every SPI driver in kernel, that's outside of my area of >> expertise. > I'm not sure what's specific about the sun4i / sun6i case here. Your > patch doesn't have anything to do with the delay before the timeout, > but the fact that we return -ETIMEDOUT in the first place. > > And I'm pretty sure that papering over an error returned by a driver > is not the right thing to do. > > Maxime > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel do you mean the changes in spi.c are not required at all ? My point was to eat ETIMEDOUT error from transfer_one() as it is just a mark and shouldn't be handled as a normal error.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index b33a727..2dcd4f6 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1028,7 +1028,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr, reinit_completion(&ctlr->xfer_completion); ret = ctlr->transfer_one(ctlr, msg->spi, xfer); - if (ret < 0) { + if (ret < 0 && ret != -ETIMEDOUT) { SPI_STATISTICS_INCREMENT_FIELD(statm, errors); SPI_STATISTICS_INCREMENT_FIELD(stats, @@ -1051,7 +1051,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr, msecs_to_jiffies(ms)); } - if (ms == 0) { + if (ms == 0 || ret == -ETIMEDOUT) { SPI_STATISTICS_INCREMENT_FIELD(statm, timedout); SPI_STATISTICS_INCREMENT_FIELD(stats, @@ -1059,6 +1059,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr, dev_err(&msg->spi->dev, "SPI transfer timed out\n"); msg->status = -ETIMEDOUT; + ret = 0; } } else { if (xfer->len)
As long as sun4i/sun6i SPI drivers have overriden the default "wait for completion" procedure then we need to properly handle -ETIMEDOUT error from transfer_one(). Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com> --- drivers/spi/spi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)