On 17.10.2016 15:29, Aric Stewart wrote:
v2: Suggestions from Sebastian Lackner Signed-off-by: Aric Stewart aric@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");elseprocess_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};
-
Sebastian Lackner