On Thu, Apr 21, 2022 at 1:23 PM Angela Czubak <acz@xxxxxxxxxxxx> wrote: > > Hi Dmitry, > > On Wed, Apr 20, 2022 at 10:51 PM Dmitry Torokhov > <dmitry.torokhov@xxxxxxxxx> 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 >
