Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex
Hi On Wed, Jun 14, 2017 at 1:58 PM, Arnd Bergmannwrote: > 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 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(>driver_input_lock)) > - return -EBUSY; > + spin_lock(>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(>driver_input_lock); > + spin_unlock(>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(>driver_lock)) > return -EINTR; > - if (down_interruptible(>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(>driver_input_lock); > + hid_device_io_start(hdev); > unlock_driver_lock: > up(>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(>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(>driver_lock); > return ret; > @@
Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex
Hi On Wed, Jun 14, 2017 at 1:58 PM, Arnd Bergmann 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 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(>driver_input_lock)) > - return -EBUSY; > + spin_lock(>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(>driver_input_lock); > + spin_unlock(>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(>driver_lock)) > return -EINTR; > - if (down_interruptible(>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(>driver_input_lock); > + hid_device_io_start(hdev); > unlock_driver_lock: > up(>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(>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(>driver_lock); > return ret; > @@ -2836,7 +2837,8 @@ struct
Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex
On Wed, Jun 14, 2017 at 9:45 AM, David Herrmannwrote: > Hey > > On Wed, Jun 14, 2017 at 9:20 AM, Arnd Bergmann wrote: >> On Wed, Jun 14, 2017 at 7:22 AM, Binoy Jayan wrote: >>> Hi, >>> >>> On 14 June 2017 at 01:55, Arnd Bergmann 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 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(>driver_input_lock)) - return -EBUSY; + spin_lock(>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(>driver_input_lock); + spin_unlock(>driver_input_lock); return ret; }
Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex
On Wed, Jun 14, 2017 at 9:45 AM, David Herrmann wrote: > Hey > > On Wed, Jun 14, 2017 at 9:20 AM, Arnd Bergmann wrote: >> On Wed, Jun 14, 2017 at 7:22 AM, Binoy Jayan wrote: >>> Hi, >>> >>> On 14 June 2017 at 01:55, Arnd Bergmann 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 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(>driver_input_lock)) - return -EBUSY; + spin_lock(>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(>driver_input_lock); + spin_unlock(>driver_input_lock); return ret; } EXPORT_SYMBOL_GPL(hid_input_report); @@ -2317,11 +2320,6 @@ static int hid_device_probe(struct device *dev)
Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex
Hey On Wed, Jun 14, 2017 at 9:20 AM, Arnd Bergmannwrote: > On Wed, Jun 14, 2017 at 7:22 AM, Binoy Jayan wrote: >> Hi, >> >> On 14 June 2017 at 01:55, Arnd Bergmann 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
Hey On Wed, Jun 14, 2017 at 9:20 AM, Arnd Bergmann wrote: > On Wed, Jun 14, 2017 at 7:22 AM, Binoy Jayan wrote: >> Hi, >> >> On 14 June 2017 at 01:55, Arnd Bergmann 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
On Wed, Jun 14, 2017 at 7:22 AM, Binoy Jayanwrote: > Hi, > > On 14 June 2017 at 01:55, Arnd Bergmann 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
On Wed, Jun 14, 2017 at 7:22 AM, Binoy Jayan wrote: > Hi, > > On 14 June 2017 at 01:55, Arnd Bergmann 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
Hi, On 14 June 2017 at 01:55, Arnd Bergmannwrote: >> 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
Hi, On 14 June 2017 at 01:55, Arnd Bergmann 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
On Tue, Jun 13, 2017 at 5:43 PM, David Herrmannwrote: > Hi > > On Tue, Jun 13, 2017 at 11:56 AM, Benjamin Tissoires > 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
On Tue, Jun 13, 2017 at 5:43 PM, David Herrmann wrote: > Hi > > On Tue, Jun 13, 2017 at 11:56 AM, Benjamin Tissoires > 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
On Tue, Jun 13, 2017 at 12:09 PM, Binoy Jayanwrote: > Hi, > > On 13 June 2017 at 15:26, Benjamin Tissoires > 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
On Tue, Jun 13, 2017 at 12:09 PM, Binoy Jayan wrote: > Hi, > > On 13 June 2017 at 15:26, Benjamin Tissoires > 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
Hi On Tue, Jun 13, 2017 at 11:56 AM, Benjamin Tissoireswrote: >> > - 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 Thanks David
Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex
Hi On Tue, Jun 13, 2017 at 11:56 AM, Benjamin Tissoires 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 Thanks David
Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex
Hi Arnd, On 13 June 2017 at 15:15, Arnd Bergmannwrote: > 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
Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex
Hi Arnd, On 13 June 2017 at 15:15, Arnd Bergmann 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
Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex
Hi, On 13 June 2017 at 15:26, Benjamin Tissoireswrote: >> 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
Hi, On 13 June 2017 at 15:26, Benjamin Tissoires 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
Hi, On Jun 13 2017 or thereabouts, Arnd Bergmann wrote: > On Tue, Jun 13, 2017 at 11:25 AM, Binoy Jayanwrote: > > 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 > > Suggested-by: Arnd Bergmann > > 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 (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(>driver_lock)) > > - return -EINTR; > > if (down_interruptible(>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(>driver_input_lock); > > -unlock_driver_lock: > > - up(>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(>driver_lock)) > > - return -EINTR; > > if (down_interruptible(>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(>driver_input_lock); > > -unlock_driver_lock: > > - up(>driver_lock); > > +end: > > return ret; > > } > > > > @@ -2745,7 +2739,6 @@ struct hid_device *hid_allocate_device(void) > > init_waitqueue_head(>debug_wait); > > INIT_LIST_HEAD(>debug_list); > > spin_lock_init(>debug_list_lock); > > - sema_init(>driver_lock, 1); > > sema_init(>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
Hi, On Jun 13 2017 or thereabouts, Arnd Bergmann wrote: > On Tue, Jun 13, 2017 at 11:25 AM, Binoy Jayan 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 > > Suggested-by: Arnd Bergmann > > 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 (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(>driver_lock)) > > - return -EINTR; > > if (down_interruptible(>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(>driver_input_lock); > > -unlock_driver_lock: > > - up(>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(>driver_lock)) > > - return -EINTR; > > if (down_interruptible(>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(>driver_input_lock); > > -unlock_driver_lock: > > - up(>driver_lock); > > +end: > > return ret; > > } > > > > @@ -2745,7 +2739,6 @@ struct hid_device *hid_allocate_device(void) > > init_waitqueue_head(>debug_wait); > > INIT_LIST_HEAD(>debug_list); > > spin_lock_init(>debug_list_lock); > > - sema_init(>driver_lock, 1); > > sema_init(>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
On Tue, Jun 13, 2017 at 11:25 AM, Binoy Jayanwrote: > 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 > Suggested-by: Arnd Bergmann 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(>driver_lock)) > - return -EINTR; > if (down_interruptible(>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(>driver_input_lock); > -unlock_driver_lock: > - up(>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(>driver_lock)) > - return -EINTR; > if (down_interruptible(>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(>driver_input_lock); > -unlock_driver_lock: > - up(>driver_lock); > +end: > return ret; > } > > @@ -2745,7 +2739,6 @@ struct hid_device *hid_allocate_device(void) > init_waitqueue_head(>debug_wait); > INIT_LIST_HEAD(>debug_list); > spin_lock_init(>debug_list_lock); > - sema_init(>driver_lock, 1); > sema_init(>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
On Tue, Jun 13, 2017 at 11:25 AM, Binoy Jayan 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 > Suggested-by: Arnd Bergmann 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(>driver_lock)) > - return -EINTR; > if (down_interruptible(>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(>driver_input_lock); > -unlock_driver_lock: > - up(>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(>driver_lock)) > - return -EINTR; > if (down_interruptible(>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(>driver_input_lock); > -unlock_driver_lock: > - up(>driver_lock); > +end: > return ret; > } > > @@ -2745,7 +2739,6 @@ struct hid_device *hid_allocate_device(void) > init_waitqueue_head(>debug_wait); > INIT_LIST_HEAD(>debug_list); > spin_lock_init(>debug_list_lock); > - sema_init(>driver_lock, 1); > sema_init(>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
[PATCH v2] HID: Replace semaphore driver_lock with mutex
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 JayanSuggested-by: Arnd Bergmann --- 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(>driver_lock)) - return -EINTR; if (down_interruptible(>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(>driver_input_lock); -unlock_driver_lock: - up(>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(>driver_lock)) - return -EINTR; if (down_interruptible(>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(>driver_input_lock); -unlock_driver_lock: - up(>driver_lock); +end: return ret; } @@ -2745,7 +2739,6 @@ struct hid_device *hid_allocate_device(void) init_waitqueue_head(>debug_wait); INIT_LIST_HEAD(>debug_list); spin_lock_init(>debug_list_lock); - sema_init(>driver_lock, 1); sema_init(>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
[PATCH v2] HID: Replace semaphore driver_lock with mutex
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 Suggested-by: Arnd Bergmann --- 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(>driver_lock)) - return -EINTR; if (down_interruptible(>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(>driver_input_lock); -unlock_driver_lock: - up(>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(>driver_lock)) - return -EINTR; if (down_interruptible(>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(>driver_input_lock); -unlock_driver_lock: - up(>driver_lock); +end: return ret; } @@ -2745,7 +2739,6 @@ struct hid_device *hid_allocate_device(void) init_waitqueue_head(>debug_wait); INIT_LIST_HEAD(>debug_list); spin_lock_init(>debug_list_lock); - sema_init(>driver_lock, 1); sema_init(>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