| Message ID | 20220118072628.1617172-1-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | i2c-hid: fixes for unnumbered reports and other improvements | expand |
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!
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,