On Thu, Apr 21, 2022 at 1:23 PM Angela Czubak [off-list ref] wrote:

Hi Dmitry,

On Wed, Apr 20, 2022 at 10:51 PM Dmitry Torokhov
[off-list ref] wrote:

quoted

Hi Angela,

On Tue, Apr 19, 2022 at 12:26:32PM +0000, Angela Czubak wrote:

quoted

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

quoted

Thanks.

--
Dmitry