Re: [PATCH v2] winebus.sys: Handle device reports for hidraw devices
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};
participants (1)
-
Sebastian Lackner