[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