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@xxxxxxxx> 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@xxxxxxxx>

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