[PATCH] winebus.sys: Do not report HID report read errors unconditionally
Device reports may come in faster than our consumers could possibly read them, this is especially true for multi-axis events: When you move a stick across its range, it will always generate at least two events, one for the x axis, and one for the y axis. This is not really an error situation, so let's report this situation only once at most. If we already know the multi-event situation, let's skip logging this completely: We won't loose any information anyway because the report contains a complete device state and only axis positions were updated. Thus, this commit adds a parameter to process_hid_report() to let it know if we are currently processing reports that are known to be sent multiple times in sequence (with updated reports). Also, if our consumers are slow to respond, then report the issue only once and not per each occurrence during the duration of one consumer read cycle. Finally, this is not really an error, it's a warning at most, thus degrade the error message to a warning to not pollute peoples consoles and logs with unimportant stuff. Especially during gaming I was seeing screens over screens full of only this single log message. This commit fixes it. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=43125 CC: Aric Stewart <aric(a)codeweavers.com> Signed-off-by: Kai Krakow <kai(a)kaishome.de> --- dlls/winebus.sys/bus.h | 2 +- dlls/winebus.sys/bus_iohid.c | 2 +- dlls/winebus.sys/bus_sdl.c | 12 ++++++------ dlls/winebus.sys/bus_udev.c | 4 ++-- dlls/winebus.sys/main.c | 18 +++++++++++++++--- 5 files changed, 25 insertions(+), 13 deletions(-) diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h index bf93c04259..1e2989e5c5 100644 --- a/dlls/winebus.sys/bus.h +++ b/dlls/winebus.sys/bus.h @@ -45,7 +45,7 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, DEVICE_OBJECT *bus_find_hid_device(const platform_vtbl *vtbl, void *platform_dev) DECLSPEC_HIDDEN; void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN; -void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) DECLSPEC_HIDDEN; +void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length, BOOL warn_multi_events) DECLSPEC_HIDDEN; DEVICE_OBJECT* bus_enumerate_hid_devices(const platform_vtbl *vtbl, enum_func function, void* context) DECLSPEC_HIDDEN; /* General Bus Functions */ diff --git a/dlls/winebus.sys/bus_iohid.c b/dlls/winebus.sys/bus_iohid.c index 501a40d26e..74e5d82c9f 100644 --- a/dlls/winebus.sys/bus_iohid.c +++ b/dlls/winebus.sys/bus_iohid.c @@ -135,7 +135,7 @@ static void handle_IOHIDDeviceIOHIDReportCallback(void *context, uint32_t reportID, uint8_t *report, CFIndex report_length) { DEVICE_OBJECT *device = (DEVICE_OBJECT*)context; - process_hid_report(device, report, report_length); + process_hid_report(device, report, report_length, TRUE); } static int compare_platform_device(DEVICE_OBJECT *device, void *platform_dev) diff --git a/dlls/winebus.sys/bus_sdl.c b/dlls/winebus.sys/bus_sdl.c index 159727f6bc..99a206cb34 100644 --- a/dlls/winebus.sys/bus_sdl.c +++ b/dlls/winebus.sys/bus_sdl.c @@ -683,7 +683,7 @@ static BOOL set_report_from_event(SDL_Event *event) set_button_value(ie->button, ie->state, private->report_buffer); - process_hid_report(device, private->report_buffer, private->buffer_length); + process_hid_report(device, private->report_buffer, private->buffer_length, TRUE); break; } case SDL_JOYAXISMOTION: @@ -693,7 +693,7 @@ static BOOL set_report_from_event(SDL_Event *event) if (ie->axis < 6) { set_axis_value(private, ie->axis, ie->value); - process_hid_report(device, private->report_buffer, private->buffer_length); + process_hid_report(device, private->report_buffer, private->buffer_length, FALSE); } break; } @@ -702,7 +702,7 @@ static BOOL set_report_from_event(SDL_Event *event) SDL_JoyBallEvent *ie = &event->jball; set_ball_value(private, ie->ball, ie->xrel, ie->yrel); - process_hid_report(device, private->report_buffer, private->buffer_length); + process_hid_report(device, private->report_buffer, private->buffer_length, FALSE); break; } case SDL_JOYHATMOTION: @@ -710,7 +710,7 @@ static BOOL set_report_from_event(SDL_Event *event) SDL_JoyHatEvent *ie = &event->jhat; set_hat_value(private, ie->hat, ie->value); - process_hid_report(device, private->report_buffer, private->buffer_length); + process_hid_report(device, private->report_buffer, private->buffer_length, TRUE); break; } default: @@ -765,7 +765,7 @@ static BOOL set_mapped_report_from_event(SDL_Event *event) if (usage >= 0) { set_button_value(usage, ie->state, private->report_buffer); - process_hid_report(device, private->report_buffer, private->buffer_length); + process_hid_report(device, private->report_buffer, private->buffer_length, TRUE); } break; } @@ -774,7 +774,7 @@ static BOOL set_mapped_report_from_event(SDL_Event *event) SDL_ControllerAxisEvent *ie = &event->caxis; set_axis_value(private, ie->axis, ie->value); - process_hid_report(device, private->report_buffer, private->buffer_length); + process_hid_report(device, private->report_buffer, private->buffer_length, FALSE); break; } default: diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c index d34e18c71c..47b9758ac2 100644 --- a/dlls/winebus.sys/bus_udev.c +++ b/dlls/winebus.sys/bus_udev.c @@ -768,7 +768,7 @@ static DWORD CALLBACK device_report_thread(void *args) else if (size == 0) TRACE_(hid_report)("Failed to read report\n"); else - process_hid_report(device, report_buffer, size); + process_hid_report(device, report_buffer, size, TRUE); } return 0; } @@ -979,7 +979,7 @@ static DWORD CALLBACK lnxev_device_report_thread(void *args) else if (size == 0) TRACE_(hid_report)("Failed to read report\n"); else if (set_report_from_event(private, &ie)) - process_hid_report(device, private->current_report_buffer, private->buffer_length); + process_hid_report(device, private->current_report_buffer, private->buffer_length, TRUE); } return 0; } diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 2f3c05aa96..624ddd5f8d 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -614,11 +614,12 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) return status; } -void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) +void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length, BOOL warn_multi_events) { struct device_extension *ext = (struct device_extension*)device->DeviceExtension; IRP *irp; LIST_ENTRY *entry; + static int overwrite_reported; if (!length || !report) return; @@ -641,8 +642,18 @@ void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) ext->buffer_size = length; } - if (!ext->last_report_read) - ERR_(hid_report)("Device reports coming in too fast, last report not read yet!\n"); + /* + * Device reports may come in faster than our consumers could possibly + * read them, this is especially true for multi-axis events: When you move + * a stick across its range, it will always generate at least two events, + * one for the x axis, and one for the y axis. This is not really an error + * situation, so let's report this situation only once at most. If we + * already know the multi-event situation, let's skip logging this + * completely: We won't loose any information anyway because the report + * contains a complete device state and only axis positions were updated. + */ + if (warn_multi_events && !ext->last_report_read && !overwrite_reported++) + WARN_(hid_report)("Device reports coming in too fast, last report not read yet!\n"); memcpy(ext->last_report, report, length); ext->last_report_size = length; @@ -659,6 +670,7 @@ void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) irp->UserBuffer, &irp->IoStatus.Information); ext->last_report_read = TRUE; IoCompleteRequest(irp, IO_NO_INCREMENT); + overwrite_reported = 0; } LeaveCriticalSection(&ext->report_cs); } -- 2.16.4
Signed-off-by: Aric Stewart <aric(a)codeweavers.com> On 7/28/18 3:24 AM, Kai Krakow wrote:
Device reports may come in faster than our consumers could possibly read them, this is especially true for multi-axis events: When you move a stick across its range, it will always generate at least two events, one for the x axis, and one for the y axis. This is not really an error situation, so let's report this situation only once at most. If we already know the multi-event situation, let's skip logging this completely: We won't loose any information anyway because the report contains a complete device state and only axis positions were updated.
Thus, this commit adds a parameter to process_hid_report() to let it know if we are currently processing reports that are known to be sent multiple times in sequence (with updated reports).
Also, if our consumers are slow to respond, then report the issue only once and not per each occurrence during the duration of one consumer read cycle.
Finally, this is not really an error, it's a warning at most, thus degrade the error message to a warning to not pollute peoples consoles and logs with unimportant stuff.
Especially during gaming I was seeing screens over screens full of only this single log message. This commit fixes it.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=43125 CC: Aric Stewart <aric(a)codeweavers.com> Signed-off-by: Kai Krakow <kai(a)kaishome.de> --- dlls/winebus.sys/bus.h | 2 +- dlls/winebus.sys/bus_iohid.c | 2 +- dlls/winebus.sys/bus_sdl.c | 12 ++++++------ dlls/winebus.sys/bus_udev.c | 4 ++-- dlls/winebus.sys/main.c | 18 +++++++++++++++--- 5 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h index bf93c04259..1e2989e5c5 100644 --- a/dlls/winebus.sys/bus.h +++ b/dlls/winebus.sys/bus.h @@ -45,7 +45,7 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, DEVICE_OBJECT *bus_find_hid_device(const platform_vtbl *vtbl, void *platform_dev) DECLSPEC_HIDDEN; void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN; -void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) DECLSPEC_HIDDEN; +void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length, BOOL warn_multi_events) DECLSPEC_HIDDEN; DEVICE_OBJECT* bus_enumerate_hid_devices(const platform_vtbl *vtbl, enum_func function, void* context) DECLSPEC_HIDDEN;
/* General Bus Functions */ diff --git a/dlls/winebus.sys/bus_iohid.c b/dlls/winebus.sys/bus_iohid.c index 501a40d26e..74e5d82c9f 100644 --- a/dlls/winebus.sys/bus_iohid.c +++ b/dlls/winebus.sys/bus_iohid.c @@ -135,7 +135,7 @@ static void handle_IOHIDDeviceIOHIDReportCallback(void *context, uint32_t reportID, uint8_t *report, CFIndex report_length) { DEVICE_OBJECT *device = (DEVICE_OBJECT*)context; - process_hid_report(device, report, report_length); + process_hid_report(device, report, report_length, TRUE); }
static int compare_platform_device(DEVICE_OBJECT *device, void *platform_dev) diff --git a/dlls/winebus.sys/bus_sdl.c b/dlls/winebus.sys/bus_sdl.c index 159727f6bc..99a206cb34 100644 --- a/dlls/winebus.sys/bus_sdl.c +++ b/dlls/winebus.sys/bus_sdl.c @@ -683,7 +683,7 @@ static BOOL set_report_from_event(SDL_Event *event)
set_button_value(ie->button, ie->state, private->report_buffer);
- process_hid_report(device, private->report_buffer, private->buffer_length); + process_hid_report(device, private->report_buffer, private->buffer_length, TRUE); break; } case SDL_JOYAXISMOTION: @@ -693,7 +693,7 @@ static BOOL set_report_from_event(SDL_Event *event) if (ie->axis < 6) { set_axis_value(private, ie->axis, ie->value); - process_hid_report(device, private->report_buffer, private->buffer_length); + process_hid_report(device, private->report_buffer, private->buffer_length, FALSE); } break; } @@ -702,7 +702,7 @@ static BOOL set_report_from_event(SDL_Event *event) SDL_JoyBallEvent *ie = &event->jball;
set_ball_value(private, ie->ball, ie->xrel, ie->yrel); - process_hid_report(device, private->report_buffer, private->buffer_length); + process_hid_report(device, private->report_buffer, private->buffer_length, FALSE); break; } case SDL_JOYHATMOTION: @@ -710,7 +710,7 @@ static BOOL set_report_from_event(SDL_Event *event) SDL_JoyHatEvent *ie = &event->jhat;
set_hat_value(private, ie->hat, ie->value); - process_hid_report(device, private->report_buffer, private->buffer_length); + process_hid_report(device, private->report_buffer, private->buffer_length, TRUE); break; } default: @@ -765,7 +765,7 @@ static BOOL set_mapped_report_from_event(SDL_Event *event) if (usage >= 0) { set_button_value(usage, ie->state, private->report_buffer); - process_hid_report(device, private->report_buffer, private->buffer_length); + process_hid_report(device, private->report_buffer, private->buffer_length, TRUE); } break; } @@ -774,7 +774,7 @@ static BOOL set_mapped_report_from_event(SDL_Event *event) SDL_ControllerAxisEvent *ie = &event->caxis;
set_axis_value(private, ie->axis, ie->value); - process_hid_report(device, private->report_buffer, private->buffer_length); + process_hid_report(device, private->report_buffer, private->buffer_length, FALSE); break; } default: diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c index d34e18c71c..47b9758ac2 100644 --- a/dlls/winebus.sys/bus_udev.c +++ b/dlls/winebus.sys/bus_udev.c @@ -768,7 +768,7 @@ static DWORD CALLBACK device_report_thread(void *args) else if (size == 0) TRACE_(hid_report)("Failed to read report\n"); else - process_hid_report(device, report_buffer, size); + process_hid_report(device, report_buffer, size, TRUE); } return 0; } @@ -979,7 +979,7 @@ static DWORD CALLBACK lnxev_device_report_thread(void *args) else if (size == 0) TRACE_(hid_report)("Failed to read report\n"); else if (set_report_from_event(private, &ie)) - process_hid_report(device, private->current_report_buffer, private->buffer_length); + process_hid_report(device, private->current_report_buffer, private->buffer_length, TRUE); } return 0; } diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 2f3c05aa96..624ddd5f8d 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -614,11 +614,12 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) return status; }
-void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) +void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length, BOOL warn_multi_events) { struct device_extension *ext = (struct device_extension*)device->DeviceExtension; IRP *irp; LIST_ENTRY *entry; + static int overwrite_reported;
if (!length || !report) return; @@ -641,8 +642,18 @@ void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) ext->buffer_size = length; }
- if (!ext->last_report_read) - ERR_(hid_report)("Device reports coming in too fast, last report not read yet!\n"); + /* + * Device reports may come in faster than our consumers could possibly + * read them, this is especially true for multi-axis events: When you move + * a stick across its range, it will always generate at least two events, + * one for the x axis, and one for the y axis. This is not really an error + * situation, so let's report this situation only once at most. If we + * already know the multi-event situation, let's skip logging this + * completely: We won't loose any information anyway because the report + * contains a complete device state and only axis positions were updated. + */ + if (warn_multi_events && !ext->last_report_read && !overwrite_reported++) + WARN_(hid_report)("Device reports coming in too fast, last report not read yet!\n");
memcpy(ext->last_report, report, length); ext->last_report_size = length; @@ -659,6 +670,7 @@ void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) irp->UserBuffer, &irp->IoStatus.Information); ext->last_report_read = TRUE; IoCompleteRequest(irp, IO_NO_INCREMENT); + overwrite_reported = 0; } LeaveCriticalSection(&ext->report_cs); }
Kai Krakow <kai(a)kaishome.de> writes:
Device reports may come in faster than our consumers could possibly read them, this is especially true for multi-axis events: When you move a stick across its range, it will always generate at least two events, one for the x axis, and one for the y axis. This is not really an error situation, so let's report this situation only once at most. If we already know the multi-event situation, let's skip logging this completely: We won't loose any information anyway because the report contains a complete device state and only axis positions were updated.
Thus, this commit adds a parameter to process_hid_report() to let it know if we are currently processing reports that are known to be sent multiple times in sequence (with updated reports).
Also, if our consumers are slow to respond, then report the issue only once and not per each occurrence during the duration of one consumer read cycle.
Finally, this is not really an error, it's a warning at most, thus degrade the error message to a warning to not pollute peoples consoles and logs with unimportant stuff.
Is it really necessary to add all that complexity then? Why not simply remove the message? -- Alexandre Julliard julliard(a)winehq.org
Hello! 2018-08-15 8:10 GMT+02:00 Alexandre Julliard <julliard(a)winehq.org>:
Kai Krakow <kai(a)kaishome.de> writes:
Device reports may come in faster than our consumers could possibly read them, this is especially true for multi-axis events: When you move a stick across its range, it will always generate at least two events, one for the x axis, and one for the y axis. This is not really an error situation, so let's report this situation only once at most. If we already know the multi-event situation, let's skip logging this completely: We won't loose any information anyway because the report contains a complete device state and only axis positions were updated.
Thus, this commit adds a parameter to process_hid_report() to let it know if we are currently processing reports that are known to be sent multiple times in sequence (with updated reports).
Also, if our consumers are slow to respond, then report the issue only once and not per each occurrence during the duration of one consumer read cycle.
Finally, this is not really an error, it's a warning at most, thus degrade the error message to a warning to not pollute peoples consoles and logs with unimportant stuff.
Is it really necessary to add all that complexity then? Why not simply remove the message?
I was also thinking about it but I'm not completely sure if there are valid cases for the warning to appear. As lined out, it may still loose button events if a reader in the queues doesn't read fast enough. And I don't know the complete driver stack in detail. 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
On 8/15/18 1:47 AM, Kai Krakow wrote:
Hello!
2018-08-15 8:10 GMT+02:00 Alexandre Julliard <julliard(a)winehq.org>:
Kai Krakow <kai(a)kaishome.de> writes:
Device reports may come in faster than our consumers could possibly read them, this is especially true for multi-axis events: When you move a stick across its range, it will always generate at least two events, one for the x axis, and one for the y axis. This is not really an error situation, so let's report this situation only once at most. If we already know the multi-event situation, let's skip logging this completely: We won't loose any information anyway because the report contains a complete device state and only axis positions were updated.
Thus, this commit adds a parameter to process_hid_report() to let it know if we are currently processing reports that are known to be sent multiple times in sequence (with updated reports).
Also, if our consumers are slow to respond, then report the issue only once and not per each occurrence during the duration of one consumer read cycle.
Finally, this is not really an error, it's a warning at most, thus degrade the error message to a warning to not pollute peoples consoles and logs with unimportant stuff.
Is it really necessary to add all that complexity then? Why not simply remove the message?
I was also thinking about it but I'm not completely sure if there are valid cases for the warning to appear. As lined out, it may still loose button events if a reader in the queues doesn't read fast enough. And I don't know the complete driver stack in detail.
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. -aric
Hello! 2018-08-15 19:49 GMT+02:00 Aric Stewart <aric(a)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. 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
Hello! 2018-08-15 20:36 GMT+02:00 Kai Krakow <kai(a)kaishome.de>:
Hello!
2018-08-15 19:49 GMT+02:00 Aric Stewart <aric(a)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
On 8/15/18 1:48 PM, Kai Krakow wrote:
Hello!
2018-08-15 20:36 GMT+02:00 Kai Krakow <kai(a)kaishome.de>:
Hello!
2018-08-15 19:49 GMT+02:00 Aric Stewart <aric(a)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
participants (3)
-
Alexandre Julliard -
Aric Stewart -
Kai Krakow