[PATCH v2] HID: Replace semaphore driver_lock with mutex

[PATCH v2] HID: Replace semaphore driver_lock with mutex

[Binoy Jayan]

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 <binoy.jayan@linaro.org>
Suggested-by: Arnd Bergmann <arnd@arndb.de>
---

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;
-- 
Binoy Jayan

Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex

[Arnd Bergmann]

On Tue, Jun 13, 2017 at 11:25 AM, Binoy Jayan <binoy.jayan@linaro.org> 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 <binoy.jayan@linaro.org>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>

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

Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex

[Benjamin Tissoires]

Hi,

On Jun 13 2017 or thereabouts, Arnd Bergmann wrote:
> On Tue, Jun 13, 2017 at 11:25 AM, Binoy Jayan <binoy.jayan@linaro.org> 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 <binoy.jayan@linaro.org>
> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> 
> 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.

Please also CC linux-input@

> 
> quoting the entire patch for reference, one more comment below:
> 

As stated by Arnd in v1, this semaphore only protects probe/removed from
being called concurrently on the same device. And as Arnd said, the
driver model should prevent this to ever happen.

So Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

(one more nitpick below too)

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

A little bit below, there is:
	bool io_started;						/* Protected by driver_lock. If IO has started */

You should probably remove the mention to driver_lock here.

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

Unless I am mistaken, this one could also be converted to a mutex (in a
separate patch, of course).

Cheers,
Benjamin

> >         struct device dev;                                              /* device */
> >         struct hid_driver *driver;
> 
> You forgot to actually drop the definition.
> 
>        Arnd

Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex

[Binoy Jayan]

Hi,

On 13 June 2017 at 15:26, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:

>> 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.
>
> Please also CC linux-input@

Will do that.
> (one more nitpick below too)
> A little bit below, there is:
>         bool io_started;                                                /* Protected by driver_lock. If IO has started */
>
> You should probably remove the mention to driver_lock here.

Will remove the reference too.

>> > -       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 */
>
> Unless I am mistaken, this one could also be converted to a mutex (in a
> separate patch, of course).

Thank you for noticing that, initially I missed it as I thought
'io_started' somehow
influences the increment of the semaphore, but its anyway used only in
hid-core.c

Thanks,
Binoy

Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex

[Arnd Bergmann]

On Tue, Jun 13, 2017 at 12:09 PM, Binoy Jayan <binoy.jayan@linaro.org> wrote:
> Hi,
>
> On 13 June 2017 at 15:26, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>
>>> 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.
>>
>> Please also CC linux-input@
>
> Will do that.
>> (one more nitpick below too)
>> A little bit below, there is:
>>         bool io_started;                                                /* Protected by driver_lock. If IO has started */
>>
>> You should probably remove the mention to driver_lock here.
>
> Will remove the reference too.
>
> Thank you for noticing that, initially I missed it as I thought
> 'io_started' somehow
> influences the increment of the semaphore, but its anyway used only in
> hid-core.c

It is also used in hid_device_io_start()  and hid_device_io_stop(), but
what's important here is that these are only ever called from inside of
hid_device_probe() and other functions called by that, so no
synchronization across CPUs is required here.

I think in theory, it could be accessed from below hid_device_remove
as well, but I did not find any instance of that.

        Arnd

Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex

[David Herrmann]

Hi

On Tue, Jun 13, 2017 at 11:56 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>> > -       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 */
>
> Unless I am mistaken, this one could also be converted to a mutex (in a
> separate patch, of course).

The mutex code clearly states mutex_trylock() must not be used in
interrupt context (see kernel/locking/mutex.c), hence we used a
semaphore here. Unless the mutex code is changed to allow this, we
cannot switch away from semaphores.

Otherwise, this patch (given Benjamin's comments are addressed) looks good:

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex

[Arnd Bergmann]

On Tue, Jun 13, 2017 at 5:43 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Tue, Jun 13, 2017 at 11:56 AM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>>> > -       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 */
>>
>> Unless I am mistaken, this one could also be converted to a mutex (in a
>> separate patch, of course).
>
> The mutex code clearly states mutex_trylock() must not be used in
> interrupt context (see kernel/locking/mutex.c), hence we used a
> semaphore here. Unless the mutex code is changed to allow this, we
> cannot switch away from semaphores.

Right, that makes a lot of sense. I don't think changing the mutex
code is an option here, but I wonder if we can replace the semaphore
with something simpler anyway.

>From what I can tell, it currently does two things:

1. it acts as a simple flag to prevent  hid_input_report from derefencing
    the hid->driver pointer during initialization and exit. I think this could
    be done equally well using a simple atomic set_bit()/test_bit() or similar.

2. it prevents the hid->driver pointer from becoming invalid while an
    asynchronous hid_input_report() is in progress. This actually seems to
    be a reference counting problem rather than a locking problem.
    I don't immediately see how to better address it, or how exactly this
    could go wrong in practice, but I would naively expect that either
    hdev->driver->remove() needs to wait for the last user of hdev->driver
    to complete, or we need kref_get/kref_put in hid_input_report()
    to trigger the actual release function.

        Arnd

Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex

[Binoy Jayan]

Hi,

On 14 June 2017 at 01:55, Arnd Bergmann <arnd@arndb.de> wrote:

>> The mutex code clearly states mutex_trylock() must not be used in
>> interrupt context (see kernel/locking/mutex.c), hence we used a
>> semaphore here. Unless the mutex code is changed to allow this, we
>> cannot switch away from semaphores.
>
> Right, that makes a lot of sense. I don't think changing the mutex
> code is an option here, but I wonder if we can replace the semaphore
> with something simpler anyway.
>
> From what I can tell, it currently does two things:
>
> 1. it acts as a simple flag to prevent  hid_input_report from derefencing
>     the hid->driver pointer during initialization and exit. I think this could
>     be done equally well using a simple atomic set_bit()/test_bit() or similar.
>
> 2. it prevents the hid->driver pointer from becoming invalid while an
>     asynchronous hid_input_report() is in progress. This actually seems to
>     be a reference counting problem rather than a locking problem.
>     I don't immediately see how to better address it, or how exactly this
>     could go wrong in practice, but I would naively expect that either
>     hdev->driver->remove() needs to wait for the last user of hdev->driver
>     to complete, or we need kref_get/kref_put in hid_input_report()
>     to trigger the actual release function.

Thank you everyone for the comments. I'll resend the patch with Benjamin's
comments incorporated and address the changes in the second semaphore later.

Regards,
Binoy

Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex

[Arnd Bergmann]

On Wed, Jun 14, 2017 at 7:22 AM, Binoy Jayan <binoy.jayan@linaro.org> wrote:
> Hi,
>
> On 14 June 2017 at 01:55, Arnd Bergmann <arnd@arndb.de> wrote:
>
>>> The mutex code clearly states mutex_trylock() must not be used in
>>> interrupt context (see kernel/locking/mutex.c), hence we used a
>>> semaphore here. Unless the mutex code is changed to allow this, we
>>> cannot switch away from semaphores.
>>
>> Right, that makes a lot of sense. I don't think changing the mutex
>> code is an option here, but I wonder if we can replace the semaphore
>> with something simpler anyway.
>>
>> From what I can tell, it currently does two things:
>>
>> 1. it acts as a simple flag to prevent  hid_input_report from derefencing
>>     the hid->driver pointer during initialization and exit. I think this could
>>     be done equally well using a simple atomic set_bit()/test_bit() or similar.
>>
>> 2. it prevents the hid->driver pointer from becoming invalid while an
>>     asynchronous hid_input_report() is in progress. This actually seems to
>>     be a reference counting problem rather than a locking problem.
>>     I don't immediately see how to better address it, or how exactly this
>>     could go wrong in practice, but I would naively expect that either
>>     hdev->driver->remove() needs to wait for the last user of hdev->driver
>>     to complete, or we need kref_get/kref_put in hid_input_report()
>>     to trigger the actual release function.
>
> Thank you everyone for the comments. I'll resend the patch with Benjamin's
> comments incorporated and address the changes in the second semaphore later.

I hope that David or someone else can provide some more feedback on
my interpretation above first so we can decide how this should be
handled. Right now, I wouldn't know how to address point 2 above.

        Arnd

Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex

[David Herrmann]

Hey

On Wed, Jun 14, 2017 at 9:20 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Jun 14, 2017 at 7:22 AM, Binoy Jayan <binoy.jayan@linaro.org> wrote:
>> Hi,
>>
>> On 14 June 2017 at 01:55, Arnd Bergmann <arnd@arndb.de> wrote:
>>
>>>> The mutex code clearly states mutex_trylock() must not be used in
>>>> interrupt context (see kernel/locking/mutex.c), hence we used a
>>>> semaphore here. Unless the mutex code is changed to allow this, we
>>>> cannot switch away from semaphores.
>>>
>>> Right, that makes a lot of sense. I don't think changing the mutex
>>> code is an option here, but I wonder if we can replace the semaphore
>>> with something simpler anyway.
>>>
>>> From what I can tell, it currently does two things:
>>>
>>> 1. it acts as a simple flag to prevent  hid_input_report from derefencing
>>>     the hid->driver pointer during initialization and exit. I think this could
>>>     be done equally well using a simple atomic set_bit()/test_bit() or similar.
>>>
>>> 2. it prevents the hid->driver pointer from becoming invalid while an
>>>     asynchronous hid_input_report() is in progress. This actually seems to
>>>     be a reference counting problem rather than a locking problem.
>>>     I don't immediately see how to better address it, or how exactly this
>>>     could go wrong in practice, but I would naively expect that either
>>>     hdev->driver->remove() needs to wait for the last user of hdev->driver
>>>     to complete, or we need kref_get/kref_put in hid_input_report()
>>>     to trigger the actual release function.

The HID design is explained in detail in
./Documentation/hid/hid-transport.txt, in case you want some
background information. The problem here is that the low-level
transport driver has a lifetime that is independent of the hid
device-driver. So the transport driver needs to be able to tell the
HID layer about coming/going devices, as well as incoming traffic. At
the same time, the HID layer can bind upper-layer hid device drivers
*anytime* (since it is exposed via the driver core interfaces in /sys
to bind drivers).

The locking architecture is very similar to 's_active' on
super-blocks, or 'active' on kernfs-nodes. However, the big difference
here is that drivers can be rebound. Hence, we're not limited to just
one driver lifetime, which is why we went with a semaphore instead.

Also note that hid_input_report() might be called from interrupt
context, hence it can never call into kref_put() or similar (unless we
want to guarantee that unbinding can run in interrupt context).

If we really want to get rid of the semaphore, a spinlock might do
fine as well. Then again, some hid device drivers might expect their
transport driver to *not* run in irq context, and thus break under a
spinlock. So if you want to fix this, we need to audit the hid device
drivers.

David

Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex

[Arnd Bergmann]

On Wed, Jun 14, 2017 at 9:45 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hey
>
> On Wed, Jun 14, 2017 at 9:20 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wed, Jun 14, 2017 at 7:22 AM, Binoy Jayan <binoy.jayan@linaro.org> wrote:
>>> Hi,
>>>
>>> On 14 June 2017 at 01:55, Arnd Bergmann <arnd@arndb.de> wrote:
>>>
>>>>> The mutex code clearly states mutex_trylock() must not be used in
>>>>> interrupt context (see kernel/locking/mutex.c), hence we used a
>>>>> semaphore here. Unless the mutex code is changed to allow this, we
>>>>> cannot switch away from semaphores.
>>>>
>>>> Right, that makes a lot of sense. I don't think changing the mutex
>>>> code is an option here, but I wonder if we can replace the semaphore
>>>> with something simpler anyway.
>>>>
>>>> From what I can tell, it currently does two things:
>>>>
>>>> 1. it acts as a simple flag to prevent  hid_input_report from derefencing
>>>>     the hid->driver pointer during initialization and exit. I think this could
>>>>     be done equally well using a simple atomic set_bit()/test_bit() or similar.
>>>>
>>>> 2. it prevents the hid->driver pointer from becoming invalid while an
>>>>     asynchronous hid_input_report() is in progress. This actually seems to
>>>>     be a reference counting problem rather than a locking problem.
>>>>     I don't immediately see how to better address it, or how exactly this
>>>>     could go wrong in practice, but I would naively expect that either
>>>>     hdev->driver->remove() needs to wait for the last user of hdev->driver
>>>>     to complete, or we need kref_get/kref_put in hid_input_report()
>>>>     to trigger the actual release function.
>
> The HID design is explained in detail in
> ./Documentation/hid/hid-transport.txt, in case you want some
> background information. The problem here is that the low-level
> transport driver has a lifetime that is independent of the hid
> device-driver. So the transport driver needs to be able to tell the
> HID layer about coming/going devices, as well as incoming traffic. At
> the same time, the HID layer can bind upper-layer hid device drivers
> *anytime* (since it is exposed via the driver core interfaces in /sys
> to bind drivers).
>
> The locking architecture is very similar to 's_active' on
> super-blocks, or 'active' on kernfs-nodes. However, the big difference
> here is that drivers can be rebound. Hence, we're not limited to just
> one driver lifetime, which is why we went with a semaphore instead.

Ok, thanks for the background information!

Does that mean that we can have a concurrent hid_device_remove()
and hid_device_probe() on the same device, using different
drivers and actually still need the driver_lock for that? I would assume
that the driver core handles that part at least.

> Also note that hid_input_report() might be called from interrupt
> context, hence it can never call into kref_put() or similar (unless we
> want to guarantee that unbinding can run in interrupt context).

I was thinking that we could do most of the unbinding in
hid_device_remove() and only do a small part (the final kfree
at the minimum) in the release() callback that gets called from
kref_put(), but I guess that also isn't easy to retrofit.

> If we really want to get rid of the semaphore, a spinlock might do
> fine as well. Then again, some hid device drivers might expect their
> transport driver to *not* run in irq context, and thus break under a
> spinlock. So if you want to fix this, we need to audit the hid device
> drivers.

Do you mean the hdrv->report or hdrv->raw_event might not work
in atomic context, or the probe/release callbacks might not work
there?

If it's only the former, we could do something like the (untested,
needs rebasing etc) patch below, which only holds the spinlock
during hid_input_report(). We test the io_started flag under the
lock, which makes the flag sufficiently atomic, and the release
function will wait for any concurrent hid_input_report() to complete
before resetting the flag.

