diff mbox series

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

Message ID 20220118072628.1617172-3-dmitry.torokhov@gmail.com (mailing list archive)
State New
Delegated to: Jiri Kosina
Headers show
Series i2c-hid: fixes for unnumbered reports and other improvements | expand

Commit Message

Dmitry Torokhov Jan. 18, 2022, 7:26 a.m. UTC
Internally kernel prepends all report buffers, for both numbered and
unnumbered reports, with report ID, therefore to properly handle unnumbered
reports we should

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@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 32 ++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

Comments

Benjamin Tissoires Feb. 3, 2022, 2:16 p.m. UTC | #1
On Tue, Jan 18, 2022 at 8:26 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> 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@gmail.com>
> ---
>  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
>
diff mbox series

Patch

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);