On Fri, May 6, 2022 at 9:08 AM Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote: > > 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 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 > > >
