From: Mark Brown broonie@linaro.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@linaro.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);
Hi Mark,
On Thu, Jan 30, 2014 at 11:38 PM, Mark Brown broonie@kernel.org wrote:
From: Mark Brown broonie@linaro.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@linaro.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@linux-m68k.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
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.
On Fri, Jan 31, 2014 at 12:49 PM, Mark Brown broonie@kernel.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@linux-m68k.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
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.
Hi Mark,
On Fri, Jan 31, 2014 at 1:26 PM, Mark Brown broonie@kernel.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@linux-m68k.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
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@kernel.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.
linaro-kernel@lists.linaro.org
-
Geert Uytterhoeven -
Mark Brown