| Message ID | 20201125223957.5921-1-dean@fourwalledcubicle.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | f43d3870cafa2a0f3854c1819c8385733db8f9ae |
| Headers | show |
| Series | [V3] Add additional hidraw input/output report ioctls. | expand |
Hi, What is the difference between V1, V2 and V3? I think generally you would add a small summary. Comments inline. On Thu, 2020-11-26 at 09:39 +1100, Dean Camera wrote: > Currently the hidraw module can only read and write feature HID > reports on demand, via dedicated ioctls. Input reports are read > from the device through the read() interface, while output reports > are written through the write interface(). > > This is insufficient; it is desirable in many situations to be > able to read and write input and output reports through the > control interface to cover additional scenarios: > - Reading an input report by its report ID, to get initial state > - Writing an input report, to set initial input state in the device > - Reading an output report by its report ID, to obtain current state > - Writing an output report by its report ID, out of band > > This patch adds these missing ioctl requests to read and write > the remaining HID report types. Note that not all HID backends will > neccesarily support this (e.g. while the USB link layer supports > setting Input reports, others may not). > > Also included are documentation and example updates. The current > hidraw documentation states that feature reports read from the > device does *not* include the report ID, however this is not the > case and the returned report will have its report ID prepended > by conforming HID devices, as the report data sent from the device > over the control endpoint must be indentical in format to those > sent over the regular transport. > > Signed-off-by: Dean Camera <dean@fourwalledcubicle.com> > --- > Documentation/hid/hidraw.rst | 45 ++++++++++++++++++++++++++++++++++-- > drivers/hid/hidraw.c | 24 ++++++++++++++++++- > include/uapi/linux/hidraw.h | 6 +++++ > samples/hidraw/hid-example.c | 2 +- > 4 files changed, 73 insertions(+), 4 deletions(-) > > diff --git a/Documentation/hid/hidraw.rst b/Documentation/hid/hidraw.rst > index 4a4a0ba1f362..f41c1f0f6252 100644 > --- a/Documentation/hid/hidraw.rst > +++ b/Documentation/hid/hidraw.rst > @@ -123,8 +123,49 @@ HIDIOCGFEATURE(len): > 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 report will be returned starting at > -the first byte of the buffer (ie: the report number is not returned). > +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. > + > +HIDIOCSINPUT(len): > + Send an Input Report > + > +This ioctl will send an input report to the device, using the control > endpoint. > +In most cases, setting an input HID report on a device is meaningless and has > +no effect, but some devices may choose to use this to set or reset an initial > +state of a report. The format of the buffer issued with this report is > identical > +to that of HIDIOCSFEATURE. > + > +HIDIOCGINPUT(len): > + Get an Input Report > + > +This ioctl will request an input report from the device using the control > +endpoint. This is slower on most devices where a dedicated In endpoint > exists > +for regular input reports, but allows the host to request the value of a > +specific report number. Typically, this is used to request the initial > states of > +an input report of a device, before an application listens for normal reports > via > +the regular device read() interface. The format of the buffer issued with > this report > +is identical to that of HIDIOCGFEATURE. I am not sure using the same approach as HIDIOCGFEATURE is a good design choice. The first byte of the supplied buffer is the report ID, but you can set is to 0 if you don't want to use numbered reports. From my understanding, this makes it impossible to use the ioctl with report ID 0, which valid per the HID spec. My suggestion would be to automatically use numbered reports or not depending if the device uses them. A HID endpoint either uses numbered reports or not, it doesn't make much sense to me to let users try to use numbered reports on devices that do not use them or the other way round. But I guess this is a question for Benjamin. I tried to track down the discussion about the addition of the HIDIOCGFEATURE ioctl but from what I saw there was no mention of this design flaw. Am I missing something here? > +HIDIOCSOUTPUT(len): > + Send an Output Report > + > +This ioctl will send an output report to the device, using the control > endpoint. > +This is slower on most devices where a dedicated Out endpoint exists for > regular > +output reports, but is added for completeness. Typically, this is used to > set > +the initial states of an output report of a device, before an application > sends > +updates via the regular device write() interface. The format of the buffer > issued > +with this report is identical to that of HIDIOCSFEATURE. > + > +HIDIOCGOUTPUT(len): > + Get an Output Report > + > +This ioctl will request an output report from the device using the control > +endpoint. Typically, this is used to retrive the initial state of > +an output report of a device, before an application updates it as necessary > either > +via a HIDIOCSOUTPUT request, or the regular device write() interface. The > format > +of the buffer issued with this report is identical to that of HIDIOCGFEATURE. > > Example > ------- > diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c > index 2eee5e31c2b7..79faac87a06f 100644 > --- a/drivers/hid/hidraw.c > +++ b/drivers/hid/hidraw.c > @@ -170,7 +170,7 @@ static ssize_t hidraw_write(struct file *file, const char > __user *buffer, size_t > /* > * This function performs a Get_Report transfer over the control endpoint > * per section 7.2.1 of the HID specification, version 1.1. The first byte > - * of buffer is the report number to request, or 0x0 if the defice does not > + * of buffer is the report number to request, or 0x0 if the device does not > * use numbered reports. The report_type parameter can be HID_FEATURE_REPORT > * or HID_INPUT_REPORT. > */ > @@ -428,6 +428,28 @@ static long hidraw_ioctl(struct file *file, unsigned int > cmd, > break; > } > > + if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSINPUT(0))) > { > + int len = _IOC_SIZE(cmd); > + ret = hidraw_send_report(file, > user_arg, len, HID_INPUT_REPORT); > + break; > + } > + if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGINPUT(0))) > { > + int len = _IOC_SIZE(cmd); > + ret = hidraw_get_report(file, > user_arg, len, HID_INPUT_REPORT); > + break; > + } > + > + if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSOUTPUT(0))) > { > + int len = _IOC_SIZE(cmd); > + ret = hidraw_send_report(file, > user_arg, len, HID_OUTPUT_REPORT); > + break; > + } > + if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGOUTPUT(0))) > { > + int len = _IOC_SIZE(cmd); > + ret = hidraw_get_report(file, > user_arg, len, HID_OUTPUT_REPORT); > + break; > + } > + > /* Begin Read-only ioctls. */ > if (_IOC_DIR(cmd) != _IOC_READ) { > ret = -EINVAL; > diff --git a/include/uapi/linux/hidraw.h b/include/uapi/linux/hidraw.h > index 4913539e5bcc..33ebad81720a 100644 > --- a/include/uapi/linux/hidraw.h > +++ b/include/uapi/linux/hidraw.h > @@ -40,6 +40,12 @@ struct hidraw_devinfo { > #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) > > #define HIDRAW_FIRST_MINOR 0 > #define HIDRAW_MAX_DEVICES 64 > diff --git a/samples/hidraw/hid-example.c b/samples/hidraw/hid-example.c > index 37a0ffcb4d63..0f73ace3c6c3 100644 > --- a/samples/hidraw/hid-example.c > +++ b/samples/hidraw/hid-example.c > @@ -128,7 +128,7 @@ int main(int argc, char **argv) > perror("HIDIOCGFEATURE"); > } else { > printf("ioctl HIDIOCGFEATURE returned: %d\n", res); > - printf("Report data (not containing the report number):\n\t"); This seems unrelated, you did not touch HIDIOCGFEATURE. > + printf("Report data:\n\t"); > for (i = 0; i < res; i++) > printf("%hhx ", buf[i]); > puts("\n");
Hi Filipe, Comments inline. - Dean On 27/11/2020 6:05 am, Filipe Laíns wrote: > Hi, > > What is the difference between V1, V2 and V3? I think generally you would add a > small summary. > Sorry, that's my fault -- the contents are identical. I am more used to modern tooling with code review platforms, pull-requests or even emailed attached patches, so the old tooling took me a few goes to get right. V1 was mangled by Thunderbird, while V2 was missing the cover letter when I submitted it via git send-email from my test machine. V3 is where I (think) I beat the tooling into submission. > > On Thu, 2020-11-26 at 09:39 +1100, Dean Camera wrote: >> Currently the hidraw module can only read and write feature HID >> reports on demand, via dedicated ioctls. Input reports are read >> from the device through the read() interface, while output reports >> are written through the write interface(). >> >> This is insufficient; it is desirable in many situations to be >> able to read and write input and output reports through the >> control interface to cover additional scenarios: >> - Reading an input report by its report ID, to get initial state >> - Writing an input report, to set initial input state in the device >> - Reading an output report by its report ID, to obtain current state >> - Writing an output report by its report ID, out of band >> >> This patch adds these missing ioctl requests to read and write >> the remaining HID report types. Note that not all HID backends will >> neccesarily support this (e.g. while the USB link layer supports >> setting Input reports, others may not). >> >> Also included are documentation and example updates. The current >> hidraw documentation states that feature reports read from the >> device does *not* include the report ID, however this is not the >> case and the returned report will have its report ID prepended >> by conforming HID devices, as the report data sent from the device >> over the control endpoint must be indentical in format to those >> sent over the regular transport. >> >> Signed-off-by: Dean Camera <dean@fourwalledcubicle.com> >> --- >> Documentation/hid/hidraw.rst | 45 ++++++++++++++++++++++++++++++++++-- >> drivers/hid/hidraw.c | 24 ++++++++++++++++++- >> include/uapi/linux/hidraw.h | 6 +++++ >> samples/hidraw/hid-example.c | 2 +- >> 4 files changed, 73 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/hid/hidraw.rst b/Documentation/hid/hidraw.rst >> index 4a4a0ba1f362..f41c1f0f6252 100644 >> --- a/Documentation/hid/hidraw.rst >> +++ b/Documentation/hid/hidraw.rst >> @@ -123,8 +123,49 @@ HIDIOCGFEATURE(len): >> 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 report will be returned starting at >> -the first byte of the buffer (ie: the report number is not returned). >> +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. >> + >> +HIDIOCSINPUT(len): >> + Send an Input Report >> + >> +This ioctl will send an input report to the device, using the control >> endpoint. >> +In most cases, setting an input HID report on a device is meaningless and has >> +no effect, but some devices may choose to use this to set or reset an initial >> +state of a report. The format of the buffer issued with this report is >> identical >> +to that of HIDIOCSFEATURE. >> + >> +HIDIOCGINPUT(len): >> + Get an Input Report >> + >> +This ioctl will request an input report from the device using the control >> +endpoint. This is slower on most devices where a dedicated In endpoint >> exists >> +for regular input reports, but allows the host to request the value of a >> +specific report number. Typically, this is used to request the initial >> states of >> +an input report of a device, before an application listens for normal reports >> via >> +the regular device read() interface. The format of the buffer issued with >> this report >> +is identical to that of HIDIOCGFEATURE. > > I am not sure using the same approach as HIDIOCGFEATURE is a good design choice. > The first byte of the supplied buffer is the report ID, but you can set is to 0 > if you don't want to use numbered reports. From my understanding, this makes it > impossible to use the ioctl with report ID 0, which valid per the HID spec. > Report ID 0 is reserved by the HID specification and may not be used in a device with multiple reports (see "Device Class Definition for HID 1.11", section "6.2.2.7 Global Items" where it states "Report ID zero is reserved and should not be used."). I think the designers of HID forsaw a sane future where in userspace everyone just assumed the report ID was present at all times, and the HID driver would just omit it on the wire if it was zero. Unfortunatly every platform seems to handle that differently now, with some always requring it, and others selectively omitting it in their APIs. > My suggestion would be to automatically use numbered reports or not depending if > the device uses them. A HID endpoint either uses numbered reports or not, it > doesn't make much sense to me to let users try to use numbered reports on > devices that do not use them or the other way round. > > But I guess this is a question for Benjamin. I'm *strongly* in favour of always having them at least in the `ioctl()`, with a (reserved) zero value indicating it is unused - like it is now. That makes userspace easier to deal with, and covers the quirk case where a device does not list report IDs in its HID report descriptor properly, but requires them anyway. It also makes the new requests consistent with the existing request, so there's no extra cognitive load from working with one then switching to the others. > > I tried to track down the discussion about the addition of the HIDIOCGFEATURE > ioctl but from what I saw there was no mention of this design flaw. > > Am I missing something here? > >> +HIDIOCSOUTPUT(len): >> + Send an Output Report >> + >> +This ioctl will send an output report to the device, using the control >> endpoint. >> +This is slower on most devices where a dedicated Out endpoint exists for >> regular >> +output reports, but is added for completeness. Typically, this is used to >> set >> +the initial states of an output report of a device, before an application >> sends >> +updates via the regular device write() interface. The format of the buffer >> issued >> +with this report is identical to that of HIDIOCSFEATURE. >> + >> +HIDIOCGOUTPUT(len): >> + Get an Output Report >> + >> +This ioctl will request an output report from the device using the control >> +endpoint. Typically, this is used to retrive the initial state of >> +an output report of a device, before an application updates it as necessary >> either >> +via a HIDIOCSOUTPUT request, or the regular device write() interface. The >> format >> +of the buffer issued with this report is identical to that of HIDIOCGFEATURE. >> >> Example >> ------- >> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c >> index 2eee5e31c2b7..79faac87a06f 100644 >> --- a/drivers/hid/hidraw.c >> +++ b/drivers/hid/hidraw.c >> @@ -170,7 +170,7 @@ static ssize_t hidraw_write(struct file *file, const char >> __user *buffer, size_t >> /* >> * This function performs a Get_Report transfer over the control endpoint >> * per section 7.2.1 of the HID specification, version 1.1. The first byte >> - * of buffer is the report number to request, or 0x0 if the defice does not >> + * of buffer is the report number to request, or 0x0 if the device does not >> * use numbered reports. The report_type parameter can be HID_FEATURE_REPORT >> * or HID_INPUT_REPORT. >> */ >> @@ -428,6 +428,28 @@ static long hidraw_ioctl(struct file *file, unsigned int >> cmd, >> break; >> } >> >> + if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSINPUT(0))) >> { >> + int len = _IOC_SIZE(cmd); >> + ret = hidraw_send_report(file, >> user_arg, len, HID_INPUT_REPORT); >> + break; >> + } >> + if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGINPUT(0))) >> { >> + int len = _IOC_SIZE(cmd); >> + ret = hidraw_get_report(file, >> user_arg, len, HID_INPUT_REPORT); >> + break; >> + } >> + >> + if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSOUTPUT(0))) >> { >> + int len = _IOC_SIZE(cmd); >> + ret = hidraw_send_report(file, >> user_arg, len, HID_OUTPUT_REPORT); >> + break; >> + } >> + if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGOUTPUT(0))) >> { >> + int len = _IOC_SIZE(cmd); >> + ret = hidraw_get_report(file, >> user_arg, len, HID_OUTPUT_REPORT); >> + break; >> + } >> + >> /* Begin Read-only ioctls. */ >> if (_IOC_DIR(cmd) != _IOC_READ) { >> ret = -EINVAL; >> diff --git a/include/uapi/linux/hidraw.h b/include/uapi/linux/hidraw.h >> index 4913539e5bcc..33ebad81720a 100644 >> --- a/include/uapi/linux/hidraw.h >> +++ b/include/uapi/linux/hidraw.h >> @@ -40,6 +40,12 @@ struct hidraw_devinfo { >> #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) >> >> #define HIDRAW_FIRST_MINOR 0 >> #define HIDRAW_MAX_DEVICES 64 >> diff --git a/samples/hidraw/hid-example.c b/samples/hidraw/hid-example.c >> index 37a0ffcb4d63..0f73ace3c6c3 100644 >> --- a/samples/hidraw/hid-example.c >> +++ b/samples/hidraw/hid-example.c >> @@ -128,7 +128,7 @@ int main(int argc, char **argv) >> perror("HIDIOCGFEATURE"); >> } else { >> printf("ioctl HIDIOCGFEATURE returned: %d\n", res); >> - printf("Report data (not containing the report number):\n\t"); > > This seems unrelated, you did not touch HIDIOCGFEATURE. > >> + printf("Report data:\n\t"); >> for (i = 0; i < res; i++) >> printf("%hhx ", buf[i]); >> puts("\n"); >
On Fri, 2020-11-27 at 08:30 +1100, Dean Camera wrote: > Hi Filipe, > > Comments inline. > > - Dean > > On 27/11/2020 6:05 am, Filipe Laíns wrote: > > Hi, > > > > What is the difference between V1, V2 and V3? I think generally you would > > add a > > small summary. > > > > Sorry, that's my fault -- the contents are identical. I am more used to > modern tooling with code review platforms, pull-requests or even emailed > attached patches, so the old tooling took me a few goes to get right. > > V1 was mangled by Thunderbird, while V2 was missing the cover letter > when I submitted it via git send-email from my test machine. V3 is where > I (think) I beat the tooling into submission. No worries, I was just wondering what the differences were, given that the patches were identical. *snip* > > I am not sure using the same approach as HIDIOCGFEATURE is a good design > > choice. > > The first byte of the supplied buffer is the report ID, but you can set is > > to 0 > > if you don't want to use numbered reports. From my understanding, this makes > > it > > impossible to use the ioctl with report ID 0, which valid per the HID spec. > > > > Report ID 0 is reserved by the HID specification and may not be used in > a device with multiple reports (see "Device Class Definition for HID > 1.11", section "6.2.2.7 Global Items" where it states "Report ID zero is > reserved and should not be used."). Yup, you are right! I missed that. > I think the designers of HID forsaw a sane future where in userspace > everyone just assumed the report ID was present at all times, and the > HID driver would just omit it on the wire if it was zero. Unfortunatly > every platform seems to handle that differently now, with some always > requring it, and others selectively omitting it in their APIs. Yes, it's unfortunate. Perhaps if the HID designers made the spec easier to read we wouldn't have this issue. > > My suggestion would be to automatically use numbered reports or not > > depending if > > the device uses them. A HID endpoint either uses numbered reports or not, it > > doesn't make much sense to me to let users try to use numbered reports on > > devices that do not use them or the other way round. > > > > But I guess this is a question for Benjamin. > > I'm *strongly* in favour of always having them at least in the > `ioctl()`, with a (reserved) zero value indicating it is unused - like > it is now. That makes userspace easier to deal with, and covers the > quirk case where a device does not list report IDs in its HID report > descriptor properly, but requires them anyway. > > It also makes the new requests consistent with the existing request, so > there's no extra cognitive load from working with one then switching to > the others. Yeah, it makes sense given that report 0 is reserved :) > > > > I tried to track down the discussion about the addition of the > > HIDIOCGFEATURE > > ioctl but from what I saw there was no mention of this design flaw. > > > > Am I missing something here? I was :D You can have my Signed-off-by: Filipe Laíns <lains@archlinux.org> Perhaps just split the hid-example.c change to another patch. *snip* Cheers, Filipe Laíns
Thank you for the review, Filipe! I'm still new to the kernel patch submission process, so I'm a little perplexed by the next steps. Who will decide if it is to be applied, and is there any further action I need to take? I am obviously keen, but not impatient, to get this feature in. I'm happy to follow whatever process is required, I just haven't found anything beyond the initial "submitting your patch" documentation. Cheers! - Dean On 27/11/2020 8:42 am, Filipe Laíns wrote: > On Fri, 2020-11-27 at 08:30 +1100, Dean Camera wrote: >> Hi Filipe, >> >> Comments inline. >> >> - Dean >> >> On 27/11/2020 6:05 am, Filipe Laíns wrote: >>> Hi, >>> >>> What is the difference between V1, V2 and V3? I think generally you would >>> add a >>> small summary. >>> >> >> Sorry, that's my fault -- the contents are identical. I am more used to >> modern tooling with code review platforms, pull-requests or even emailed >> attached patches, so the old tooling took me a few goes to get right. >> >> V1 was mangled by Thunderbird, while V2 was missing the cover letter >> when I submitted it via git send-email from my test machine. V3 is where >> I (think) I beat the tooling into submission. > > No worries, I was just wondering what the differences were, given that the > patches were identical. > > *snip* > >>> I am not sure using the same approach as HIDIOCGFEATURE is a good design >>> choice. >>> The first byte of the supplied buffer is the report ID, but you can set is >>> to 0 >>> if you don't want to use numbered reports. From my understanding, this makes >>> it >>> impossible to use the ioctl with report ID 0, which valid per the HID spec. >>> >> >> Report ID 0 is reserved by the HID specification and may not be used in >> a device with multiple reports (see "Device Class Definition for HID >> 1.11", section "6.2.2.7 Global Items" where it states "Report ID zero is >> reserved and should not be used."). > > Yup, you are right! I missed that. > >> I think the designers of HID forsaw a sane future where in userspace >> everyone just assumed the report ID was present at all times, and the >> HID driver would just omit it on the wire if it was zero. Unfortunatly >> every platform seems to handle that differently now, with some always >> requring it, and others selectively omitting it in their APIs. > > Yes, it's unfortunate. Perhaps if the HID designers made the spec easier to read > we wouldn't have this issue. > >>> My suggestion would be to automatically use numbered reports or not >>> depending if >>> the device uses them. A HID endpoint either uses numbered reports or not, it >>> doesn't make much sense to me to let users try to use numbered reports on >>> devices that do not use them or the other way round. >>> >>> But I guess this is a question for Benjamin. >> >> I'm *strongly* in favour of always having them at least in the >> `ioctl()`, with a (reserved) zero value indicating it is unused - like >> it is now. That makes userspace easier to deal with, and covers the >> quirk case where a device does not list report IDs in its HID report >> descriptor properly, but requires them anyway. >> >> It also makes the new requests consistent with the existing request, so >> there's no extra cognitive load from working with one then switching to >> the others. > > Yeah, it makes sense given that report 0 is reserved :) > >>> >>> I tried to track down the discussion about the addition of the >>> HIDIOCGFEATURE >>> ioctl but from what I saw there was no mention of this design flaw. >>> >>> Am I missing something here? > > I was :D > > You can have my > Signed-off-by: Filipe Laíns <lains@archlinux.org> > > Perhaps just split the hid-example.c change to another patch. > > *snip* > > Cheers, > Filipe Laíns >
On Thu, 26 Nov 2020, Dean Camera wrote: > Currently the hidraw module can only read and write feature HID > reports on demand, via dedicated ioctls. Input reports are read > from the device through the read() interface, while output reports > are written through the write interface(). > > This is insufficient; it is desirable in many situations to be > able to read and write input and output reports through the > control interface to cover additional scenarios: > - Reading an input report by its report ID, to get initial state > - Writing an input report, to set initial input state in the device > - Reading an output report by its report ID, to obtain current state > - Writing an output report by its report ID, out of band > > This patch adds these missing ioctl requests to read and write > the remaining HID report types. Note that not all HID backends will > neccesarily support this (e.g. while the USB link layer supports > setting Input reports, others may not). > > Also included are documentation and example updates. The current > hidraw documentation states that feature reports read from the > device does *not* include the report ID, however this is not the > case and the returned report will have its report ID prepended > by conforming HID devices, as the report data sent from the device > over the control endpoint must be indentical in format to those > sent over the regular transport. This one is now queued in hid.git@for-5.11/core as well. Thanks,
On Fri, 2020-11-27 at 15:05 +1100, Dean Camera wrote: > Thank you for the review, Filipe! > > I'm still new to the kernel patch submission process, so I'm a little > perplexed by the next steps. Who will decide if it is to be applied, and > is there any further action I need to take? No worries :) That'd probably Benjamin Tissoires, co-maintainer of the HID subsystem. There is no further action you need to take, just wait. Please understand that these things can sometimes take a while as maintainer time is limited and they may have a long backlog to fight through. Also be aware that maintainers might be a bit conservative about adding new features as they will be required to maintain them in the long run, so don't take it personally if they chose not to to merge some of your patches. > I am obviously keen, but not impatient, to get this feature in. I'm > happy to follow whatever process is required, I just haven't found > anything beyond the initial "submitting your patch" documentation. Yeah, I sympathize. The process essentially is to send the patch and wait :P Just one thing, in the kernel mailing lists you are discouraged to top-post. A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? > Cheers! > - Dean Cheers, Filipe Laíns
On 28/11/2020 7:11 am, Filipe Laíns wrote: > On Fri, 2020-11-27 at 15:05 +1100, Dean Camera wrote: >> Thank you for the review, Filipe! >> >> I'm still new to the kernel patch submission process, so I'm a little >> perplexed by the next steps. Who will decide if it is to be applied, and >> is there any further action I need to take? > > No worries :) > That'd probably Benjamin Tissoires, co-maintainer of the HID subsystem. > There is no further action you need to take, just wait. Please understand that > these things can sometimes take a while as maintainer time is limited and they > may have a long backlog to fight through. > I completely understand - I maintain some open source projects myself, so I know how it is. I'm just a millenial used to pull requests, issue trackers and/or email attachments, so I wasn't too sure how to proceed here and didn't want to mis-step and end up with it getting lost due to my own inaction. Thank you both sincerely for your time and efforts, they are very much appreciated. > Also be aware that maintainers might be a bit conservative about adding new > features as they will be required to maintain them in the long run, so don't > take it personally if they chose not to to merge some of your patches. > >> I am obviously keen, but not impatient, to get this feature in. I'm >> happy to follow whatever process is required, I just haven't found >> anything beyond the initial "submitting your patch" documentation. > > Yeah, I sympathize. The process essentially is to send the patch and wait :P > I do wish there was a way to track the patches a little better - I fully understand about the time pressures involved, but having a way to at least know it was seen and added to a backlog somewhere would be nice. Alas, I fear such talk is somewhat like the proverbial ocean boiling. > Just one thing, in the kernel mailing lists you are discouraged to top-post. Thanks for the advice, taken under consideration :). > > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? > > A: Top-posting. > Q: What is the most annoying thing in e-mail? > > A: No. > Q: Should I include quotations after my reply? > >> Cheers! >> - Dean > > Cheers, > Filipe Laíns > Cheers! - Dean
On Sat, 2020-11-28 at 18:02 +1100, Dean Camera wrote: > On 28/11/2020 7:11 am, Filipe Laíns wrote: > > On Fri, 2020-11-27 at 15:05 +1100, Dean Camera wrote: > > > Thank you for the review, Filipe! > > > > > > I'm still new to the kernel patch submission process, so I'm a little > > > perplexed by the next steps. Who will decide if it is to be applied, and > > > is there any further action I need to take? > > > > No worries :) > > That'd probably Benjamin Tissoires, co-maintainer of the HID subsystem. > > There is no further action you need to take, just wait. Please understand > > that > > these things can sometimes take a while as maintainer time is limited and > > they > > may have a long backlog to fight through. > > > > I completely understand - I maintain some open source projects myself, > so I know how it is. I'm just a millenial used to pull requests, issue > trackers and/or email attachments, so I wasn't too sure how to proceed > here and didn't want to mis-step and end up with it getting lost due to > my own inaction. Me too :P It can be a bit difficult to get started. > Thank you both sincerely for your time and efforts, they are very much > appreciated. > > > Also be aware that maintainers might be a bit conservative about adding new > > features as they will be required to maintain them in the long run, so don't > > take it personally if they chose not to to merge some of your patches. > > > > > I am obviously keen, but not impatient, to get this feature in. I'm > > > happy to follow whatever process is required, I just haven't found > > > anything beyond the initial "submitting your patch" documentation. > > > > Yeah, I sympathize. The process essentially is to send the patch and wait :P > > > > I do wish there was a way to track the patches a little better - I fully > understand about the time pressures involved, but having a way to at > least know it was seen and added to a backlog somewhere would be nice. > Alas, I fear such talk is somewhat like the proverbial ocean boiling. There is https://patchwork.kernel.org/project/linux-input/patch/20201125223957.5921-1-dean@fourwalledcubicle.com/ > Just one thing, in the kernel mailing lists you are discouraged to top-post. Thanks for the advice, taken under consideration :). > > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? > > A: Top-posting. > Q: What is the most annoying thing in e-mail? > > A: No. > Q: Should I include quotations after my reply? > > > Cheers! > > - Dean > > Cheers, > Filipe Laíns > Cheers! - Dean Cheers, Filipe Laíns
diff --git a/Documentation/hid/hidraw.rst b/Documentation/hid/hidraw.rst index 4a4a0ba1f362..f41c1f0f6252 100644 --- a/Documentation/hid/hidraw.rst +++ b/Documentation/hid/hidraw.rst @@ -123,8 +123,49 @@ HIDIOCGFEATURE(len): 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 report will be returned starting at -the first byte of the buffer (ie: the report number is not returned). +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. + +HIDIOCSINPUT(len): + Send an Input Report + +This ioctl will send an input report to the device, using the control endpoint. +In most cases, setting an input HID report on a device is meaningless and has +no effect, but some devices may choose to use this to set or reset an initial +state of a report. The format of the buffer issued with this report is identical +to that of HIDIOCSFEATURE. + +HIDIOCGINPUT(len): + Get an Input Report + +This ioctl will request an input report from the device using the control +endpoint. This is slower on most devices where a dedicated In endpoint exists +for regular input reports, but allows the host to request the value of a +specific report number. Typically, this is used to request the initial states of +an input report of a device, before an application listens for normal reports via +the regular device read() interface. The format of the buffer issued with this report +is identical to that of HIDIOCGFEATURE. + +HIDIOCSOUTPUT(len): + Send an Output Report + +This ioctl will send an output report to the device, using the control endpoint. +This is slower on most devices where a dedicated Out endpoint exists for regular +output reports, but is added for completeness. Typically, this is used to set +the initial states of an output report of a device, before an application sends +updates via the regular device write() interface. The format of the buffer issued +with this report is identical to that of HIDIOCSFEATURE. + +HIDIOCGOUTPUT(len): + Get an Output Report + +This ioctl will request an output report from the device using the control +endpoint. Typically, this is used to retrive the initial state of +an output report of a device, before an application updates it as necessary either +via a HIDIOCSOUTPUT request, or the regular device write() interface. The format +of the buffer issued with this report is identical to that of HIDIOCGFEATURE. Example ------- diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c index 2eee5e31c2b7..79faac87a06f 100644 --- a/drivers/hid/hidraw.c +++ b/drivers/hid/hidraw.c @@ -170,7 +170,7 @@ static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t /* * This function performs a Get_Report transfer over the control endpoint * per section 7.2.1 of the HID specification, version 1.1. The first byte - * of buffer is the report number to request, or 0x0 if the defice does not + * of buffer is the report number to request, or 0x0 if the device does not * use numbered reports. The report_type parameter can be HID_FEATURE_REPORT * or HID_INPUT_REPORT. */ @@ -428,6 +428,28 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd, break; } + if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSINPUT(0))) { + int len = _IOC_SIZE(cmd); + ret = hidraw_send_report(file, user_arg, len, HID_INPUT_REPORT); + break; + } + if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGINPUT(0))) { + int len = _IOC_SIZE(cmd); + ret = hidraw_get_report(file, user_arg, len, HID_INPUT_REPORT); + break; + } + + if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSOUTPUT(0))) { + int len = _IOC_SIZE(cmd); + ret = hidraw_send_report(file, user_arg, len, HID_OUTPUT_REPORT); + break; + } + if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGOUTPUT(0))) { + int len = _IOC_SIZE(cmd); + ret = hidraw_get_report(file, user_arg, len, HID_OUTPUT_REPORT); + break; + } + /* Begin Read-only ioctls. */ if (_IOC_DIR(cmd) != _IOC_READ) { ret = -EINVAL; diff --git a/include/uapi/linux/hidraw.h b/include/uapi/linux/hidraw.h index 4913539e5bcc..33ebad81720a 100644 --- a/include/uapi/linux/hidraw.h +++ b/include/uapi/linux/hidraw.h @@ -40,6 +40,12 @@ struct hidraw_devinfo { #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) #define HIDRAW_FIRST_MINOR 0 #define HIDRAW_MAX_DEVICES 64 diff --git a/samples/hidraw/hid-example.c b/samples/hidraw/hid-example.c index 37a0ffcb4d63..0f73ace3c6c3 100644 --- a/samples/hidraw/hid-example.c +++ b/samples/hidraw/hid-example.c @@ -128,7 +128,7 @@ int main(int argc, char **argv) perror("HIDIOCGFEATURE"); } else { printf("ioctl HIDIOCGFEATURE returned: %d\n", res); - printf("Report data (not containing the report number):\n\t"); + printf("Report data:\n\t"); for (i = 0; i < res; i++) printf("%hhx ", buf[i]); puts("\n");
Currently the hidraw module can only read and write feature HID reports on demand, via dedicated ioctls. Input reports are read from the device through the read() interface, while output reports are written through the write interface(). This is insufficient; it is desirable in many situations to be able to read and write input and output reports through the control interface to cover additional scenarios: - Reading an input report by its report ID, to get initial state - Writing an input report, to set initial input state in the device - Reading an output report by its report ID, to obtain current state - Writing an output report by its report ID, out of band This patch adds these missing ioctl requests to read and write the remaining HID report types. Note that not all HID backends will neccesarily support this (e.g. while the USB link layer supports setting Input reports, others may not). Also included are documentation and example updates. The current hidraw documentation states that feature reports read from the device does *not* include the report ID, however this is not the case and the returned report will have its report ID prepended by conforming HID devices, as the report data sent from the device over the control endpoint must be indentical in format to those sent over the regular transport. Signed-off-by: Dean Camera <dean@fourwalledcubicle.com> --- Documentation/hid/hidraw.rst | 45 ++++++++++++++++++++++++++++++++++-- drivers/hid/hidraw.c | 24 ++++++++++++++++++- include/uapi/linux/hidraw.h | 6 +++++ samples/hidraw/hid-example.c | 2 +- 4 files changed, 73 insertions(+), 4 deletions(-)