Re: [PATCH] hidclass.sys: When processing reads fill all the buffers
On 26.01.2017 13:57, Aric Stewart wrote:
This is a follow-up fix for continuing work on bug 42225
Signed-off-by: Aric Stewart <aric(a)codeweavers.com> --- dlls/hidclass.sys/buffer.c | 34 +++++++++++++++++++++++++++++++++- dlls/hidclass.sys/device.c | 2 +- dlls/hidclass.sys/hid.h | 3 ++- 3 files changed, 36 insertions(+), 3 deletions(-)
0001-hidclass.sys-When-processing-reads-fill-all-the-buffer.txt
diff --git a/dlls/hidclass.sys/buffer.c b/dlls/hidclass.sys/buffer.c index 0b29f97b69..41bbb8e241 100644 --- a/dlls/hidclass.sys/buffer.c +++ b/dlls/hidclass.sys/buffer.c @@ -127,7 +127,7 @@ NTSTATUS RingBuffer_SetSize(struct ReportRingBuffer *ring, UINT size) return STATUS_SUCCESS; }
-void RingBuffer_Read(struct ReportRingBuffer *ring, UINT index, void *output, UINT *size) +void RingBuffer_ReadNew(struct ReportRingBuffer *ring, UINT index, void *output, UINT *size) { void *ret = NULL;
@@ -155,6 +155,38 @@ void RingBuffer_Read(struct ReportRingBuffer *ring, UINT index, void *output, UI } }
+void RingBuffer_Read(struct ReportRingBuffer *ring, UINT index, void *output, UINT *size) +{ + int pointer = ring->pointers[index]; + void *ret = NULL; + + EnterCriticalSection(&ring->lock); + if (index >= ring->pointer_alloc || ring->pointers[index] == POINTER_UNUSED + || ring->end == ring->start) + { + LeaveCriticalSection(&ring->lock); + *size = 0; + return; + } + + if (pointer == ring->end) + pointer--; + + if (pointer < 0) + pointer = ring->size - 1; + + ret = &ring->buffer[pointer * ring->buffer_size]; + memcpy(output, ret, ring->buffer_size); + if (pointer == ring->pointers[index]) + { + ring->pointers[index]++; + if (ring->pointers[index] == ring->size) + ring->pointers[index] = 0; + } + LeaveCriticalSection(&ring->lock); + *size = ring->buffer_size;
Have you been able to reproduce a similar behavior on Windows? The new code looks very suspicious and I'm not sure if this is the right approach to fix it. Also please note that this will effectively disable any asynchronous Read requests - HID_Device_read() will always use the first code path when there is at least one package in the ring buffer and never return STATUS_PENDING.
+} + UINT RingBuffer_AddPointer(struct ReportRingBuffer *ring) { UINT idx; diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index b241016e72..d6a0a358e3 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -662,7 +662,7 @@ NTSTATUS WINAPI HID_Device_read(DEVICE_OBJECT *device, IRP *irp) ptr = PtrToUlong( irp->Tail.Overlay.OriginalFileObject->FsContext );
irp->IoStatus.Information = 0; - RingBuffer_Read(ext->ring_buffer, ptr, packet, &buffer_size); + RingBuffer_ReadNew(ext->ring_buffer, ptr, packet, &buffer_size);
if (buffer_size) { diff --git a/dlls/hidclass.sys/hid.h b/dlls/hidclass.sys/hid.h index adc547cc43..1829319ccf 100644 --- a/dlls/hidclass.sys/hid.h +++ b/dlls/hidclass.sys/hid.h @@ -62,7 +62,8 @@ typedef struct _BASE_DEVICE_EXTENSION { void RingBuffer_Write(struct ReportRingBuffer *buffer, void *data) DECLSPEC_HIDDEN; UINT RingBuffer_AddPointer(struct ReportRingBuffer *buffer) DECLSPEC_HIDDEN; void RingBuffer_RemovePointer(struct ReportRingBuffer *ring, UINT index) DECLSPEC_HIDDEN; -void RingBuffer_Read(struct ReportRingBuffer *buffer, UINT index, void *output, UINT *size) DECLSPEC_HIDDEN; +void RingBuffer_Read(struct ReportRingBuffer *ring, UINT index, void *output, UINT *size) DECLSPEC_HIDDEN; +void RingBuffer_ReadNew(struct ReportRingBuffer *buffer, UINT index, void *output, UINT *size) DECLSPEC_HIDDEN; UINT RingBuffer_GetBufferSize(struct ReportRingBuffer *buffer) DECLSPEC_HIDDEN; UINT RingBuffer_GetSize(struct ReportRingBuffer *buffer) DECLSPEC_HIDDEN; void RingBuffer_Destroy(struct ReportRingBuffer *buffer) DECLSPEC_HIDDEN;
Hi Sebastian, On 1/26/17 8:03 AM, Sebastian Lackner wrote:
On 26.01.2017 13:57, Aric Stewart wrote:
This is a follow-up fix for continuing work on bug 42225
Signed-off-by: Aric Stewart <aric(a)codeweavers.com> --- dlls/hidclass.sys/buffer.c | 34 +++++++++++++++++++++++++++++++++- dlls/hidclass.sys/device.c | 2 +- dlls/hidclass.sys/hid.h | 3 ++- 3 files changed, 36 insertions(+), 3 deletions(-)
0001-hidclass.sys-When-processing-reads-fill-all-the-buffer.txt
diff --git a/dlls/hidclass.sys/buffer.c b/dlls/hidclass.sys/buffer.c index 0b29f97b69..41bbb8e241 100644 --- a/dlls/hidclass.sys/buffer.c +++ b/dlls/hidclass.sys/buffer.c @@ -127,7 +127,7 @@ NTSTATUS RingBuffer_SetSize(struct ReportRingBuffer *ring, UINT size) return STATUS_SUCCESS; }
-void RingBuffer_Read(struct ReportRingBuffer *ring, UINT index, void *output, UINT *size) +void RingBuffer_ReadNew(struct ReportRingBuffer *ring, UINT index, void *output, UINT *size) { void *ret = NULL;
@@ -155,6 +155,38 @@ void RingBuffer_Read(struct ReportRingBuffer *ring, UINT index, void *output, UI } }
+void RingBuffer_Read(struct ReportRingBuffer *ring, UINT index, void *output, UINT *size) +{ + int pointer = ring->pointers[index]; + void *ret = NULL; + + EnterCriticalSection(&ring->lock); + if (index >= ring->pointer_alloc || ring->pointers[index] == POINTER_UNUSED + || ring->end == ring->start) + { + LeaveCriticalSection(&ring->lock); + *size = 0; + return; + } + + if (pointer == ring->end) + pointer--; + + if (pointer < 0) + pointer = ring->size - 1; + + ret = &ring->buffer[pointer * ring->buffer_size]; + memcpy(output, ret, ring->buffer_size); + if (pointer == ring->pointers[index]) + { + ring->pointers[index]++; + if (ring->pointers[index] == ring->size) + ring->pointers[index] = 0; + } + LeaveCriticalSection(&ring->lock); + *size = ring->buffer_size;
Have you been able to reproduce a similar behavior on Windows? The new code
Yes, I have a test case that produces behavior similar to bug 42225, But I have not been successful at getting even the start of the testing framework in place to be able to put in more tests.
looks very suspicious and I'm not sure if this is the right approach to fix
Ok, suggestions?
it. Also please note that this will effectively disable any asynchronous Read requests - HID_Device_read() will always use the first code path when there is at least one package in the ring buffer and never return STATUS_PENDING.
Nope, it will not. HID_Device_read uses RingBuffer_ReadNew. So the behavior there will be unchanged. This only affects HID_Device_processQueue, which is only called when the device has received a report and has pending IRP requests. The trick is that if a single FileOpen makes multiple asynchronous FileRead requests all of them should be fulfilled not just the first one. -aric
+} + UINT RingBuffer_AddPointer(struct ReportRingBuffer *ring) { UINT idx; diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index b241016e72..d6a0a358e3 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -662,7 +662,7 @@ NTSTATUS WINAPI HID_Device_read(DEVICE_OBJECT *device, IRP *irp) ptr = PtrToUlong( irp->Tail.Overlay.OriginalFileObject->FsContext );
irp->IoStatus.Information = 0; - RingBuffer_Read(ext->ring_buffer, ptr, packet, &buffer_size); + RingBuffer_ReadNew(ext->ring_buffer, ptr, packet, &buffer_size);
if (buffer_size) { diff --git a/dlls/hidclass.sys/hid.h b/dlls/hidclass.sys/hid.h index adc547cc43..1829319ccf 100644 --- a/dlls/hidclass.sys/hid.h +++ b/dlls/hidclass.sys/hid.h @@ -62,7 +62,8 @@ typedef struct _BASE_DEVICE_EXTENSION { void RingBuffer_Write(struct ReportRingBuffer *buffer, void *data) DECLSPEC_HIDDEN; UINT RingBuffer_AddPointer(struct ReportRingBuffer *buffer) DECLSPEC_HIDDEN; void RingBuffer_RemovePointer(struct ReportRingBuffer *ring, UINT index) DECLSPEC_HIDDEN; -void RingBuffer_Read(struct ReportRingBuffer *buffer, UINT index, void *output, UINT *size) DECLSPEC_HIDDEN; +void RingBuffer_Read(struct ReportRingBuffer *ring, UINT index, void *output, UINT *size) DECLSPEC_HIDDEN; +void RingBuffer_ReadNew(struct ReportRingBuffer *buffer, UINT index, void *output, UINT *size) DECLSPEC_HIDDEN; UINT RingBuffer_GetBufferSize(struct ReportRingBuffer *buffer) DECLSPEC_HIDDEN; UINT RingBuffer_GetSize(struct ReportRingBuffer *buffer) DECLSPEC_HIDDEN; void RingBuffer_Destroy(struct ReportRingBuffer *buffer) DECLSPEC_HIDDEN;
participants (2)
-
Aric Stewart -
Sebastian Lackner