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

From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Bastien Nocera <hadess@hadess.net>
Cc: "Filipe Laíns" <lains@riseup.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: Mon, 26 Jun 2023 16:01:09 +0200	[thread overview]
Message-ID: <CAO-hwJJCvVUKgxZVwnksd7t5By6vc4UBzAOARS1WcNkbs2XWTA@mail.gmail.com> (raw)
In-Reply-To: <df4cc4a907c6d617036aea6da6f06de6bba30ca1.camel@hadess.net>

On Sun, Jun 25, 2023 at 10:30 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Wed, 2023-06-21 at 11:42 +0200, Benjamin Tissoires wrote:
> > Make the code looks less like Pascal.
>
> Honestly, while this was written in jest in an email is fine, putting
> this in the commit message is quite insulting.
>
> The "retry" patch tried to fix real world problems by making minimal
> code changes, eg. avoiding the review problem that the present patch
> has, and even then, all of us missed the logic bug.
>
> I also haven't written any Pascal code since 1996.

Apologies for that. I honestly took Linus' remark to myself only,
because I was fixing your fix on my original code.
And while initially fixing your for loop, I should have realized that
this was very hard to follow, because of the "if (sth; sth < 1 && foo
&& bar; sth+=1)".

I'll amend v2

>
> > 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),
>
> "wasn't either".
>
> > 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)
> >  {
> > -       int ret = -1;
> > -       int max_retries = 3;
> > -
> > -       mutex_lock(&hidpp->send_mutex);
> > +       int ret;
> >
> >         hidpp->send_receive_buf = response;
> >         hidpp->answer_available = false;
> > @@ -300,41 +299,62 @@ static int hidpp_send_message_sync(struct
> > hidpp_device *hidpp,
> >          */
> >         *response = *message;
> >
> > -       for (; max_retries != 0 && ret; max_retries--) {
> > -               ret = __hidpp_send_report(hidpp->hid_dev, message);
> > +       ret = __hidpp_send_report(hidpp->hid_dev, message);
> > +       if (ret) {
> > +               dbg_hid("__hidpp_send_report returned err: %d\n",
> > ret);
> > +               memset(response, 0, sizeof(struct hidpp_report));
> > +               return ret;
> > +       }
> >
> > -               if (ret) {
> > -                       dbg_hid("__hidpp_send_report returned err:
> > %d\n", ret);
> > -                       memset(response, 0, sizeof(struct
> > hidpp_report));
> > -                       break;
> > -               }
> > +       if (!wait_event_timeout(hidpp->wait, hidpp->answer_available,
> > +                               5*HZ)) {
> > +               dbg_hid("%s:timeout waiting for response\n",
> > __func__);
> > +               memset(response, 0, sizeof(struct hidpp_report));
> > +               return -ETIMEDOUT;
> > +       }
> >
> > -               if (!wait_event_timeout(hidpp->wait, hidpp-
> > >answer_available,
> > -                                       5*HZ)) {
> > -                       dbg_hid("%s:timeout waiting for response\n",
> > __func__);
> > -                       memset(response, 0, sizeof(struct
> > hidpp_report));
> > -                       ret = -ETIMEDOUT;
> > -                       break;
> > -               }
> > +       if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> > +           response->rap.sub_id == HIDPP_ERROR) {
> > +               ret = response->rap.params[1];
> > +               dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
> > +               return ret;
> > +       }
> >
> > -               if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> > -                   response->rap.sub_id == HIDPP_ERROR) {
> > -                       ret = response->rap.params[1];
> > -                       dbg_hid("%s:got hidpp error %02X\n",
> > __func__, ret);
> > +       if ((response->report_id == REPORT_ID_HIDPP_LONG ||
> > +            response->report_id == REPORT_ID_HIDPP_VERY_LONG) &&
> > +           response->fap.feature_index == HIDPP20_ERROR) {
> > +               ret = response->fap.params[1];
> > +               dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__,
> > ret);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * 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...)
>
> Do we really need to re-explain the possible return values that were
> already explained above __do_hidpp_send_message_sync()?

Right, maybe we don't need to duplicate the comment after all.

>
> If we do, why don't also do it for hidpp_send_fap_command_sync() and
> hidpp_send_rap_command_sync(), or their callers?

In a way it would make sense to do, because this is non standard.

>
> If it's absolutely necessary, a "see __do_hidpp_send_message_sync()"
> should be enough.

Good point.

>
> I've double-checked that none of the existing callers expected a
> partially filled in "response" struct on error.
>
> Reviewed-by: Bastien Nocera <hadess@hadess.net>

Thanks!

Cheers,
Benjamin

>
> > + */
> > +static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> > +       struct hidpp_report *message,
> > +       struct hidpp_report *response)
> > +{
> > +       int ret;
> > +       int max_retries = 3;
> > +
> > +       mutex_lock(&hidpp->send_mutex);
> > +
> > +       do {
> > +               ret = __do_hidpp_send_message_sync(hidpp, message,
> > response);
> > +               if (ret != HIDPP20_ERROR_BUSY)
> >                         break;
> > -               }
> >
> > -               if ((response->report_id == REPORT_ID_HIDPP_LONG ||
> > -                    response->report_id ==
> > REPORT_ID_HIDPP_VERY_LONG) &&
> > -                   response->fap.feature_index == HIDPP20_ERROR) {
> > -                       ret = response->fap.params[1];
> > -                       if (ret != HIDPP20_ERROR_BUSY) {
> > -                               dbg_hid("%s:got hidpp 2.0 error
> > %02X\n", __func__, ret);
> > -                               break;
> > -                       }
> > -                       dbg_hid("%s:got busy hidpp 2.0 error %02X,
> > retrying\n", __func__, ret);
> > -               }
> > -       }
> > +               dbg_hid("%s:got busy hidpp 2.0 error %02X,
> > retrying\n", __func__, ret);
> > +       } while (--max_retries);
> >
> >         mutex_unlock(&hidpp->send_mutex);
> >         return ret;
> >
> > ---
> > base-commit: b98ec211af5508457e2b1c4cc99373630a83fa81
> > change-id: 20230621-logitech-fixes-a4c0e66ea2ad
> >
> > Best regards,
>