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@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");elseprocess_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};