Message ID | 1457430543-15179-1-git-send-email-jonathanh@nvidia.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Adding Vignesh ... On 08/03/16 09:49, Jon Hunter wrote: > The function __spi_pump_messages() is called by spi_pump_messages() and > __spi_sync(). The function __spi_sync() has an argument 'bus_locked' > that indicates if it is called with the SPI bus mutex held or not. If > 'bus_locked' is false then __spi_sync() will acquire the mutex itself. > > Commit 556351f14e74 ("spi: introduce accelerated read support for spi > flash devices") made a change to acquire the SPI bus mutex within > __spi_pump_messages(). However, this change did not check to see if the > mutex is already held. If __spi_sync() is called with the mutex held > (ie. 'bus_locked' is true), then a deadlock occurs when > __spi_pump_messages() is called. > > Fix this deadlock by passing the 'bus_locked' state from __spi_sync() to > __spi_pump_messages() and only acquire the mutex if not already held. In > the case where __spi_pump_messages() is called from spi_pump_messages() > it is assumed that the mutex is not held and so call > __spi_pump_messages() with 'bus_locked' set to false. Finally, move the > unlocking of the mutex to the end of the __spi_pump_messages() function > to simplify the code and only call cond_resched() if there are no > errors. > > Fixes: 556351f14e74 ("spi: introduce accelerated read support for spi flash devices") > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > > This deadlock is seen on the Tegra124 Nyan Big chromebook and prevents > the board from booting -next. > > drivers/spi/spi.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index fe0196328aa0..e699fec9ddc5 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -1062,7 +1062,8 @@ 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) > +static void __spi_pump_messages(struct spi_master *master, bool in_kthread, > + bool bus_locked) > { > unsigned long flags; > bool was_busy = false; > @@ -1158,7 +1159,9 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread) > } > } > > - mutex_lock(&master->bus_lock_mutex); > + if (!bus_locked) > + mutex_lock(&master->bus_lock_mutex); > + > trace_spi_message_start(master->cur_msg); > > if (master->prepare_message) { > @@ -1168,8 +1171,7 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread) > "failed to prepare message: %d\n", ret); > master->cur_msg->status = ret; > spi_finalize_current_message(master); > - mutex_unlock(&master->bus_lock_mutex); > - return; > + goto out; > } > master->cur_msg_prepared = true; > } > @@ -1178,21 +1180,23 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread) > if (ret) { > master->cur_msg->status = ret; > spi_finalize_current_message(master); > - mutex_unlock(&master->bus_lock_mutex); > - return; > + goto out; > } > > ret = master->transfer_one_message(master, master->cur_msg); > if (ret) { > dev_err(&master->dev, > "failed to transfer one message from queue\n"); > - mutex_unlock(&master->bus_lock_mutex); > - return; > + goto out; > } > - mutex_unlock(&master->bus_lock_mutex); > + > +out: > + if (!bus_locked) > + mutex_unlock(&master->bus_lock_mutex); > > /* Prod the scheduler in case transfer_one() was busy waiting */ > - cond_resched(); > + if (!ret) > + cond_resched(); > } > > /** > @@ -1204,7 +1208,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); > + __spi_pump_messages(master, true, false); > } > > static int spi_init_queue(struct spi_master *master) > @@ -2814,7 +2818,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); > + __spi_pump_messages(master, false, bus_locked); > } > > wait_for_completion(&done); > -- 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
Hi Jon, [auto build test WARNING on spi/for-next] [also build test WARNING on next-20160308] [cannot apply to v4.5-rc7] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Jon-Hunter/spi-core-Fix-deadlock-when-sending-messages/20160308-175230 base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi for-next reproduce: make htmldocs All warnings (new ones prefixed by >>): include/linux/init.h:1: warning: no structured comments found kernel/sys.c:1: warning: no structured comments found drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found drivers/dma-buf/reservation.c:1: warning: no structured comments found include/linux/reservation.h:1: warning: no structured comments found include/linux/spi/spi.h:933: error: Cannot parse struct or union! >> drivers/spi/spi.c:1067: warning: No description found for parameter 'bus_locked' drivers/spi/spi.c:2358: warning: No description found for parameter 'gfp' >> drivers/spi/spi.c:1067: warning: No description found for parameter 'bus_locked' drivers/spi/spi.c:2358: warning: No description found for parameter 'gfp' vim +/bus_locked +1067 drivers/spi/spi.c b158935f7 Mark Brown 2013-10-05 1051 ffbbdd213 Linus Walleij 2012-02-22 1052 /** fc9e0f71f Mark Brown 2014-12-10 1053 * __spi_pump_messages - function which processes spi message queue fc9e0f71f Mark Brown 2014-12-10 1054 * @master: master to process queue for fc9e0f71f Mark Brown 2014-12-10 1055 * @in_kthread: true if we are in the context of the message pump thread ffbbdd213 Linus Walleij 2012-02-22 1056 * ffbbdd213 Linus Walleij 2012-02-22 1057 * This function checks if there is any spi message in the queue that ffbbdd213 Linus Walleij 2012-02-22 1058 * needs processing and if so call out to the driver to initialize hardware ffbbdd213 Linus Walleij 2012-02-22 1059 * and transfer each message. ffbbdd213 Linus Walleij 2012-02-22 1060 * 0461a4149 Mark Brown 2014-12-09 1061 * Note that it is called both from the kthread itself and also from 0461a4149 Mark Brown 2014-12-09 1062 * inside spi_sync(); the queue extraction handling at the top of the 0461a4149 Mark Brown 2014-12-09 1063 * function should deal with this safely. ffbbdd213 Linus Walleij 2012-02-22 1064 */ 1f9706090 Jon Hunter 2016-03-08 1065 static void __spi_pump_messages(struct spi_master *master, bool in_kthread, 1f9706090 Jon Hunter 2016-03-08 1066 bool bus_locked) ffbbdd213 Linus Walleij 2012-02-22 @1067 { ffbbdd213 Linus Walleij 2012-02-22 1068 unsigned long flags; ffbbdd213 Linus Walleij 2012-02-22 1069 bool was_busy = false; ffbbdd213 Linus Walleij 2012-02-22 1070 int ret; ffbbdd213 Linus Walleij 2012-02-22 1071 983aee5d7 Mark Brown 2014-12-09 1072 /* Lock queue */ ffbbdd213 Linus Walleij 2012-02-22 1073 spin_lock_irqsave(&master->queue_lock, flags); 983aee5d7 Mark Brown 2014-12-09 1074 983aee5d7 Mark Brown 2014-12-09 1075 /* Make sure we are not already running a message */ :::::: The code at line 1067 was first introduced by commit :::::: ffbbdd21329f3e15eeca6df2d4bc11c04d9d91c0 spi: create a message queueing infrastructure :::::: TO: Linus Walleij <linus.walleij@linaro.org> :::::: CC: Grant Likely <grant.likely@secretlab.ca> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index fe0196328aa0..e699fec9ddc5 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1062,7 +1062,8 @@ 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) +static void __spi_pump_messages(struct spi_master *master, bool in_kthread, + bool bus_locked) { unsigned long flags; bool was_busy = false; @@ -1158,7 +1159,9 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread) } } - mutex_lock(&master->bus_lock_mutex); + if (!bus_locked) + mutex_lock(&master->bus_lock_mutex); + trace_spi_message_start(master->cur_msg); if (master->prepare_message) { @@ -1168,8 +1171,7 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread) "failed to prepare message: %d\n", ret); master->cur_msg->status = ret; spi_finalize_current_message(master); - mutex_unlock(&master->bus_lock_mutex); - return; + goto out; } master->cur_msg_prepared = true; } @@ -1178,21 +1180,23 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread) if (ret) { master->cur_msg->status = ret; spi_finalize_current_message(master); - mutex_unlock(&master->bus_lock_mutex); - return; + goto out; } ret = master->transfer_one_message(master, master->cur_msg); if (ret) { dev_err(&master->dev, "failed to transfer one message from queue\n"); - mutex_unlock(&master->bus_lock_mutex); - return; + goto out; } - mutex_unlock(&master->bus_lock_mutex); + +out: + if (!bus_locked) + mutex_unlock(&master->bus_lock_mutex); /* Prod the scheduler in case transfer_one() was busy waiting */ - cond_resched(); + if (!ret) + cond_resched(); } /** @@ -1204,7 +1208,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); + __spi_pump_messages(master, true, false); } static int spi_init_queue(struct spi_master *master) @@ -2814,7 +2818,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); + __spi_pump_messages(master, false, bus_locked); } wait_for_completion(&done);
The function __spi_pump_messages() is called by spi_pump_messages() and __spi_sync(). The function __spi_sync() has an argument 'bus_locked' that indicates if it is called with the SPI bus mutex held or not. If 'bus_locked' is false then __spi_sync() will acquire the mutex itself. Commit 556351f14e74 ("spi: introduce accelerated read support for spi flash devices") made a change to acquire the SPI bus mutex within __spi_pump_messages(). However, this change did not check to see if the mutex is already held. If __spi_sync() is called with the mutex held (ie. 'bus_locked' is true), then a deadlock occurs when __spi_pump_messages() is called. Fix this deadlock by passing the 'bus_locked' state from __spi_sync() to __spi_pump_messages() and only acquire the mutex if not already held. In the case where __spi_pump_messages() is called from spi_pump_messages() it is assumed that the mutex is not held and so call __spi_pump_messages() with 'bus_locked' set to false. Finally, move the unlocking of the mutex to the end of the __spi_pump_messages() function to simplify the code and only call cond_resched() if there are no errors. Fixes: 556351f14e74 ("spi: introduce accelerated read support for spi flash devices") Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- This deadlock is seen on the Tegra124 Nyan Big chromebook and prevents the board from booting -next. drivers/spi/spi.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)