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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux