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

From: David Herrmann <hidden>
Date: 2017-06-19 10:20:16
Also in: lkml

Hi

On Wed, Jun 14, 2017 at 1:58 PM, Arnd Bergmann [off-list ref] 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.

quoted

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.

quoted

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

quoted hunk ↗ jump to hunk

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);
 }

 /**

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help