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 <[email protected]>
---
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
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 <[email protected]> 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 <[email protected]>
>
> ---
> ?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
Olof,
On Sun, Jun 24, 2012 at 4:54 PM, Olof Johansson <[email protected]> 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
On Tue, Jun 26, 2012 at 1:07 AM, Doug Anderson <[email protected]> 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
On Sat, Jun 23, 2012 at 7:53 AM, Bryan Freed <[email protected]> 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 <[email protected]>
Yes, this looks correct to me! Good catch.
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
From: Bryan Freed <[email protected]>
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 <[email protected]>
Reviewed-by: Doug Anderson <[email protected]>
Signed-off-by: Doug Anderson <[email protected]>
Acked-by: Linus Walleij <[email protected]>
---
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
On Wed, Mar 13, 2013 at 11:17:40AM -0700, Doug Anderson wrote:
> From: Bryan Freed <[email protected]>
>
> 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.