Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

[Sebastian Lackner]

On 17.10.2016 17:14, Aric Stewart wrote:
> v2: Suggestions from Sebastian Lackner
> v3: Further suggestions from Sebastian Lackner
> Signed-off-by: Aric Stewart
>
> dlls/winebus.sys/bus.h      |   3 +
>  dlls/winebus.sys/bus_udev.c |  73 ++++++++++++++++++++++++
>  dlls/winebus.sys/main.c     | 133 +++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 208 insertions(+), 1 deletion(-)

Now that all the handling for last_report is in main.c, I would suggest to split
it out into a separate patch for easier reviewing. The first patch could contain
IOCTL_HID_READ_REPORT and corresponding platform code only. Caching of the last
record can then be implemented in the second patch.
> v3-0003-winebus.sys-Handle-device-reports-for-hidraw-device.txt
>
> diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h
> index 7ce54bf..339449d 100644
> --- a/dlls/winebus.sys/bus.h
> +++ b/dlls/winebus.sys/bus.h
> @@ -25,6 +25,8 @@ typedef struct
>      int (*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev);
>      NTSTATUS (*get_reportdescriptor)(DEVICE_OBJECT *device, BYTE *buffer, DWORD length, DWORD *out_length);
>      NTSTATUS (*get_string)(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DWORD length);
> - NTSTATUS (*begin_report_processing)(DEVICE_OBJECT *device);
> - void (*end_report_processing)(DEVICE_OBJECT *device);
>
> } platform_vtbl;
>
> void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
> @@ -37,3 +39,4 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
>  DEVICE_OBJECT *bus_find_hid_device(const platform_vtbl *vtbl, void *platform_dev) DECLSPEC_HIDDEN;
>  void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
>  NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
> +void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) DECLSPEC_HIDDEN;
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> index 243aabb..517a7d3 100644
> --- a/dlls/winebus.sys/bus_udev.c
> +++ b/dlls/winebus.sys/bus_udev.c
> @@ -61,6 +61,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
>
> #ifdef HAVE_UDEV
>
> +WINE_DECLARE_DEBUG_CHANNEL(hid_report);
> -
>
> static struct udev *udev_context = NULL;
>  static DRIVER_OBJECT *udev_driver_obj = NULL;
>
> @@ -73,6 +75,11 @@ struct platform_private
>  {
>      struct udev_device *udev_device;
>      int device_fd;
> -
> - BYTE *report_buffer;
> - int buffer_length;
> - HANDLE report_thread;
> - int control_pipe[2];
>
> };
>
> static inline struct platform_private *impl_from_DEVICE_OBJECT(DEVICE_OBJECT *device)
> @@ -222,11 +229,77 @@ static NTSTATUS hidraw_get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buf
>      return STATUS_SUCCESS;
>  }
>
> +static DWORD CALLBACK device_report_thread(void *args)
> +{
> - DEVICE_OBJECT *device = (DEVICE_OBJECT*)args;
> - struct platform_private *private = impl_from_DEVICE_OBJECT(device);
> - struct pollfd plfds[2];
> -
> - plfds[0].fd = private->device_fd;
> - plfds[0].events = POLLIN;
> - plfds[0].revents = 0;
> - plfds[1].fd = private->control_pipe[0];
> - plfds[1].events = POLLIN;
> - plfds[1].revents = 0;
> -
> - while (1)
> - {
> -
>    int size;
> -
>    if (poll(plfds, 2, -1) <= 0) continue;
> -
>    if (plfds[1].revents || !private->report_buffer || private->buffer_length == 0)
> -
>        break;
> -
>    size = read(plfds[0].fd, private->report_buffer, private->buffer_length);
> -
>    if (size == -1)
> -
>        TRACE_(hid_report)("Read failed. Likely an unplugged device\n");
> -
>    else if (size == 0)
> -
>        TRACE_(hid_report)("Failed to read report\n");
> -
>    else
> -
>        process_hid_report(device, private->report_buffer, size);
> - }
> - return 0;
>
> +}
> -
>
> +static NTSTATUS begin_report_processing(DEVICE_OBJECT *device)
> +{
> - struct platform_private *private = impl_from_DEVICE_OBJECT(device);
> - private->buffer_length = 1024;
> - private->report_buffer = HeapAlloc(GetProcessHeap(), 0, private->buffer_length);

My understanding is that report_buffer is only used from the device_report_thread. Do
we really have to store it in the platform private data then? For such a small amount
of data, you could probably even allocate it directly on the stack in device_report_thread.
> -
> - pipe(private->control_pipe);

Please also check for success here.
> - private->report_thread = CreateThread(NULL, 0, device_report_thread, device, 0, NULL);
> - if (!private->report_thread)
> - {
> -
>    ERR("Unable to create device report thread\n");
> -
>    close(private->control_pipe[0]);
> -
>    close(private->control_pipe[1]);
> -
>    return STATUS_UNSUCCESSFUL;
> - }
> - else
> -
>    return STATUS_SUCCESS;
>
> +}
> -
>
> +static void end_report_processing(DEVICE_OBJECT *device)
> +{
> - struct platform_private *private = impl_from_DEVICE_OBJECT(device);
> - if (private->report_thread)
> - {
> -
>    write(private->control_pipe[1], "q", 1);
> -
>    WaitForSingleObject(private->report_thread, INFINITE);
> -
>    close(private->control_pipe[0]);
> -
>    close(private->control_pipe[1]);

report_thread should also be closed when no longer needed.
> - }
> - private->buffer_length = 0;
> - HeapFree(GetProcessHeap(), 0, private->report_buffer);
> - private->report_buffer = NULL;
>
> +}
> -
>
> static const platform_vtbl hidraw_vtbl =
>  {
>      compare_platform_device,
>      hidraw_get_reportdescriptor,
>      hidraw_get_string,
> - begin_report_processing,
> - end_report_processing,
>
> };
>
> static void try_add_device(struct udev_device *dev)
> diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c
> index 86adbf5..0bd2c76 100644
> --- a/dlls/winebus.sys/main.c
> +++ b/dlls/winebus.sys/main.c
> @@ -40,6 +40,7 @@
>  #include "bus.h"
>
> WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
> +WINE_DECLARE_DEBUG_CHANNEL(hid_report);
>
> struct pnp_device
>  {
> @@ -58,6 +59,14 @@ struct device_extension
>      const WCHAR *busid;  /* Expected to be a static constant */
>  const platform_vtbl *vtbl;
> -
> - BOOL processing_started;
> - BYTE *last_report;
> - DWORD last_report_size;
> - DWORD buffer_size;
> - LIST_ENTRY irp_queue;
> - CRITICAL_SECTION report_cs;
> - BYTE platform_private[1];
>
> };
>
> @@ -214,6 +223,10 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
>      ext->busid      = busidW;
>      ext->vtbl       = vtbl;

