On Wed, 13 Feb 2013, Andrew de los Reyes wrote: > Here's the latest version of the patch. I believe it addresses all > outstanding comments. > > Thanks! > -andrew > > This patch separates struct hid_device's driver_lock into two. The > goal is to allow hid device drivers to receive input during their > probe() or remove() function calls. This is necessary because some > drivers need to communicate with the device to determine parameters > needed during probe (e.g., size of a multi-touch surface), and if > possible, may perfer to communicate with a device on host-initiated > disconnect (e.g., to put it into a low-power state). > > Historically, three functions used driver_lock: > > - hid_device_probe: blocks to acquire lock > - hid_device_remove: blocks to acquire lock > - hid_input_report: if locked returns -EBUSY, else acquires lock > > This patch adds another lock (driver_input_lock) which is used to > block input from occurring. The lock behavior is now: > > - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock > - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock > - hid_input_report: if driver_input_lock locked returns -EBUSY, else > acquires driver_input_lock > > This patch also adds two helper functions to be called during probe() > or remove(): hid_device_io_start() and hid_device_io_stop(). These > functions lock and unlock, respectively, driver_input_lock; they also > make a note of whether they did so that hid-core knows if a driver has > changed the lock state. > > This patch results in no behavior change for existing devices and > drivers. However, during a probe() or remove() function call in a > driver, that driver may now selectively call hid_device_io_start() to > let input events come through, then optionally call > hid_device_io_stop() to stop them. > > Change-Id: I737f6fc15911134b51273acf8d3de92fa5cc0f85
Hi, thanks for finally proceeding on this front. Please provide your Signed-off-by line; without that, the patch can't be accepted. > --- > drivers/hid/hid-core.c | 24 +++++++++++++++++++++--- > include/linux/hid.h | 38 +++++++++++++++++++++++++++++++++++++- > 2 files changed, 58 insertions(+), 4 deletions(-) > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 4da66b4..714d8b7 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -1097,7 +1097,7 @@ int hid_input_report(struct hid_device *hid, int > type, u8 *data, int size, int i > if (!hid) > return -ENODEV; > > - if (down_trylock(&hid->driver_lock)) > + if (down_trylock(&hid->driver_input_lock)) > return -EBUSY; > > if (!hid->driver) { > @@ -1150,7 +1150,7 @@ nomem: > hid_report_raw_event(hid, type, data, size, interrupt); > > unlock: > - up(&hid->driver_lock); > + up(&hid->driver_input_lock); > return ret; > } > EXPORT_SYMBOL_GPL(hid_input_report); > @@ -1703,6 +1703,11 @@ static int hid_device_probe(struct device *dev) > > if (down_interruptible(&hdev->driver_lock)) > return -EINTR; > + if (down_interruptible(&hdev->driver_input_lock)) { > + ret = -EINTR; > + goto unlock_driver_lock; > + } > + hdev->io_started = false; > > if (!hdev->driver) { > id = hid_match_device(hdev, hdrv); > @@ -1726,6 +1731,9 @@ static int hid_device_probe(struct device *dev) > hdev->driver = NULL; > } > unlock: > + if (!hdev->io_started) > + up(&hdev->driver_input_lock); > +unlock_driver_lock: > up(&hdev->driver_lock); > return ret; > } > @@ -1734,9 +1742,15 @@ static int hid_device_remove(struct device *dev) > { > struct hid_device *hdev = container_of(dev, struct hid_device, dev); > struct hid_driver *hdrv; > + int ret = 0; > > if (down_interruptible(&hdev->driver_lock)) > return -EINTR; > + if (down_interruptible(&hdev->driver_input_lock)) { > + ret = -EINTR; > + goto unlock_driver_lock; > + } > + hdev->io_started = false; > > hdrv = hdev->driver; > if (hdrv) { > @@ -1747,8 +1761,11 @@ static int hid_device_remove(struct device *dev) > hdev->driver = NULL; > } > > + if (!hdev->io_started) > + up(&hdev->driver_input_lock); > +unlock_driver_lock: > up(&hdev->driver_lock); > - return 0; > + return ret; > } > > static int hid_uevent(struct device *dev, struct kobj_uevent_env *env) > @@ -2126,6 +2143,7 @@ struct hid_device *hid_allocate_device(void) > init_waitqueue_head(&hdev->debug_wait); > INIT_LIST_HEAD(&hdev->debug_list); > sema_init(&hdev->driver_lock, 1); > + sema_init(&hdev->driver_input_lock, 1); > > return hdev; > err: > diff --git a/include/linux/hid.h b/include/linux/hid.h > index 3a95da6..27282a1 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -481,7 +481,8 @@ struct hid_device { > /* device report descriptor */ > unsigned country; /* HID > country */ > struct hid_report_enum report_enum[HID_REPORT_TYPES]; > > - struct semaphore driver_lock; /* > protects the current driver */ > + struct semaphore driver_lock; /* > protects the current driver, > except during input */ > + struct semaphore driver_input_lock; /* > protects the current driver */ > struct device dev; /* > device */ > struct hid_driver *driver; > struct hid_ll_driver *ll_driver; > @@ -502,6 +503,7 @@ struct hid_device { > /* device report descriptor */ > unsigned int status; /* see > STAT flags above */ > unsigned claimed; /* > Claimed by hidinput, hiddev? */ > unsigned quirks; /* > Various quirks the device can pull on us */ > + bool io_started; /* > Protected by driver_lock. If IO has started */ > > struct list_head inputs; /* The > list of inputs */ > void *hiddev; /* The > hiddev structure */ > @@ -622,6 +624,10 @@ struct hid_usage_id { > * @resume: invoked on resume if device was not reset (NULL means nop) > * @reset_resume: invoked on resume if device was reset (NULL means nop) > * > + * probe should return -errno on error, or 0 on success. During probe, > + * input will not be passed to raw_event unless hid_device_io_start is > + * called. > + * > * raw_event and event should return 0 on no action performed, 1 when no > * further processing should be done and negative on error > * > @@ -742,6 +748,36 @@ const struct hid_device_id *hid_match_id(struct > hid_device *hdev, > const struct hid_device_id *id); > > /** > + * hid_device_io_start - enable HID input during probe, remove > + * > + * @hid - the device > + * > + * This should only be called during probe or remove and only be > + * called by the thread calling 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) { > + hid->io_started = true; > + up(&hid->driver_input_lock); > +} > + > +/** > + * hid_device_io_stop - disable HID input during probe, remove > + * > + * @hid - the device > + * > + * Should only be called after hid_device_io_start. It will prevent > + * incoming packets from going to the driver for the duration of > + * probe, remove. If called during probe, packets will still go to the > + * driver after probe is complete. This function should only be called > + * by the thread calling probe or remove. > + */ > +static inline void hid_device_io_stop(struct hid_device *hid) { > + hid->io_started = false; > + down(&hid->driver_input_lock); > +} Is there any particular reason to have hid_device_io_start() and hid_device_io_stop() indentation not use tabs? Also, the functions are currently unused, so I'd rather suggest adding them together when individual device driver(s) are converted to using it. Thanks again, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html