[PATCH] spi: Add a timeout when waiting for transfers

[PATCH] spi: Add a timeout when waiting for transfers

[Mark Brown]

From: Mark Brown <broonie-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Don't wait indefinitely for transfers to complete but time out after 10ms
more than we expect the transfer to take on the wire.

Signed-off-by: Mark Brown <broonie-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 7f23cf9afa79..1826a50c2aaf 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -710,6 +710,7 @@ static int spi_transfer_one_message(struct spi_master *master,
 	bool cur_cs = true;
 	bool keep_cs = false;
 	int ret = 0;
+	int ms = 1;
 
 	spi_set_cs(msg->spi, true);
 
@@ -727,7 +728,16 @@ static int spi_transfer_one_message(struct spi_master *master,
 
 		if (ret > 0) {
 			ret = 0;
-			wait_for_completion(&master->xfer_completion);
+			ms = xfer->len * 8 * 1000 / xfer->speed_hz;
+			ms += 10; /* some tolerance */
+
+			ms = wait_for_completion_timeout(&master->xfer_completion,
+							 msecs_to_jiffies(ms));
+		}
+
+		if (ms == 0) {
+			dev_err(&msg->spi->dev, "SPI transfer timed out\n");
+			msg->status = -ETIMEDOUT;
 		}
 
 		trace_spi_transfer_stop(msg, xfer);
-- 
1.9.rc1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Add a timeout when waiting for transfers

[Geert Uytterhoeven]

Hi Mark,

On Thu, Jan 30, 2014 at 11:38 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> From: Mark Brown <broonie-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> Don't wait indefinitely for transfers to complete but time out after 10ms
> more than we expect the transfer to take on the wire.
>
> Signed-off-by: Mark Brown <broonie-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/spi/spi.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 7f23cf9afa79..1826a50c2aaf 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -710,6 +710,7 @@ static int spi_transfer_one_message(struct spi_master *master,
>         bool cur_cs = true;
>         bool keep_cs = false;
>         int ret = 0;
> +       int ms = 1;
>
>         spi_set_cs(msg->spi, true);
>
> @@ -727,7 +728,16 @@ static int spi_transfer_one_message(struct spi_master *master,
>
>                 if (ret > 0) {
>                         ret = 0;
> -                       wait_for_completion(&master->xfer_completion);
> +                       ms = xfer->len * 8 * 1000 / xfer->speed_hz;
> +                       ms += 10; /* some tolerance */
> +
> +                       ms = wait_for_completion_timeout(&master->xfer_completion,
> +                                                        msecs_to_jiffies(ms));
> +               }
> +
> +               if (ms == 0) {
> +                       dev_err(&msg->spi->dev, "SPI transfer timed out\n");
> +                       msg->status = -ETIMEDOUT;
>                 }
>
>                 trace_spi_transfer_stop(msg, xfer);

What if it still completes in the driver a little bit later? The
driver will also call
spi_finalize_current_message(), and we'll get a NULL pointer dereference
as master->cur_msg is now NULL.

So I think we need a check like (whitespace damaged) below:

--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -829,6 +829,9 @@ void spi_finalize_current_message(struct spi_master *master)
          queue_kthread_work(&master->kworker, &master->pump_messages);
          spin_unlock_irqrestore(&master->queue_lock, flags);

+         if (!mesg)
+                 return NULL;
+
          if (master->cur_msg_prepared && master->unprepare_message) {
                  ret = master->unprepare_message(master, mesg);
          if (ret) {

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Add a timeout when waiting for transfers

[Mark Brown]

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

On Fri, Jan 31, 2014 at 08:49:45AM +0100, Geert Uytterhoeven wrote:

> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -829,6 +829,9 @@ void spi_finalize_current_message(struct spi_master *master)
>           queue_kthread_work(&master->kworker, &master->pump_messages);
>           spin_unlock_irqrestore(&master->queue_lock, flags);
> 
> +         if (!mesg)
> +                 return NULL;
> +

Note that this is for transfers, not for messages, and we don't change
the completion path for the message here so I'm not sure why the message
would be affected?  That said there is definitely an issue here - we
should probably be adding some sort of error teardown callback to try to
stop the hardware if we do hit a timeout.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Add a timeout when waiting for transfers

[Geert Uytterhoeven]

On Fri, Jan 31, 2014 at 12:49 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Jan 31, 2014 at 08:49:45AM +0100, Geert Uytterhoeven wrote:
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -829,6 +829,9 @@ void spi_finalize_current_message(struct spi_master *master)
>>           queue_kthread_work(&master->kworker, &master->pump_messages);
>>           spin_unlock_irqrestore(&master->queue_lock, flags);
>>
>> +         if (!mesg)
>> +                 return NULL;
>> +
>
> Note that this is for transfers, not for messages, and we don't change
> the completion path for the message here so I'm not sure why the message
> would be affected?  That said there is definitely an issue here - we
> should probably be adding some sort of error teardown callback to try to
> stop the hardware if we do hit a timeout.

RIght. Sorry, I mixed them up.

One other thing: I haven't tried your patch yet, but I'm afraid the 10 ms
may be too small.

E.g. with PIO-based RSPI I don't get more than 2 Mbps, even though
spi-max-frequency = <30000000>, due to the PIO and interrupt overhead.
Hence a 1 MiB read would take ca. 4s, while your timeout would be 300 ms.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Add a timeout when waiting for transfers

[Mark Brown]

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

On Fri, Jan 31, 2014 at 01:00:31PM +0100, Geert Uytterhoeven wrote:

> One other thing: I haven't tried your patch yet, but I'm afraid the 10 ms
> may be too small.

> E.g. with PIO-based RSPI I don't get more than 2 Mbps, even though
> spi-max-frequency = <30000000>, due to the PIO and interrupt overhead.
> Hence a 1 MiB read would take ca. 4s, while your timeout would be 300 ms.

Hrm, I wouldn't have expected something doing PIO in more than one burst
to be letting the transfer run in the background.  Though I suppose that
might make sense in some situations...

I was wondering if that was cutting it a bit fine but more for scheduler
reasons, it's what the s3c64xx driver has been using for a while without
complaints but may not translate so well with greater exposure.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Add a timeout when waiting for transfers

[Geert Uytterhoeven]

Hi Mark,

On Fri, Jan 31, 2014 at 1:26 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Jan 31, 2014 at 01:00:31PM +0100, Geert Uytterhoeven wrote:
>> One other thing: I haven't tried your patch yet, but I'm afraid the 10 ms
>> may be too small.
>
>> E.g. with PIO-based RSPI I don't get more than 2 Mbps, even though
>> spi-max-frequency = <30000000>, due to the PIO and interrupt overhead.
>> Hence a 1 MiB read would take ca. 4s, while your timeout would be 300 ms.
>
> Hrm, I wouldn't have expected something doing PIO in more than one burst
> to be letting the transfer run in the background.  Though I suppose that
> might make sense in some situations...

While I've been thinking about moving more code to the interrupt handler,
the current RSPI transfer_one() is indeed still synchronous, so the timeout
doesn't apply yet.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Add a timeout when waiting for transfers

[Mark Brown]

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

On Mon, Feb 03, 2014 at 09:55:33AM +0100, Geert Uytterhoeven wrote:
> On Fri, Jan 31, 2014 at 1:26 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > to be letting the transfer run in the background.  Though I suppose that
> > might make sense in some situations...

> While I've been thinking about moving more code to the interrupt handler,
> the current RSPI transfer_one() is indeed still synchronous, so the timeout
> doesn't apply yet.

OK, that's interesting.  Actually one of the things that this
refactoring into the core is driving at is that hopefully we'll be able
to deploy some of the optimisation techniques like that in the core
rather than having individual drivers open coding them - this should on
average increase performance.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]