On Wed, 13 Feb 2013 21:08:20 -0800 Andrew de los Reyes wrote:

quoted

quoted

quoted

 /**
+ * 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