[REPOST PATCH] spi: Unlock a spinlock before calling into the controller driver.
From: Bryan Freed bfr...@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 bfr...@chromium.org
Reviewed-by: Doug Anderson diand...@chromium.org
Signed-off-by: Doug Anderson diand...@chromium.org
Acked-by: Linus Walleij linus.wall...@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
--
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] spi: Unlock a spinlock before calling into the controller driver.
On Tue, Jun 26, 2012 at 1:07 AM, Doug Anderson diand...@chromium.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/ ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] spi: Unlock a spinlock before calling into the controller driver.
On Sat, Jun 23, 2012 at 7:53 AM, Bryan Freed bfr...@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 bfr...@chromium.org Yes, this looks correct to me! Good catch. Acked-by: Linus Walleij linus.wall...@linaro.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/ ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] spi: Unlock a spinlock before calling into the controller driver.
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 bfr...@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 bfr...@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
--
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/
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
[PATCH] spi: Unlock a spinlock before calling into the controller driver.
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 bfr...@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
--
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/
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
