Re: [PATCH 02/12] HID: i2c-hid: fix GET/SET_REPORT for unnumbered reports

[Benjamin Tissoires]

On Tue, Jan 18, 2022 at 8:26 AM Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
>
> Internally kernel prepends all report buffers, for both numbered and
> unnumbered reports, with report ID, therefore to properly handle unnumbered
> reports we should

Nitpick but it seems that this sentence is not

:)

Cheers,
Benjamin

>
> For the same reason we should skip the first byte of the buffer when
> calling i2c_hid_set_or_send_report() which then will take care of properly
> formatting the transfer buffer based on its separate report ID argument
> along with report payload.
>
> Fixes: 9b5a9ae88573 ("HID: i2c-hid: implement ll_driver transport-layer callbacks")
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 32 ++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index bd7b0eeca3ea..b383003ff676 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -611,6 +611,17 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
>         if (report_type == HID_OUTPUT_REPORT)
>                 return -EINVAL;
>
> +       /*
> +        * In case of unnumbered reports the response from the device will
> +        * not have the report ID that the upper layers expect, so we need
> +        * to stash it the buffer ourselves and adjust the data size.
> +        */
> +       if (!report_number) {
> +               buf[0] = 0;
> +               buf++;
> +               count--;
> +       }
> +
>         /* +2 bytes to include the size of the reply in the query buffer */
>         ask_count = min(count + 2, (size_t)ihid->bufsize);
>
> @@ -632,6 +643,9 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
>         count = min(count, ret_count - 2);
>         memcpy(buf, ihid->rawbuf + 2, count);
>
> +       if (!report_number)
> +               count++;
> +
>         return count;
>  }
>
> @@ -648,17 +662,19 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
>
>         mutex_lock(&ihid->reset_lock);
>
> -       if (report_id) {
> -               buf++;
> -               count--;
> -       }
> -
> +       /*
> +        * Note that both numbered and unnumbered reports passed here
> +        * are supposed to have report ID stored in the 1st byte of the
> +        * buffer, so we strip it off unconditionally before passing payload
> +        * to i2c_hid_set_or_send_report which takes care of encoding
> +        * everything properly.
> +        */
>         ret = i2c_hid_set_or_send_report(client,
>                                 report_type == HID_FEATURE_REPORT ? 0x03 : 0x02,
> -                               report_id, buf, count, use_data);
> +                               report_id, buf + 1, count - 1, use_data);
>
> -       if (report_id && ret >= 0)
> -               ret++; /* add report_id to the number of transfered bytes */
> +       if (ret >= 0)
> +               ret++; /* add report_id to the number of transferred bytes */
>
>         mutex_unlock(&ihid->reset_lock);
>
> --
> 2.34.1.703.g22d0c6ccf7-goog
>