[prev in list] [next in list] [prev in thread] [next in thread]
List: wine-devel
Subject: Re: [PATCH v6] winebus.sys: Watch for hid raw device addition and removal
From: Aric Stewart <aric () codeweavers ! com>
Date: 2016-09-30 12:02:35
Message-ID: cbd7864a-1b3f-404c-8564-260cc1584861 () codeweavers ! com
[Download RAW message or body]
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@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;
> >
> >
> >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic