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

From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: "Filipe Laíns" <lains@riseup.net>,
	"Bastien Nocera" <hadess@hadess.net>,
	"Jiri Kosina" <jikos@kernel.org>,
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH] HID: logitech-hidpp: rework one more time the retries attempts
Date: Fri, 23 Jun 2023 10:37:48 +0200	[thread overview]
Message-ID: <qbvmv3eexohswyagmllfh3xsxoftwa3wbmsdafmwak2bxlnlft@jz74dijlfxlz> (raw)
In-Reply-To: <2023062156-trespass-pandemic-7f4f@gregkh>

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@vger.kernel.org
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> > as requested by https://lore.kernel.org/all/CAHk-=wiMbF38KCNhPFiargenpSBoecSXTLQACKS2UMyo_Vu2ww@mail.gmail.com/
> > 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.

Cheers,
Benjamin