Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.

["Bruno Prémont" <bonbons@linux-vserver.org>]

On Wed, 13 Feb 2013 21:08:20 -0800 Andrew de los Reyes wrote:
> > > >  /**
> > > > + * hid_device_io_start - enable HID input during probe, remove
> > > > + *
> > > > + * @hid - the device
> > > > + *
> > > > + * This should only be called during probe or remove. It will allow
> > > > + * incoming packets to be delivered to the driver.
> > > > + */
> > > > +static inline void hid_device_io_start(struct hid_device *hid) {
> > > > +  up(&hid->driver_input_lock);
> > > > +  hid->io_started = true;
> > >
> > > Shouldn't these lines be swapped? Doesn't matter but it looks weird to
> > > me this way.
> > >
> > > But more importantly, we must go sure this is called from the same
> > > thread that probe() is called on. Other use-cases are not supported by
> > > semaphores and might break due to missing barriers. So maybe the
> > > comment could include that?
> >
> > Maybe even check what value hid->io_started has and WARN() [+skip
> > up()/down()]
> > in case hid_device_io_start()/stop() was not called in a balanced way.
> 
> It is a goal to support hid_device_io_start()/stop() not being called
> in a balanced way. This is specifically needed by a change I'm making
> to hid-logitech-dj.c: During probe(), the driver will send out a
> request to the USB dongle to list any attached mice/keyboards. The
> reply packets aren't needed during probe(), so probe will return, but
> the driver wants packets to flow in, and it doesn't mind if they come
> in while probe() is still running or not. In this case, during
> probe(), the driver will call hid_device_io_start() to allow incoming
> packets, but then not call hid_device_io_stop().

With unbalanced I meant calling hid_device_io_start()/stop() twice or
more in a row in _probe() (or _remove()) without corresponding
_stop()/_start() call.
That kind of cases might happen (mostly) with error paths.

Like the following (simplified) example when both if() match:

int driver_probe(...)
{
   ...
   if (...) {
      ...
      hid_device_io_start();
      ...
   }
   ...
   if (...) {
      ...
      hid_device_io_start();
      ...
   }
   ...
}


Bruno