Re: [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
> >
> > 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;