For reference only, do not apply.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 5f87dbe28336..300c65bd40a1 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1532,8 +1532,11 @@ 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_input_lock))
-               return -EBUSY;
+       spin_lock(&hid->driver_input_lock);
+       if (!hid->io_started) {
+               ret = -EBUSY;
+               goto unlock;
+       }

        if (!hid->driver) {
                ret = -ENODEV;
@@ -1568,7 +1571,7 @@ int hid_input_report(struct hid_device *hid, int
type, u8 *data, int size, int i
        ret = hid_report_raw_event(hid, type, data, size, interrupt);

 unlock:
-       up(&hid->driver_input_lock);
+       spin_unlock(&hid->driver_input_lock);
        return ret;
 }
 EXPORT_SYMBOL_GPL(hid_input_report);
@@ -2317,11 +2320,6 @@ 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);
@@ -2345,7 +2343,7 @@ static int hid_device_probe(struct device *dev)
        }
 unlock:
        if (!hdev->io_started)
-               up(&hdev->driver_input_lock);
+               hid_device_io_start(hdev);
 unlock_driver_lock:
        up(&hdev->driver_lock);
        return ret;
@@ -2363,7 +2361,7 @@ static int hid_device_remove(struct device *dev)
                ret = -EINTR;
                goto unlock_driver_lock;
        }
