On 17.10.2016 15:29, Aric Stewart wrote:
> v2: Suggestions from Sebastian Lackner
> Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> ---
> dlls/winebus.sys/bus.h | 3 ++
> dlls/winebus.sys/bus_udev.c | 73 ++++++++++++++++++++++++++
> dlls/winebus.sys/main.c | 123 +++++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 198 insertions(+), 1 deletion(-)
>
>
>
> v2-0001-winebus.sys-Handle-device-reports-for-hidraw-device.txt
>
>
> diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h
> index 7ce54bf..bcb1163 100644
> --- a/dlls/winebus.sys/bus.h
> +++ b/dlls/winebus.sys/bus.h
> @@ -25,6 +25,8 @@ typedef struct
> int (*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev);
> NTSTATUS (*get_reportdescriptor)(DEVICE_OBJECT *device, BYTE *buffer, DWORD length, DWORD *out_length);
> NTSTATUS (*get_string)(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DWORD length);
> + NTSTATUS (*begin_report_processing)(DEVICE_OBJECT *device, BYTE **report_buffer, DWORD *buffer_length);
> + void (*end_report_processing)(DEVICE_OBJECT *device, BYTE *report_buffer);
> } platform_vtbl;
>
> void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
> @@ -37,3 +39,4 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
> 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;
> +void process_hid_report(DEVICE_OBJECT *device, const WCHAR *busidW, BYTE *report, DWORD length) DECLSPEC_HIDDEN;
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> index 243aabb..37c8412 100644
> --- a/dlls/winebus.sys/bus_udev.c
> +++ b/dlls/winebus.sys/bus_udev.c
> @@ -61,6 +61,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
>
> #ifdef HAVE_UDEV
>
> +WINE_DECLARE_DEBUG_CHANNEL(hid_report);
> +
> static struct udev *udev_context = NULL;
> static DRIVER_OBJECT *udev_driver_obj = NULL;
>
> @@ -73,6 +75,11 @@ struct platform_private
> {
> struct udev_device *udev_device;
> int device_fd;
> +
> + BYTE *report_buffer;
> + int buffer_length;
> + HANDLE report_thread;
> + int control_pipe[2];
> };
>
> static inline struct platform_private *impl_from_DEVICE_OBJECT(DEVICE_OBJECT *device)
> @@ -222,11 +229,77 @@ static NTSTATUS hidraw_get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buf
> return STATUS_SUCCESS;
> }
>
> +static DWORD CALLBACK device_report_thread(void *args)
> +{
> + DEVICE_OBJECT *device = (DEVICE_OBJECT*)args;
> + struct platform_private *private = impl_from_DEVICE_OBJECT(device);
> + struct pollfd plfds[2];
> +
> + plfds[0].fd = private->device_fd;
> + plfds[0].events = POLLIN;
> + plfds[0].revents = 0;
> + plfds[1].fd = private->control_pipe[0];
> + plfds[1].events = POLLIN;
> + plfds[1].revents = 0;
> +
> + while (1)
> + {
> + int size;
> + if (poll(plfds, 2, -1) <= 0) continue;
> + if (plfds[1].revents || !private->report_buffer || private->buffer_length == 0)
> + break;
> + size = read(plfds[0].fd, private->report_buffer, private->buffer_length);
This could still cause problems when another thread copies the last_report while it is updated.
I don't know how you are planning to implement it for other platforms, but wouldn't it be better
to use a separate buffer in this thread, and let process_hid_report() do the copying?
> + if (size == -1)
> + TRACE_(hid_report)("Read failed. Likely an unplugged device\n");
> + else if (size == 0)
> + TRACE_(hid_report)("Failed to read report\n");
> + else
> + process_hid_report(device, hidraw_busidW, private->report_buffer, size);
> + }
> + return 0;
> +}
> +
> +static NTSTATUS begin_report_processing(DEVICE_OBJECT *device, BYTE **buffer, DWORD *length)
> +{
> + struct platform_private *private = impl_from_DEVICE_OBJECT(device);
> + *length = private->buffer_length = 1024;
> + *buffer = private->report_buffer = HeapAlloc(GetProcessHeap(), 0, *length);
> +
> + pipe(private->control_pipe);
> + private->report_thread = CreateThread(NULL, 0, device_report_thread, device, 0, NULL);
> + if (!private->report_thread)
> + {
> + ERR("Unable to create device report thread\n");
> + close(private->control_pipe[0]);
> + close(private->control_pipe[1]);
> + return STATUS_UNSUCCESSFUL;
> + }
> + else
> + return STATUS_SUCCESS;
> +}
> +
> +static void end_report_processing(DEVICE_OBJECT *device, BYTE *buffer)
> +{
> + struct platform_private *private = impl_from_DEVICE_OBJECT(device);
> + if (private->report_thread)
> + {
> + write(private->control_pipe[1], "q", 1);
> + WaitForSingleObject(private->report_thread, INFINITE);
> + close(private->control_pipe[0]);
> + close(private->control_pipe[1]);
> + }
> + private->buffer_length = 0;
> + private->report_buffer = NULL;
> + HeapFree(GetProcessHeap(), 0, buffer);
> +}
> +
> static const platform_vtbl hidraw_vtbl =
> {
> compare_platform_device,
> hidraw_get_reportdescriptor,
> hidraw_get_string,
> + begin_report_processing,
> + end_report_processing,
> };
>
> static void try_add_device(struct udev_device *dev)
> diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c
> index 86adbf5..49dcc97 100644
> --- a/dlls/winebus.sys/main.c
> +++ b/dlls/winebus.sys/main.c
> @@ -40,6 +40,7 @@
> #include "bus.h"
>
> WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
> +WINE_DECLARE_DEBUG_CHANNEL(hid_report);
>
> struct pnp_device
> {
> @@ -58,6 +59,14 @@ struct device_extension
> const WCHAR *busid; /* Expected to be a static constant */
>
> const platform_vtbl *vtbl;
> +
> + BOOL processing_started;
> + BYTE *last_report;
> + DWORD last_report_size;
> + DWORD buffer_size;
> + LIST_ENTRY irp_queue;
> + CRITICAL_SECTION report_cs;
> +
> BYTE platform_private[1];
> };
>
> @@ -214,6 +223,9 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
> ext->busid = busidW;
> ext->vtbl = vtbl;
>
> + InitializeListHead(&ext->irp_queue);
> + InitializeCriticalSection(&ext->report_cs);
You might want to give this CS a name, like done in other parts of wine. For example:
--- snip ---
InitializeCriticalSection(&src->lock);
src->lock.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": XA2SourceImpl.lock");
--- snip ---
> +
> /* add to list of pnp devices */
> pnp_dev->device = device;
> list_add_tail(&pnp_devset, &pnp_dev->entry);
> @@ -271,6 +283,8 @@ void bus_remove_hid_device(DEVICE_OBJECT *device)
> {
> struct device_extension *ext = (struct device_extension *)device->DeviceExtension;
> struct pnp_device *pnp_device = ext->pnp_device;
> + LIST_ENTRY *entry;
> + IRP *irp;
>
> TRACE("(%p)\n", device);
>
> @@ -279,6 +293,23 @@ void bus_remove_hid_device(DEVICE_OBJECT *device)
> LeaveCriticalSection(&device_list_cs);
>
> IoInvalidateDeviceRelations(device, RemovalRelations);
> +
> + if (ext->processing_started)
> + ext->vtbl->end_report_processing(device, ext->last_report);
> +
> + /* Cancel pending IRPs */
> + EnterCriticalSection(&ext->report_cs);
> + entry = RemoveHeadList(&ext->irp_queue);
> + while(entry != &ext->irp_queue)
> + {
> + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry);
> + irp->IoStatus.u.Status = STATUS_CANCELLED;
> + irp->IoStatus.Information = 0;
> + IoCompleteRequest(irp, IO_NO_INCREMENT);
> + entry = RemoveHeadList(&ext->irp_queue);
> + }
> + LeaveCriticalSection(&ext->report_cs);
> +
You will have to delete the CS here. Also make sure to reset Spare[0] if it was previously
set to a debug string. For example:
--- snip ---
src->lock.DebugInfo->Spare[0] = 0;
DeleteCriticalSection(&src->lock);
--- snip ---
> HeapFree(GetProcessHeap(), 0, ext->serial);
> IoDeleteDevice(device);
>
> @@ -432,6 +463,58 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp)
> irp->IoStatus.Information = (strlenW((WCHAR *)irp->UserBuffer) + 1) * sizeof(WCHAR);
> break;
> }
> + case IOCTL_HID_GET_INPUT_REPORT:
> + {
> + HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer);
> + TRACE_(hid_report)("IOCTL_HID_GET_INPUT_REPORT\n");
> + EnterCriticalSection(&ext->report_cs);
> + if (!ext->processing_started)
> + {
> + irp->IoStatus.u.Status = status = ext->vtbl->begin_report_processing(device, &ext->last_report, &ext->buffer_size);
> + if (status == STATUS_SUCCESS)
> + ext->processing_started = TRUE;
> + else
> + {
> + LeaveCriticalSection(&ext->report_cs);
> + break;
> + }
> + }
> +
> + if (packet->reportBufferLen < ext->last_report_size)
> + {
> + irp->IoStatus.u.Status = status = STATUS_BUFFER_TOO_SMALL;
> + LeaveCriticalSection(&ext->report_cs);
> + break;
> + }
> +
> + ZeroMemory(packet->reportBuffer, packet->reportBufferLen);
Do we need this line? Usually callers do not expect valid data after the length
provided by irp->IoStatus.Information. Is it different in this case?
> + memcpy(packet->reportBuffer, ext->last_report, ext->last_report_size);
> + packet->reportBufferLen = ext->last_report_size;
> + irp->IoStatus.Information = ext->last_report_size;
> + irp->IoStatus.u.Status = status = STATUS_SUCCESS;
> + LeaveCriticalSection(&ext->report_cs);
> + break;
> + }
> + case IOCTL_HID_READ_REPORT:
> + {
> + TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n");
> + EnterCriticalSection(&ext->report_cs);
> + if (!ext->processing_started)
> + {
> + if (ext->vtbl->begin_report_processing(device, &ext->last_report, &ext->buffer_size) == STATUS_SUCCESS)
> + ext->processing_started = TRUE;
> + else
> + {
> + irp->IoStatus.u.Status = status;
> + LeaveCriticalSection(&ext->report_cs);
> + break;
> + }
> + }
> + InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry);
> + status = STATUS_PENDING;
> + LeaveCriticalSection(&ext->report_cs);
> + break;
> + }
> default:
> {
> ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode;
> @@ -441,11 +524,49 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp)
> }
> }
>
> - IoCompleteRequest(irp, IO_NO_INCREMENT);
> + if (status != STATUS_PENDING)
> + IoCompleteRequest(irp, IO_NO_INCREMENT);
>
> return status;
> }
>
> +void process_hid_report(DEVICE_OBJECT *device, const WCHAR *busidW, BYTE *report, DWORD length)
> +{
> + struct device_extension *ext = (struct device_extension*)device->DeviceExtension;
> + IRP *irp;
> + LIST_ENTRY *entry;
> +
> + EnterCriticalSection(&ext->report_cs);
> + if (ext->last_report && report != ext->last_report && ext->buffer_size <= length)
Shouldn't it be "length <= ext->buffer_size" ?
> + {
> + memcpy(ext->last_report, report, length);
> + ext->last_report_size = length;
> + }
> + entry = RemoveHeadList(&ext->irp_queue);
> + while(entry != &ext->irp_queue)
> + {
> + IO_STACK_LOCATION *irpsp;
> + TRACE_(hid_report)("Processing Request\n");
> + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry);
> + irpsp = IoGetCurrentIrpStackLocation(irp);
> +
> + if (irpsp->Parameters.DeviceIoControl.OutputBufferLength < length)
> + {
> + irp->IoStatus.Information = 0;
> + irp->IoStatus.u.Status = STATUS_BUFFER_TOO_SMALL;
> + }
> + else
> + {
> + memcpy(irp->UserBuffer, report, length);
> + irp->IoStatus.Information = length;
> + irp->IoStatus.u.Status = STATUS_SUCCESS;
> + }
> + IoCompleteRequest(irp, IO_NO_INCREMENT);
> + entry = RemoveHeadList(&ext->irp_queue);
> + }
> + LeaveCriticalSection(&ext->report_cs);
> +}
> +
> NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path )
> {
> static const WCHAR udevW[] = {'\\','D','r','i','v','e','r','\\','U','D','E','V',0};
>
>
>
