[PATCH] spi: fix one potential spin_lock issue

[PATCH] spi: fix one potential spin_lock issue

[Huiquan Zhong]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/spi.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

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;
 }
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: fix one potential spin_lock issue

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCH] spi: fix one potential spin_lock issue

[Zhong, Huiquan]

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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org] 
Sent: Saturday, April 18, 2015 8:49 PM
To: Zhong, Huiquan
Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; huiquanz.zhong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html