| Message ID | 1429084832-8953-1-git-send-email-huiquan.zhong@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 15, 2015 at 04:00:32PM +0800, Huiquan Zhong wrote: > master->pump_messages workqueue can be queued by spi_start_queue(), > __spi_queued_transfer(). > > there is one case that if one resume thread call spi_start_queue(), > and at the same time another spi_device thread call spi_queued_transfer() > to do spi transfer, then the first workqueue will start the spi transfer, > but the next workqueue queued before SPI transfer complete, will unusual > terminate spi transfer. I've applied this, it took a while mostly because it is very hard for me to understand your commit message - I'm not clear what mechanism will cause a problem for a running transfer ("unusual terminate spi transfer"). I can see we might end up with the work being added to the queue more often than is needed but not how that ends up causing anything other than a little wasted work. Your commit message suggests it's to do with multiple devices adding to the queue simultaneously but the queue access in __spi_queued_transfer() and __spi_pump_messages() all appears to be done with the queue lock held... I'm still not 100% clear that this is doing anything other than making the locking a bit more consistent.
Seems that issue can't reproduce in latest kernel since there is one patch (comments is 983aee5d7090cf12b624f18533777caa09d067b1) to check if the device is processing a message before idle.
So no need my patch with the latest kernel upstream. Sorry to disturb.
commit 983aee5d7090cf12b624f18533777caa09d067b1
Author: Mark Brown <broonie@kernel.org>
Date: Tue Dec 9 19:46:56 2014 +0000
spi: Check to see if the device is processing a message before we idle
cur_msg is updated under the queue lock and holds the message we are
currently processing. Since currently we only ever do removals in the
pump kthread it doesn't matter in what order we do things but we want
to be able to push things out from the submitting thread so pull the
check to see if we're currently handling a message before we check to
see if the queue is idle.
Thanks,
Huiquan
-----Original Message-----
From: Mark Brown [mailto:broonie@kernel.org]
Sent: Saturday, April 18, 2015 8:49 PM
To: Zhong, Huiquan
Cc: linux-spi@vger.kernel.org; huiquanz.zhong@intel.com
Subject: Re: [PATCH] spi: fix one potential spin_lock issue
On Wed, Apr 15, 2015 at 04:00:32PM +0800, Huiquan Zhong wrote:
> master->pump_messages workqueue can be queued by spi_start_queue(),
> __spi_queued_transfer().
>
> there is one case that if one resume thread call spi_start_queue(),
> and at the same time another spi_device thread call
> spi_queued_transfer() to do spi transfer, then the first workqueue
> will start the spi transfer, but the next workqueue queued before SPI
> transfer complete, will unusual terminate spi transfer.
I've applied this, it took a while mostly because it is very hard for me to understand your commit message - I'm not clear what mechanism will cause a problem for a running transfer ("unusual terminate spi transfer"). I can see we might end up with the work being added to the queue more often than is needed but not how that ends up causing anything other than a little wasted work.
Your commit message suggests it's to do with multiple devices adding to the queue simultaneously but the queue access in __spi_queued_transfer() and __spi_pump_messages() all appears to be done with the queue lock held...
I'm still not 100% clear that this is doing anything other than making the locking a bit more consistent.
--
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
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index d5d7d22..2bc387b 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1017,9 +1017,11 @@ static int spi_start_queue(struct spi_master *master) master->running = true; master->cur_msg = NULL; - spin_unlock_irqrestore(&master->queue_lock, flags); - queue_kthread_work(&master->kworker, &master->pump_messages); + if (!list_empty(&master->queue)) + queue_kthread_work(&master->kworker, &master->pump_messages); + + spin_unlock_irqrestore(&master->queue_lock, flags); return 0; }
master->pump_messages workqueue can be queued by spi_start_queue(), __spi_queued_transfer(). there is one case that if one resume thread call spi_start_queue(), and at the same time another spi_device thread call spi_queued_transfer() to do spi transfer, then the first workqueue will start the spi transfer, but the next workqueue queued before SPI transfer complete, will unusual terminate spi transfer. Add spin_lock protection to avoid this case. Signed-off-by: Huiquan Zhong <huiquan.zhong@intel.com> --- drivers/spi/spi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)