| Message ID | CAG_cf+cXnNSyxcrsZncfr2KKFRhiCJ60vHrTt3_aB-Bc7m5eVw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
Hi Andrew, [CCing David Herrmann] On Sun, 25 November 2012 Andrew de los Reyes wrote: > Benjamin Tissoires and Nestor Lopez Casado have been helping me to add > Linux support for new Logitech Touch Mice (T620, T400). After running > into a road-block in hid-core, and solving it with this patch, we > thought it was good to show the community and see if this is okay, or > if there's a better solution that we've missed. > > Thanks for your comments, > -andrew > > This patch separates struct hid_device's driver_lock into two. The > goal is to allow hid device drivers to receive input during their > probe() function call. This is necessary because some drivers need to > communicate with the device to determine parameters needed during > probe (e.g., size of a multi-touch surface). > > Historically, three functions used driver_lock: > > - hid_device_probe: blocks to acquire lock > - hid_device_remove: blocks to acquire lock > - hid_input_report: if locked returns -EBUSY, else acquires lock > > This patch adds another lock (driver_input_lock) which is used to > block input from occurring. The lock behavior is now: > > - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock > - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock > - hid_input_report: if driver_input_lock locked returns -EBUSY, else > acquires driver_input_lock > > This results in no behavior change for existing devices and > drivers. However, during a probe() function call in a driver, that > driver may now selectively unlock driver_input_lock to let input > events come through, then re-lock. That is more or less a new approach to release the restriction added in commit 4ea5454203d991ec85264f64f89ca8855fce69b0 (HID: Fix race condition between driver core and ll-driver). From your suggested change the question could be if releasing the input lock should be connected to hw_start() / hw_stop() calls... Though connecting those together would certainly require some review of existing drivers in order not to reopen the can of worms closed by above mentioned commit. Bruno -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bruno, On Mon, Nov 26, 2012 at 6:56 PM, Bruno Prémont <bonbons@linux-vserver.org> wrote: > Hi Andrew, > > [CCing David Herrmann] > > > On Sun, 25 November 2012 Andrew de los Reyes wrote: >> Benjamin Tissoires and Nestor Lopez Casado have been helping me to add >> Linux support for new Logitech Touch Mice (T620, T400). After running >> into a road-block in hid-core, and solving it with this patch, we >> thought it was good to show the community and see if this is okay, or >> if there's a better solution that we've missed. >> >> Thanks for your comments, >> -andrew >> >> This patch separates struct hid_device's driver_lock into two. The >> goal is to allow hid device drivers to receive input during their >> probe() function call. This is necessary because some drivers need to >> communicate with the device to determine parameters needed during >> probe (e.g., size of a multi-touch surface). >> >> Historically, three functions used driver_lock: >> >> - hid_device_probe: blocks to acquire lock >> - hid_device_remove: blocks to acquire lock >> - hid_input_report: if locked returns -EBUSY, else acquires lock >> >> This patch adds another lock (driver_input_lock) which is used to >> block input from occurring. The lock behavior is now: >> >> - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock >> - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock >> - hid_input_report: if driver_input_lock locked returns -EBUSY, else >> acquires driver_input_lock >> >> This results in no behavior change for existing devices and >> drivers. However, during a probe() function call in a driver, that >> driver may now selectively unlock driver_input_lock to let input >> events come through, then re-lock. > > That is more or less a new approach to release the restriction added in > commit 4ea5454203d991ec85264f64f89ca8855fce69b0 > (HID: Fix race condition between driver core and ll-driver). > > From your suggested change the question could be if releasing the input > lock should be connected to hw_start() / hw_stop() calls... > > Though connecting those together would certainly require some review of > existing drivers in order not to reopen the can of worms closed by above > mentioned commit. > It is clear that the goal is to make commit 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive. The problem is that this commit stops any communication with the device, even configuration communication. Logitech devices use their own protocol on top of the HID standard protocol. For touch devices, this proprietary protocol requires to ask the device for axis ranges, etc... So here, the idea is not to open the can of worm for every hid devices through hw_start() / hw_stop() calls, I think the idea of Andrew is just to allow hid-logitech-dj to get rid of this restriction in some particular circumstances. Consider this as a controlled backdoor of the can of worms :) Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On Mon, Nov 26, 2012 at 7:34 PM, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote: > Hi Bruno, > > On Mon, Nov 26, 2012 at 6:56 PM, Bruno Prémont > <bonbons@linux-vserver.org> wrote: >> Hi Andrew, >> >> [CCing David Herrmann] >> >> >> On Sun, 25 November 2012 Andrew de los Reyes wrote: >>> Benjamin Tissoires and Nestor Lopez Casado have been helping me to add >>> Linux support for new Logitech Touch Mice (T620, T400). After running >>> into a road-block in hid-core, and solving it with this patch, we >>> thought it was good to show the community and see if this is okay, or >>> if there's a better solution that we've missed. >>> >>> Thanks for your comments, >>> -andrew >>> >>> This patch separates struct hid_device's driver_lock into two. The >>> goal is to allow hid device drivers to receive input during their >>> probe() function call. This is necessary because some drivers need to >>> communicate with the device to determine parameters needed during >>> probe (e.g., size of a multi-touch surface). >>> >>> Historically, three functions used driver_lock: >>> >>> - hid_device_probe: blocks to acquire lock >>> - hid_device_remove: blocks to acquire lock >>> - hid_input_report: if locked returns -EBUSY, else acquires lock >>> >>> This patch adds another lock (driver_input_lock) which is used to >>> block input from occurring. The lock behavior is now: >>> >>> - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock >>> - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock >>> - hid_input_report: if driver_input_lock locked returns -EBUSY, else >>> acquires driver_input_lock >>> >>> This results in no behavior change for existing devices and >>> drivers. However, during a probe() function call in a driver, that >>> driver may now selectively unlock driver_input_lock to let input >>> events come through, then re-lock. >> >> That is more or less a new approach to release the restriction added in >> commit 4ea5454203d991ec85264f64f89ca8855fce69b0 >> (HID: Fix race condition between driver core and ll-driver). >> >> From your suggested change the question could be if releasing the input >> lock should be connected to hw_start() / hw_stop() calls... >> >> Though connecting those together would certainly require some review of >> existing drivers in order not to reopen the can of worms closed by above >> mentioned commit. >> > > It is clear that the goal is to make commit > 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive. > The problem is that this commit stops any communication with the > device, even configuration communication. > > Logitech devices use their own protocol on top of the HID standard > protocol. For touch devices, this proprietary protocol requires to ask > the device for axis ranges, etc... > > So here, the idea is not to open the can of worm for every hid devices > through hw_start() / hw_stop() calls, I think the idea of Andrew is > just to allow hid-logitech-dj to get rid of this restriction in some > particular circumstances. > Consider this as a controlled backdoor of the can of worms :) The problems I was facing back then when writing the patch was that a driver wasn't able to properly set itself up before input events might be received. Hence, I locked down the whole .probe function. We should make sure that this is guaranteed for all drivers when fixing this as most of them silently depend on this behavior. Releasing the lock with hw_start() would actually be the most logical thing to do, but the implementation is ugly as we would release the lock inside of the callback that ought to be protected by the lock. Therefore, I didn't do this. Furthermore, it is unclear whether this fixes all the race-conditions of all hid-drivers. The fix you are proposing actually looks pretty nice, although it puts the burden of locking to the hid-driver instead of the hid-core. So every .probe function must go sure that the lock is held when returning. This means if you unlock the input-lock, you need to lock it before calling return so hid-core can unlock it again. This opens a small race-condition where the hid-core might drop important input-events as input-lock is held during "return". Any ideas here? Regards David -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 26, 2012 at 10:54 AM, David Herrmann <dh.herrmann@googlemail.com> wrote: > Hi > > On Mon, Nov 26, 2012 at 7:34 PM, Benjamin Tissoires > <benjamin.tissoires@gmail.com> wrote: >> Hi Bruno, >> >> On Mon, Nov 26, 2012 at 6:56 PM, Bruno Prémont >> <bonbons@linux-vserver.org> wrote: >>> Hi Andrew, >>> >>> [CCing David Herrmann] >>> >>> >>> On Sun, 25 November 2012 Andrew de los Reyes wrote: >>>> Benjamin Tissoires and Nestor Lopez Casado have been helping me to add >>>> Linux support for new Logitech Touch Mice (T620, T400). After running >>>> into a road-block in hid-core, and solving it with this patch, we >>>> thought it was good to show the community and see if this is okay, or >>>> if there's a better solution that we've missed. >>>> >>>> Thanks for your comments, >>>> -andrew >>>> >>>> This patch separates struct hid_device's driver_lock into two. The >>>> goal is to allow hid device drivers to receive input during their >>>> probe() function call. This is necessary because some drivers need to >>>> communicate with the device to determine parameters needed during >>>> probe (e.g., size of a multi-touch surface). >>>> >>>> Historically, three functions used driver_lock: >>>> >>>> - hid_device_probe: blocks to acquire lock >>>> - hid_device_remove: blocks to acquire lock >>>> - hid_input_report: if locked returns -EBUSY, else acquires lock >>>> >>>> This patch adds another lock (driver_input_lock) which is used to >>>> block input from occurring. The lock behavior is now: >>>> >>>> - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock >>>> - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock >>>> - hid_input_report: if driver_input_lock locked returns -EBUSY, else >>>> acquires driver_input_lock >>>> >>>> This results in no behavior change for existing devices and >>>> drivers. However, during a probe() function call in a driver, that >>>> driver may now selectively unlock driver_input_lock to let input >>>> events come through, then re-lock. >>> >>> That is more or less a new approach to release the restriction added in >>> commit 4ea5454203d991ec85264f64f89ca8855fce69b0 >>> (HID: Fix race condition between driver core and ll-driver). >>> >>> From your suggested change the question could be if releasing the input >>> lock should be connected to hw_start() / hw_stop() calls... >>> >>> Though connecting those together would certainly require some review of >>> existing drivers in order not to reopen the can of worms closed by above >>> mentioned commit. >>> >> >> It is clear that the goal is to make commit >> 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive. >> The problem is that this commit stops any communication with the >> device, even configuration communication. >> >> Logitech devices use their own protocol on top of the HID standard >> protocol. For touch devices, this proprietary protocol requires to ask >> the device for axis ranges, etc... >> >> So here, the idea is not to open the can of worm for every hid devices >> through hw_start() / hw_stop() calls, I think the idea of Andrew is >> just to allow hid-logitech-dj to get rid of this restriction in some >> particular circumstances. >> Consider this as a controlled backdoor of the can of worms :) > > The problems I was facing back then when writing the patch was that a > driver wasn't able to properly set itself up before input events might > be received. Hence, I locked down the whole .probe function. We should > make sure that this is guaranteed for all drivers when fixing this as > most of them silently depend on this behavior. > Releasing the lock with hw_start() would actually be the most logical > thing to do, but the implementation is ugly as we would release the > lock inside of the callback that ought to be protected by the lock. > Therefore, I didn't do this. Furthermore, it is unclear whether this > fixes all the race-conditions of all hid-drivers. > > The fix you are proposing actually looks pretty nice, although it puts > the burden of locking to the hid-driver instead of the hid-core. So > every .probe function must go sure that the lock is held when > returning. This means if you unlock the input-lock, you need to lock > it before calling return so hid-core can unlock it again. This opens a > small race-condition where the hid-core might drop important > input-events as input-lock is held during "return". Any ideas here? Good point. I hadn't considered that some drivers may want to do this. In the case that probe() is going to fail and return error, it seems okay to lock the lock within probe() and stop events from coming in, as this driver will cease to be used anyway. So that leaves us with the case that a driver's probe() is successful and wants to not interrupt the flow of incoming events. In that case, the simplest solution seems like a new return value. Existing code returns 0 on success or -errno on failure, it seems. Perhaps a return value of 0x1 could signify that the lock has been already unlocked? -andrew > > Regards > David > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 26 Nov 2012, Benjamin Tissoires wrote: > It is clear that the goal is to make commit > 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive. > The problem is that this commit stops any communication with the > device, even configuration communication. > > Logitech devices use their own protocol on top of the HID standard > protocol. For touch devices, this proprietary protocol requires to ask > the device for axis ranges, etc... > > So here, the idea is not to open the can of worm for every hid devices > through hw_start() / hw_stop() calls, I think the idea of Andrew is > just to allow hid-logitech-dj to get rid of this restriction in some > particular circumstances. > Consider this as a controlled backdoor of the can of worms :) I have been thinking for this for a while, and I wasn't able to come up with anything I'd personally consider substantially better aproach than splitting the lock. So I don't have strong objections at the very moment. This will also allow us to get rid of what commit 596264082f introduced in unifying driver, right? (adding Nestor to CC).
No html now ... Cheers, Nestor On Mon, Dec 3, 2012 at 5:17 AM, Jiri Kosina <jkosina@suse.cz> wrote: > On Mon, 26 Nov 2012, Benjamin Tissoires wrote: > >> It is clear that the goal is to make commit >> 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive. >> The problem is that this commit stops any communication with the >> device, even configuration communication. >> >> Logitech devices use their own protocol on top of the HID standard >> protocol. For touch devices, this proprietary protocol requires to ask >> the device for axis ranges, etc... >> >> So here, the idea is not to open the can of worm for every hid devices >> through hw_start() / hw_stop() calls, I think the idea of Andrew is >> just to allow hid-logitech-dj to get rid of this restriction in some >> particular circumstances. >> Consider this as a controlled backdoor of the can of worms :) > > I have been thinking for this for a while, and I wasn't able to come up > with anything I'd personally consider substantially better aproach than > splitting the lock. So I don't have strong objections at the very moment. > > This will also allow us to get rid of what commit 596264082f introduced in > unifying driver, right? (adding Nestor to CC). That is correct. Moreover, we are planning additional changes to hid-logitech-dj that will somehow revert that commit. As Andrew mentioned earlier, the drivers for Unifying devices need to query several parameters during their probe() execution, and besides this lock issue, there is still another problem with the current hid-logitech-dj strategy. Unifying devices can only be queried when the radio link is active, and as such, we are planning to postpone calling hid_add_device() until we are sure that the device has an active radio link. With both issues resolved, the path will be clear to add native multitouch support for several Unifying devices like t650, t620 and t400. > -- > Jiri Kosina > SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 26 Nov 2012, David Herrmann wrote: > The fix you are proposing actually looks pretty nice, although it puts > the burden of locking to the hid-driver instead of the hid-core. So > every .probe function must go sure that the lock is held when > returning. This means if you unlock the input-lock, you need to lock > it before calling return so hid-core can unlock it again. This opens a > small race-condition where the hid-core might drop important > input-events as input-lock is held during "return". Any ideas here? But why should we care deeply? The lost events wouldn't be the ones that are needed for initialization of the device, as all the probing as been finished already. So they would be 'regular' events sent by the device. And as the initialization hasn't been finished yet, it really can't be predicted which events make it and which don't. Thanks,
On Mon, 3 Dec 2012, Nestor Lopez Casado wrote: > >> It is clear that the goal is to make commit > >> 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive. > >> The problem is that this commit stops any communication with the > >> device, even configuration communication. > >> > >> Logitech devices use their own protocol on top of the HID standard > >> protocol. For touch devices, this proprietary protocol requires to ask > >> the device for axis ranges, etc... > >> > >> So here, the idea is not to open the can of worm for every hid devices > >> through hw_start() / hw_stop() calls, I think the idea of Andrew is > >> just to allow hid-logitech-dj to get rid of this restriction in some > >> particular circumstances. > >> Consider this as a controlled backdoor of the can of worms :) > > > > I have been thinking for this for a while, and I wasn't able to come up > > with anything I'd personally consider substantially better aproach than > > splitting the lock. So I don't have strong objections at the very moment. > > > > This will also allow us to get rid of what commit 596264082f introduced in > > unifying driver, right? (adding Nestor to CC). > > That is correct. Moreover, we are planning additional changes to > hid-logitech-dj that will somehow revert that commit. As Andrew > mentioned earlier, the drivers for Unifying devices need to query > several parameters during their probe() execution, and besides this > lock issue, there is still another problem with the current > hid-logitech-dj strategy. Unifying devices can only be queried when > the radio link is active, and as such, we are planning to postpone > calling hid_add_device() until we are sure that the device has an > active radio link. > > With both issues resolved, the path will be clear to add native > multitouch support for several Unifying devices like t650, t620 and > t400. Nestor, is there anything happening in this area, please?
Hi Jiri, Andrew has been progressing on a patchset that he will soon post. for info: https://github.com/adlr/linux/commits/logitech6 I've been following his updates to hid-logitech-dj as well as hid-logitech-wtp. We will soon get back to you. Thanks for asking! Cheers, Nestor On Tue, Feb 5, 2013 at 4:07 PM, Jiri Kosina <jkosina@suse.cz> wrote: > On Mon, 3 Dec 2012, Nestor Lopez Casado wrote: > >> >> It is clear that the goal is to make commit >> >> 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive. >> >> The problem is that this commit stops any communication with the >> >> device, even configuration communication. >> >> >> >> Logitech devices use their own protocol on top of the HID standard >> >> protocol. For touch devices, this proprietary protocol requires to ask >> >> the device for axis ranges, etc... >> >> >> >> So here, the idea is not to open the can of worm for every hid devices >> >> through hw_start() / hw_stop() calls, I think the idea of Andrew is >> >> just to allow hid-logitech-dj to get rid of this restriction in some >> >> particular circumstances. >> >> Consider this as a controlled backdoor of the can of worms :) >> > >> > I have been thinking for this for a while, and I wasn't able to come up >> > with anything I'd personally consider substantially better aproach than >> > splitting the lock. So I don't have strong objections at the very moment. >> > >> > This will also allow us to get rid of what commit 596264082f introduced in >> > unifying driver, right? (adding Nestor to CC). >> >> That is correct. Moreover, we are planning additional changes to >> hid-logitech-dj that will somehow revert that commit. As Andrew >> mentioned earlier, the drivers for Unifying devices need to query >> several parameters during their probe() execution, and besides this >> lock issue, there is still another problem with the current >> hid-logitech-dj strategy. Unifying devices can only be queried when >> the radio link is active, and as such, we are planning to postpone >> calling hid_add_device() until we are sure that the device has an >> active radio link. >> >> With both issues resolved, the path will be clear to add native >> multitouch support for several Unifying devices like t650, t620 and >> t400. > > Nestor, > > is there anything happening in this area, please? > > -- > Jiri Kosina > SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Nestor On Tue, Feb 5, 2013 at 5:38 PM, Andrew de los Reyes <andrew-vger@gizmolabs.org> wrote: > Yes, I'm hoping to upstream soon as part of a patch series to get WTP > support in. The current version of the commit you are asking about is > specifically here: > https://github.com/adlr/linux/commit/2a4fae572bd88462c4a558a77dc53d9eb3f9e91c > > The main difference from before is that there is a new return value (1) > which signifies that the event lock has been been up()ed, so the hid-core > doesn't need to call up() itself. This way during probe(), a driver can > allow incoming events continuously from midway through probe() indefinitely. The patch looks nice. Some comments: 1) I would prefer constants HID_IS_LOCKED and HID_IS_UNLOCKED or similar instead of 1. This makes the code much more readable than returning 0 or 1. Maybe you can find better names than my suggestions? 2) in hid_device_probe() you can use "ret = -EINTR; goto unlock;" instead of "up(input_lock); return -EINTR;". I think this is more consistent. 3) in hid_device_probe() maybe check that hid_hw_start() doesn't return "1" accidentally? The current code deadlocks if hid_hw_start() would be changed to return 1 (which would be bad as is, but maybe be rather safe than sorry?). 4) Maybe we can add a similar logic to hid_device_remove? That is, hid_device_remove() shouldn't lock input_lock if the driver wants to perform I/O during remove, too. Because this may cause the driver to drop input events while the lock is held. At least the hid-wiimote driver could make use of this feature during removal. 5) In current code, if a driver unlocks input_lock but still fails, it needs to re-lock input_lock and then return the appropriate error code. Is that the intended behavior? 6) Documentation! At least add some comments to hid_driver_probe/remove so others can make use of this feature without looking at other drivers. > If you are interested, I can send just that one commit to the linux-input > list now, instead of waiting on WTP to be ready for posting to linux-input > (which may be a couple weeks). I would prefer if you send an RFC patch to the list so we can all comment on it. btw. how about two helpers: static inline void hid_device_io_start(struct hid_device *d) { up(&d->driver_input_lock); } static inline void hid_device_io_stop(struct hid_device *d) { down(&d->driver_input_lock); } This would guarantee that drivers don't use down_interruptible() (which would be wrong in error-paths in ->probe()) and it would be more readable. The return-value could then be called something like HID_START_IO & HID_IO_RUNNING.. Or we can even set a flag on hid_device in io_start/io_stop so the return-value isn't needed at all. Thanks for the patch! David -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi David, I agree with your suggestions. Nestor On Tue, Feb 5, 2013 at 6:22 PM, David Herrmann <dh.herrmann@googlemail.com> wrote: > Hi Nestor > > On Tue, Feb 5, 2013 at 5:38 PM, Andrew de los Reyes > <andrew-vger@gizmolabs.org> wrote: >> Yes, I'm hoping to upstream soon as part of a patch series to get WTP >> support in. The current version of the commit you are asking about is >> specifically here: >> https://github.com/adlr/linux/commit/2a4fae572bd88462c4a558a77dc53d9eb3f9e91c >> >> The main difference from before is that there is a new return value (1) >> which signifies that the event lock has been been up()ed, so the hid-core >> doesn't need to call up() itself. This way during probe(), a driver can >> allow incoming events continuously from midway through probe() indefinitely. > > The patch looks nice. Some comments: > > 1) I would prefer constants HID_IS_LOCKED and HID_IS_UNLOCKED or > similar instead of 1. This makes the code much more readable than > returning 0 or 1. Maybe you can find better names than my suggestions? > > 2) in hid_device_probe() you can use "ret = -EINTR; goto unlock;" > instead of "up(input_lock); return -EINTR;". I think this is more > consistent. > > 3) in hid_device_probe() maybe check that hid_hw_start() doesn't > return "1" accidentally? The current code deadlocks if hid_hw_start() > would be changed to return 1 (which would be bad as is, but maybe be > rather safe than sorry?). > > 4) Maybe we can add a similar logic to hid_device_remove? That is, > hid_device_remove() shouldn't lock input_lock if the driver wants to > perform I/O during remove, too. Because this may cause the driver to > drop input events while the lock is held. > At least the hid-wiimote driver could make use of this feature during removal. > > 5) In current code, if a driver unlocks input_lock but still fails, it > needs to re-lock input_lock and then return the appropriate error > code. Is that the intended behavior? > > 6) Documentation! At least add some comments to > hid_driver_probe/remove so others can make use of this feature without > looking at other drivers. > >> If you are interested, I can send just that one commit to the linux-input >> list now, instead of waiting on WTP to be ready for posting to linux-input >> (which may be a couple weeks). > > I would prefer if you send an RFC patch to the list so we can all comment on it. > > btw. how about two helpers: > > static inline void hid_device_io_start(struct hid_device *d) > { > up(&d->driver_input_lock); > } > > static inline void hid_device_io_stop(struct hid_device *d) > { > down(&d->driver_input_lock); > } > > This would guarantee that drivers don't use down_interruptible() > (which would be wrong in error-paths in ->probe()) and it would be > more readable. The return-value could then be called something like > HID_START_IO & HID_IO_RUNNING.. Or we can even set a flag on > hid_device in io_start/io_stop so the return-value isn't needed at > all. > > Thanks for the patch! > David > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html On Tue, Feb 5, 2013 at 6:22 PM, David Herrmann <dh.herrmann@googlemail.com> wrote: > Hi Nestor > > On Tue, Feb 5, 2013 at 5:38 PM, Andrew de los Reyes > <andrew-vger@gizmolabs.org> wrote: >> Yes, I'm hoping to upstream soon as part of a patch series to get WTP >> support in. The current version of the commit you are asking about is >> specifically here: >> https://github.com/adlr/linux/commit/2a4fae572bd88462c4a558a77dc53d9eb3f9e91c >> >> The main difference from before is that there is a new return value (1) >> which signifies that the event lock has been been up()ed, so the hid-core >> doesn't need to call up() itself. This way during probe(), a driver can >> allow incoming events continuously from midway through probe() indefinitely. > > The patch looks nice. Some comments: > > 1) I would prefer constants HID_IS_LOCKED and HID_IS_UNLOCKED or > similar instead of 1. This makes the code much more readable than > returning 0 or 1. Maybe you can find better names than my suggestions? > > 2) in hid_device_probe() you can use "ret = -EINTR; goto unlock;" > instead of "up(input_lock); return -EINTR;". I think this is more > consistent. > > 3) in hid_device_probe() maybe check that hid_hw_start() doesn't > return "1" accidentally? The current code deadlocks if hid_hw_start() > would be changed to return 1 (which would be bad as is, but maybe be > rather safe than sorry?). > > 4) Maybe we can add a similar logic to hid_device_remove? That is, > hid_device_remove() shouldn't lock input_lock if the driver wants to > perform I/O during remove, too. Because this may cause the driver to > drop input events while the lock is held. > At least the hid-wiimote driver could make use of this feature during removal. > > 5) In current code, if a driver unlocks input_lock but still fails, it > needs to re-lock input_lock and then return the appropriate error > code. Is that the intended behavior? > > 6) Documentation! At least add some comments to > hid_driver_probe/remove so others can make use of this feature without > looking at other drivers. > >> If you are interested, I can send just that one commit to the linux-input >> list now, instead of waiting on WTP to be ready for posting to linux-input >> (which may be a couple weeks). > > I would prefer if you send an RFC patch to the list so we can all comment on it. > > btw. how about two helpers: > > static inline void hid_device_io_start(struct hid_device *d) > { > up(&d->driver_input_lock); > } > > static inline void hid_device_io_stop(struct hid_device *d) > { > down(&d->driver_input_lock); > } > > This would guarantee that drivers don't use down_interruptible() > (which would be wrong in error-paths in ->probe()) and it would be > more readable. The return-value could then be called something like > HID_START_IO & HID_IO_RUNNING.. Or we can even set a flag on > hid_device in io_start/io_stop so the return-value isn't needed at > all. > > Thanks for the patch! > David > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 5 Feb 2013, David Herrmann wrote: > > Yes, I'm hoping to upstream soon as part of a patch series to get WTP > > support in. The current version of the commit you are asking about is > > specifically here: > > https://github.com/adlr/linux/commit/2a4fae572bd88462c4a558a77dc53d9eb3f9e91c > > > > The main difference from before is that there is a new return value (1) > > which signifies that the event lock has been been up()ed, so the hid-core > > doesn't need to call up() itself. This way during probe(), a driver can > > allow incoming events continuously from midway through probe() indefinitely. > > The patch looks nice. Some comments: > > 1) I would prefer constants HID_IS_LOCKED and HID_IS_UNLOCKED or > similar instead of 1. This makes the code much more readable than > returning 0 or 1. Maybe you can find better names than my suggestions? > > 2) in hid_device_probe() you can use "ret = -EINTR; goto unlock;" > instead of "up(input_lock); return -EINTR;". I think this is more > consistent. > > 3) in hid_device_probe() maybe check that hid_hw_start() doesn't > return "1" accidentally? The current code deadlocks if hid_hw_start() > would be changed to return 1 (which would be bad as is, but maybe be > rather safe than sorry?). > > 4) Maybe we can add a similar logic to hid_device_remove? That is, > hid_device_remove() shouldn't lock input_lock if the driver wants to > perform I/O during remove, too. Because this may cause the driver to > drop input events while the lock is held. > At least the hid-wiimote driver could make use of this feature during removal. David, what use-case exactly are you thinking of here, please? Thanks,
Hi Jiri On Wed, Feb 6, 2013 at 4:10 PM, Jiri Kosina <jkosina@suse.cz> wrote: > On Tue, 5 Feb 2013, David Herrmann wrote: > >> > Yes, I'm hoping to upstream soon as part of a patch series to get WTP >> > support in. The current version of the commit you are asking about is >> > specifically here: >> > https://github.com/adlr/linux/commit/2a4fae572bd88462c4a558a77dc53d9eb3f9e91c >> > >> > The main difference from before is that there is a new return value (1) >> > which signifies that the event lock has been been up()ed, so the hid-core >> > doesn't need to call up() itself. This way during probe(), a driver can >> > allow incoming events continuously from midway through probe() indefinitely. >> >> The patch looks nice. Some comments: >> >> 1) I would prefer constants HID_IS_LOCKED and HID_IS_UNLOCKED or >> similar instead of 1. This makes the code much more readable than >> returning 0 or 1. Maybe you can find better names than my suggestions? >> >> 2) in hid_device_probe() you can use "ret = -EINTR; goto unlock;" >> instead of "up(input_lock); return -EINTR;". I think this is more >> consistent. >> >> 3) in hid_device_probe() maybe check that hid_hw_start() doesn't >> return "1" accidentally? The current code deadlocks if hid_hw_start() >> would be changed to return 1 (which would be bad as is, but maybe be >> rather safe than sorry?). >> >> 4) Maybe we can add a similar logic to hid_device_remove? That is, >> hid_device_remove() shouldn't lock input_lock if the driver wants to >> perform I/O during remove, too. Because this may cause the driver to >> drop input events while the lock is held. >> At least the hid-wiimote driver could make use of this feature during removal. > > David, > > what use-case exactly are you thinking of here, please? If you unbind a driver via sysfs, hid-wiimote could suspend several peripherals of a Wii Remote to safe energy. I don't know whether that's a use-case that we actively support, but I thought it would be nice to have if we are on it, anyway. I know that during normal device removal no I/O is possible (obviously), but during manual probe/remove this might happen. Regards David -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 4da66b4..3eb4398 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1097,7 +1097,7 @@ 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_lock)) + if (down_trylock(&hid->driver_input_lock)) return -EBUSY; if (!hid->driver) { @@ -1150,7 +1150,7 @@ nomem: hid_report_raw_event(hid, type, data, size, interrupt); unlock: - up(&hid->driver_lock); + up(&hid->driver_input_lock); return ret; } EXPORT_SYMBOL_GPL(hid_input_report); @@ -1703,6 +1703,10 @@ static int hid_device_probe(struct device *dev) if (down_interruptible(&hdev->driver_lock)) return -EINTR; + if (down_interruptible(&hdev->driver_input_lock)) { + up(&hdev->driver_lock); + return -EINTR; + } if (!hdev->driver) { id = hid_match_device(hdev, hdrv); @@ -1726,6 +1730,7 @@ static int hid_device_probe(struct device *dev) hdev->driver = NULL; } unlock: + up(&hdev->driver_input_lock); up(&hdev->driver_lock); return ret; } @@ -1737,6 +1742,10 @@ static int hid_device_remove(struct device *dev) if (down_interruptible(&hdev->driver_lock)) return -EINTR; + if (down_interruptible(&hdev->driver_input_lock)) { + up(&hdev->driver_lock); + return -EINTR; + } hdrv = hdev->driver; if (hdrv) { @@ -1747,6 +1756,7 @@ static int hid_device_remove(struct device *dev) hdev->driver = NULL; } + up(&hdev->driver_input_lock); up(&hdev->driver_lock); return 0; } @@ -2126,6 +2136,7 @@ struct hid_device *hid_allocate_device(void) init_waitqueue_head(&hdev->debug_wait); INIT_LIST_HEAD(&hdev->debug_list); sema_init(&hdev->driver_lock, 1); + sema_init(&hdev->driver_input_lock, 1); return hdev; err: diff --git a/include/linux/hid.h b/include/linux/hid.h index 3a95da6..7e9d235 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -481,7 +481,8 @@ struct hid_device { /* device report descriptor */ unsigned country; /* HID country */ struct hid_report_enum report_enum[HID_REPORT_TYPES]; - struct semaphore driver_lock; /* protects the current driver */ + struct semaphore 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;