Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <31ce32e018a9fa410e9e1f3e5900621b16a56091.camel@hadess.net>
Subject: Re: [PATCH] HID: logitech-hidpp: rework one more time the retries
 attempts
From:   Bastien Nocera <hadess@hadess.net>
To:     Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        Greg KH <gregkh@linuxfoundation.org>
Cc:     Filipe =?ISO-8859-1?Q?La=EDns?= <lains@riseup.net>,
        Jiri Kosina <jikos@kernel.org>, linux-input@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Date:   Sun, 25 Jun 2023 10:29:59 +0200
In-Reply-To: <qbvmv3eexohswyagmllfh3xsxoftwa3wbmsdafmwak2bxlnlft@jz74dijlfxlz>
References: <20230621-logitech-fixes-v1-1-32e70933c0b0@redhat.com>
         <2023062156-trespass-pandemic-7f4f@gregkh>
         <qbvmv3eexohswyagmllfh3xsxoftwa3wbmsdafmwak2bxlnlft@jz74dijlfxlz>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
User-Agent: Evolution 3.48.3 (3.48.3-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

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@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.

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.