Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752255AbdFMJqB (ORCPT ); Tue, 13 Jun 2017 05:46:01 -0400 Received: from mail-oi0-f68.google.com ([209.85.218.68]:36003 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751891AbdFMJp7 (ORCPT ); Tue, 13 Jun 2017 05:45:59 -0400 MIME-Version: 1.0 In-Reply-To: <1497345926-3262-1-git-send-email-binoy.jayan@linaro.org> References: <1497345926-3262-1-git-send-email-binoy.jayan@linaro.org> From: Arnd Bergmann Date: Tue, 13 Jun 2017 11:45:58 +0200 X-Google-Sender-Auth: 8bIPG6SM6xf3BQxW9dNN5xpIrqA Message-ID: Subject: Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex To: Binoy Jayan Cc: Linux Kernel Mailing List , Rajendra , Mark Brown , Jiri Kosina , Benjamin Tissoires , David Herrmann , Andrew de los Reyes Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3636 Lines: 99 On Tue, Jun 13, 2017 at 11:25 AM, Binoy Jayan wrote: > The semaphore 'driver_lock' is used as a simple mutex, and > also unnecessary as suggested by Arnd. Hence removing it, as > the concurrency between the probe and remove is already > handled in the driver core. > > Signed-off-by: Binoy Jayan > Suggested-by: Arnd Bergmann Looks good to me, but I see you didn't include David and Andrew on Cc, it would be good for at least one of them to provide an Ack as well. quoting the entire patch for reference, one more comment below: > --- > > v1 --> v2 > > Removed driver_lock > > drivers/hid/hid-core.c | 15 ++++----------- > include/linux/hid.h | 2 +- > 2 files changed, 5 insertions(+), 12 deletions(-) > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 04cee65..559533b 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -2225,11 +2225,9 @@ static int hid_device_probe(struct device *dev) > const struct hid_device_id *id; > int ret = 0; > > - if (down_interruptible(&hdev->driver_lock)) > - return -EINTR; > if (down_interruptible(&hdev->driver_input_lock)) { > ret = -EINTR; > - goto unlock_driver_lock; > + goto end; > } > hdev->io_started = false; > > @@ -2256,8 +2254,7 @@ static int hid_device_probe(struct device *dev) > unlock: > if (!hdev->io_started) > up(&hdev->driver_input_lock); > -unlock_driver_lock: > - up(&hdev->driver_lock); > +end: > return ret; > } > > @@ -2267,11 +2264,9 @@ static int hid_device_remove(struct 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; > + goto end; > } > hdev->io_started = false; > > @@ -2287,8 +2282,7 @@ static int hid_device_remove(struct device *dev) > > if (!hdev->io_started) > up(&hdev->driver_input_lock); > -unlock_driver_lock: > - up(&hdev->driver_lock); > +end: > return ret; > } > > @@ -2745,7 +2739,6 @@ struct hid_device *hid_allocate_device(void) > init_waitqueue_head(&hdev->debug_wait); > INIT_LIST_HEAD(&hdev->debug_list); > spin_lock_init(&hdev->debug_list_lock); > - sema_init(&hdev->driver_lock, 1); > sema_init(&hdev->driver_input_lock, 1); > > return hdev; > diff --git a/include/linux/hid.h b/include/linux/hid.h > index 5be325d..1add2b3 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -516,7 +516,7 @@ struct hid_device { /* device report descriptor */ > struct hid_report_enum report_enum[HID_REPORT_TYPES]; > struct work_struct led_work; /* delayed LED worker */ > > - struct semaphore driver_lock; /* protects the current driver, except during input */ > + struct mutex 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; You forgot to actually drop the definition. Arnd