lkml.org 
[lkml]   [2012]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] spi: Unlock a spinlock before calling into the controller driver.
    From
    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-06-25 02:21    [W:0.053 / U:0.480 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site