[PATCH v3] winebus.sys: Handle device reports for hidraw devices
Sebastian Lackner
sebastian at fds-team.de
Tue Oct 18 11:58:24 CDT 2016
On 17.10.2016 17:14, Aric Stewart wrote:
> v2: Suggestions from Sebastian Lackner
> v3: Further suggestions from Sebastian Lackner
> Signed-off-by: Aric Stewart <aric at codeweavers.com>
> ---
> dlls/winebus.sys/bus.h | 3 +
> dlls/winebus.sys/bus_udev.c | 73 ++++++++++++++++++++++++
> dlls/winebus.sys/main.c | 133 +++++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 208 insertions(+), 1 deletion(-)
>
>
Now that all the handling for last_report is in main.c, I would suggest to split
it out into a separate patch for easier reviewing. The first patch could contain
IOCTL_HID_READ_REPORT and corresponding platform code only. Caching of the last
record can then be implemented in the second patch.
>
> v3-0003-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..339449d 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);
> + void (*end_report_processing)(DEVICE_OBJECT *device);
> } 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, BYTE *report, DWORD length) DECLSPEC_HIDDEN;
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> index 243aabb..517a7d3 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);
> + 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, private->report_buffer, size);
> + }
> + return 0;
> +}
> +
> +static NTSTATUS begin_report_processing(DEVICE_OBJECT *device)
> +{
> + struct platform_private *private = impl_from_DEVICE_OBJECT(device);
> + private->buffer_length = 1024;
> + private->report_buffer = HeapAlloc(GetProcessHeap(), 0, private->buffer_length);
My understanding is that report_buffer is only used from the device_report_thread. Do
we really have to store it in the platform private data then? For such a small amount
of data, you could probably even allocate it directly on the stack in device_report_thread.
> +
> + pipe(private->control_pipe);
Please also check for success here.
> + 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)
> +{
> + 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]);
report_thread should also be closed when no longer needed.
> + }
> + private->buffer_length = 0;
> + HeapFree(GetProcessHeap(), 0, private->report_buffer);
> + private->report_buffer = NULL;
> +}
> +
> 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..0bd2c76 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,10 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
> ext->busid = busidW;
> ext->vtbl = vtbl;
>
It might be better to explicitly initialize all fields here. I don't know if you
can rely on the fact that Wines current implementation nulls the driver extension.
> + InitializeListHead(&ext->irp_queue);
> + InitializeCriticalSection(&ext->report_cs);
> + ext->report_cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": report_cs");
> +
> /* add to list of pnp devices */
> pnp_dev->device = device;
> list_add_tail(&pnp_devset, &pnp_dev->entry);
> @@ -271,6 +284,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,7 +294,28 @@ 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);
At this point the device_fd was already closed (too early). Do we really need the
callback, or could we also do the platform specific cleanup in try_remove_device()?
> +
> + /* Cancel pending IRPs */
> + EnterCriticalSection(&ext->report_cs);
> + entry = RemoveHeadList(&ext->irp_queue);
> + while(entry != &ext->irp_queue)
You could write this loop a bit easier like:
while ((entry = RemoveHeadList(&ext->irp_queue)) != &ext->irp_queue)
{
...
}
The same also applies to the code below.
> + {
> + 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);
> +
> + ext->report_cs.DebugInfo->Spare[0] = 0;
> + DeleteCriticalSection(&ext->report_cs);
> +
> HeapFree(GetProcessHeap(), 0, ext->serial);
> + HeapFree(GetProcessHeap(), 0, ext->last_report);
> IoDeleteDevice(device);
>
> /* pnp_device must be released after the device is gone */
> @@ -432,6 +468,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);
> + 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;
> + }
> +
> + if (ext->last_report_size)
> + 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) == STATUS_SUCCESS)
Did you want to assign the result to "status" here?
Also, it is a bit unfortunate that we have tracking of report processing in both the common
code and also in the platform specific code. If you modify begin_report_processing to return
STATUS_SUCCESS if it already has been started, you could simplify this code to something like:
--- snip ---
status = ext->vtbl->begin_report_processing(device);
if (status != STATUS_SUCCESS)
{
...
}
--- snip ---
It also ensures that the status in both components never gets out of sync. Not sure if it is
a problem, but due to the fact that the device has been opened a long time before starting to
read from it, couldn't there be a large amount of reports in the kernel buffers? Do you think
it makes sense to skip such outdated reports?
> + 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 +529,54 @@ 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, BYTE *report, DWORD length)
> +{
> + struct device_extension *ext = (struct device_extension*)device->DeviceExtension;
> + IRP *irp;
> + LIST_ENTRY *entry;
> +
> + EnterCriticalSection(&ext->report_cs);
> + if (length > ext->buffer_size)
> + {
> + HeapFree(GetProcessHeap(), 0, ext->last_report);
> + ext->last_report = HeapAlloc(GetProcessHeap(), 0, length);
It would be better to handle failures here.
> + ext->buffer_size = length;
> + }
> +
> + 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};
>
>
>
More information about the wine-devel
mailing list