It might be better to explicitly initialize all fields here. I don't know if you
can rely on the fact that Wines current implementation nulls the driver extension.
> - InitializeListHead(&ext->irp_queue);
> - InitializeCriticalSection(&ext->report_cs);
> - ext->report_cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": report_cs");
> - /* add to list of pnp devices */
>    pnp_dev->device = device;
>    list_add_tail(&pnp_devset, &pnp_dev->entry);
>
> @@ -271,6 +284,8 @@ void bus_remove_hid_device(DEVICE_OBJECT *device)
>  {
>      struct device_extension *ext = (struct device_extension *)device->DeviceExtension;
>      struct pnp_device *pnp_device = ext->pnp_device;
> -
>
> LIST_ENTRY *entry;
> -
>
> IRP *irp;
>
> TRACE("(%p)\n", device);
>
> @@ -279,7 +294,28 @@ void bus_remove_hid_device(DEVICE_OBJECT *device)
>      LeaveCriticalSection(&device_list_cs);
>  IoInvalidateDeviceRelations(device, RemovalRelations);
> -
> - if (ext->processing_started)
> -
>    ext->vtbl->end_report_processing(device);

At this point the device_fd was already closed (too early). Do we really need the
callback, or could we also do the platform specific cleanup in try_remove_device()?
> -
> - /* Cancel pending IRPs */
> - EnterCriticalSection(&ext->report_cs);
> - entry = RemoveHeadList(&ext->irp_queue);
> - while(entry != &ext->irp_queue)

You could write this loop a bit easier like:

while ((entry = RemoveHeadList(&ext->irp_queue)) != &ext->irp_queue)
{
    ...
}

The same also applies to the code below.
> -
>
> {
> -
>    irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry);
> -
>    irp->IoStatus.u.Status = STATUS_CANCELLED;
> -
>    irp->IoStatus.Information = 0;
> -
>    IoCompleteRequest(irp, IO_NO_INCREMENT);
> -
>    entry = RemoveHeadList(&ext->irp_queue);
> -
>
> }
> -
>
> LeaveCriticalSection(&ext->report_cs);
> -
> -
>
> ext->report_cs.DebugInfo->Spare[0] = 0;
> -
>
> DeleteCriticalSection(&ext->report_cs);
> -
>
> HeapFree(GetProcessHeap(), 0, ext->serial);
> -
>
> HeapFree(GetProcessHeap(), 0, ext->last_report);
> IoDeleteDevice(device);
>
> /* pnp_device must be released after the device is gone */
>
> @@ -432,6 +468,58 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp)
>                  irp->IoStatus.Information = (strlenW((WCHAR *)irp->UserBuffer) + 1) * sizeof(WCHAR);
>              break;
>          }
> -
>    case IOCTL_HID_GET_INPUT_REPORT:
> -
>    {
> -
>        HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer);
> -
>        TRACE_(hid_report)("IOCTL_HID_GET_INPUT_REPORT\n");
> -
>        EnterCriticalSection(&ext->report_cs);
> -
>        if (!ext->processing_started)
> -
>        {
> -
>            irp->IoStatus.u.Status = status = ext->vtbl->begin_report_processing(device);
> -
>            if (status == STATUS_SUCCESS)
> -
>                ext->processing_started = TRUE;
> -
>            else
> -
>            {
> -
>                LeaveCriticalSection(&ext->report_cs);
> -
>                break;
> -
>            }
> -
>        }
> -
> -
>        if (packet->reportBufferLen < ext->last_report_size)
> -
>        {
> -
>            irp->IoStatus.u.Status = status = STATUS_BUFFER_TOO_SMALL;
> -
>            LeaveCriticalSection(&ext->report_cs);
> -
>            break;
> -
>        }
> -
> -
>        if (ext->last_report_size)
> -
>            memcpy(packet->reportBuffer, ext->last_report, ext->last_report_size);
> -
>        packet->reportBufferLen = ext->last_report_size;
> -
>        irp->IoStatus.Information = ext->last_report_size;
> -
>        irp->IoStatus.u.Status = status = STATUS_SUCCESS;
> -
>        LeaveCriticalSection(&ext->report_cs);
> -
>        break;
> -
>    }
> -
>    case IOCTL_HID_READ_REPORT:
> -
>    {
> -
>        TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n");
> -
>        EnterCriticalSection(&ext->report_cs);
> -
>        if (!ext->processing_started)
> -
>        {
> -
>            if (ext->vtbl->begin_report_processing(device) == STATUS_SUCCESS)

Did you want to assign the result to "status" here?

Also, it is a bit unfortunate that we have tracking of report processing in both the common
code and also in the platform specific code. If you modify begin_report_processing to return
STATUS_SUCCESS if it already has been started, you could simplify this code to something like:

--- snip ---
               status = ext->vtbl->begin_report_processing(device);
               if (status != STATUS_SUCCESS)
               {
                   ...
               }
--- snip ---

It also ensures that the status in both components never gets out of sync. Not sure if it is
a problem, but due to the fact that the device has been opened a long time before starting to
read from it, couldn't there be a large amount of reports in the kernel buffers? Do you think
it makes sense to skip such outdated reports?
> -
>                ext->processing_started = TRUE;
> -
>            else
> -
>            {
> -
>                irp->IoStatus.u.Status = status;
> -
>                LeaveCriticalSection(&ext->report_cs);
> -
>                break;
> -
>            }
> -
>        }
> -
>        InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry);
> -
>        status = STATUS_PENDING;
> -
>        LeaveCriticalSection(&ext->report_cs);
> -
>        break;
> -
>    }
>    default:
>    {
>        ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode;
>
> @@ -441,11 +529,54 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp)
>          }
>      }
> - IoCompleteRequest(irp, IO_NO_INCREMENT);
> -
>
> if (status != STATUS_PENDING)
> -
>    IoCompleteRequest(irp, IO_NO_INCREMENT);
>
> return status;
>
> }
>
> +void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length)
> +{
> - struct device_extension *ext = (struct device_extension*)device->DeviceExtension;
> - IRP *irp;
> - LIST_ENTRY *entry;
> -
> - EnterCriticalSection(&ext->report_cs);
> - if (length > ext->buffer_size)
> - {
> -
>    HeapFree(GetProcessHeap(), 0, ext->last_report);
> -
>    ext->last_report = HeapAlloc(GetProcessHeap(), 0, length);

It would be better to handle failures here.
> -
>    ext->buffer_size = length;
> - }
> -
> - memcpy(ext->last_report, report, length);
> - ext->last_report_size = length;
> -
> - entry = RemoveHeadList(&ext->irp_queue);
> - while(entry != &ext->irp_queue)
> - {
> -
>    IO_STACK_LOCATION *irpsp;
> -
>    TRACE_(hid_report)("Processing Request\n");
> -
>    irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry);
> -
>    irpsp = IoGetCurrentIrpStackLocation(irp);
> -
> -
>    if (irpsp->Parameters.DeviceIoControl.OutputBufferLength < length)
> -
>    {
> -
>        irp->IoStatus.Information = 0;
> -
>        irp->IoStatus.u.Status = STATUS_BUFFER_TOO_SMALL;
> -
>    }
> -
>    else
> -
>    {
> -
>        memcpy(irp->UserBuffer, report, length);
> -
>        irp->IoStatus.Information = length;
> -
>        irp->IoStatus.u.Status = STATUS_SUCCESS;
> -
>    }
> -
>    IoCompleteRequest(irp, IO_NO_INCREMENT);
> -
>    entry = RemoveHeadList(&ext->irp_queue);
> - }
> - LeaveCriticalSection(&ext->report_cs);
>
> +}
> -
>
> NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path )
>  {
>      static const WCHAR udevW[] = {'\','D','r','i','v','e','r','\','U','D','E','V',0};

[PATCH v3] winebus.sys: Handle device reports for hidraw devices

[Sebastian Lackner]

On 18.10.2016 18:58, Sebastian Lackner wrote:
> Not sure if it is
> a problem, but due to the fact that the device has been opened a long time before starting to
> read from it, couldn't there be a large amount of reports in the kernel buffers? Do you think
> it makes sense to skip such outdated reports?

Actually, isn't there also a problem that reports are skipped in certain situations? Handling
of hidclass looks correct to me, but there is a risk that process_hid_report() will drop the
report if there is no pending IRP in the list. This happens for a short time inbetween of the
IOCTL_HID_READ_REPORT ioctls.

Regards,
Sebastian

[PATCH v3] winebus.sys: Handle device reports for hidraw devices

[Aric Stewart]

On 10/18/16 7:12 PM, Sebastian Lackner wrote:
> On 18.10.2016 18:58, Sebastian Lackner wrote:
> > Not sure if it is
> > a problem, but due to the fact that the device has been opened a long time before starting to
> > read from it, couldn't there be a large amount of reports in the kernel buffers? Do you think
> > it makes sense to skip such outdated reports?
>
> Actually, isn't there also a problem that reports are skipped in certain situations? Handling
> of hidclass looks correct to me, but there is a risk that process_hid_report() will drop the
> report if there is no pending IRP in the list. This happens for a short time inbetween of the
> IOCTL_HID_READ_REPORT ioctls.
>
> Regards,
> Sebastian

I dont think that is a big problem. Not that it wont happen but that it is ok to have happen. The 2 ways to read from the minidriver/device layer is to either get the last report from the device or current device state (IOCTL_HID_GET_INPUT_REPORT) or to wait for the next report from the device(IOCTL_HID_READ_REPORT).

There is not specification of a need for a ring buffer or anything like that in the MSDN docs that I can see so it seem like keeping it simple and direct is the best.

As for your other review points, I will not get a real block of time to work on it until maybe Monday. But I will give it a try then. Of course if you have more comments before then they would be welcome.

-aric

[PATCH v3] winebus.sys: Handle device reports for hidraw devices

[Sebastian Lackner]

On 19.10.2016 10:50, Aric Stewart wrote:
> I dont think that is a big problem. Not that it wont happen but that
> it is ok to have happen. The 2 ways to read from the
> minidriver/device layer is to either get the last report from the
> device or current device state (IOCTL_HID_GET_INPUT_REPORT) or to
> wait for the next report from the device(IOCTL_HID_READ_REPORT).
>
> There is not specification of a need for a ring buffer or anything
> like that in the MSDN docs that I can see so it seem like keeping it
> simple and direct is the best.

Even if missing a report is not a problem, doesn't it cause an arbitrary
long delay when we have to wait for the next report? I have not been able
to find much information about buffering in HID drivers, but at least
some drivers seem to implement that. For example:

https://www.julianloehr.de/wp-content/uploads/2014/07/Paper_HIDWiimote.pdf
"""In case the Wiimote state changes when there is no read request
currently buffered, a flag is used to signal whether a newly received
read request shall be processed and completed immediately."""

In Wine we have to cache the last report anyway, so we could implement
something similar very easy. Do you think it makes sense? The
implementation of IOCTL_HID_GET_INPUT_REPORT and IOCTL_HID_READ_REPORT
would also not be much more difficult, as shown in the following
pseudocode:

--- snip ---
case IOCTL_HID_GET_INPUT_REPORT:
case IOCTL_HID_READ_REPORT:
    EnterCriticalSection(report_cs);
    ... start processing if not done yet ...
    if (ioctl == IOCTL_HID_READ_REPORT && (!last_report || last_report_seen))
    {
        ... async handling ...
        break;
    }
    ... synchronous handlng ...
    last_report_seen = TRUE;
    LeaveCriticalSection(report_cs);
    break;
--- snip ---

Regards,
Sebastian

[PATCH v3] winebus.sys: Handle device reports for hidraw devices

[Aric Stewart]

Hi,

It is important to remember that there is a big difference between the user level communications with the HID device and the minidriver level communications. The top of the stack, the HID device, has a ring buffer and such.

On 10/19/16 12:32 PM, Sebastian Lackner wrote:
> On 19.10.2016 10:50, Aric Stewart wrote:
> > I dont think that is a big problem. Not that it wont happen but that
> > it is ok to have happen. The 2 ways to read from the
> > minidriver/device layer is to either get the last report from the
> > device or current device state (IOCTL_HID_GET_INPUT_REPORT) or to
> > wait for the next report from the device(IOCTL_HID_READ_REPORT).
> >
> > There is not specification of a need for a ring buffer or anything
> > like that in the MSDN docs that I can see so it seem like keeping it
> > simple and direct is the best.
>
> Even if missing a report is not a problem, doesn't it cause an arbitrary
> long delay when we have to wait for the next report? I have not been able
> to find much information about buffering in HID drivers, but at least
> some drivers seem to implement that. For example:

Depends on the call that is made. IOCTL_HID_GET_INPUT_REPORT should always return immediately, if there is no report then it returns 0. If there is a report it returns the last report from the device.

IOCTL_HID_READ_REPORT is expected to pend until a report is available. In the grand scheme of things IOCTL_HID_READ_REPORT is expected to block until a report is available because the general use is that hidclass will be in a loop where it does a IOCTL_HID_READ_REPORT call and when the call succeeds it places that report into the ring buffer and then immediately calls again to get the next report.

IOCTL_HID_GET_INPUT_REPORT is generated by the corresponding IOCTL_HID_GET_INPUT_REPORT in the class driver and HidD_GetInputReport.

IOCTL_HID_READ_REPORT is like doing a FileRead.

So they need to behave quite a bit differently.
> https://www.julianloehr.de/wp-content/uploads/2014/07/Paper_HIDWiimote.pdf
> """In case the Wiimote state changes when there is no read request
> currently buffered, a flag is used to signal whether a newly received
> read request shall be processed and completed immediately."""

That would not be hard, just a flag that showed if the last reports has been read or not. In all practical terms the minidriver only has 1 consumer, the hidclass class driver devices, so in many ways the whole IRP queue is overkill as there should only ever be 1 client.  It would not be hard to add that.
> In Wine we have to cache the last report anyway, so we could implement
> something similar very easy. Do you think it makes sense? The
> implementation of IOCTL_HID_GET_INPUT_REPORT and IOCTL_HID_READ_REPORT
> would also not be much more difficult, as shown in the following
> pseudocode:
>
> --- snip ---
> case IOCTL_HID_GET_INPUT_REPORT:
> case IOCTL_HID_READ_REPORT:
>     EnterCriticalSection(report_cs);
>     ... start processing if not done yet ...
>     if (ioctl == IOCTL_HID_READ_REPORT && (!last_report || last_report_seen))
>     {
>         ... async handling ...
>         break;
>     }
>     ... synchronous handlng ...
>     last_report_seen = TRUE;
>     LeaveCriticalSection(report_cs);
>     break;
> --- snip ---

I will have to explore this when I have more time to code things but the basic idea looks pretty good though IOCTL_HID_GET_INPUT_REPORT uses HID_XFER_PACKETs and IOCTL_HID_READ_REPORT does not so that complicates things a fair bit more.

-aric

[PATCH v3] winebus.sys: Handle device reports for hidraw devices

[Sebastian Lackner]

On 19.10.2016 21:32, Aric Stewart wrote:
> Hi,
>
> It is important to remember that there is a big difference between
> the user level communications with the HID device and the minidriver
> level communications. The top of the stack, the HID device, has a
> ring buffer and such.

[...]
> Depends on the call that is made. IOCTL_HID_GET_INPUT_REPORT should
> always return immediately, if there is no report then it returns 0.
> If there is a report it returns the last report from the device.
>
> IOCTL_HID_READ_REPORT is expected to pend until a report is
> available. In the grand scheme of things IOCTL_HID_READ_REPORT is
> expected to block until a report is available because the general use
> is that hidclass will be in a loop where it does a
> IOCTL_HID_READ_REPORT call and when the call succeeds it places that
> report into the ring buffer and then immediately calls again to get
> the next report.
>
> IOCTL_HID_GET_INPUT_REPORT is generated by the corresponding
> IOCTL_HID_GET_INPUT_REPORT in the class driver and
> HidD_GetInputReport.
>
> IOCTL_HID_READ_REPORT is like doing a FileRead.

I'm aware of those differences. The part I'm more interested in is how
IOCTL_HID_READ_REPORT is implemented in Windows drivers. ;)
Even if Windows does not explicitly mention any kind of buffering, I
believe that data loss is much more unlikely than with the proposed Wine
implementation.

Usually hardware events trigger an interrupt service routine, which can
immediately complete the request. The code responsible for queuing a new
IOCTL_HID_READ_REPORT ioctl also runs in kernel mode, maybe even with a
higher priority to avoid any interruptions by user mode processes.
In addition, devices might also do their own buffering, to ensure that
the transmission works reliable.
> So they need to behave quite a bit differently.
> > > https://www.julianloehr.de/wp-content/uploads/2014/07/Paper_HIDWiimote.pdf
> > >
>
> """In case the Wiimote state changes when there is no read request
> > > currently buffered, a flag is used to signal whether a newly
> > > received read request shall be processed and completed
> > > immediately."""
>
> That would not be hard, just a flag that showed if the last reports
> has been read or not. In all practical terms the minidriver only has
> 1 consumer, the hidclass class driver devices, so in many ways the
> whole IRP queue is overkill as there should only ever be 1 client.
> It would not be hard to add that.
> > > In Wine we have to cache the last report anyway, so we could
> > > implement something similar very easy. Do you think it makes
> > > sense? The implementation of IOCTL_HID_GET_INPUT_REPORT and
> > > IOCTL_HID_READ_REPORT would also not be much more difficult, as
> > > shown in the following pseudocode:
> > >
> > > --- snip --- case IOCTL_HID_GET_INPUT_REPORT: case
> > > IOCTL_HID_READ_REPORT: EnterCriticalSection(report_cs); ... start
> > > processing if not done yet ... if (ioctl == IOCTL_HID_READ_REPORT
> > > && (!last_report || last_report_seen)) { ... async handling ... 
> > > break; } ... synchronous handlng ... last_report_seen = TRUE; 
> > > LeaveCriticalSection(report_cs); break; --- snip ---
>
> I will have to explore this when I have more time to code things but
> the basic idea looks pretty good though IOCTL_HID_GET_INPUT_REPORT
> uses HID_XFER_PACKETs and IOCTL_HID_READ_REPORT does not so that
> complicates things a fair bit more.

You are right, it would be a bit more complicated. Probably we could
move some of the code to a helper function, but I guess we will have
to try it out. ;)

