Re: [PATCH] HID: logitech-hidpp: rework one more time the retries attempts

[Bastien Nocera]

On Fri, 2023-06-23 at 10:37 +0200, Benjamin Tissoires wrote:
> 
> On Jun 21 2023, Greg KH wrote:
> > 
> > On Wed, Jun 21, 2023 at 11:42:30AM +0200, Benjamin Tissoires wrote:
> > > Make the code looks less like Pascal.
> > > 
> > > Extract the internal code inside a helper function, fix the
> > > initialization of the parameters used in the helper function
> > > (`hidpp->answer_available` was not reset and `*response` wasn't
> > > too),
> > > and use a `do {...} while();` loop.
> > > 
> > > Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when
> > > device is busy")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> > > ---
> > > as requested by
> > > https://lore.kernel.org/all/CAHk-=wiMbF38KCNhPFiargenpSBoecSXTLQACKS2UMyo_Vu2ww@xxxxxxxxxxxxxx/
> > > This is a rewrite of that particular piece of code.
> > > ---
> > >  drivers/hid/hid-logitech-hidpp.c | 102 +++++++++++++++++++++++--
> > > --------------
> > >  1 file changed, 61 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> > > logitech-hidpp.c
> > > index dfe8e09a18de..3d1ffe199f08 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -275,21 +275,20 @@ static int __hidpp_send_report(struct
> > > hid_device *hdev,
> > >  }
> > >  
> > >  /*
> > > - * hidpp_send_message_sync() returns 0 in case of success, and
> > > something else
> > > - * in case of a failure.
> > > - * - If ' something else' is positive, that means that an error
> > > has been raised
> > > - *   by the protocol itself.
> > > - * - If ' something else' is negative, that means that we had a
> > > classic error
> > > - *   (-ENOMEM, -EPIPE, etc...)
> > > + * Effectively send the message to the device, waiting for its
> > > answer.
> > > + *
> > > + * Must be called with hidpp->send_mutex locked
> > > + *
> > > + * Same return protocol than hidpp_send_message_sync():
> > > + * - success on 0
> > > + * - negative error means transport error
> > > + * - positive value means protocol error
> > >   */
> > > -static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> > > +static int __do_hidpp_send_message_sync(struct hidpp_device
> > > *hidpp,
> > >         struct hidpp_report *message,
> > >         struct hidpp_report *response)
> > 
> > __must_hold(&hidpp->send_mutex)  ?
> > 
> 
> Good point. I'll add this in v2.
> 
> I'm still waiting for some feedback from the people who particpated
> in
> the original BZ, but the new bug is harder to reproduce. Anyway,
> there
> is no rush IMO.

The problem is only ever going to show up in very limited circumstances
after the logic fix was applied.

You need a hardware problem (such as the controller being too busy to
answer) to trigger the problems fixed by this patch. I don't see a way
to reliably reproduce it unless you inject that hardware error.