Re: [3/3] winebus.sys: Add implementation for device removal. (v9)

[Aric Stewart]

On 10/3/16 6:29 AM, Sebastian Lackner wrote:
> From: Aric Stewart <aric(a)codeweavers.com>
>
> Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> Signed-off-by: Sebastian Lackner <sebastian(a)fds-team.de>
> ---
>
> Second part of 126774. Besides splitting, I've also done the following changes:
>
> * Rename find_hid_device -> bus_find_hid_device, for consistency with other bus_*_hid_*() functions.
> * Do not append timestamp to the device names. To make the code more robust, I've modified
>   bus_remove_hid_device() to defer the release of the pnp_device pointer. If there are more
>   problems I would prefer to fix them properly, adding the timestamp seems more like a hack to me.
> * Some style improvements.
>
> @Aric: I'm not sure if you are aware of it, but there is currently still a bug somewhere which
> causes a crash during RemovalRelations when trying to remove a driver which was never added properly.
> I feel like the right place to fix that is somewhere in ntoskrnl. Are you aware of this issue?

Yes, that bug is fixed by patch 126650,  which has not been reviewed or committed yet.

-aric
> dlls/winebus.sys/bus.h      |   16 +++++++--
>  dlls/winebus.sys/bus_udev.c |   38 ++++++++++++++++++++--
>  dlls/winebus.sys/main.c     |   74 ++++++++++++++++++++++++++++++++++++++------
>  3 files changed, 111 insertions(+), 17 deletions(-)
>
> diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h
> index dcb50f2..099558b 100644
> --- a/dlls/winebus.sys/bus.h
> +++ b/dlls/winebus.sys/bus.h
> @@ -19,8 +19,18 @@
>  /* Busses */
>  NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry_path) DECLSPEC_HIDDEN;
>
> +/* Native device function table */
> +typedef struct
> +{
> +    int (*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev);
> +} platform_vtbl;
> +
> +void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
> +
>  /* HID Plug and Play Bus */
>  NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
> -DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, void *native, WORD vid,
> -                                     WORD pid, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad,
> -                                     const GUID *class) DECLSPEC_HIDDEN;
> +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 platform_vtbl *vtbl, DWORD platform_data_size) DECLSPEC_HIDDEN;
> +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;
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> index b1d6c3f..d798c53 100644
> --- a/dlls/winebus.sys/bus_udev.c
> +++ b/dlls/winebus.sys/bus_udev.c
> @@ -61,6 +61,16 @@ static const WCHAR hidraw_busidW[] = {'H','I','D','R','A','W',0};
>  #include "initguid.h"
>  DEFINE_GUID(GUID_DEVCLASS_HIDRAW, 0x3def44ad,0x242e,0x46e5,0x82,0x6d,0x70,0x72,0x13,0xf3,0xaa,0x81);
>
> +struct platform_private
> +{
> +    struct udev_device *udev_device;
> +};
> +
> +static inline struct platform_private *impl_from_DEVICE_OBJECT(DEVICE_OBJECT *device)
> +{
> +    return (struct platform_private *)get_platform_private(device);
> +}
> +
>  static DWORD get_sysattr_dword(struct udev_device *dev, const char *sysattr, int base)
>  {
>      const char *attr = udev_device_get_sysattr_value(dev, sysattr);
> @@ -88,6 +98,18 @@ static WCHAR *get_sysattr_string(struct udev_device *dev, const char *sysattr)
>      return dst;
>  }
>
> +static int compare_platform_device(DEVICE_OBJECT *device, void *platform_dev)
> +{
> +    struct udev_device *dev1 = impl_from_DEVICE_OBJECT(device)->udev_device;
> +    struct udev_device *dev2 = platform_dev;
> +    return strcmp(udev_device_get_syspath(dev1), udev_device_get_syspath(dev2));
> +}
> +
> +static const platform_vtbl hidraw_vtbl =
> +{
> +    compare_platform_device,
> +};
> +
>  static void try_add_device(struct udev_device *dev)
>  {
>      DWORD vid = 0, pid = 0, version = 0;
> @@ -123,12 +145,15 @@ static void try_add_device(struct udev_device *dev)
>      subsystem = udev_device_get_subsystem(dev);
>      if (strcmp(subsystem, "hidraw") == 0)
>      {
> -        device = bus_create_hid_device(udev_driver_obj, hidraw_busidW, dev, vid, pid,
> -                                       version, 0, serial, FALSE, &GUID_DEVCLASS_HIDRAW);
> +        device = bus_create_hid_device(udev_driver_obj, hidraw_busidW, vid, pid, version, 0, serial, FALSE,
> +                                       &GUID_DEVCLASS_HIDRAW, &hidraw_vtbl, sizeof(struct platform_private));
>      }
>
> if (device)
> -        udev_device_ref(dev);
> +    {
> +        impl_from_DEVICE_OBJECT(device)->udev_device = udev_device_ref(dev);
> +        IoInvalidateDeviceRelations(device, BusRelations);
> +    }
>      else
>          WARN("Ignoring device %s with subsystem %s\n", debugstr_a(devnode), subsystem);
>
> @@ -137,7 +162,12 @@ static void try_add_device(struct udev_device *dev)
>
> static void try_remove_device(struct udev_device *dev)
>  {
> -    /* FIXME */
> +    DEVICE_OBJECT *device = bus_find_hid_device(&hidraw_vtbl, dev);
> +    if (!device) return;
> +
> +    dev = impl_from_DEVICE_OBJECT(device)->udev_device;
> +    bus_remove_hid_device(device);
> +    udev_device_unref(dev);
>  }
>
> static void build_initial_deviceset(void)
> diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c
> index 1364eab..09a59d6 100644
> --- a/dlls/winebus.sys/main.c
> +++ b/dlls/winebus.sys/main.c
> @@ -47,13 +47,16 @@ struct pnp_device
>
> struct device_extension
>  {
> -    void *native;  /* Must be the first member of the structure */
> +    struct pnp_device *pnp_device;
>
> WORD vid, pid;
>      DWORD uid, version, index;
>      BOOL is_gamepad;
>      WCHAR *serial;
>      const WCHAR *busid;  /* Expected to be a static constant */
> +
> +    const platform_vtbl *vtbl;
> +    BYTE platform_private[1];
>  };
>
> static CRITICAL_SECTION device_list_cs;
> @@ -80,6 +83,12 @@ static inline WCHAR *strdupW(const WCHAR *src)
>      return dst;
>  }
>
> +void *get_platform_private(DEVICE_OBJECT *device)
> +{
> +    struct device_extension *ext = (struct device_extension *)device->DeviceExtension;
> +    return ext->platform_private;
> +}
> +
>  static DWORD get_vidpid_index(WORD vid, WORD pid)
>  {
>      struct pnp_device *ptr;
> @@ -157,9 +166,9 @@ static WCHAR *get_compatible_ids(DEVICE_OBJECT *device)
>      return dst;
>  }
>
> -DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, void *native, WORD vid,
> -                                     WORD pid, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad,
> -                                     const GUID *class)
> +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 platform_vtbl *vtbl, DWORD platform_data_size)
>  {
>      static const WCHAR device_name_fmtW[] = {'\\','D','e','v','i','c','e','\\','%','s','#','%','p',0};
>      struct device_extension *ext;
> @@ -169,16 +178,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,
> -          vid, pid, version, uid, debugstr_w(serialW), is_gamepad, debugstr_guid(class));
> +    TRACE("(%p, %s, %04x, %04x, %u, %u, %s, %u, %s, %p, %u)\n", driver, debugstr_w(busidW), vid, pid,
> +          version, uid, debugstr_w(serialW), is_gamepad, debugstr_guid(class), vtbl, platform_data_size);
>
> 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);
>      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 +201,7 @@ 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->pnp_device = pnp_dev;
>      ext->vid        = vid;
>      ext->pid        = pid;
>      ext->uid        = uid;
> @@ -199,6 +210,7 @@ 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->vtbl       = vtbl;
>
> /* add to list of pnp devices */
>      pnp_dev->device = device;
> @@ -226,10 +238,52 @@ 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;
>  }
>
> +DEVICE_OBJECT *bus_find_hid_device(const platform_vtbl *vtbl, void *platform_dev)
> +{
> +    struct pnp_device *dev;
> +    DEVICE_OBJECT *ret = NULL;
> +
> +    TRACE("(%p, %p)\n", vtbl, platform_dev);
> +
> +    EnterCriticalSection(&device_list_cs);
> +    LIST_FOR_EACH_ENTRY(dev, &pnp_devset, struct pnp_device, entry)
> +    {
> +        struct device_extension *ext = (struct device_extension *)dev->device->DeviceExtension;
> +        if (ext->vtbl != vtbl) continue;
> +        if (ext->vtbl->compare_platform_device(dev->device, platform_dev) == 0)
> +        {
> +            ret = dev->device;
> +            break;
> +        }
> +    }
> +    LeaveCriticalSection(&device_list_cs);
> +
> +    TRACE("returning %p\n", ret);
> +    return ret;
> +}
> +
> +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;
> +
> +    TRACE("(%p)\n", device);
> +
> +    EnterCriticalSection(&device_list_cs);
> +    list_remove(&pnp_device->entry);
> +    LeaveCriticalSection(&device_list_cs);
> +
> +    IoInvalidateDeviceRelations(device, RemovalRelations);
> +    HeapFree(GetProcessHeap(), 0, ext->serial);
> +    IoDeleteDevice(device);
> +
> +    /* pnp_device must be released after the device is gone */
> +    HeapFree(GetProcessHeap(), 0, pnp_device);
> +}
> +
>  static NTSTATUS handle_IRP_MN_QUERY_ID(DEVICE_OBJECT *device, IRP *irp)
>  {
>      NTSTATUS status = irp->IoStatus.u.Status;

[3/3] winebus.sys: Add implementation for device removal. (v9)

[Sebastian Lackner]

On 03.10.2016 17:27, Aric Stewart wrote:
> On 10/3/16 6:29 AM, Sebastian Lackner wrote:
> > > From: Aric Stewart <aric(a)codeweavers.com>
> > >
> > > Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> > > Signed-off-by: Sebastian Lackner <sebastian(a)fds-team.de>
> > > ---
> > >
> > > Second part of 126774. Besides splitting, I've also done the following changes:
> > >
> > > * Rename find_hid_device -> bus_find_hid_device, for consistency with other bus_*_hid_*() functions.
> > > * Do not append timestamp to the device names. To make the code more robust, I've modified
> > >   bus_remove_hid_device() to defer the release of the pnp_device pointer. If there are more
> > >   problems I would prefer to fix them properly, adding the timestamp seems more like a hack to me.
> > > * Some style improvements.
> > >
> > > @Aric: I'm not sure if you are aware of it, but there is currently still a bug somewhere which
> > > causes a crash during RemovalRelations when trying to remove a driver which was never added properly.
> > > I feel like the right place to fix that is somewhere in ntoskrnl. Are you aware of this issue?
> >
> > Yes, that bug is fixed by patch 126650,  which has not been reviewed or committed yet.

The patch does not help here. It also sounds like you are trying to fix a different
problem because the issue I have can be reproduced without winehid/hidclass.
Nevertheless, one way to improve 126650 would be to remove the unnecessary "break;"
after the return.

Regards,
Sebastian

[3/3] winebus.sys: Add implementation for device removal. (v9)

[Aric Stewart]

On 10/3/16 2:05 PM, Sebastian Lackner wrote:
> On 03.10.2016 17:27, Aric Stewart wrote:
> > On 10/3/16 6:29 AM, Sebastian Lackner wrote:
> > > > From: Aric Stewart <aric(a)codeweavers.com>
> > > >
> > > > Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> > > > Signed-off-by: Sebastian Lackner <sebastian(a)fds-team.de>
> > > > ---
> > > >
> > > > Second part of 126774. Besides splitting, I've also done the following changes:
> > > >
> > > > * Rename find_hid_device -> bus_find_hid_device, for consistency with other bus_*_hid_*() functions.
> > > > * Do not append timestamp to the device names. To make the code more robust, I've modified
> > > >   bus_remove_hid_device() to defer the release of the pnp_device pointer. If there are more
> > > >   problems I would prefer to fix them properly, adding the timestamp seems more like a hack to me.
> > > > * Some style improvements.
> > > >
> > > > @Aric: I'm not sure if you are aware of it, but there is currently still a bug somewhere which
> > > > causes a crash during RemovalRelations when trying to remove a driver which was never added properly.
> > > > I feel like the right place to fix that is somewhere in ntoskrnl. Are you aware of this issue?
> > >
> > > Yes, that bug is fixed by patch 126650,  which has not been reviewed or committed yet.
>
> The patch does not help here. It also sounds like you are trying to fix a different
> problem because the issue I have can be reproduced without winehid/hidclass.
> Nevertheless, one way to improve 126650 would be to remove the unnecessary "break;"
> after the return.

Ahh yes, Got ahead of myself. I will try to get a chance to circle up on that and fix 126650, As for this crash I am betting it has to do with not handling the power ioctls in the base device. I will try to dig into that also when I get a chance. Traveling for the next 3 weeks so my time may be intermittent but I will try to carve out a chance to get to this. 

-aric
> Regards,
> Sebastian