From patchwork Sun Feb 10 17:47:17 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Andrew de los Reyes X-Patchwork-Id: 2122521 X-Patchwork-Delegate: jikos@jikos.cz Return-Path: X-Original-To: patchwork-linux-input@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 49D16DF24C for ; Sun, 10 Feb 2013 17:47:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755980Ab3BJRri (ORCPT ); Sun, 10 Feb 2013 12:47:38 -0500 Received: from mail-ia0-f182.google.com ([209.85.210.182]:61838 "EHLO mail-ia0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756040Ab3BJRrh (ORCPT ); Sun, 10 Feb 2013 12:47:37 -0500 Received: by mail-ia0-f182.google.com with SMTP id w33so5760738iag.41 for ; Sun, 10 Feb 2013 09:47:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:mime-version:sender:from:date:x-google-sender-auth :message-id:subject:to:cc:content-type; bh=39W2LZkn6E0qJHQLaa998EU+BKjGt2dbRWGnA4pX5J8=; b=iQDlql7PoeI48yXOU8anDUwNK0KjJVmu1P6tifS6P5Q+T5BtC25lRSdWmclIIpDKOJ KS325lm7/gHbOxUNNynDxIvL4CnbWUU76h27856bSWWRYQQuOqzw1f8iWg9+3gRvwqHQ Q8GGEFhFgoxDvXEa5ET8dabWhuBD9tv5xwE2LALkyHGuw8ge3ItsrrpOaGe+Vqhnw97B D8ZMyv6qaYtWH7VDy5q4sLYw3UPCJmWUfVD9SjouPxnB12+2rGdbeK3WdYgVls2iiPMt BLVfumJO4DmucXD3GwwKoFBrR9VEE5Ez72gAS0//5zcJTLflpj6vRZNVCdeoYzxUVNSQ Xltg== X-Received: by 10.42.70.142 with SMTP id f14mr15641615icj.3.1360518457052; Sun, 10 Feb 2013 09:47:37 -0800 (PST) MIME-Version: 1.0 Received: by 10.42.11.130 with HTTP; Sun, 10 Feb 2013 09:47:17 -0800 (PST) From: Andrew de los Reyes Date: Sun, 10 Feb 2013 09:47:17 -0800 X-Google-Sender-Auth: -43k8YgWolQdbMwVoaTejXZYzh0 Message-ID: Subject: [PATCH] HID: Separate struct hid_device's driver_lock into two locks. To: Linux Input , Nestor Lopez Casado , Benjamin Tissoires , =?ISO-8859-1?Q?Bruno_Pr=E9mont?= , David Herrmann Cc: Jiri Kosina Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org Hi Linux-Input and others, This is the latest version of the patch to allow device drivers to communicate with the corresponding device during probe(). I've incorporated many of David Herrmann's suggestions into this revision. The most notable change is that by using helper functions, we no longer need to have a special magic number return value from probe(). This patch is part of a patch series to support Logitech touch devices (e.g., Wireless Touchpad). The rest of the series is not yet ready for discussion here, but those curious can see it here: https://github.com/adlr/linux/commits/logitech7 Thanks for your comments, -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 Reviewed-by: Bruno Prémont --- drivers/hid/hid-core.c | 24 +++++++++++++++++++++--- include/linux/hid.h | 36 +++++++++++++++++++++++++++++++++++- 2 files changed, 56 insertions(+), 4 deletions(-) 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,34 @@ 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. 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; +} + +/** + * 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. + */ +static inline void hid_device_io_stop(struct hid_device *hid) { + hid->io_started = false; + down(&hid->driver_input_lock); +} + +/** * hid_map_usage - map usage input bits * * @hidinput: hidinput which we are interested in diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 4da66b4..6a04b72 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) + hid_device_io_start(hdev); +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) + hid_device_io_start(hdev); +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..ae7d32d 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;