[PATCH] spi: Unlock a spinlock before calling into the controller driver.

[PATCH] spi: Unlock a spinlock before calling into the controller driver.

[Bryan Freed]

spi_pump_messages() calls into a controller driver with
unprepare_transfer_hardware() which is documented as "This may sleep".
As in the prepare_transfer_hardware() call below, we should release the
queue_lock spinlock before making the call.
Rework the logic a bit to hold queue_lock to protect the 'busy' flag,
then release it to call unprepare_transfer_hardware().

Signed-off-by: Bryan Freed <bfreed@chromium.org>

---
 drivers/spi/spi.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index fc0da39..f7f9df9 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -532,17 +532,16 @@ static void spi_pump_messages(struct kthread_work *work)
 	/* Lock queue and check for queue work */
 	spin_lock_irqsave(&master->queue_lock, flags);
 	if (list_empty(&master->queue) || !master->running) {
-		if (master->busy && master->unprepare_transfer_hardware) {
-			ret = master->unprepare_transfer_hardware(master);
-			if (ret) {
-				spin_unlock_irqrestore(&master->queue_lock, flags);
-				dev_err(&master->dev,
-					"failed to unprepare transfer hardware\n");
-				return;
-			}
+		if (!master->busy) {
+			spin_unlock_irqrestore(&master->queue_lock, flags);
+			return;
 		}
 		master->busy = false;
 		spin_unlock_irqrestore(&master->queue_lock, flags);
+		if (master->unprepare_transfer_hardware &&
+		    master->unprepare_transfer_hardware(master))
+			dev_err(&master->dev,
+				"failed to unprepare transfer hardware\n");
 		return;
 	}
 
-- 
1.7.7.3

Re: [PATCH] spi: Unlock a spinlock before calling into the controller driver.

[Olof Johansson]

Hi,

(Adding Linus Walleij and Mark Brown since they were the ones doing
the queue changes).

On Fri, Jun 22, 2012 at 4:53 PM, Bryan Freed <bfreed@chromium.org> wrote:
> spi_pump_messages() calls into a controller driver with
> unprepare_transfer_hardware() which is documented as "This may sleep".
> As in the prepare_transfer_hardware() call below, we should release the
> queue_lock spinlock before making the call.
> Rework the logic a bit to hold queue_lock to protect the 'busy' flag,
> then release it to call unprepare_transfer_hardware().
>
> Signed-off-by: Bryan Freed <bfreed@chromium.org>
>
> ---
>  drivers/spi/spi.c |   15 +++++++--------
>  1 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index fc0da39..f7f9df9 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -532,17 +532,16 @@ static void spi_pump_messages(struct kthread_work *work)
>        /* Lock queue and check for queue work */
>        spin_lock_irqsave(&master->queue_lock, flags);
>        if (list_empty(&master->queue) || !master->running) {
> -               if (master->busy && master->unprepare_transfer_hardware) {
> -                       ret = master->unprepare_transfer_hardware(master);
> -                       if (ret) {
> -                               spin_unlock_irqrestore(&master->queue_lock, flags);
> -                               dev_err(&master->dev,
> -                                       "failed to unprepare transfer hardware\n");
> -                               return;
> -                       }
> +               if (!master->busy) {
> +                       spin_unlock_irqrestore(&master->queue_lock, flags);
> +                       return;
>                }
>                master->busy = false;
>                spin_unlock_irqrestore(&master->queue_lock, flags);
> +               if (master->unprepare_transfer_hardware &&
> +                   master->unprepare_transfer_hardware(master))
> +                       dev_err(&master->dev,
> +                               "failed to unprepare transfer hardware\n");


I'm not sure this is safe to do. The lock is dropped for the prepare
side, but in that case we can be sure that the above code can't come
in and unprepare at the same time since there is per definition a
request on the queue at that time.

On the other hand, between the lock drop and the call to unprepare
above, another code path can come in and queue up a request and either
not do prepare properly, or there will be a prepare that races with
the unprepare.

It might make more sense to use a workqueue here and schedule a
unprepare call that way instead (and cancelling appropriately before
the prepare call if needed). I'm open for other suggestions as well.


-Olof

Re: [PATCH] spi: Unlock a spinlock before calling into the controller driver.

[Doug Anderson]

Olof,


On Sun, Jun 24, 2012 at 4:54 PM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:

> I'm not sure this is safe to do. The lock is dropped for the prepare
> side, but in that case we can be sure that the above code can't come
> in and unprepare at the same time since there is per definition a
> request on the queue at that time.


I spent a bunch of time staring at the code and I'm pretty sure that
there's no race and that the code is safe (if not super obvious).

Specifically there should be only one instance of spi_pump_messages()
running at a time per master.  That's because it's a kthread work
function.  ...so we can't possibly get a prepare in the middle of the
unprepare when prepare is called because the only caller to
prepare/unprepare is spi_pump_messages().


I can't comment on whether it's better to do something like add a
workqueue (which might be more obvious / less fragile) or just to add
a comment.  I will let others comment on that.  :)

-Doug

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

Re: [PATCH] spi: Unlock a spinlock before calling into the controller driver.

[Linus Walleij]

On Tue, Jun 26, 2012 at 1:07 AM, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:

> Specifically there should be only one instance of spi_pump_messages()
> running at a time per master.  That's because it's a kthread work
> function.  ...so we can't possibly get a prepare in the middle of the
> unprepare when prepare is called because the only caller to
> prepare/unprepare is spi_pump_messages().

Yes that's how the message pump is designed.

> I can't comment on whether it's better to do something like add a
> workqueue (which might be more obvious / less fragile) or just to add
> a comment.  I will let others comment on that.  :)

The message pump initially used a workqueue, but was converted
to a kthread because we needed to push the queue to run as
realtime for some important low-latency workloads across
SPI. The code is basically a tweaked workqueue if you dive down
in the implementation.

Yours,
Linus Walleij

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

Re: [PATCH] spi: Unlock a spinlock before calling into the controller driver.

[Linus Walleij]

On Sat, Jun 23, 2012 at 7:53 AM, Bryan Freed <bfreed-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:

> spi_pump_messages() calls into a controller driver with
> unprepare_transfer_hardware() which is documented as "This may sleep".
> As in the prepare_transfer_hardware() call below, we should release the
> queue_lock spinlock before making the call.
> Rework the logic a bit to hold queue_lock to protect the 'busy' flag,
> then release it to call unprepare_transfer_hardware().
>
> Signed-off-by: Bryan Freed <bfreed-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Yes, this looks correct to me! Good catch.
Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Yours,
Linus Walleij

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

[REPOST PATCH] spi: Unlock a spinlock before calling into the controller driver.

[Doug Anderson]

From: Bryan Freed <bfreed@chromium.org>

spi_pump_messages() calls into a controller driver with
unprepare_transfer_hardware() which is documented as "This may sleep".
As in the prepare_transfer_hardware() call below, we should release the
queue_lock spinlock before making the call.
Rework the logic a bit to hold queue_lock to protect the 'busy' flag,
then release it to call unprepare_transfer_hardware().

Signed-off-by: Bryan Freed <bfreed@chromium.org>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Doug Anderson <dianders@chromium.org>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
During a rebase we noticed that this old patch never actually landed
anywhere.  I haven't gone through an reproduced the original bug on
ToT but I believe that this patch is still required.  Perhaps it could
land somewhere?  ;)  Thanks!

 drivers/spi/spi.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index f996c60..5b96250 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -543,17 +543,16 @@ static void spi_pump_messages(struct kthread_work *work)
 	/* Lock queue and check for queue work */
 	spin_lock_irqsave(&master->queue_lock, flags);
 	if (list_empty(&master->queue) || !master->running) {
-		if (master->busy && master->unprepare_transfer_hardware) {
-			ret = master->unprepare_transfer_hardware(master);
-			if (ret) {
-				spin_unlock_irqrestore(&master->queue_lock, flags);
-				dev_err(&master->dev,
-					"failed to unprepare transfer hardware\n");
-				return;
-			}
+		if (!master->busy) {
+			spin_unlock_irqrestore(&master->queue_lock, flags);
+			return;
 		}
 		master->busy = false;
 		spin_unlock_irqrestore(&master->queue_lock, flags);
+		if (master->unprepare_transfer_hardware &&
+		    master->unprepare_transfer_hardware(master))
+			dev_err(&master->dev,
+				"failed to unprepare transfer hardware\n");
 		return;
 	}
 
-- 
1.8.1.3

Re: [REPOST PATCH] spi: Unlock a spinlock before calling into the controller driver.

[Mark Brown]

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

On Wed, Mar 13, 2013 at 11:17:40AM -0700, Doug Anderson wrote:
> From: Bryan Freed <bfreed@chromium.org>
> 
> spi_pump_messages() calls into a controller driver with
> unprepare_transfer_hardware() which is documented as "This may sleep".
> As in the prepare_transfer_hardware() call below, we should release the
> queue_lock spinlock before making the call.
> Rework the logic a bit to hold queue_lock to protect the 'busy' flag,
> then release it to call unprepare_transfer_hardware().

Applied, thanks.  However...

>  		spin_unlock_irqrestore(&master->queue_lock, flags);
> +		if (master->unprepare_transfer_hardware &&
> +		    master->unprepare_transfer_hardware(master))
> +			dev_err(&master->dev,
> +				"failed to unprepare transfer hardware\n");

...it'd be nicer to pay attention to and log the error code if we fail
to unprepare.

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