[PATCH v6] winebus.sys: Watch for hid raw device addition and removal
Aric Stewart
aric at codeweavers.com
Fri Sep 30 07:02:35 CDT 2016
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 at 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;
>>
>>
>>
>
More information about the wine-devel
mailing list