| Message ID | CAMuHMdUVmL5Zq8AYPVmUyWEBTzokSMV-Xg4uFzpxr7c2uSmU7g@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
--- 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) {