[PATCH] HID: i2c-hid: fix the report-id passed in via set_or_send_report

[PATCH] HID: i2c-hid: fix the report-id passed in via set_or_send_report

[Tero Kristo]

The formatting of the data passed to the i2c HID data register was
changed with the re-work of the i2c-hid-core. Previously the report ID
passed in was encoded as 0xF if the report-id was greater than 0xF
(similar to what is done with the command portion.) Now with the rework,
a full report-id is passed in always, and this causes the messages to be
rejected by the i2c controller. Fix this by encoding the report-id
field in the same manner as previously was done.

Fixes: dbe0dd5fd2e0 ("HID: i2c-hid: explicitly code setting and sending
reports")
Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index c078f09a2318..156b12f840c4 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -296,6 +296,9 @@ static size_t i2c_hid_format_report(u8 *buf, int report_id,
 {
 	size_t length = sizeof(__le16); /* reserve space to store size */
 
+	if (report_id > 0xF)
+		report_id = 0xF;
+
 	if (report_id)
 		buf[length++] = report_id;
 
-- 
2.25.1

Re: [PATCH] HID: i2c-hid: fix the report-id passed in via set_or_send_report

[Angela Czubak]

Hi Tero,

I believe this is not the right solution. To my mind the spec does not
mention anything that the ID in the payload of SET_REPORT command
should be altered; you have a full byte to us, so why not?
Other than that this will result into problems with sending reports
via output register: for any report with report ID >=0xF we will get
the same report ID in the payload, so how could the device know which
one we have in mind?
Dmitry's rework was intended to actually solve problems with large
report IDs being incorrectly overwritten with 0xF.

Regards,
Angela

On Tue, Jul 26, 2022 at 10:05 AM Tero Kristo
<tero.kristo@linux.intel.com> wrote:
>
> The formatting of the data passed to the i2c HID data register was
> changed with the re-work of the i2c-hid-core. Previously the report ID
> passed in was encoded as 0xF if the report-id was greater than 0xF
> (similar to what is done with the command portion.) Now with the rework,
> a full report-id is passed in always, and this causes the messages to be
> rejected by the i2c controller. Fix this by encoding the report-id
> field in the same manner as previously was done.
>
> Fixes: dbe0dd5fd2e0 ("HID: i2c-hid: explicitly code setting and sending
> reports")
> Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index c078f09a2318..156b12f840c4 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -296,6 +296,9 @@ static size_t i2c_hid_format_report(u8 *buf, int report_id,
>  {
>         size_t length = sizeof(__le16); /* reserve space to store size */
>
> +       if (report_id > 0xF)
> +               report_id = 0xF;
> +
>         if (report_id)
>                 buf[length++] = report_id;
>
> --
> 2.25.1
>

Re: [PATCH] HID: i2c-hid: fix the report-id passed in via set_or_send_report

[Tero Kristo]

Hello,

Yes, I think this is a problem. Maybe we need to introduce some sort of 
quirk for the devices that are broken and expect the 0xf device id to be 
passed to them?

-Tero

On 18/08/2022 13:27, Angela Czubak wrote:
> Hi Tero,
>
> I believe this is not the right solution. To my mind the spec does not
> mention anything that the ID in the payload of SET_REPORT command
> should be altered; you have a full byte to us, so why not?
> Other than that this will result into problems with sending reports
> via output register: for any report with report ID >=0xF we will get
> the same report ID in the payload, so how could the device know which
> one we have in mind?
> Dmitry's rework was intended to actually solve problems with large
> report IDs being incorrectly overwritten with 0xF.
>
> Regards,
> Angela
>
> On Tue, Jul 26, 2022 at 10:05 AM Tero Kristo
> <tero.kristo@linux.intel.com> wrote:
>> The formatting of the data passed to the i2c HID data register was
>> changed with the re-work of the i2c-hid-core. Previously the report ID
>> passed in was encoded as 0xF if the report-id was greater than 0xF
>> (similar to what is done with the command portion.) Now with the rework,
>> a full report-id is passed in always, and this causes the messages to be
>> rejected by the i2c controller. Fix this by encoding the report-id
>> field in the same manner as previously was done.
>>
>> Fixes: dbe0dd5fd2e0 ("HID: i2c-hid: explicitly code setting and sending
>> reports")
>> Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
>> ---
>>   drivers/hid/i2c-hid/i2c-hid-core.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
>> index c078f09a2318..156b12f840c4 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>> @@ -296,6 +296,9 @@ static size_t i2c_hid_format_report(u8 *buf, int report_id,
>>   {
>>          size_t length = sizeof(__le16); /* reserve space to store size */
>>
>> +       if (report_id > 0xF)
>> +               report_id = 0xF;
>> +
>>          if (report_id)
>>                  buf[length++] = report_id;
>>
>> --
>> 2.25.1
>>