| Message ID | 20221205210354.11846-1-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Handling of non-numbered feature reports by hidraw | expand |
Hi On Mon, 5 Dec 2022 at 22:04, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > I'm working on a firmware of a device that exposes a HID interface via > USB and/or BLE and uses, among other things, non-numbered feature > reports. Included in this series are two paches I had to create in > order for hidraw devices created for aforementioned subsystems to > behave in the same way when exerciesd by the same test tool. > > I don't know if the patches are acceptable as-is WRT to not breaking > existing userspace, hence the RFC tag. Can you elaborate why you remove the special handling from USBHID but add it to UHID? They both operate logically on the same level, so shouldn't we simply adjust uhid to include the report-id in buf[0]? Also, you override buf[0] in UHID, so I wonder what UHID currently returns there? IOW, can you elaborate a bit what the current behavior of each of the involved modules is, and what behavior you would expect? This would allow to better understand what you are trying to achieve. The more context you can give, the easier it is to understand what happens there. Thanks! David
On Thu, Dec 8, 2022 at 7:46 AM David Rheinsberg <david.rheinsberg@gmail.com> wrote: > > Hi > > On Mon, 5 Dec 2022 at 22:04, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > I'm working on a firmware of a device that exposes a HID interface via > > USB and/or BLE and uses, among other things, non-numbered feature > > reports. Included in this series are two paches I had to create in > > order for hidraw devices created for aforementioned subsystems to > > behave in the same way when exerciesd by the same test tool. > > > > I don't know if the patches are acceptable as-is WRT to not breaking > > existing userspace, hence the RFC tag. > > Can you elaborate why you remove the special handling from USBHID but > add it to UHID? They both operate logically on the same level, so > shouldn't we simply adjust uhid to include the report-id in buf[0]? > > Also, you override buf[0] in UHID, so I wonder what UHID currently > returns there? > > IOW, can you elaborate a bit what the current behavior of each of the > involved modules is, and what behavior you would expect? This would > allow to better understand what you are trying to achieve. The more > context you can give, the easier it is to understand what happens > there. > Sorry it's not very clear, so the difference between the cases is that in the case of UHID the report ID ends up being included as a part of "SET_FEATURE", so BlueZ checks UHID_DEV_NUMBERED_FEATURE_REPORTS, which is not set (correctly) and tries to send the whole payload. This ends up as a maxlen + 1 (extra byte) write to a property that is maxlen long, which gets rejected by device's BLE stack. In the case of USBHID the problem happens in "GET_FEATURE" path. When userspace reads the expected data back it gets an extra 0 prepended to the payload, so all of the actual payload has an offset of 1. This doesn't happen with UHID, which I think is the correct behavior here. Hopefully that explains the difference, let me know if something is unclear
Hi On Thu, 8 Dec 2022 at 21:59, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > On Thu, Dec 8, 2022 at 7:46 AM David Rheinsberg > <david.rheinsberg@gmail.com> wrote: > > > > Hi > > > > On Mon, 5 Dec 2022 at 22:04, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > > I'm working on a firmware of a device that exposes a HID interface via > > > USB and/or BLE and uses, among other things, non-numbered feature > > > reports. Included in this series are two paches I had to create in > > > order for hidraw devices created for aforementioned subsystems to > > > behave in the same way when exerciesd by the same test tool. > > > > > > I don't know if the patches are acceptable as-is WRT to not breaking > > > existing userspace, hence the RFC tag. > > > > Can you elaborate why you remove the special handling from USBHID but > > add it to UHID? They both operate logically on the same level, so > > shouldn't we simply adjust uhid to include the report-id in buf[0]? > > > > Also, you override buf[0] in UHID, so I wonder what UHID currently > > returns there? > > > > IOW, can you elaborate a bit what the current behavior of each of the > > involved modules is, and what behavior you would expect? This would > > allow to better understand what you are trying to achieve. The more > > context you can give, the easier it is to understand what happens > > there. > > > > Sorry it's not very clear, so the difference between the cases is that > in the case of UHID the report ID ends up being included as a part of > "SET_FEATURE", so BlueZ checks UHID_DEV_NUMBERED_FEATURE_REPORTS, > which is not set (correctly) and tries to send the whole payload. This > ends up as a maxlen + 1 (extra byte) write to a property that is > maxlen long, which gets rejected by device's BLE stack. > > In the case of USBHID the problem happens in "GET_FEATURE" path. When > userspace reads the expected data back it gets an extra 0 prepended to > the payload, so all of the actual payload has an offset of 1. This > doesn't happen with UHID, which I think is the correct behavior here. > > Hopefully that explains the difference, let me know if something is unclear Yes, thanks, I completely missed that. Lets continue discussion on the patches. Thanks! David