Another idea we could consider (at least for Linux) would be to use
wine_server_fd_to_handle(), and then NtReadFile(). This would allow
to get rid of the thread, and we could use an APC to run IoCompleteRequest().
Have you actually looked into that? And which mechanism is required
on other platforms, do they also require a separate thread?
> -aric

[PATCH v3] winebus.sys: Handle device reports for hidraw devices

[Aric Stewart]

On 10/19/16 10:21 PM, Sebastian Lackner wrote:
> On 19.10.2016 21:32, Aric Stewart wrote:
> > Hi,
> >
> > It is important to remember that there is a big difference between
> > the user level communications with the HID device and the minidriver
> > level communications. The top of the stack, the HID device, has a
> > ring buffer and such.
>
> [...]
> > Depends on the call that is made. IOCTL_HID_GET_INPUT_REPORT should
> > always return immediately, if there is no report then it returns 0.
> > If there is a report it returns the last report from the device.
> >
> > IOCTL_HID_READ_REPORT is expected to pend until a report is
> > available. In the grand scheme of things IOCTL_HID_READ_REPORT is
> > expected to block until a report is available because the general use
> > is that hidclass will be in a loop where it does a
> > IOCTL_HID_READ_REPORT call and when the call succeeds it places that
> > report into the ring buffer and then immediately calls again to get
> > the next report.
> >
> > IOCTL_HID_GET_INPUT_REPORT is generated by the corresponding
> > IOCTL_HID_GET_INPUT_REPORT in the class driver and
> > HidD_GetInputReport.
> >
> > IOCTL_HID_READ_REPORT is like doing a FileRead.
>
> I'm aware of those differences. The part I'm more interested in is how
> IOCTL_HID_READ_REPORT is implemented in Windows drivers. ;)
> Even if Windows does not explicitly mention any kind of buffering, I
> believe that data loss is much more unlikely than with the proposed Wine
> implementation.
>
> Usually hardware events trigger an interrupt service routine, which can
> immediately complete the request. The code responsible for queuing a new
> IOCTL_HID_READ_REPORT ioctl also runs in kernel mode, maybe even with a
> higher priority to avoid any interruptions by user mode processes.
> In addition, devices might also do their own buffering, to ensure that
> the transmission works reliable.
> > So they need to behave quite a bit differently.
> > > > https://www.julianloehr.de/wp-content/uploads/2014/07/Paper_HIDWiimote.pdf
> > > >
> >
> > """In case the Wiimote state changes when there is no read request
> > > > currently buffered, a flag is used to signal whether a newly
> > > > received read request shall be processed and completed
> > > > immediately."""
> >
> > That would not be hard, just a flag that showed if the last reports
> > has been read or not. In all practical terms the minidriver only has
> > 1 consumer, the hidclass class driver devices, so in many ways the
> > whole IRP queue is overkill as there should only ever be 1 client.
> > It would not be hard to add that.
> > > > In Wine we have to cache the last report anyway, so we could
> > > > implement something similar very easy. Do you think it makes
> > > > sense? The implementation of IOCTL_HID_GET_INPUT_REPORT and
> > > > IOCTL_HID_READ_REPORT would also not be much more difficult, as
> > > > shown in the following pseudocode:
> > > >
> > > > --- snip --- case IOCTL_HID_GET_INPUT_REPORT: case
> > > > IOCTL_HID_READ_REPORT: EnterCriticalSection(report_cs); ... start
> > > > processing if not done yet ... if (ioctl == IOCTL_HID_READ_REPORT
> > > > && (!last_report || last_report_seen)) { ... async handling ... 
> > > > break; } ... synchronous handlng ... last_report_seen = TRUE; 
> > > > LeaveCriticalSection(report_cs); break; --- snip ---
> >
> > I will have to explore this when I have more time to code things but
> > the basic idea looks pretty good though IOCTL_HID_GET_INPUT_REPORT
> > uses HID_XFER_PACKETs and IOCTL_HID_READ_REPORT does not so that
> > complicates things a fair bit more.
>
> You are right, it would be a bit more complicated. Probably we could
> move some of the code to a helper function, but I guess we will have
> to try it out. ;)
>
> Another idea we could consider (at least for Linux) would be to use
> wine_server_fd_to_handle(), and then NtReadFile(). This would allow
> to get rid of the thread, and we could use an APC to run IoCompleteRequest().
> Have you actually looked into that? And which mechanism is required
> on other platforms, do they also require a separate thread?

