lkml.org 
[lkml]   [2010]   [Mar]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 1/6] hid: new driver for PicoLCD device
    On Sun, 21 March 2010 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
    > On Sun, Mar 21, 2010 at 05:37:37PM +0100, Bruno Prémont wrote:
    > > On Sat, 20 March 2010 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
    > > > > + usbhid_submit_report(data->hdev, report, USB_DIR_OUT);
    > > > > + complete_all(&data->ready);
    > > > > + INIT_COMPLETION(data->ready);
    > > >
    > > > Umm, what does this do, exactly?
    > >
    > > It wakes up anyone waiting on the completion and then resets the completion
    > > as otherwise any future attempt to wait on it would succeed immediately.
    > >
    >
    > You realize that if you re-initialize the completion right after
    > signalling it there is a big chance the waiters will miss it (they do
    > check completion->done flags that you reset right away.
    >
    > In general completions are suited for something that happens once (a
    > single request - allocated - processed - signalled) but not for
    > repeating use.

    Would below approach be more correct?

    - move the completion to struct picolcd_pending
    so it meets the "happens once" requirement

    - protect data->pending with a mutex
    (though also use spinlock to prevent race between event which
    signals the completion and picolcd_send_and_wait() around timeout)

    - use the data->lock spinlock to protect multi-report requests from
    interleaving

    In speudo-code this would be something like:

    picolcd_send_and_wait(...)
    {
    struct picolcd_pending pending;
    ...
    init_completion(&pending->ready);
    aquire_mutex(data->mutex);
    spin_lock_irqsave(&data->lock, flags);
    ... // prepare report
    data->pending = &pending;
    usbhid_submit_report(data->hdev, report, USB_DIR_OUT);
    spin_unlock_irqrestore(&data->lock, flags);
    wait_for_completion_interruptible_timeout(&pending->ready, HZ*2);
    spin_lock_irqsave(&data->lock, flags);
    if (data->pending == &pending)
    data->pending = NULL;
    spin_unlock_irqrestore(&data->lock, flags);
    release_mutext(data->mutex);
    ...
    }

    picolcd_raw_event(...)
    {
    ...
    spin_lock_irqsave(&data->lock, flags);
    if (data->pending) {
    // copy event data to pending
    ...
    complete(&pending->ready);
    }
    spin_unlock_irqrestore(&data->lock, flags);
    ...
    }

    In picolcd_remove() and picolcd_reset() I could then do something
    similar to picolcd_raw_event() to trigger completion with no
    data/error in order to skip timeout on the requester side.

    Though how should I prevent races on hot-unplug?

    Thanks,
    Bruno
    --
    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: 2010-03-22 12:41    [W:0.025 / U:0.180 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site