lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 25 Jun 2023 10:29:54 +0200
From:   Bastien Nocera <hadess@...ess.net>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Filipe Laíns <lains@...eup.net>,
        Jiri Kosina <jikos@...nel.org>
Cc:     linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH] HID: logitech-hidpp: rework one more time the retries
 attempts

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.

> 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@...r.kernel.org
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.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()?

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

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

I've double-checked that none of the existing callers expected a
partially filled in "response" struct on error.

Reviewed-by: Bastien Nocera <hadess@...ess.net>

> + */
> +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,

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