Re: [PATCH] HID: add HID device reset callback

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

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
>