diff mbox

[v2,1/6] spi: core: handle timeout error from transfer_one()

Message ID 20180403152905.1524-2-ssuloev@orpaltech.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sergey Suloev April 3, 2018, 3:29 p.m. UTC
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(-)

Comments

Mark Brown April 3, 2018, 3:52 p.m. UTC | #1
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?
Sergey Suloev April 3, 2018, 4 p.m. UTC | #2
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.
Mark Brown April 3, 2018, 4:18 p.m. UTC | #3
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.
Sergey Suloev April 3, 2018, 4:24 p.m. UTC | #4
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.
Maxime Ripard April 4, 2018, 7:08 a.m. UTC | #5
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
Mark Brown April 4, 2018, 10:42 a.m. UTC | #6
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.
Sergey Suloev April 4, 2018, 7:19 p.m. UTC | #7
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 mbox

Patch

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)