| Message ID | 1469142737-2814-1-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 22, 2016 at 12:12:17AM +0100, Mark Brown wrote: > The current SPI code attempts to use bus_lock_mutex for two purposes. One > is to implement spi_bus_lock() which grants exclusive access to the bus. > The other is to serialize access to the physical hardware. This duplicate > purpose causes confusion which leads to cases where access is not locked > when a caller holds the bus lock mutex. Fix this by splitting out the I/O > functionality into a new io_mutex. > > This means taking both mutexes in the DMA path, replacing the existing > mutex with the new I/O one in the message pump (the mutex now always > being taken in the message pump) and taking the bus lock mutex in > spi_sync(), allowing __spi_sync() to have no mutex handling. > > While we're at it hoist the mutex further up the message pump before we > power up the device so that all power up/down of the block is covered by > it and there are no races with in-line pumping of messages. > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > > *Completely* untested, I've not even tried to run this yet - just > quickly throwing it out for testing. > > drivers/spi/spi.c | 38 ++++++++++++++++++-------------------- > include/linux/spi/spi.h | 6 +++++- > 2 files changed, 23 insertions(+), 21 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index be96fcd072e4..fd32dcf599a8 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -1088,7 +1088,6 @@ EXPORT_SYMBOL_GPL(spi_finalize_current_transfer); > * __spi_pump_messages - function which processes spi message queue > * @master: master to process queue for > * @in_kthread: true if we are in the context of the message pump thread > - * @bus_locked: true if the bus mutex is held when calling this function > * > * This function checks if there is any spi message in the queue that > * needs processing and if so call out to the driver to initialize hardware > @@ -1098,8 +1097,7 @@ EXPORT_SYMBOL_GPL(spi_finalize_current_transfer); > * inside spi_sync(); the queue extraction handling at the top of the > * function should deal with this safely. > */ > -static void __spi_pump_messages(struct spi_master *master, bool in_kthread, > - bool bus_locked) > +static void __spi_pump_messages(struct spi_master *master, bool in_kthread) > { > unsigned long flags; > bool was_busy = false; > @@ -1171,6 +1169,8 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread, > master->busy = true; > spin_unlock_irqrestore(&master->queue_lock, flags); > > + mutex_lock(&master->io_mutex); > + > if (!was_busy && master->auto_runtime_pm) { > ret = pm_runtime_get_sync(master->dev.parent); > if (ret < 0) { > @@ -1195,9 +1195,6 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread, > } > } > > - if (!bus_locked) > - mutex_lock(&master->bus_lock_mutex); > - > trace_spi_message_start(master->cur_msg); > > if (master->prepare_message) { > @@ -1227,8 +1224,7 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread, > } > > out: > - if (!bus_locked) > - mutex_unlock(&master->bus_lock_mutex); > + mutex_unlock(&master->io_mutex); > > /* Prod the scheduler in case transfer_one() was busy waiting */ > if (!ret) > @@ -1244,7 +1240,7 @@ static void spi_pump_messages(struct kthread_work *work) > struct spi_master *master = > container_of(work, struct spi_master, pump_messages); > > - __spi_pump_messages(master, true, master->bus_lock_flag); > + __spi_pump_messages(master, true); > } > > static int spi_init_queue(struct spi_master *master) > @@ -1917,6 +1913,7 @@ int spi_register_master(struct spi_master *master) > spin_lock_init(&master->queue_lock); > spin_lock_init(&master->bus_lock_spinlock); > mutex_init(&master->bus_lock_mutex); > + mutex_init(&master->io_mutex); > master->bus_lock_flag = 0; > init_completion(&master->xfer_completion); > if (!master->max_dma_len) > @@ -2800,6 +2797,7 @@ int spi_flash_read(struct spi_device *spi, > } > > mutex_lock(&master->bus_lock_mutex); > + mutex_lock(&master->io_mutex); > if (master->dma_rx) { > rx_dev = master->dma_rx->device->dev; > ret = spi_map_buf(master, rx_dev, &msg->rx_sg, > @@ -2812,6 +2810,7 @@ int spi_flash_read(struct spi_device *spi, > if (msg->cur_msg_mapped) > spi_unmap_buf(master, rx_dev, &msg->rx_sg, > DMA_FROM_DEVICE); > + mutex_unlock(&master->io_mutex); > mutex_unlock(&master->bus_lock_mutex); > > if (master->auto_runtime_pm) This chunk does not apply cleanly to upstream kernel; I assume it depends on other changes in your development branch. After compensating for that, it seems to be working correctly. Aside from the point that it works, I like this patch a lot because it clarifies what's going on with the code and what the locks are used for. It took a lot of effort to even start to make sense of what was wrong with the old locking, whereas now it's pretty self-explanatory. Rich -- 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, Jul 22, 2016 at 12:12:17AM +0100, Mark Brown wrote: > The current SPI code attempts to use bus_lock_mutex for two purposes. One > is to implement spi_bus_lock() which grants exclusive access to the bus. > The other is to serialize access to the physical hardware. This duplicate > purpose causes confusion which leads to cases where access is not locked > when a caller holds the bus lock mutex. Fix this by splitting out the I/O > functionality into a new io_mutex. > > This means taking both mutexes in the DMA path, replacing the existing > mutex with the new I/O one in the message pump (the mutex now always > being taken in the message pump) and taking the bus lock mutex in > spi_sync(), allowing __spi_sync() to have no mutex handling. > > While we're at it hoist the mutex further up the message pump before we > power up the device so that all power up/down of the block is covered by > it and there are no races with in-line pumping of messages. > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > > *Completely* untested, I've not even tried to run this yet - just > quickly throwing it out for testing. I saw it's in the asoc tree now -- will this be making it upstream for this merge window? Hoping so. The issue it fixes is a nasty data corruption regression that's been around for a while now and although we can keep using out-of-tree patches or workarounds it would be nice to have it fixed in unpatched kernels. Thanks again for the quick patch and for doing it in a way that makes the code clearer & more accessible to developers reading and trying to understand it. Rich > drivers/spi/spi.c | 38 ++++++++++++++++++-------------------- > include/linux/spi/spi.h | 6 +++++- > 2 files changed, 23 insertions(+), 21 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index be96fcd072e4..fd32dcf599a8 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -1088,7 +1088,6 @@ EXPORT_SYMBOL_GPL(spi_finalize_current_transfer); > * __spi_pump_messages - function which processes spi message queue > * @master: master to process queue for > * @in_kthread: true if we are in the context of the message pump thread > - * @bus_locked: true if the bus mutex is held when calling this function > * > * This function checks if there is any spi message in the queue that > * needs processing and if so call out to the driver to initialize hardware > @@ -1098,8 +1097,7 @@ EXPORT_SYMBOL_GPL(spi_finalize_current_transfer); > * inside spi_sync(); the queue extraction handling at the top of the > * function should deal with this safely. > */ > -static void __spi_pump_messages(struct spi_master *master, bool in_kthread, > - bool bus_locked) > +static void __spi_pump_messages(struct spi_master *master, bool in_kthread) > { > unsigned long flags; > bool was_busy = false; > @@ -1171,6 +1169,8 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread, > master->busy = true; > spin_unlock_irqrestore(&master->queue_lock, flags); > > + mutex_lock(&master->io_mutex); > + > if (!was_busy && master->auto_runtime_pm) { > ret = pm_runtime_get_sync(master->dev.parent); > if (ret < 0) { > @@ -1195,9 +1195,6 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread, > } > } > > - if (!bus_locked) > - mutex_lock(&master->bus_lock_mutex); > - > trace_spi_message_start(master->cur_msg); > > if (master->prepare_message) { > @@ -1227,8 +1224,7 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread, > } > > out: > - if (!bus_locked) > - mutex_unlock(&master->bus_lock_mutex); > + mutex_unlock(&master->io_mutex); > > /* Prod the scheduler in case transfer_one() was busy waiting */ > if (!ret) > @@ -1244,7 +1240,7 @@ static void spi_pump_messages(struct kthread_work *work) > struct spi_master *master = > container_of(work, struct spi_master, pump_messages); > > - __spi_pump_messages(master, true, master->bus_lock_flag); > + __spi_pump_messages(master, true); > } > > static int spi_init_queue(struct spi_master *master) > @@ -1917,6 +1913,7 @@ int spi_register_master(struct spi_master *master) > spin_lock_init(&master->queue_lock); > spin_lock_init(&master->bus_lock_spinlock); > mutex_init(&master->bus_lock_mutex); > + mutex_init(&master->io_mutex); > master->bus_lock_flag = 0; > init_completion(&master->xfer_completion); > if (!master->max_dma_len) > @@ -2800,6 +2797,7 @@ int spi_flash_read(struct spi_device *spi, > } > > mutex_lock(&master->bus_lock_mutex); > + mutex_lock(&master->io_mutex); > if (master->dma_rx) { > rx_dev = master->dma_rx->device->dev; > ret = spi_map_buf(master, rx_dev, &msg->rx_sg, > @@ -2812,6 +2810,7 @@ int spi_flash_read(struct spi_device *spi, > if (msg->cur_msg_mapped) > spi_unmap_buf(master, rx_dev, &msg->rx_sg, > DMA_FROM_DEVICE); > + mutex_unlock(&master->io_mutex); > mutex_unlock(&master->bus_lock_mutex); > > if (master->auto_runtime_pm) > @@ -2833,8 +2832,7 @@ static void spi_complete(void *arg) > complete(arg); > } > > -static int __spi_sync(struct spi_device *spi, struct spi_message *message, > - int bus_locked) > +static int __spi_sync(struct spi_device *spi, struct spi_message *message) > { > DECLARE_COMPLETION_ONSTACK(done); > int status; > @@ -2852,9 +2850,6 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message, > SPI_STATISTICS_INCREMENT_FIELD(&master->statistics, spi_sync); > SPI_STATISTICS_INCREMENT_FIELD(&spi->statistics, spi_sync); > > - if (!bus_locked) > - mutex_lock(&master->bus_lock_mutex); > - > /* If we're not using the legacy transfer method then we will > * try to transfer in the calling context so special case. > * This code would be less tricky if we could remove the > @@ -2872,9 +2867,6 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message, > status = spi_async_locked(spi, message); > } > > - if (!bus_locked) > - mutex_unlock(&master->bus_lock_mutex); > - > if (status == 0) { > /* Push out the messages in the calling context if we > * can. > @@ -2884,7 +2876,7 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message, > spi_sync_immediate); > SPI_STATISTICS_INCREMENT_FIELD(&spi->statistics, > spi_sync_immediate); > - __spi_pump_messages(master, false, bus_locked); > + __spi_pump_messages(master, false); > } > > wait_for_completion(&done); > @@ -2917,7 +2909,13 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message, > */ > int spi_sync(struct spi_device *spi, struct spi_message *message) > { > - return __spi_sync(spi, message, spi->master->bus_lock_flag); > + int ret; > + > + mutex_lock(&spi->master->bus_lock_mutex); > + ret = __spi_sync(spi, message); > + mutex_unlock(&spi->master->bus_lock_mutex); > + > + return ret; > } > EXPORT_SYMBOL_GPL(spi_sync); > > @@ -2939,7 +2937,7 @@ EXPORT_SYMBOL_GPL(spi_sync); > */ > int spi_sync_locked(struct spi_device *spi, struct spi_message *message) > { > - return __spi_sync(spi, message, 1); > + return __spi_sync(spi, message); > } > EXPORT_SYMBOL_GPL(spi_sync_locked); > > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index ab7215c63873..25d6e83e5afe 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -312,8 +312,9 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) > * @flags: other constraints relevant to this driver > * @max_transfer_size: function that returns the max transfer size for > * a &spi_device; may be %NULL, so the default %SIZE_MAX will be used. > + * @io_mutex: mutex for physical bus access > * @bus_lock_spinlock: spinlock for SPI bus locking > - * @bus_lock_mutex: mutex for SPI bus locking > + * @bus_lock_mutex: mutex for exclusion of multiple callers > * @bus_lock_flag: indicates that the SPI bus is locked for exclusive use > * @setup: updates the device mode and clocking records used by a > * device's SPI controller; protocol code may call this. This > @@ -447,6 +448,9 @@ struct spi_master { > */ > size_t (*max_transfer_size)(struct spi_device *spi); > > + /* I/O mutex */ > + struct mutex io_mutex; > + > /* lock and mutex for SPI bus locking */ > spinlock_t bus_lock_spinlock; > struct mutex bus_lock_mutex; > -- > 2.8.1 -- 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 Wed, Jul 27, 2016 at 10:23:14PM -0400, Rich Felker wrote: > I saw it's in the asoc tree now -- will this be making it upstream for > this merge window? Hoping so. The issue it fixes is a nasty data > corruption regression that's been around for a while now and although > we can keep using out-of-tree patches or workarounds it would be nice > to have it fixed in unpatched kernels. Thanks again for the quick > patch and for doing it in a way that makes the code clearer & more > accessible to developers reading and trying to understand it. It was sent to Linus via the SPI tree earlier this week, the message about ASoC was a result of a misfiring script.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index be96fcd072e4..fd32dcf599a8 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1088,7 +1088,6 @@ EXPORT_SYMBOL_GPL(spi_finalize_current_transfer); * __spi_pump_messages - function which processes spi message queue * @master: master to process queue for * @in_kthread: true if we are in the context of the message pump thread - * @bus_locked: true if the bus mutex is held when calling this function * * This function checks if there is any spi message in the queue that * needs processing and if so call out to the driver to initialize hardware @@ -1098,8 +1097,7 @@ EXPORT_SYMBOL_GPL(spi_finalize_current_transfer); * inside spi_sync(); the queue extraction handling at the top of the * function should deal with this safely. */ -static void __spi_pump_messages(struct spi_master *master, bool in_kthread, - bool bus_locked) +static void __spi_pump_messages(struct spi_master *master, bool in_kthread) { unsigned long flags; bool was_busy = false; @@ -1171,6 +1169,8 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread, master->busy = true; spin_unlock_irqrestore(&master->queue_lock, flags); + mutex_lock(&master->io_mutex); + if (!was_busy && master->auto_runtime_pm) { ret = pm_runtime_get_sync(master->dev.parent); if (ret < 0) { @@ -1195,9 +1195,6 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread, } } - if (!bus_locked) - mutex_lock(&master->bus_lock_mutex); - trace_spi_message_start(master->cur_msg); if (master->prepare_message) { @@ -1227,8 +1224,7 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread, } out: - if (!bus_locked) - mutex_unlock(&master->bus_lock_mutex); + mutex_unlock(&master->io_mutex); /* Prod the scheduler in case transfer_one() was busy waiting */ if (!ret) @@ -1244,7 +1240,7 @@ static void spi_pump_messages(struct kthread_work *work) struct spi_master *master = container_of(work, struct spi_master, pump_messages); - __spi_pump_messages(master, true, master->bus_lock_flag); + __spi_pump_messages(master, true); } static int spi_init_queue(struct spi_master *master) @@ -1917,6 +1913,7 @@ int spi_register_master(struct spi_master *master) spin_lock_init(&master->queue_lock); spin_lock_init(&master->bus_lock_spinlock); mutex_init(&master->bus_lock_mutex); + mutex_init(&master->io_mutex); master->bus_lock_flag = 0; init_completion(&master->xfer_completion); if (!master->max_dma_len) @@ -2800,6 +2797,7 @@ int spi_flash_read(struct spi_device *spi, } mutex_lock(&master->bus_lock_mutex); + mutex_lock(&master->io_mutex); if (master->dma_rx) { rx_dev = master->dma_rx->device->dev; ret = spi_map_buf(master, rx_dev, &msg->rx_sg, @@ -2812,6 +2810,7 @@ int spi_flash_read(struct spi_device *spi, if (msg->cur_msg_mapped) spi_unmap_buf(master, rx_dev, &msg->rx_sg, DMA_FROM_DEVICE); + mutex_unlock(&master->io_mutex); mutex_unlock(&master->bus_lock_mutex); if (master->auto_runtime_pm) @@ -2833,8 +2832,7 @@ static void spi_complete(void *arg) complete(arg); } -static int __spi_sync(struct spi_device *spi, struct spi_message *message, - int bus_locked) +static int __spi_sync(struct spi_device *spi, struct spi_message *message) { DECLARE_COMPLETION_ONSTACK(done); int status; @@ -2852,9 +2850,6 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message, SPI_STATISTICS_INCREMENT_FIELD(&master->statistics, spi_sync); SPI_STATISTICS_INCREMENT_FIELD(&spi->statistics, spi_sync); - if (!bus_locked) - mutex_lock(&master->bus_lock_mutex); - /* If we're not using the legacy transfer method then we will * try to transfer in the calling context so special case. * This code would be less tricky if we could remove the @@ -2872,9 +2867,6 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message, status = spi_async_locked(spi, message); } - if (!bus_locked) - mutex_unlock(&master->bus_lock_mutex); - if (status == 0) { /* Push out the messages in the calling context if we * can. @@ -2884,7 +2876,7 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message, spi_sync_immediate); SPI_STATISTICS_INCREMENT_FIELD(&spi->statistics, spi_sync_immediate); - __spi_pump_messages(master, false, bus_locked); + __spi_pump_messages(master, false); } wait_for_completion(&done); @@ -2917,7 +2909,13 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message, */ int spi_sync(struct spi_device *spi, struct spi_message *message) { - return __spi_sync(spi, message, spi->master->bus_lock_flag); + int ret; + + mutex_lock(&spi->master->bus_lock_mutex); + ret = __spi_sync(spi, message); + mutex_unlock(&spi->master->bus_lock_mutex); + + return ret; } EXPORT_SYMBOL_GPL(spi_sync); @@ -2939,7 +2937,7 @@ EXPORT_SYMBOL_GPL(spi_sync); */ int spi_sync_locked(struct spi_device *spi, struct spi_message *message) { - return __spi_sync(spi, message, 1); + return __spi_sync(spi, message); } EXPORT_SYMBOL_GPL(spi_sync_locked); diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index ab7215c63873..25d6e83e5afe 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -312,8 +312,9 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) * @flags: other constraints relevant to this driver * @max_transfer_size: function that returns the max transfer size for * a &spi_device; may be %NULL, so the default %SIZE_MAX will be used. + * @io_mutex: mutex for physical bus access * @bus_lock_spinlock: spinlock for SPI bus locking - * @bus_lock_mutex: mutex for SPI bus locking + * @bus_lock_mutex: mutex for exclusion of multiple callers * @bus_lock_flag: indicates that the SPI bus is locked for exclusive use * @setup: updates the device mode and clocking records used by a * device's SPI controller; protocol code may call this. This @@ -447,6 +448,9 @@ struct spi_master { */ size_t (*max_transfer_size)(struct spi_device *spi); + /* I/O mutex */ + struct mutex io_mutex; + /* lock and mutex for SPI bus locking */ spinlock_t bus_lock_spinlock; struct mutex bus_lock_mutex;
The current SPI code attempts to use bus_lock_mutex for two purposes. One is to implement spi_bus_lock() which grants exclusive access to the bus. The other is to serialize access to the physical hardware. This duplicate purpose causes confusion which leads to cases where access is not locked when a caller holds the bus lock mutex. Fix this by splitting out the I/O functionality into a new io_mutex. This means taking both mutexes in the DMA path, replacing the existing mutex with the new I/O one in the message pump (the mutex now always being taken in the message pump) and taking the bus lock mutex in spi_sync(), allowing __spi_sync() to have no mutex handling. While we're at it hoist the mutex further up the message pump before we power up the device so that all power up/down of the block is covered by it and there are no races with in-line pumping of messages. Signed-off-by: Mark Brown <broonie@kernel.org> --- *Completely* untested, I've not even tried to run this yet - just quickly throwing it out for testing. drivers/spi/spi.c | 38 ++++++++++++++++++-------------------- include/linux/spi/spi.h | 6 +++++- 2 files changed, 23 insertions(+), 21 deletions(-)