Re: [PATCH v3 1/5] winebus.sys: Handle linux input event device via udev

[Sebastian Lackner]

I haven't done a full review yet, but here are some first comments.

On 01.03.2017 17:16, Aric Stewart wrote:
> - subsystem = udev_device_get_subsystem(dev);
> usbdev = udev_device_get_parent_with_subsystem_devtype(dev, "usb", "usb_device");
> if (usbdev)
> {
> - #ifdef HAS_PROPER_INPUT_HEADER
> -
>    const platform_vtbl *other_vtbl = NULL;
> -
>    DEVICE_OBJECT *dup = NULL;
> -
>    if (strcmp(subsystem, "hidraw") == 0)
> -
>        other_vtbl = &lnxev_vtbl;
> -
>    else if (strcmp(subsystem, "input") == 0)
> -
>        other_vtbl = &hidraw_vtbl;
> -
> -
>    if (other_vtbl)
> -
>        dup = bus_enumerate_hid_devices(other_vtbl, check_same_device, dev);

Shouldn't we have some preference of either hidraw or linux event devices?
Otherwise this could lead to different results in each run.
> }
>
> +DEVICE_OBJECT* bus_enumerate_hid_devices(const platform_vtbl *vtbl, enum_func function, void* context)
> +{
> - struct pnp_device *dev;
> - DEVICE_OBJECT *ret = NULL;
> -
> - TRACE("(%p)\n", vtbl);
> -
> - 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 (function(dev->device, context) == 0)
> -
>        ret = dev->device;
> -
>        break;

This is wrong and will always abort after the first function call.
> - }
> - LeaveCriticalSection(&device_list_cs);
> - return ret;
>
> +}
> -

[PATCH v3 1/5] winebus.sys: Handle linux input event device via udev

[Aric Stewart]

On 3/1/17 10:53 AM, Sebastian Lackner wrote:
> I haven't done a full review yet, but here are some first comments.
>
> On 01.03.2017 17:16, Aric Stewart wrote:
> > - subsystem = udev_device_get_subsystem(dev);
> > usbdev = udev_device_get_parent_with_subsystem_devtype(dev, "usb", "usb_device");
> > if (usbdev)
> > {
> > - #ifdef HAS_PROPER_INPUT_HEADER
> > -
> >    const platform_vtbl *other_vtbl = NULL;
> > -
> >    DEVICE_OBJECT *dup = NULL;
> > -
> >    if (strcmp(subsystem, "hidraw") == 0)
> > -
> >        other_vtbl = &lnxev_vtbl;
> > -
> >    else if (strcmp(subsystem, "input") == 0)
> > -
> >        other_vtbl = &hidraw_vtbl;
> > -
> > -
> >    if (other_vtbl)
> > -
> >        dup = bus_enumerate_hid_devices(other_vtbl, check_same_device, dev);
>
> Shouldn't we have some preference of either hidraw or linux event devices?
> Otherwise this could lead to different results in each run.

Preference is much harder because we don't know what order the devices come in so we don't know if given an input device if we will ever get a readable hidraw device or not, and vice versa. It would involve having to get all the devices, waiting for things to settle and then deciding. This would mean that devices would not be initialized for wine apps until after this settle, and also it would mean we would have to do this collect, wait, decide on every plug in as well. It felt highly impractical.
> > }
> >
> > +DEVICE_OBJECT* bus_enumerate_hid_devices(const platform_vtbl *vtbl, enum_func function, void* context)
> > +{
> > - struct pnp_device *dev;
> > - DEVICE_OBJECT *ret = NULL;
> > -
> > - TRACE("(%p)\n", vtbl);
> > -
> > - 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 (function(dev->device, context) == 0)
> > -
> >        ret = dev->device;
> > -
> >        break;
>
> This is wrong and will always abort after the first function call.

Doh! Too much python work recently.

-aric
> > - }
> > - LeaveCriticalSection(&device_list_cs);
> > - return ret;
> >
> > +}
> > -