Hi,

This series came about after I reviewed Angela's patch that fixed issues
with incorrect handling of high-numbered reports (15 and above) in
i2c-hid driver:

- it appears to me that the driver did not handle unnumbered reports
  correctly as the kernel internally expects unnumbered reports to be
  still prepended with report number 0, but i2c_hid_get_raw_report() and
  i2c_hid_output_raw_report() only expected report ID to be present for
  numbered reports.

- the driver tried to consolidate both command handling and sending
  output reports in __i2c_hid_command() but the rules for different
  commands vary significantly and because of that the logic was hard to
  follow and it bled out from __i2c_hid_command() to callers. I decided
  to introduce a few simple helpers and have the data encoding for
  individual commands done at the call site. I believe this made it
  easier to validate the rules and the logic and allowed to remove
  special handling for the HID descriptor retrieval, among other things.

- the driver does too many copying of data; encoding the data for
  commands at the call site allowed to only copy data once into the
  transfer buffers.

I tested this on a couple of Chromebooks with i2c-hid touchscreens, but
it would be great if other folks tried it out as well.

Thanks.

Angela Czubak (1):
  HID: i2c-hid: fix handling numbered reports with IDs of 15 and above

Dmitry Torokhov (11):
  HID: i2c-hid: fix GET/SET_REPORT for unnumbered reports
  HID: i2c-hid: use "struct i2c_hid" as argument in most calls
  HID: i2c-hid: refactor reset command
  HID: i2c-hid: explicitly code setting and sending reports
  HID: i2c-hid: define i2c_hid_read_register() and use it
  HID: i2c-hid: create a helper for SET_POWER command
  HID: i2c-hid: convert i2c_hid_execute_reset() to use i2c_hid_xfer()
  HID: i2c-hid: rework i2c_hid_get_report() to use i2c_hid_xfer()
  HID: i2c-hid: use helpers to do endian conversion in i2c_hid_get_input()
  HID: i2c-hid: no longer need raw access to HID descriptor structure
  HID: i2c-hid: note that I2C xfer buffers are DMA-safe

 drivers/hid/i2c-hid/i2c-hid-core.c | 593 +++++++++++++++--------------
 1 file changed, 313 insertions(+), 280 deletions(-)

-- 
Dmitry

On Mon, 17 Jan 2022, Dmitry Torokhov wrote:

> Hi,
> 
> This series came about after I reviewed Angela's patch that fixed issues
> with incorrect handling of high-numbered reports (15 and above) in
> i2c-hid driver:
> 
> - it appears to me that the driver did not handle unnumbered reports
>   correctly as the kernel internally expects unnumbered reports to be
>   still prepended with report number 0, but i2c_hid_get_raw_report() and
>   i2c_hid_output_raw_report() only expected report ID to be present for
>   numbered reports.
> 
> - the driver tried to consolidate both command handling and sending
>   output reports in __i2c_hid_command() but the rules for different
>   commands vary significantly and because of that the logic was hard to
>   follow and it bled out from __i2c_hid_command() to callers. I decided
>   to introduce a few simple helpers and have the data encoding for
>   individual commands done at the call site. I believe this made it
>   easier to validate the rules and the logic and allowed to remove
>   special handling for the HID descriptor retrieval, among other things.
> 
> - the driver does too many copying of data; encoding the data for
>   commands at the call site allowed to only copy data once into the
>   transfer buffers.
> 
> I tested this on a couple of Chromebooks with i2c-hid touchscreens, but
> it would be great if other folks tried it out as well.

Benjamin,

