Re: [7/13](resend)hidclass.sys: Implement IRP_MJ_READ for HID Devices

[Andrew Eikum]

On Wed, Sep 02, 2015 at 07:02:11AM -0500, Aric Stewart wrote:
> diff --git a/dlls/hidclass.sys/buffer.c b/dlls/hidclass.sys/buffer.c
> index 2561235..6f09393 100644
> --- a/dlls/hidclass.sys/buffer.c
> +++ b/dlls/hidclass.sys/buffer.c
> @@ -80,6 +80,39 @@ void RingBuffer_Destroy(struct ReportRingBuffer *ring)
>      HeapFree(GetProcessHeap(), 0, ring);
>  }
>
> +UINT RingBuffer_GetBufferSize(struct ReportRingBuffer *ring)
> +{
> - return ring->buffer_size;
>
> +}
> -
>
> +void RingBuffer_Read(struct ReportRingBuffer *ring, UINT index, void *output, UINT *size)
> +{
> - void *ret = NULL;
> -
> - EnterCriticalSection(&ring->lock);
> - if (index >= ring->pointer_alloc || ring->pointers[index] == 0xffffffff)
> - {
> -
>    LeaveCriticalSection(&ring->lock);
> -
>    *size = 0;
> -
>    return;
> - }
> - if (ring->pointers[index] == ring->end)
> - {
> -
>    LeaveCriticalSection(&ring->lock);
> -
>    *size = 0;
> - }
> - else
> - {
> -
>    ret = &ring->buffer[ring->pointers[index] * ring->buffer_size];
> -
>    memcpy(output, ret, ring->buffer_size);

Is it necessary to make a copy of the data in the ring buffer? Seems
like returning a raw pointer would be more efficient. I flipped
through the rest of the patch sequence and all uses of RingBuffer_Read
seemed to be read-only.
> -
>    ring->pointers[index]++;
> -
>    if (ring->pointers[index] == ring->size)
> -
>        ring->pointers[index] = 0;
> -
>    LeaveCriticalSection(&ring->lock);
> -
>    *size = ring->buffer_size;
> - }
>
> +}
> -
>
> UINT RingBuffer_AddPointer(struct ReportRingBuffer *ring)
>  {
>      UINT idx;
> diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
> index a6660db..326c994 100644
> --- a/dlls/hidclass.sys/device.c
> +++ b/dlls/hidclass.sys/device.c
> @@ -36,6 +36,7 @@
>  #include "devguid.h"
>
> WINE_DEFAULT_DEBUG_CHANNEL(hid);
> +WINE_DECLARE_DEBUG_CHANNEL(hid_report);
>
> static const WCHAR device_name_fmtW[] = {'\','D','e','v','i','c','e',
>      '\','H','I','D','#','%','p','&','%','p',0};
> @@ -353,6 +354,48 @@ NTSTATUS WINAPI HID_Device_ioctl(DEVICE_OBJECT *device, IRP *irp)
>      return rc;
>  }
>
> +NTSTATUS WINAPI HID_Device_read(DEVICE_OBJECT *device, IRP *irp)
> +{
> - HID_XFER_PACKET *packet;
> - BASE_DEVICE_EXTENSION *ext = device->DeviceExtension;
> - UINT buffer_size = RingBuffer_GetBufferSize(ext->ring_buffer);
> - NTSTATUS rc = STATUS_SUCCESS;
> - int ptr = -1;
> -
> - packet = HeapAlloc(GetProcessHeap(), 0, buffer_size);
> - ptr = (int)irp->Tail.Overlay.OriginalFileObject->FsContext;
> -
> - irp->IoStatus.Information = 0;
> - RingBuffer_Read(ext->ring_buffer, ptr, packet, &buffer_size);
> -
> - if (buffer_size)
> - {
> -
>    IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation( irp );
> -
>    TRACE_(hid_report)("Got Packet %p %i\n", packet->reportBuffer, packet->reportBufferLen);
> -
>    if (irpsp->Parameters.Read.Length >= packet->reportBufferLen)
> -
>    {
> -
>        memcpy(irp->AssociatedIrp.SystemBuffer, packet->reportBuffer, packet->reportBufferLen);
> -
>        irp->IoStatus.Information = packet->reportBufferLen;
> -
>        irp->IoStatus.u.Status = STATUS_SUCCESS;
> -
>    }
> -
>    else
> -
>    {
> -
>        irp->IoStatus.Information = 0;
> -
>        irp->IoStatus.u.Status = STATUS_BUFFER_OVERFLOW;
> -
>    }
> -
>    IoCompleteRequest( irp, IO_NO_INCREMENT );
> - }
> - else
> - {
> -
>    TRACE_(hid_report)("Queue irp\n");
> -
>    InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry);
> -
>    rc = STATUS_PENDING;
> - }
> - HeapFree(GetProcessHeap(), 0, packet);
> -
> - return rc;
>
> +}
> -
>
> NTSTATUS WINAPI HID_Device_create(DEVICE_OBJECT *device, IRP *irp)
>  {
>      BASE_DEVICE_EXTENSION *ext = device->DeviceExtension;
> diff --git a/dlls/hidclass.sys/hid.h b/dlls/hidclass.sys/hid.h
> index c9bc338..34ec241 100644
> --- a/dlls/hidclass.sys/hid.h
> +++ b/dlls/hidclass.sys/hid.h
> @@ -58,6 +58,8 @@ typedef struct _BASE_DEVICE_EXTENSTION {
>
> 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;
> +UINT RingBuffer_GetBufferSize(struct ReportRingBuffer *buffer) DECLSPEC_HIDDEN;
>  void RingBuffer_Destroy(struct ReportRingBuffer *buffer) DECLSPEC_HIDDEN;
>  struct ReportRingBuffer* RingBuffer_Create(UINT buffer_size) DECLSPEC_HIDDEN;
>
> @@ -81,6 +83,7 @@ NTSTATUS HID_LinkDevice(DEVICE_OBJECT *device, LPCWSTR serial, LPCWSTR index) DE
>  void HID_DeleteDevice(HID_MINIDRIVER_REGISTRATION *driver, DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
>
> NTSTATUS WINAPI HID_Device_ioctl(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
> +NTSTATUS WINAPI HID_Device_read(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
>  NTSTATUS WINAPI HID_Device_create(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
>  NTSTATUS WINAPI HID_Device_close(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
>
> diff --git a/dlls/hidclass.sys/main.c b/dlls/hidclass.sys/main.c
> index 1e0e802..4be4409 100644
> --- a/dlls/hidclass.sys/main.c
> +++ b/dlls/hidclass.sys/main.c
> @@ -69,6 +69,7 @@ NTSTATUS WINAPI HidRegisterMinidriver(HID_MINIDRIVER_REGISTRATION *registration)
>      registration->DriverObject->DriverUnload = UnloadDriver;
>  registration->DriverObject->MajorFunction[IRP_MJ_DEVICE_CONTROL] = HID_Device_ioctl;
> - registration->DriverObject->MajorFunction[IRP_MJ_READ] = HID_Device_read;
> registration->DriverObject->MajorFunction[IRP_MJ_CREATE] = HID_Device_create;
> registration->DriverObject->MajorFunction[IRP_MJ_CLOSE] = HID_Device_close;
>

[7/13](resend)hidclass.sys: Implement IRP_MJ_READ for HID Devices

[Vincent Povirk]

On Wed, Sep 2, 2015 at 3:34 PM, Andrew Eikum  wrote:
> Is it necessary to make a copy of the data in the ring buffer? Seems
> like returning a raw pointer would be more efficient. I flipped
> through the rest of the patch sequence and all uses of RingBuffer_Read
> seemed to be read-only.

We have to hold the critical section while reading the data to prevent
another thread from writing to it (RingBuffer_Write added in next
patch).

This doesn't mean we have to copy it, but the alternative is to
require readers to "lock" when reading and "unlock" when finished with
the data. And I don't know if it's a good idea to call
IoCompleteRequest while holding a mutex.

[7/13](resend)hidclass.sys: Implement IRP_MJ_READ for HID Devices

[Aric Stewart]

On 9/2/15 7:12 PM, Vincent Povirk wrote:
> On Wed, Sep 2, 2015 at 3:34 PM, Andrew Eikum  wrote:
> > Is it necessary to make a copy of the data in the ring buffer? Seems
> > like returning a raw pointer would be more efficient. I flipped
> > through the rest of the patch sequence and all uses of RingBuffer_Read
> > seemed to be read-only.
>
> We have to hold the critical section while reading the data to prevent
> another thread from writing to it (RingBuffer_Write added in next
> patch).
>
> This doesn't mean we have to copy it, but the alternative is to
> require readers to "lock" when reading and "unlock" when finished with
> the data. And I don't know if it's a good idea to call
> IoCompleteRequest while holding a mutex.

Yeah, I did not want to have to worry about holding a lock during processing, which mean basically i was copying the data out either on the RingBuffer_Read or locking, doing a read, then copying and unlocking anyway.

-aric