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

[Kai Krakow]

Hello!

2018-08-15 19:49 GMT+02:00 Aric Stewart :
> 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.

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