is this something you could feed into your testing machinery as well? (not 
sure whether it's handling i2c-hid specifics).

Thanks!

-- 
Jiri Kosina
SUSE Labs

On Wed, Feb 2, 2022 at 2:56 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Mon, 17 Jan 2022, Dmitry Torokhov wrote:
>
> > Hi,
> >
> > This series came about after I reviewed Angela's patch that fixed issues
> > with incorrect handling of high-numbered reports (15 and above) in
> > i2c-hid driver:
> >
> > - it appears to me that the driver did not handle unnumbered reports
> >   correctly as the kernel internally expects unnumbered reports to be
> >   still prepended with report number 0, but i2c_hid_get_raw_report() and
> >   i2c_hid_output_raw_report() only expected report ID to be present for
> >   numbered reports.
> >
> > - the driver tried to consolidate both command handling and sending
> >   output reports in __i2c_hid_command() but the rules for different
> >   commands vary significantly and because of that the logic was hard to
> >   follow and it bled out from __i2c_hid_command() to callers. I decided
> >   to introduce a few simple helpers and have the data encoding for
> >   individual commands done at the call site. I believe this made it
> >   easier to validate the rules and the logic and allowed to remove
> >   special handling for the HID descriptor retrieval, among other things.
> >
> > - the driver does too many copying of data; encoding the data for
> >   commands at the call site allowed to only copy data once into the
> >   transfer buffers.
> >
> > I tested this on a couple of Chromebooks with i2c-hid touchscreens, but
> > it would be great if other folks tried it out as well.
>
> Benjamin,
>
> is this something you could feed into your testing machinery as well? (not
> sure whether it's handling i2c-hid specifics).

Not really. I mean I have a sample touchpad connected on an USB to I2C
bridge that I included in the automated tests (though it may be
failing in the latest release because I need to update the patch that
enables it), but it would probably not catch all the corner cases.

I can add this series to my local tree and test on my XPS with both
and Elan touchpad and a Wacom touchscreen/panel. It will still add a
few more tests :)

I'll try to report that tomorrow now that I think I fixed my tablet series.

Cheers,
Benjamin

>
> Thanks!
>
> --
> Jiri Kosina
> SUSE Labs
>

On Wed, Feb 2, 2022 at 6:59 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Feb 2, 2022 at 2:56 PM Jiri Kosina <jikos@kernel.org> wrote:
> >
> > On Mon, 17 Jan 2022, Dmitry Torokhov wrote:
> >
> > > Hi,
> > >
> > > This series came about after I reviewed Angela's patch that fixed issues
> > > with incorrect handling of high-numbered reports (15 and above) in
> > > i2c-hid driver:
> > >
> > > - it appears to me that the driver did not handle unnumbered reports
> > >   correctly as the kernel internally expects unnumbered reports to be
> > >   still prepended with report number 0, but i2c_hid_get_raw_report() and
> > >   i2c_hid_output_raw_report() only expected report ID to be present for
> > >   numbered reports.
> > >
> > > - the driver tried to consolidate both command handling and sending
> > >   output reports in __i2c_hid_command() but the rules for different
> > >   commands vary significantly and because of that the logic was hard to
> > >   follow and it bled out from __i2c_hid_command() to callers. I decided
> > >   to introduce a few simple helpers and have the data encoding for
> > >   individual commands done at the call site. I believe this made it
> > >   easier to validate the rules and the logic and allowed to remove
> > >   special handling for the HID descriptor retrieval, among other things.
> > >
> > > - the driver does too many copying of data; encoding the data for
> > >   commands at the call site allowed to only copy data once into the
> > >   transfer buffers.
> > >
> > > I tested this on a couple of Chromebooks with i2c-hid touchscreens, but
> > > it would be great if other folks tried it out as well.
> >
> > Benjamin,
> >
> > is this something you could feed into your testing machinery as well? (not
> > sure whether it's handling i2c-hid specifics).
>
> Not really. I mean I have a sample touchpad connected on an USB to I2C
> bridge that I included in the automated tests (though it may be
> failing in the latest release because I need to update the patch that
> enables it), but it would probably not catch all the corner cases.
>
> I can add this series to my local tree and test on my XPS with both
> and Elan touchpad and a Wacom touchscreen/panel. It will still add a
> few more tests :)

OK, So I applied the series on my development laptop.
I had to apply it on top of v5.16 and then rebase on top of
hid.git/for-next because there is a minor conflict.

I changed the register as mentioned in 5/12, and gave it a try.
Both the Elan touchpad and the Wacom panel on my XPS-13 are behaving
properly, suspend/resume works also as expected.

Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

I gave a quick look at the individual patches, they all seem sane to
me, but haven't dug into enough detail to be able to formally give my
reviewed-by.
Note that I have a small comment on patch 2, but if you want to apply
it nevertheless Jiri (with the change in 5/12) it should be fine.

Cheers,
Benjamin

> I'll try to report that tomorrow now that I think I fixed my tablet series.
>
> Cheers,
> Benjamin
>
> >
> > Thanks!
> >
> > --
> > Jiri Kosina
> > SUSE Labs
> >

On Thu, 3 Feb 2022, Benjamin Tissoires wrote:

> OK, So I applied the series on my development laptop.
> I had to apply it on top of v5.16 and then rebase on top of
> hid.git/for-next because there is a minor conflict.
> 
> I changed the register as mentioned in 5/12, and gave it a try.
> Both the Elan touchpad and the Wacom panel on my XPS-13 are behaving
> properly, suspend/resume works also as expected.
> 
> Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> I gave a quick look at the individual patches, they all seem sane to
> me, but haven't dug into enough detail to be able to formally give my
> reviewed-by.
> Note that I have a small comment on patch 2, but if you want to apply
> it nevertheless Jiri (with the change in 5/12) it should be fine.

Thanks a lot for testing. I've tested in on my I2C hardware, and haven't 
spotted any issues either.

I plan to finish going through the whole set today and tomorrow, and apply 
it right afterwards with the 5/12 change.

Thanks,

-- 
Jiri Kosina
SUSE Labs