Re: Clarification about the hidraw documentation on HIDIOCGFEATURE

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

On Wed, Jun 21, 2023 at 1:27 PM Xiaofan Chen <xiaofanc@gmail.com> wrote:
>
> On Tue, Jun 20, 2023 at 7:28 AM Xiaofan Chen <xiaofanc@gmail.com> wrote:
> >
> > On Mon, Jun 19, 2023 at 11:14 PM Xiaofan Chen <xiaofanc@gmail.com> wrote:
> > >
> > > On Mon, Jun 19, 2023 at 2:09 PM Xiaofan Chen <xiaofanc@gmail.com> wrote:
> > > >
> > > > I know that thurrent documentation has been there since it was created by
> > > > Alan Ott many years ago. And he started the HIDAPI project around that
> > > > time as well. However, I am starting to doubt whether it is correct or not
> > > > based on the testing using HIDAPI.
> > > >
> > > > Please help to clarify. Thanks.
> > > >
> > > > https://docs.kernel.org/hid/hidraw.html
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > HIDIOCGFEATURE(len):
> > > >
> > > > Get a Feature Report
> > > >
> > > > This ioctl will request a feature report from the device using the
> > > > control endpoint. The first byte of the supplied buffer should be
> > > > set to the report number of the requested report. For devices
> > > > which do not use numbered reports, set the first byte to 0. The
> > > > returned report buffer will contain the report number in the first
> > > > byte, followed by the report data read from the device. For devices
> > > > which do not use numbered reports, the report data will begin at the
> > > > first byte of the returned buffer.


Yep, this is wrong.

This should be read:
```
The returned report buffer will contain the report number in the first 
byte or 0 when the device doesn't use numbered reports. The data read 
from the device will be stored in the following bytes.
```

FWIW, the difficulty to find out what the code does is because this part 
is handled in each HID transport driver: USB, Bluetooth, I2C.

Looking at drivers/hid/usbhid/hid-core.c, lines 869+, the function 
usbhid_get_raw_report() is the one used by hidraw in the end and is 
still the original code from Alan:

```
/* Byte 0 is the report number. Report data starts at byte 1.*/
buf[0] = report_number;
if (report_number == 0x0) {
	/* Offset the return buffer by 1, so that the report ID
	   will remain in byte 0. */
	buf++;
	count--;
	skipped_report_id = 1;
}
```
  >
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >
> > > > I have doubts about the last sentence. It seems to me that for
> > > > devices which do not use numbered reports, the first byte will
> > > > be 0 and the report data begins in the second byte.
> > > >
> > > > This is based on testing results using hidapi and hidapitester, which
> > > > use the ioctl.
> > > > int HID_API_EXPORT hid_get_feature_report(hid_device *dev, unsigned
> > > > char *data, size_t length)
> > > > {
> > > >     int res;
> > > >
> > > >     register_device_error(dev, NULL);
> > > >
> > > >     res = ioctl(dev->device_handle, HIDIOCGFEATURE(length), data);
> > > >     if (res < 0)
> > > >          register_device_error_format(dev, "ioctl (GFEATURE): %s",
> > > > strerror(errno));
> > > >
> > > >     return res;
> > > > }
> > > >
> > > > Reference discussion:
> > > > https://github.com/libusb/hidapi/issues/589
> > > >
> > > > Test device is Jan Axelson's generic HID example which I have tested using
> > > > libusb and hidapi across platforms as well as using Windows HID API.
> > > > So I believe the FW is good.
> > > > http://janaxelson.com/hidpage.htm#MyExampleCode (USB PIC MCU)
> > > >
> > >
> > > Modified testing code from the following Linux kernel
> > > samples/hidraw/hid-example.c
> > > https://github.com/libusb/hidapi/issues/589#issuecomment-1597356054
> > >
> > > Results are shown here. We can clearly see that the "Fake Report ID" 0 is
> > > in the first byte of the returned buffer, matching the output from
> > > hidapi/hidapitester,
> > >
> > > mcuee@UbuntuSwift3:~/build/hid/hidraw$ gcc -o myhid-example myhid-example.c
> > >
> > > mcuee@UbuntuSwift3:~/build/hid/hidraw$ sudo ./myhid-example
> > > Report Descriptor Size: 47
> > > Report Descriptor:
> > > 6 a0 ff 9 1 a1 1 9 3 15 0 26 ff 0 75 8 95 3f 81 2 9 4 15 0 26 ff 0 75
> > > 8 95 3f 91 2 9 5 15 0 26 ff 0 75 8 95 3f b1 2 c0
> > >
> > > Raw Name: Microchip Technology Inc. Generic HID
> > > Raw Phys: usb-0000:00:14.0-1/input0
> > > Raw Info:
> > > bustype: 3 (USB)
> > > vendor: 0x0925
> > > product: 0x7001
> > > ioctl HIDIOCSFEATURE returned: 64
> > > ioctl HIDIOCGFEATURE returned: 64
> > > Report data:
> > > 0 41 42 43 44 45 46 47 48 14 4 18 4 4d 72 31 50 51 52 53 54 55 56 57
> > > 58 59 5a 5b 5c 5d 5e 5f 60 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e
> > > 6f 70 71 72 73 74 75 76 77 78 79 7a 7b 7c 7d 7e 7f
> > >
> > > write() wrote 64 bytes
> > > read() read 63 bytes:
> > > 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 36 37
> > > 38 39 3a 3b 3c 3d 3e 3f 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e
> > > 4f 50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f
> > >
> >
> > Moreover the kernel codes here seem to prove that the documentation
> > is wrong for both HIDIOCGFEATURE and HIDIOCGINPUT.
> >
> > https://github.com/torvalds/linux/blob/master/include/uapi/linux/hidraw.h
> >
> > /* The first byte of SFEATURE and GFEATURE is the report number */
> > #define HIDIOCSFEATURE(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x06, len)
> > #define HIDIOCGFEATURE(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x07, len)
> > #define HIDIOCGRAWUNIQ(len)     _IOC(_IOC_READ, 'H', 0x08, len)
> > /* The first byte of SINPUT and GINPUT is the report number */
> > #define HIDIOCSINPUT(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x09, len)
> > #define HIDIOCGINPUT(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0A, len)
> > /* The first byte of SOUTPUT and GOUTPUT is the report number */
> > #define HIDIOCSOUTPUT(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0B, len)
> > #define HIDIOCGOUTPUT(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0C, len)
>
> Hi Jiri and Benjamin,
>
> Sorry to ping the two maintainers, hopefully you two can give the
> answer. Thanks.

See above, I also think the documentation is wrong.

Cheers,
Benjamin

>
>
> --
> Xiaofan
>