[PATCH] winebus.sys: Do not report HID report read errors unconditionally

[Kai Krakow]

Hello!

2018-08-15 20:36 GMT+02:00 Kai Krakow <kai at kaishome.de>:
> Hello!
>
> 2018-08-15 19:49 GMT+02:00 Aric Stewart <aric at codeweavers.com>:
>> On 8/15/18 1:47 AM, Kai Krakow wrote:
>>> Discarding the message only for axis events is safe, tho. This is why
>>> the boolean parameter was added. The other change just reduces the log
>>> impact in case the warning would be displayed.
>>>
>>> The problem here is that the intermediate layer needs to translate
>>> between HID events from Linux to full device reports for Windows, so
>>> it just coalesces events into the report state. But it's not a queue.
>>> Thus, having the warning may help debugging any problems left.
>>>
>>> But if someone with more insight into the whole driver stack sees no
>>> more problems, I'm okay with removing the message completely.
>>>
>>> @Aric: What do you think?
>>>
>>>
>>> Regards,
>>> Kai
>>>
>>
>> I have no strong opinion in that regard. It does seem like a lot of complexity to simply silence a message. I see no more problems with the driver stack and have no problem removing the message if Kai feels like it really is as harmless as it seems.
>
> I've made a v2 patch and sent it to the list. I adjusted the commit
> message to reflect the change.
>
> I also tried to get completely rid of `last_report_read` with this but
> it doesn't work because it is used in at least one place to track
> delivery of the report just before working on the queues, it seems. I
> wonder if this is necessary but this needs more investigation.

BTW: The error also disappears by just adding the boolean flag I
introduced. At least I did not longer see it. So it seems only the
axis events are causing this. That could be another variant of the
patch.


> Aric, it in `dlls/winebus.sys/main.c`:
>
> 547         case IOCTL_HID_READ_REPORT:
> 548         {
> 549             TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n");
> 550             EnterCriticalSection(&ext->report_cs);
> 551             status = ext->vtbl->begin_report_processing(device);
> 552             if (status != STATUS_SUCCESS)
> 553             {
> 554                 irp->IoStatus.u.Status = status;
> 555                 LeaveCriticalSection(&ext->report_cs);
> 556                 break;
> 557             }
> 558             if (!ext->last_report_read)
> ^^^ Could this also be discarded?
> 559             {
> 560                 irp->IoStatus.u.Status = status = deliver_last_report(ext,
> 561                     irpsp->Parameters.DeviceIoControl.OutputBufferLength,
> 562                     irp->UserBuffer, &irp->IoStatus.Information);
> 563                 ext->last_report_read = TRUE;
> 564             }
> 565             else
> 566             {
> 567                 InsertTailList(&ext->irp_queue,
> &irp->Tail.Overlay.s.ListEntry);
> 568                 status = STATUS_PENDING;
> 569             }
> 570             LeaveCriticalSection(&ext->report_cs);
> 571             break;
>
>
> Regards,
> Kai