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);
> }
>
> /**
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