I have not explored wine_server_fd_to_handle as I was not really aware of it, Mac and IOHID uses a callback, so a run loop thread is needed, but no read of any sort.  If you thinking doing that wine magic is correct then I would be up for exploring it, but it seems like the sort of thing that Alexandre would frown on.

-aric
> > -aric

[PATCH v3] winebus.sys: Handle device reports for hidraw devices

[Sebastian Lackner]

On 21.10.2016 05:39, Aric Stewart wrote:
> > > Another idea we could consider (at least for Linux) would be to
> > > use wine_server_fd_to_handle(), and then NtReadFile(). This would
> > > allow to get rid of the thread, and we could use an APC to run
> > > IoCompleteRequest(). Have you actually looked into that? And
> > > which mechanism is required on other platforms, do they also
> > > require a separate thread?
>
> I have not explored wine_server_fd_to_handle as I was not really
> aware of it, Mac and IOHID uses a callback, so a run loop thread is
> needed, but no read of any sort.  If you thinking doing that wine
> magic is correct then I would be up for exploring it, but it seems
> like the sort of thing that Alexandre would frown on.
>
> -aric

Well, using async NtReadFile would have the advantage that we can
start listening for reports at any time, without spawning a thread.
Nevertheless, if other platforms do not offer some similar mechanism,
it probably does not make any sense to implement it like that. I'm
also fine with a thread based approach.

