linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] HID: Replace semaphore driver_lock with mutex
@ 2017-06-08 10:01 Binoy Jayan
  2017-06-08 14:27 ` Arnd Bergmann
  0 siblings, 1 reply; 2+ messages in thread
From: Binoy Jayan @ 2017-06-08 10:01 UTC (permalink / raw)
  To: Binoy Jayan
  Cc: linux-kernel, Arnd Bergmann, Rajendra, Mark Brown, Jiri Kosina,
	Benjamin Tissoires

The semaphore 'driver_lock' is used as a simple mutex, so
it should be written as one. Semaphores are going away in the future.

Signed-off-by: Binoy Jayan <binoy.jayan@linaro.org>
---

This patch is part of a bigger effort to eliminate unwanted
semaphores from the linux kernel.

 drivers/hid/hid-core.c | 10 +++++-----
 include/linux/hid.h    |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 37084b6..bf21dac 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2222,7 +2222,7 @@ static int hid_device_probe(struct device *dev)
 	const struct hid_device_id *id;
 	int ret = 0;
 
-	if (down_interruptible(&hdev->driver_lock))
+	if (mutex_lock_interruptible(&hdev->driver_lock))
 		return -EINTR;
 	if (down_interruptible(&hdev->driver_input_lock)) {
 		ret = -EINTR;
@@ -2254,7 +2254,7 @@ static int hid_device_probe(struct device *dev)
 	if (!hdev->io_started)
 		up(&hdev->driver_input_lock);
 unlock_driver_lock:
-	up(&hdev->driver_lock);
+	mutex_unlock(&hdev->driver_lock);
 	return ret;
 }
 
@@ -2264,7 +2264,7 @@ static int hid_device_remove(struct device *dev)
 	struct hid_driver *hdrv;
 	int ret = 0;
 
-	if (down_interruptible(&hdev->driver_lock))
+	if (mutex_lock_interruptible(&hdev->driver_lock))
 		return -EINTR;
 	if (down_interruptible(&hdev->driver_input_lock)) {
 		ret = -EINTR;
@@ -2285,7 +2285,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);
+	mutex_unlock(&hdev->driver_lock);
 	return ret;
 }
 
@@ -2742,7 +2742,7 @@ 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);
+	mutex_init(&hdev->driver_lock);
 	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;
-- 
Binoy Jayan

^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH] HID: Replace semaphore driver_lock with mutex
  2017-06-08 10:01 [PATCH] HID: Replace semaphore driver_lock with mutex Binoy Jayan
@ 2017-06-08 14:27 ` Arnd Bergmann
  0 siblings, 0 replies; 2+ messages in thread
From: Arnd Bergmann @ 2017-06-08 14:27 UTC (permalink / raw)
  To: Binoy Jayan
  Cc: Linux Kernel Mailing List, Rajendra, Mark Brown, Jiri Kosina,
	Benjamin Tissoires, David Herrmann, Andrew de los Reyes

On Thu, Jun 8, 2017 at 12:01 PM, Binoy Jayan <binoy.jayan@linaro.org> wrote:
> The semaphore 'driver_lock' is used as a simple mutex, so
> it should be written as one. Semaphores are going away in the future.
>
> Signed-off-by: Binoy Jayan <binoy.jayan@linaro.org>
> ---
>
> This patch is part of a bigger effort to eliminate unwanted
> semaphores from the linux kernel.

Adding David Herrmann and Andrew de los Reyes to Cc, as David introduced
the semaphore originally and Andrew split it in two.

Your conversion looks entirely safe to me, as the driver_lock follows
the rules for a mutex, and therefore should not be a full semaphore.

However, I think we can simply remove it altogether, as the only thing
it does is to prevent a concurrent hid_device_probe and
hid_device_remove on the same device, and the driver core code
should already protect us from doing that.

driver_input_lock is still needed to block the remove function during
hid_input_report (otherwise it could be a simple flag), but I suspect
that could also be done with a mutex.

       Arnd

>  drivers/hid/hid-core.c | 10 +++++-----
>  include/linux/hid.h    |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 37084b6..bf21dac 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2222,7 +2222,7 @@ static int hid_device_probe(struct device *dev)
>         const struct hid_device_id *id;
>         int ret = 0;
>
> -       if (down_interruptible(&hdev->driver_lock))
> +       if (mutex_lock_interruptible(&hdev->driver_lock))
>                 return -EINTR;
>         if (down_interruptible(&hdev->driver_input_lock)) {
>                 ret = -EINTR;
> @@ -2254,7 +2254,7 @@ static int hid_device_probe(struct device *dev)
>         if (!hdev->io_started)
>                 up(&hdev->driver_input_lock);
>  unlock_driver_lock:
> -       up(&hdev->driver_lock);
> +       mutex_unlock(&hdev->driver_lock);
>         return ret;
>  }
>
> @@ -2264,7 +2264,7 @@ static int hid_device_remove(struct device *dev)
>         struct hid_driver *hdrv;
>         int ret = 0;
>
> -       if (down_interruptible(&hdev->driver_lock))
> +       if (mutex_lock_interruptible(&hdev->driver_lock))
>                 return -EINTR;
>         if (down_interruptible(&hdev->driver_input_lock)) {
>                 ret = -EINTR;
> @@ -2285,7 +2285,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);
> +       mutex_unlock(&hdev->driver_lock);
>         return ret;
>  }
>
> @@ -2742,7 +2742,7 @@ 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);
> +       mutex_init(&hdev->driver_lock);
>         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;
> --
> Binoy Jayan
>

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-06-08 14:27 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-08 10:01 [PATCH] HID: Replace semaphore driver_lock with mutex Binoy Jayan
2017-06-08 14:27 ` Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).