* [PATCH] HID: add HID device reset callback @ 2022-04-19 12:26 Angela Czubak 2022-04-20 20:51 ` Dmitry Torokhov 0 siblings, 1 reply; 5+ messages in thread From: Angela Czubak @ 2022-04-19 12:26 UTC (permalink / raw) To: linux-input Cc: upstream, benjamin.tissoires, jikos, dmitry.torokhov, Angela Czubak HID-over-I2C devices might reset on their own. Any device configuration applied before the reset might be brought back to defaults so we need to reconfigure to make sure the driver state is consistent. Add a reset callback to the hid driver structure. Issue it if the driver implements it and the device reset gets observed. Signed-off-by: Angela Czubak <acz@semihalf.com> --- Hi guys, This is a patch introducing callbacks to be issued when device reset gets noticed. The discussion started in one of the previous linux-input threads [0]. Please let me know if there is anything to fix. Regards, Angela [0] https://lore.kernel.org/linux-input/CAO-hwJ+OgLMkAy+Ms1xgHz3RTYxS+5LCSSP3njju+joTYWZMqA@mail.gmail.com/T/#t drivers/hid/i2c-hid/i2c-hid-core.c | 2 ++ include/linux/hid.h | 2 ++ 2 files changed, 4 insertions(+) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index c078f09a2318..96739d7a4191 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -529,6 +529,8 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) /* host or device initiated RESET completed */ if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)) wake_up(&ihid->wait); + if (ihid->hid && ihid->hid->driver && ihid->hid->driver->reset) + ihid->hid->driver->reset(ihid->hid); return; } diff --git a/include/linux/hid.h b/include/linux/hid.h index 4363a63b9775..50b9dde2c870 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -755,6 +755,7 @@ struct hid_usage_id { * @suspend: invoked on suspend (NULL means nop) * @resume: invoked on resume if device was not reset (NULL means nop) * @reset_resume: invoked on resume if device was reset (NULL means nop) + * @reset: invoked if device was reset (NULL means nop) * * probe should return -errno on error, or 0 on success. During probe, * input will not be passed to raw_event unless hid_device_io_start is @@ -811,6 +812,7 @@ struct hid_driver { int (*resume)(struct hid_device *hdev); int (*reset_resume)(struct hid_device *hdev); #endif + int (*reset)(struct hid_device *hdev); /* private: */ struct device_driver driver; }; -- 2.36.0.rc2.479.g8af0fa9b8e-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] HID: add HID device reset callback 2022-04-19 12:26 [PATCH] HID: add HID device reset callback Angela Czubak @ 2022-04-20 20:51 ` Dmitry Torokhov 2022-04-21 11:23 ` Angela Czubak 0 siblings, 1 reply; 5+ messages in thread From: Dmitry Torokhov @ 2022-04-20 20:51 UTC (permalink / raw) To: Angela Czubak; +Cc: linux-input, upstream, benjamin.tissoires, jikos Hi Angela, On Tue, Apr 19, 2022 at 12:26:32PM +0000, Angela Czubak wrote: > @@ -529,6 +529,8 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) > /* host or device initiated RESET completed */ > if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)) > wake_up(&ihid->wait); > + if (ihid->hid && ihid->hid->driver && ihid->hid->driver->reset) > + ihid->hid->driver->reset(ihid->hid); I wonder if this would not be better to execute the reset callback first, before signalling that the reset has completed instead of racing with i2c_hid_hw_reset()? Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] HID: add HID device reset callback 2022-04-20 20:51 ` Dmitry Torokhov @ 2022-04-21 11:23 ` Angela Czubak 2022-05-06 7:08 ` Benjamin Tissoires 0 siblings, 1 reply; 5+ messages in thread From: Angela Czubak @ 2022-04-21 11:23 UTC (permalink / raw) To: Dmitry Torokhov Cc: open list:HID CORE LAYER, upstream, Benjamin Tissoires, Jiri Kosina Hi Dmitry, On Wed, Apr 20, 2022 at 10:51 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > Hi Angela, > > On Tue, Apr 19, 2022 at 12:26:32PM +0000, Angela Czubak wrote: > > @@ -529,6 +529,8 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) > > /* host or device initiated RESET completed */ > > if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)) > > wake_up(&ihid->wait); > > + if (ihid->hid && ihid->hid->driver && ihid->hid->driver->reset) > > + ihid->hid->driver->reset(ihid->hid); > > I wonder if this would not be better to execute the reset callback > first, before signalling that the reset has completed instead of racing > with i2c_hid_hw_reset()? > I think it could result in a deadlock. If we don't clear I2C_HID_RESET_PENDING, and if it has been set, then reset_lock is still taken. This way, if the reset callback wants to send a report to the device, it will keep spinning on reset_lock in i2c_hid_output_raw_report(). Since the reset callback will be most likely used to re-configure the device, we need to be able to send any report and not hang on reset_lock. Let me know if you think this not an issue or there is an additional comment needed in the patch so that the reasoning standing by the order of issuing the callback and clearing the bit is clear. > Thanks. > > -- > Dmitry ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] HID: add HID device reset callback 2022-04-21 11:23 ` Angela Czubak @ 2022-05-06 7:08 ` Benjamin Tissoires 2022-05-10 17:23 ` Angela Czubak 0 siblings, 1 reply; 5+ messages in thread From: Benjamin Tissoires @ 2022-05-06 7:08 UTC (permalink / raw) To: Angela Czubak Cc: Dmitry Torokhov, open list:HID CORE LAYER, upstream, Jiri Kosina On Thu, Apr 21, 2022 at 1:23 PM Angela Czubak <acz@semihalf.com> wrote: > > Hi Dmitry, > > On Wed, Apr 20, 2022 at 10:51 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > Hi Angela, > > > > On Tue, Apr 19, 2022 at 12:26:32PM +0000, Angela Czubak wrote: > > > @@ -529,6 +529,8 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) > > > /* host or device initiated RESET completed */ > > > if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)) > > > wake_up(&ihid->wait); > > > + if (ihid->hid && ihid->hid->driver && ihid->hid->driver->reset) > > > + ihid->hid->driver->reset(ihid->hid); > > > > I wonder if this would not be better to execute the reset callback > > first, before signalling that the reset has completed instead of racing > > with i2c_hid_hw_reset()? > > > > I think it could result in a deadlock. If we don't clear > I2C_HID_RESET_PENDING, and if it has been set, then reset_lock > is still taken. This way, if the reset callback wants to send a report > to the device, it will keep spinning on reset_lock > in i2c_hid_output_raw_report(). > Since the reset callback will be most likely used to re-configure > the device, we need to be able to send any report and not hang > on reset_lock. > Let me know if you think this not an issue or there is an additional > comment needed in the patch so that the reasoning standing > by the order of issuing the callback and clearing the bit is clear. I think you are both correct, and that this patch thus needs some changes: - first, I'd like to have one user at least of this reset callback in a subsequent patch. Adding one callback without user is just adding dead code - then there are 2 types of reset that probably each need a special treatment: * host initiated resets: those are the ones "racing" with i2c_hid_hwreset(), in a sense that this function call might also call POWER_ON on some devices, which means we can not immediately do transfers to the device with this current code * device initiated resets (when I2C_HID_RESET_PENDING is not set): that code is fine in that case, because we have no other entry point - there is a third type of resets happening: on probe and resume, so maybe there we do not want to call this callback simply because we already have probe and reset_resume callbacks. Cheers, Benjamin > > > Thanks. > > > > -- > > Dmitry > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] HID: add HID device reset callback 2022-05-06 7:08 ` Benjamin Tissoires @ 2022-05-10 17:23 ` Angela Czubak 0 siblings, 0 replies; 5+ messages in thread From: Angela Czubak @ 2022-05-10 17:23 UTC (permalink / raw) To: Benjamin Tissoires Cc: Dmitry Torokhov, open list:HID CORE LAYER, upstream, Jiri Kosina On Fri, May 6, 2022 at 9:08 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Thu, Apr 21, 2022 at 1:23 PM Angela Czubak <acz@semihalf.com> wrote: > > > > Hi Dmitry, > > > > On Wed, Apr 20, 2022 at 10:51 PM Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > > > > Hi Angela, > > > > > > On Tue, Apr 19, 2022 at 12:26:32PM +0000, Angela Czubak wrote: > > > > @@ -529,6 +529,8 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) > > > > /* host or device initiated RESET completed */ > > > > if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)) > > > > wake_up(&ihid->wait); > > > > + if (ihid->hid && ihid->hid->driver && ihid->hid->driver->reset) > > > > + ihid->hid->driver->reset(ihid->hid); > > > > > > I wonder if this would not be better to execute the reset callback > > > first, before signalling that the reset has completed instead of racing > > > with i2c_hid_hw_reset()? > > > > > > > I think it could result in a deadlock. If we don't clear > > I2C_HID_RESET_PENDING, and if it has been set, then reset_lock > > is still taken. This way, if the reset callback wants to send a report > > to the device, it will keep spinning on reset_lock > > in i2c_hid_output_raw_report(). > > Since the reset callback will be most likely used to re-configure > > the device, we need to be able to send any report and not hang > > on reset_lock. > > Let me know if you think this not an issue or there is an additional > > comment needed in the patch so that the reasoning standing > > by the order of issuing the callback and clearing the bit is clear. > > I think you are both correct, and that this patch thus needs some changes: > - first, I'd like to have one user at least of this reset callback in > a subsequent patch. Adding one callback without user is just adding > dead code ACK, I will send it with a new version of haptic patches. > - then there are 2 types of reset that probably each need a special treatment: > * host initiated resets: those are the ones "racing" with > i2c_hid_hwreset(), in a sense that this function call might also call > POWER_ON on some devices, which means we can not immediately do > transfers to the device with this current code > * device initiated resets (when I2C_HID_RESET_PENDING is not set): > that code is fine in that case, because we have no other entry point > - there is a third type of resets happening: on probe and resume, so > maybe there we do not want to call this callback simply because we > already have probe and reset_resume callbacks. Now that I look at the code it looks as if the reset callback should not be able to take reset_lock at all as it will be executed in the interrupt context. I am not sure if I understand all of the issue, so here is my plan: - issue the reset callback only if the I2C_HID_RESET_PENDING bit has not been set, - add a comment stating that the callback must not wait/sleep as it will be called in the interrupt context (any feature reports must be deferred). Are there any races left in this scenario? I suppose reset_lock should be enough to make sure we don't send any feature reports when i2c_hid_hwreset() is being executed. > > Cheers, > Benjamin > > > > > > Thanks. > > > > > > -- > > > Dmitry > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-05-10 17:24 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-19 12:26 [PATCH] HID: add HID device reset callback Angela Czubak 2022-04-20 20:51 ` Dmitry Torokhov 2022-04-21 11:23 ` Angela Czubak 2022-05-06 7:08 ` Benjamin Tissoires 2022-05-10 17:23 ` Angela Czubak
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.