Thanks for the review, just 1 comment On 9/29/16 2:44 PM, Sebastian Lackner wrote:
On 29.09.2016 20:48, Aric Stewart wrote:
v2: Correct poll timeout Style changes v3: vtable style instead of a callback v4: Suggestions from Sebastian Lackner v5: Fixing handling of other native in the compare function v6: Tweaks suggested by Sebastian Lackner
Includes an implementation of a common bus_remove_hid_device
Signed-off-by: Aric Stewart <aric(a)codeweavers.com> --- dlls/winebus.sys/bus.h | 13 +++++- dlls/winebus.sys/bus_udev.c | 106 +++++++++++++++++++++++++++++++++++++++++++- dlls/winebus.sys/main.c | 64 +++++++++++++++++++++----- 3 files changed, 169 insertions(+), 14 deletions(-)
v6-0001-winebus.sys-Watch-for-hid-raw-device-addition-and-r.txt
diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h . . . -DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, void *native, WORD vid, +DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, WORD vid, WORD pid, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad, - const GUID *class) + const GUID *class, const platform_vtbl *vtbl, DWORD platform_data_size) { - static const WCHAR device_name_fmtW[] = {'\\','D','e','v','i','c','e','\\','%','s','#','%','p',0}; + static const WCHAR device_name_fmtW[] = {'\\','D','e','v','i','c','e','\\','%','s','#','%','p','#','%','x',0}; struct device_extension *ext; struct pnp_device *pnp_dev; DEVICE_OBJECT *device; @@ -169,16 +171,18 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, WCHAR dev_name[256]; HDEVINFO devinfo; NTSTATUS status; + DWORD length;
- TRACE("(%p, %s, %p, %04x, %04x, %u, %u, %s, %u, %s)\n", driver, debugstr_w(busidW), native, + TRACE("(%p, %s, %04x, %04x, %u, %u, %s, %u, %s)\n", driver, debugstr_w(busidW), vid, pid, version, uid, debugstr_w(serialW), is_gamepad, debugstr_guid(class));
You forgot to add the vtbl and platform_data_size parameters to the TRACE.
if (!(pnp_dev = HeapAlloc(GetProcessHeap(), 0, sizeof(*pnp_dev)))) return NULL;
- sprintfW(dev_name, device_name_fmtW, busidW, native); + sprintfW(dev_name, device_name_fmtW, busidW, pnp_dev, GetTickCount());
Do you think the memory pointer (pnp_dev) alone is not sufficient? If not, it was already a problem before because "native" is also just a memory address for UDEV.
This was mostly because I know that when a memory address is freed wine can give out the same address quite easily. I was unsure how robust our device deletion creation process was for identically named devices so I wanted to add just that one more element of uniqueness so as to avoid this possible problem. I am sure just using native would exhibit the same potential issues. -aric
RtlInitUnicodeString(&nameW, dev_name); - status = IoCreateDevice(driver, sizeof(*ext), &nameW, 0, 0, FALSE, &device); + length = FIELD_OFFSET(struct device_extension, platform_private[platform_data_size]); + status = IoCreateDevice(driver, length, &nameW, 0, 0, FALSE, &device); if (status) { FIXME("failed to create device error %x\n", status); @@ -190,7 +194,6 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
/* fill out device_extension struct */ ext = (struct device_extension *)device->DeviceExtension; - ext->native = native; ext->vid = vid; ext->pid = pid; ext->uid = uid; @@ -199,6 +202,8 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, ext->is_gamepad = is_gamepad; ext->serial = strdupW(serialW); ext->busid = busidW; + ext->pnp_device = pnp_dev; + ext->vtbl = vtbl;
/* add to list of pnp devices */ pnp_dev->device = device; @@ -226,7 +231,6 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, else ERR("failed to get ClassDevs: %x\n", GetLastError());
- IoInvalidateDeviceRelations(device, BusRelations); return device; }
@@ -265,6 +269,46 @@ static NTSTATUS handle_IRP_MN_QUERY_ID(DEVICE_OBJECT *device, IRP *irp) return status; }
+void *get_platform_private(DEVICE_OBJECT *device) +{ + struct device_extension *ext = (struct device_extension*)device->DeviceExtension; + return ext->platform_private; +} + +DEVICE_OBJECT *find_hid_device(const platform_vtbl* vtbl, void *device) +{ + struct pnp_device *dev, *ptr; + DEVICE_OBJECT *ret = NULL; + + EnterCriticalSection(&device_list_cs); + LIST_FOR_EACH_ENTRY_SAFE(dev, ptr, &pnp_devset, struct pnp_device, entry)
You do no longer need the _SAFE enumeration here because the deletion is done separately.
+ { + struct device_extension *ext = (struct device_extension*)dev->device->DeviceExtension; + if ((vtbl == ext->vtbl) && + (ext->vtbl->compare_platform_device(dev->device, device) == 0)) + { + TRACE("Found as device %p\n", dev->device); + ret = dev->device; + break; + } + } + LeaveCriticalSection(&device_list_cs); + return ret; +} + +void bus_remove_hid_device(DEVICE_OBJECT *device) +{ + struct device_extension *ext = (struct device_extension*)device->DeviceExtension; + TRACE("Remove hid device %p\n", device); + EnterCriticalSection(&device_list_cs); + list_remove(&ext->pnp_device->entry); + LeaveCriticalSection(&device_list_cs); + IoInvalidateDeviceRelations(device, RemovalRelations); + HeapFree(GetProcessHeap(), 0, ext->serial); + HeapFree(GetProcessHeap(), 0, ext->pnp_device); + IoDeleteDevice(device); +} + NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) { NTSTATUS status = irp->IoStatus.u.Status;