-       hdev->io_started = false;
+       hid_device_io_stop(hdev);

        hdrv = hdev->driver;
        if (hdrv) {
@@ -2375,8 +2373,11 @@ static int hid_device_remove(struct device *dev)
                hdev->driver = NULL;
        }

-       if (!hdev->io_started)
-               up(&hdev->driver_input_lock);
+       if (!hdev->io_started) {
+               dev_warn(dev, "hdrv->remove left io active\n");
+               hid_device_io_stop(hdev);
+       }
+
 unlock_driver_lock:
        up(&hdev->driver_lock);
        return ret;
@@ -2836,7 +2837,8 @@ struct hid_device *hid_allocate_device(void)
        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);
+       spin_lock_init(&hdev->driver_input_lock, 1);
+       hdev->io_started = false;
        mutex_init(&hdev->ll_open_lock);

        return hdev;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 5006f9b5d837..00e9f4042a03 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -527,7 +527,7 @@ struct hid_device {
                 /* device report descriptor */
        struct work_struct led_work;
 /* delayed LED worker */

        struct semaphore driver_lock;
 /* protects the current driver, except during input */
-       struct semaphore driver_input_lock;
 /* protects the current driver */
+       spinlock_t driver_input_lock;
 /* protects the current driver */
        struct device dev;
 /* device */
        struct hid_driver *driver;

@@ -854,12 +854,12 @@ __u32 hid_field_extract(const struct hid_device
*hid, __u8 *report,
  * incoming packets to be delivered to the driver.
  */
 static inline void hid_device_io_start(struct hid_device *hid) {
+       spin_lock(&hid->driver_input_lock);
        if (hid->io_started) {
                dev_warn(&hid->dev, "io already started\n");
-               return;
        }
        hid->io_started = true;
-       up(&hid->driver_input_lock);
+       spin_unlock(&hid->driver_input_lock);
 }

 /**
@@ -874,12 +874,12 @@ static inline void hid_device_io_start(struct
hid_device *hid) {
  * by the thread calling probe or remove.
  */
 static inline void hid_device_io_stop(struct hid_device *hid) {
+       spin_lock(&hid->driver_input_lock);
        if (!hid->io_started) {
                dev_warn(&hid->dev, "io already stopped\n");
-               return;
        }
        hid->io_started = false;
-       down(&hid->driver_input_lock);
+       spin_unlock(&hid->driver_input_lock);
 }

 /**

Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex

[David Herrmann]

Hi

On Wed, Jun 14, 2017 at 1:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> Does that mean that we can have a concurrent hid_device_remove()
> and hid_device_probe() on the same device, using different
> drivers and actually still need the driver_lock for that? I would assume
> that the driver core handles that part at least.

Nope. Only one device can be probed per physical device. Driver core
locking is sufficient.

>> Also note that hid_input_report() might be called from interrupt
>> context, hence it can never call into kref_put() or similar (unless we
>> want to guarantee that unbinding can run in interrupt context).
>
> I was thinking that we could do most of the unbinding in
> hid_device_remove() and only do a small part (the final kfree
> at the minimum) in the release() callback that gets called from
> kref_put(), but I guess that also isn't easy to retrofit.

Not a big fan of putting such restrictions on unbinding. Also unlikely
to retrofit now. But I also think it is not needed.

>> If we really want to get rid of the semaphore, a spinlock might do
>> fine as well. Then again, some hid device drivers might expect their
>> transport driver to *not* run in irq context, and thus break under a
>> spinlock. So if you want to fix this, we need to audit the hid device
>> drivers.
>
> Do you mean the hdrv->report or hdrv->raw_event might not work
> in atomic context, or the probe/release callbacks might not work
> there?

Correct. I assume that there are hid-device-drivers that use raw_event
(or other) callbacks, that assume that they're *not* in atomic
context.

For instance, the bluetooth ll-drivers call hid_input_report() from a
workqueue. Hence, any device-driver running on bluetooth might have
put kmalloc(GFP_KERNEL) calls into its callbacks without ever noticing
that this is a bad idea. This is obviously not correct, since the
device driver might very well be probed on USB and then fault. But...
yeah... I wouldn't bet on all drivers to be correct in that regard.

> If it's only the former, we could do something like the (untested,
> needs rebasing etc) patch below, which only holds the spinlock
> during hid_input_report(). We test the io_started flag under the
> lock, which makes the flag sufficiently atomic, and the release
> function will wait for any concurrent hid_input_report() to complete
> before resetting the flag.
>
> For reference only, do not apply.
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I like the patch. It should be sufficient for what we want. If it
breaks things, lets fix those device drivers then.

Thanks
David

> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 5f87dbe28336..300c65bd40a1 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1532,8 +1532,11 @@ 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_input_lock))
> -               return -EBUSY;
> +       spin_lock(&hid->driver_input_lock);
> +       if (!hid->io_started) {
> +               ret = -EBUSY;
> +               goto unlock;
> +       }
>
>         if (!hid->driver) {
>                 ret = -ENODEV;
> @@ -1568,7 +1571,7 @@ int hid_input_report(struct hid_device *hid, int
> type, u8 *data, int size, int i
>         ret = hid_report_raw_event(hid, type, data, size, interrupt);
>
>  unlock:
> -       up(&hid->driver_input_lock);
> +       spin_unlock(&hid->driver_input_lock);
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(hid_input_report);
> @@ -2317,11 +2320,6 @@ 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);
> @@ -2345,7 +2343,7 @@ static int hid_device_probe(struct device *dev)
>         }
>  unlock:
>         if (!hdev->io_started)
> -               up(&hdev->driver_input_lock);
> +               hid_device_io_start(hdev);
>  unlock_driver_lock:
>         up(&hdev->driver_lock);
>         return ret;
> @@ -2363,7 +2361,7 @@ static int hid_device_remove(struct device *dev)
>                 ret = -EINTR;
>                 goto unlock_driver_lock;
>         }
> -       hdev->io_started = false;
> +       hid_device_io_stop(hdev);
>
>         hdrv = hdev->driver;
>         if (hdrv) {
> @@ -2375,8 +2373,11 @@ static int hid_device_remove(struct device *dev)
>                 hdev->driver = NULL;
>         }
>
> -       if (!hdev->io_started)
> -               up(&hdev->driver_input_lock);
> +       if (!hdev->io_started) {
> +               dev_warn(dev, "hdrv->remove left io active\n");
> +               hid_device_io_stop(hdev);
> +       }
> +
>  unlock_driver_lock:
>         up(&hdev->driver_lock);
>         return ret;
> @@ -2836,7 +2837,8 @@ struct hid_device *hid_allocate_device(void)
>         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);
> +       spin_lock_init(&hdev->driver_input_lock, 1);
> +       hdev->io_started = false;
>         mutex_init(&hdev->ll_open_lock);
>
>         return hdev;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 5006f9b5d837..00e9f4042a03 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -527,7 +527,7 @@ struct hid_device {
>                  /* device report descriptor */
>         struct work_struct led_work;
>  /* delayed LED worker */
>
>         struct semaphore driver_lock;
>  /* protects the current driver, except during input */
> -       struct semaphore driver_input_lock;
>  /* protects the current driver */
> +       spinlock_t driver_input_lock;
>  /* protects the current driver */
>         struct device dev;
>  /* device */
>         struct hid_driver *driver;
>
> @@ -854,12 +854,12 @@ __u32 hid_field_extract(const struct hid_device
> *hid, __u8 *report,
>   * incoming packets to be delivered to the driver.
>   */
>  static inline void hid_device_io_start(struct hid_device *hid) {
> +       spin_lock(&hid->driver_input_lock);
>         if (hid->io_started) {
>                 dev_warn(&hid->dev, "io already started\n");
> -               return;
>         }
>         hid->io_started = true;
> -       up(&hid->driver_input_lock);
> +       spin_unlock(&hid->driver_input_lock);
>  }
>
>  /**
> @@ -874,12 +874,12 @@ static inline void hid_device_io_start(struct
> hid_device *hid) {
>   * by the thread calling probe or remove.
>   */
>  static inline void hid_device_io_stop(struct hid_device *hid) {
> +       spin_lock(&hid->driver_input_lock);
>         if (!hid->io_started) {
>                 dev_warn(&hid->dev, "io already stopped\n");
> -               return;
>         }
>         hid->io_started = false;
> -       down(&hid->driver_input_lock);
> +       spin_unlock(&hid->driver_input_lock);
>  }
>
>  /**

Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex

[Binoy Jayan]

Hi Arnd,

On 13 June 2017 at 15:15, Arnd Bergmann <arnd@arndb.de> wrote:
> 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.

Will include them, thank you yet again for reminding me.

> You forgot to actually drop the definition.

And for this too.

Regards,
Binoy