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

[Aric Stewart]

On 8/15/18 1:48 PM, Kai Krakow wrote:
> 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.
> 


Ok that is great to know.

Thanks

-aric

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