I would still suggest to do further investigations regarding report
loss though. If there is not sufficient evidence that this happens
on Windows aswell, I would still prefer a more reliable Wine
implementation. MSDN suggests that in order to avoid dropped HID
reports, applications can try to increase HidD_SetNumInputBuffers -
but that will obviously not help if we already drop them in the
lower level drivers. The idea with a simple flags only works if we
can be sure that the thread runs fast enough to ensure records
are never dropped. Otherwise a more complicated approach would be
required.

Regards,
Sebastian

[PATCH v3] winebus.sys: Handle device reports for hidraw devices

[Aric Stewart]

On 10/21/16 7:21 AM, Sebastian Lackner wrote:
> On 21.10.2016 05:39, Aric Stewart wrote:
> > > > Another idea we could consider (at least for Linux) would be to
> > > > use wine_server_fd_to_handle(), and then NtReadFile(). This would
> > > > allow to get rid of the thread, and we could use an APC to run
> > > > IoCompleteRequest(). Have you actually looked into that? And
> > > > which mechanism is required on other platforms, do they also
> > > > require a separate thread?
> >
> > I have not explored wine_server_fd_to_handle as I was not really
> > aware of it, Mac and IOHID uses a callback, so a run loop thread is
> > needed, but no read of any sort.  If you thinking doing that wine
> > magic is correct then I would be up for exploring it, but it seems
> > like the sort of thing that Alexandre would frown on.
> >
> > -aric
>
> Well, using async NtReadFile would have the advantage that we can
> start listening for reports at any time, without spawning a thread.
> Nevertheless, if other platforms do not offer some similar mechanism,
> it probably does not make any sense to implement it like that. I'm
> also fine with a thread based approach.
>
> I would still suggest to do further investigations regarding report
> loss though. If there is not sufficient evidence that this happens
> on Windows aswell, I would still prefer a more reliable Wine
> implementation. MSDN suggests that in order to avoid dropped HID
> reports, applications can try to increase HidD_SetNumInputBuffers -
> but that will obviously not help if we already drop them in the
> lower level drivers. The idea with a simple flags only works if we
> can be sure that the thread runs fast enough to ensure records
> are never dropped. Otherwise a more complicated approach would be
> required.

Yes HidD_SetNumInputBuffers only affects the size of the ring buffer at the upper level HID device.

I my preferences is to go forward with the flag and wait to see if this is really an issue or not and then revisit it if we really see an issue.

-aric
> Regards,
> Sebastian