| Message ID | 20220726080504.4185715-1-tero.kristo@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: i2c-hid: fix the report-id passed in via set_or_send_report | expand |
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 >
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 >>
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;
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(+)