Re: [PATCH v2 1/1] USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL

[Chris Wulff]

On Wed, Jul 31, 2024 at 4:30 AM Greg KH <greg@xxxxxxxxx> wrote:
>
> On Wed, Apr 17, 2024 at 06:29:27PM +0000, Chris Wulff wrote:
> > While supporting GET_REPORT is a mandatory request per the HID
> > specification the current implementation of the GET_REPORT request responds
> > to the USB Host with an empty reply of the request length. However, some
> > USB Hosts will request the contents of feature reports via the GET_REPORT
> > request. In addition, some proprietary HID 'protocols' will expect
> > different data, for the same report ID, to be to become available in the
> > feature report by sending a preceding SET_REPORT to the USB Device that
> > defines what data is to be presented when that feature report is
> > subsequently retrieved via GET_REPORT (with a very fast < 5ms turn around
> > between the SET_REPORT and the GET_REPORT).
> >
> > There are two other patch sets already submitted for adding GET_REPORT
> > support. The first [1] allows for pre-priming a list of reports via IOCTLs
> > which then allows the USB Host to perform the request, with no further
> > userspace interaction possible during the GET_REPORT request. And another
> > [2] which allows for a single report to be setup by userspace via IOCTL,
> > which will be fetched and returned by the kernel for subsequent GET_REPORT
> > requests by the USB Host, also with no further userspace interaction
> > possible.
> >
> > This patch, while loosely based on both the patch sets, differs by allowing
> > the option for userspace to respond to each GET_REPORT request by setting
> > up a poll to notify userspace that a new GET_REPORT request has arrived. To
> > support this, two extra IOCTLs are supplied. The first of which is used to
> > retrieve the report ID of the GET_REPORT request (in the case of having
> > non-zero report IDs in the HID descriptor). The second IOCTL allows for
> > storing report responses in a list for responding to requests.
> >
> > The report responses are stored in a list (it will be either added if it
> > does not exist or updated if it exists already). A flag (userspace_req) can
> > be set to whether subsequent requests notify userspace or not.
> >
> > Basic operation when a GET_REPORT request arrives from USB Host:
> >
> > - If the report ID exists in the list and it is set for immediate return
> >   (i.e. userspace_req == false) then response is sent immediately,
> > userspace is not notified
> >
> > - The report ID does not exist, or exists but is set to notify userspace
> >   (i.e. userspace_req == true) then notify userspace via poll:
> >
> >       - If userspace responds, and either adds or update the response in
> >         the list and respond to the host with the contents
> >
> >       - If userspace does not respond within the fixed timeout (2500ms)
> >         but the report has been set prevously, then send 'old' report
> >         contents
> >
> >       - If userspace does not respond within the fixed timeout (2500ms)
> >         and the report does not exist in the list then send an empty
> >         report
> >
> > Note that userspace could 'prime' the report list at any other time.
> >
> > While this patch allows for flexibility in how the system responds to
> > requests, and therefore the HID 'protocols' that could be supported, a
> > drawback is the time it takes to service the requests and therefore the
> > maximum throughput that would be achievable. The USB HID Specification
> > v1.11 itself states that GET_REPORT is not intended for periodic data
> > polling, so this limitation is not severe.
> >
> > Testing on an iMX8M Nano Ultra Lite with a heavy multi-core CPU loading
> > showed that userspace can typically respond to the GET_REPORT request
> > within 1200ms - which is well within the 5000ms most operating systems seem
> > to allow, and within the 2500ms set by this patch.
> >
> > [1] https://marc.info/?t=165968296600006 [2]
> > https://marc.info/?t=165879768900004
> >
> > Signed-off-by: David Sands <david.sands@xxxxxxxxx>
> > Signed-off-by: Chris Wulff <chris.wulff@xxxxxxxxx>
> > ---
> >  drivers/usb/gadget/function/f_hid.c | 270 +++++++++++++++++++++++++++-
> >  include/uapi/linux/usb/g_hid.h      |  40 +++++
> >  include/uapi/linux/usb/gadgetfs.h   |   2 +-
> >  3 files changed, 304 insertions(+), 8 deletions(-)
> >  create mode 100644 include/uapi/linux/usb/g_hid.h
>
> Can you rebase this and resubmit against the latest kernel tree?  It's
> been a while since this was last submitted, sorry for the delay in
> reviewing it.

Yeah, I'll rebase it and make sure it still works and then send out an update.

No worries about the review delay. Seeing the quantity of traffic on
the usb list
I'm sure you keep pretty busy.

>
> greg k-h
>