[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
>