[PATCH] hidclass.sys: When processing reads fill all the buffers

[Aric Stewart]

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