[PATCH] winebus.sys: Implement IOCTL_HID_GET_DEVICE_ATTRIBUTES for hid devices

[Sebastian Lackner]

On 06.10.2016 04:51, Aric Stewart wrote:
> Signed-off-by: Aric Stewart <aric at codeweavers.com>
> ---
>  dlls/winebus.sys/bus.h      |  1 +
>  dlls/winebus.sys/bus_udev.c |  1 +
>  dlls/winebus.sys/main.c     | 41 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 43 insertions(+)
> 
> 
> 
> 0001-winebus.sys-Implement-IOCTL_HID_GET_DEVICE_ATTRIBUTES-.txt
> 
> 
> diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h
> index 099558b..342a66b 100644
> --- a/dlls/winebus.sys/bus.h
> +++ b/dlls/winebus.sys/bus.h
> @@ -34,3 +34,4 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
>                                       const GUID *class, const platform_vtbl *vtbl, DWORD platform_data_size) DECLSPEC_HIDDEN;
>  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;
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> index 93e56f3..a01d88e 100644
> --- a/dlls/winebus.sys/bus_udev.c
> +++ b/dlls/winebus.sys/bus_udev.c
> @@ -299,6 +299,7 @@ NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry
>  
>      udev_driver_obj = driver;
>      driver->MajorFunction[IRP_MJ_PNP] = common_pnp_dispatch;
> +    driver->MajorFunction[IRP_MJ_INTERNAL_DEVICE_CONTROL] = hid_internal_dispatch;

I guess you intentionally did not name it "common_*" because non-HID devices might
need a different handler, is that correct?

>  
>      if (!(events[0] = CreateEventW(NULL, TRUE, FALSE, NULL)))
>          goto error;
> diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c
> index 09a59d6..ef19322 100644
> --- a/dlls/winebus.sys/main.c
> +++ b/dlls/winebus.sys/main.c
> @@ -30,7 +30,9 @@
>  #include "winternl.h"
>  #include "winreg.h"
>  #include "setupapi.h"
> +#include "winioctl.h"
>  #include "ddk/wdm.h"
> +#include "ddk/hidport.h"
>  #include "wine/debug.h"
>  #include "wine/unicode.h"
>  #include "wine/list.h"
> @@ -346,6 +348,45 @@ NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp)
>      return status;
>  }
>  
> +NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp)
> +{
> +    NTSTATUS rc = irp->IoStatus.u.Status;

I would suggest to use rename the variable to "status".

> +    IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp);
> +    struct device_extension *extension = (struct device_extension *)device->DeviceExtension;
> +
> +    TRACE("(%p, %p)\n", device, irp);
> +
> +    switch(irpsp->Parameters.DeviceIoControl.IoControlCode)

There is a space missing after "switch".

> +    {
> +        case IOCTL_HID_GET_DEVICE_ATTRIBUTES:
> +        {
> +            PHID_DEVICE_ATTRIBUTES attr = (PHID_DEVICE_ATTRIBUTES)irp->UserBuffer;

It would be better to use HID_DEVICE_ATTRIBUTES * instead of the version with the "P" prefix.
Also, please add a check for Parameters.DeviceIoControl.OutputBufferLength to make sure
the buffer is big enough.


> +            TRACE("IOCTL_HID_GET_DEVICE_ATTRIBUTES\n");
> +
> +            RtlZeroMemory(attr, sizeof(HID_DEVICE_ATTRIBUTES));

Is there any specific reason to use RtlZeroMemory instead of memset? Something like
memset(attr, 0, sizeof(*attr)) would be easier. ;)

> +            attr->Size = sizeof(HID_DEVICE_ATTRIBUTES);
> +            attr->VendorID = extension->vid;
> +            attr->ProductID = extension->pid;
> +            attr->VersionNumber = extension->version;
> +            irp->IoStatus.u.Status = rc = STATUS_SUCCESS;
> +            irp->IoStatus.Information = sizeof(HID_DEVICE_ATTRIBUTES);
> +            break;
> +        }
> +        default:
> +        {
> +            ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode;
> +            FIXME("Unsupported ioctl %x (device=%x access=%x func=%x method=%x)\n",
> +                  code, code >> 16, (code >> 14) & 3, (code >> 2) & 0xfff, code & 3);
> +            break;
> +        }
> +    }
> +
> +    if (rc != STATUS_PENDING)
> +        IoCompleteRequest( irp, IO_NO_INCREMENT );

Do you need to handle asynchronous requests in this function? If not you can remove the
if check. For consistency reasons, you should probably remove the space after "(" and
before ")" in the IoCompleteRequest call.

> +
> +    return rc;
> +}
> +
>  NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path )
>  {
>      static const WCHAR udevW[] = {'\\','D','r','i','v','e','r','\\','U','D','E','V',0};
> 
> 
>