Re: [PATCH v7] winebus.sys: Build initial hidraw device set

[Sebastian Lackner]

On 08.09.2016 20:07, Aric Stewart wrote:
> v2: style changes
> v4: changes required by previous updates
> v6: removed disableing busses
> v7: Rebased, suggestions from Sebastian Lackner
>
> Includes implementation of common HID_PNP_CreateDevice
>
> Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> ---
>  dlls/winebus.sys/Makefile.in |   3 +-
>  dlls/winebus.sys/bus.h       |   2 +
>  dlls/winebus.sys/bus_udev.c  | 117 ++++++++++++++++++++++++++++++++
>  dlls/winebus.sys/main.c      | 156 +++++++++++++++++++++++++++++++++++++++++++
>  loader/wine.inf.in           |  15 +++--
>  5 files changed, 288 insertions(+), 5 deletions(-)
>
> v7-0002-winebus.sys-Build-initial-hidraw-device-set.txt
>
> diff --git a/dlls/winebus.sys/Makefile.in b/dlls/winebus.sys/Makefile.in
> index 99ff4d2..2d29105 100644
> --- a/dlls/winebus.sys/Makefile.in
> +++ b/dlls/winebus.sys/Makefile.in
> @@ -1,5 +1,6 @@
>  MODULE    = winebus.sys
> -IMPORTS   = ntoskrnl
> +IMPORTS   = ntoskrnl setupapi
> +EXTRALIBS = $(UDEV_LIBS)

It might be useful to set EXTRAINCL aswell, for example:

EXTRAINCL = $(UDEV_CFLAGS)

The value is also obtained using pkg-config. In my case its empty but I don't
know if thats true for all systems.
> EXTRADLLFLAGS = -Wb,--subsystem,native
>
> C_SRCS = \
> diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h
> index 3d53a46..09920dd 100644
> --- a/dlls/winebus.sys/bus.h
> +++ b/dlls/winebus.sys/bus.h
> @@ -21,3 +21,5 @@ NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry
>
> /* 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, LPVOID native, WORD vid, WORD pid, DWORD version, DWORD uid, const WCHAR *serial, BOOL is_gamepad, const GUID *class) DECLSPEC_HIDDEN;
> +DWORD check_bus_option(UNICODE_STRING *registry_path, const UNICODE_STRING *option) DECLSPEC_HIDDEN;
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> index f040e60..e6319cb 100644
> --- a/dlls/winebus.sys/bus_udev.c
> +++ b/dlls/winebus.sys/bus_udev.c
> @@ -20,6 +20,19 @@
>
> #include "config.h"
>  #include <stdarg.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +
> +#ifdef HAVE_SYS_IOCTL_H
> +# include <sys/ioctl.h>
> +#endif
> +
> +#ifdef HAVE_LIBUDEV_H
> +# include <libudev.h>
> +#endif
> +
> +#include <errno.h>
>
> #define NONAMELESSUNION
>
> @@ -27,6 +40,7 @@
>  #define WIN32_NO_STATUS
>  #include "windef.h"
>  #include "winbase.h"
> +#include "winnls.h"
>  #include "winternl.h"
>  #include "ddk/wdm.h"
>  #include "wine/debug.h"
> @@ -39,13 +53,116 @@ WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
>
> static DRIVER_OBJECT *udev_driver_obj = NULL;
>
> +#include "initguid.h"
> +DEFINE_GUID(GUID_DEVCLASS_HIDRAW, 0x3def44ad,0x242e,0x46e5,0x82,0x6d,0x70,0x72,0x13,0xf3,0xaa,0x81);
> +
> +static const WCHAR hidraw_busidW[] = {'H','I','D','R','A','W',0};
> +
> +/* udev based PNP support */
> +static struct udev *udev = NULL;
> +
> +static void try_add_device(void *dev)

The parameter should have the type "struct udev_device *".
> +{
> +    const char *devnode;
> +    int fd;
> +    DWORD vid=0, pid=0, uid=0, version=0;
> +    WCHAR serial[255] = {'0','0','0','0','0',0};
> +    DEVICE_OBJECT *device = NULL;
> +    struct udev_device *usbdev;
> +    const char *subsystem;
> +
> +    devnode = udev_device_get_devnode(dev);

According to the documentation this function can also return NULL under specific circumstances:
"the device node file name of the udev device, or NULL if no device node exists".
> +    if ((fd = open(devnode, O_RDWR)) == -1)
> +    {
> +        TRACE("Unable to open udev device %s\n",devnode);

Please use debugstr_a() for tracing strings of arbitrary length. It also wouldn't hurt to promote
it to a WARN() and trace an error description, for example using strerror().
> +        return;
> +    }
> +    close(fd);
> +
> +    TRACE("Got udev device %s\n", devnode);

Similar to above, please use debugstr_a().
> +
> +    usbdev = udev_device_get_parent_with_subsystem_devtype(dev, "usb",
> +             "usb_device");
> +    if (usbdev)
> +    {
> +        char *endptr;
> +        const char *srl = NULL;
> +        vid = strtol(udev_device_get_sysattr_value(usbdev,"idVendor"), &endptr, 16);
> +        pid = strtol(udev_device_get_sysattr_value(usbdev,"idProduct"), &endptr, 16);
> +        version = strtol(udev_device_get_sysattr_value(usbdev,"version"), &endptr, 10);

Those calls could fail:
"the content of a sys attribute file, or NULL if there is no sys attribute value.".
> +        srl = udev_device_get_sysattr_value(usbdev,"serial");
> +        if (srl)
> +            MultiByteToWideChar(CP_UNIXCP, 0, srl, -1, serial, 255);
> +    }
> +
> +    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, uid, serial, FALSE, &GUID_DEVCLASS_HIDRAW);
> +    else
> +        ERR("Device has unknown subsystem %s\n", subsystem);
> +
> +    if (!device)
> +        ERR("Failed to create device\n");
> +}
> +
> +static void build_initial_deviceset(void *udev, const char **subsystems)

The function takes a parameter "udev", but there is also a global variable with the same name.
This seems a bit redundant. Also, I would suggest to remove the subsystems parameter here, and
enumerate all types of devices which are supported. You can still filter them in try_add_device
if required later.
> +{
> +    struct udev_enumerate *enumerate;
> +    struct udev_list_entry *udevices, *dev_list_entry;
> +    const char **p;
> +
> +    enumerate = udev_enumerate_new(udev);
> +    p = subsystems;
> +    while (*p)
> +    {
> +        udev_enumerate_add_match_subsystem(enumerate, *p);
> +        p++;
> +    }
> +    udev_enumerate_scan_devices(enumerate);
> +    udevices = udev_enumerate_get_list_entry(enumerate);
> +
> +    udev_list_entry_foreach(dev_list_entry, udevices) {
> +        const char *path;
> +        struct udev_device *dev;
> +
> +        path = udev_list_entry_get_name(dev_list_entry);
> +        dev = udev_device_new_from_syspath(udev, path);
> +        try_add_device(dev);

According to the documentation some of these udev functions can fail. It would be better to
catch errors here, especially for udev_enumerate_new() and udev_device_new_from_syspath().
There are also refcounting issues, udev_device_new_from_syspath creates a new object which
must be released when no longer needed (for example if try_add_device fails).
> +    }
> +
> +    udev_enumerate_unref(enumerate);
> +}
> +
>  NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry_path)
>  {
> +    static const WCHAR hidraw_disabledW[] = {'D','i','s','a','b','l','e',' ','h','i','d','r','a','w',0};
> +    static const UNICODE_STRING hidraw_disabled = {sizeof(hidraw_disabledW) - sizeof(WCHAR), sizeof(hidraw_disabledW), (WCHAR*)hidraw_disabledW};
> +    static const char *hidraw = "hidraw";
> +    static const char *subsystems[2] = {0, 0};
> +
>      TRACE("(%p, %s)\n", driver, debugstr_w(registry_path->Buffer));
>
> udev_driver_obj = driver;
>      driver->MajorFunction[IRP_MJ_PNP] = common_pnp_dispatch;
>
> +    if (check_bus_option(registry_path, &hidraw_disabled))
> +        TRACE("UDEV hidraw devices disabled in registry\n");
> +    else
> +        subsystems[0] = hidraw;
> +
> +    udev = udev_new();
> +
> +    if (!udev)
> +    {
> +        ERR("Can't create udev\n");
> +        return STATUS_UNSUCCESSFUL;
> +    }
> +
> +    /* PNP Bus initialization */
> +    if (subsystems[0])
> +        build_initial_deviceset(udev, subsystems);
> +
>      return STATUS_SUCCESS;
>  }
>
> diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c
> index 43de25a..94f1b4b 100644
> --- a/dlls/winebus.sys/main.c
> +++ b/dlls/winebus.sys/main.c
> @@ -18,6 +18,7 @@
>   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
>   */
>
> +#include "config.h"
>  #include <stdarg.h>
>
> #define NONAMELESSUNION
> @@ -26,14 +27,143 @@
>  #define WIN32_NO_STATUS
>  #include "windef.h"
>  #include "winbase.h"
> +#include "winuser.h"
>  #include "winternl.h"
> +#include "winreg.h"
> +#include "setupapi.h"
> +#include "cfgmgr32.h"
>  #include "ddk/wdm.h"
>  #include "wine/debug.h"
> +#include "wine/unicode.h"
> +#include "wine/list.h"
>
> #include "bus.h"
>
> WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
>
> +static const WCHAR hwidfmtW[] =  {'%','s','\\','V','i','d','_','%','0','4','x','&','P','i','d','_','%','0','4','x','&','%','s','\\','%','i','&','%','s','&','%','p',0};
> +static const WCHAR im_fmtW[] = {'I','M','_','%','i',0};
> +static const WCHAR ig_fmtW[] = {'I','G','_','%','i',0};
> +
> +typedef struct _pnp_device {
> +    struct list entry;
> +
> +    DEVICE_OBJECT *device;
> +    DWORD vidpid;

It would not really be necessary to store the vid/pid information twice. You can also access
it using device->DeviceExtension.
> +} pnp_device;
> +
> +typedef struct {
> +    LPVOID native;  /* Must be the first member of the structure */
> +
> +    DWORD vid, pid, version, uid;
> +    BOOL is_gamepad;
> +    WCHAR serial[255];
> +    WCHAR const *busidW; /* Expected to be a static constant */
> +} extension;
> +
> +static struct list pnp_devset = LIST_INIT(pnp_devset);

Its up to you if you want to add it now, but sooner or later you will need locking for such a list
when it can be accessed by multiple threads.
Also, as I saw that you have to convert the list back to a linear array in some of the functions
implemented in following patches. Do you think it makes sense to store it as an array from the start?
(Lock would still be needed but no conversion).
> +
> +static void add_device(DEVICE_OBJECT *device, DWORD vidpid)
> +{
> +    pnp_device *pnp_dev;
> +
> +    pnp_dev = HeapAlloc(GetProcessHeap(), 0, sizeof(*pnp_dev));
> +    pnp_dev->device = device;
> +    pnp_dev->vidpid = vidpid;
> +    list_add_tail(&pnp_devset, &pnp_dev->entry);
> +}
> +
> +static INT get_vidpid_index(DEVICE_OBJECT *device, DWORD vidpid)
> +{
> +    pnp_device *ptr;
> +    int index = 0;
> +    LIST_FOR_EACH_ENTRY(ptr, &pnp_devset, pnp_device, entry)
> +    {
> +        if (ptr->vidpid == vidpid)
> +            index++;
> +        if (ptr->device == device)
> +            return index;
> +    }
> +    ERR("Device not found in list\n");
> +    return 0;
> +}

Does this work properly when elements are deleted? You might return the same index
for different devices.
> +
> +static int get_instance_id(DEVICE_OBJECT *device, WCHAR *id)
> +{
> +    int idx;
> +    WCHAR index_string[256];
> +
> +    extension *ext = (extension*)device->DeviceExtension;
> +
> +    idx = get_vidpid_index(device, MAKELONG(ext->vid,ext->pid));
> +
> +    if (ext->is_gamepad)
> +        sprintfW(index_string, ig_fmtW, idx);
> +    else
> +        sprintfW(index_string, im_fmtW, idx);
> +
> +    return sprintfW(id, hwidfmtW, ext->busidW, ext->vid, ext->pid, index_string, ext->version, ext->serial, ext->uid);

It looks like you use %p in the format string, but uid has type DWORD?
> +}
> +
> +DEVICE_OBJECT* bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, LPVOID native, WORD vid, WORD pid, DWORD version, DWORD uid, const WCHAR *serial, BOOL is_gamepad, const GUID *class)
> +{

Please note that LPVOID types should be avoided in new wine code.
> +    NTSTATUS status;
> +    DEVICE_OBJECT *device;
> +    static const WCHAR device_name_fmtW[] = {'\\','D','e','v','i','c','e',
> +    '\\','%','s','#','%','p',0};
> +    static const WCHAR zero_serialW[]= {'0','0','0','0',0};
> +    WCHAR dev_name[255];
> +    UNICODE_STRING nameW;
> +    HDEVINFO devinfo;
> +    extension *ext;
> +
> +    TRACE("Create HID bus device for native device %p\n", native);
> +
> +    sprintfW(dev_name, device_name_fmtW, busidW, native);
> +    RtlInitUnicodeString(&nameW, dev_name);
> +    status = IoCreateDevice(driver, sizeof(extension), &nameW, 0, 0, FALSE, &device);
> +    if (status)
> +    {
> +        FIXME( "failed to create device error %x\n", status );
> +        return NULL;
> +    }
> +
> +    ext = (extension*)device->DeviceExtension;
> +    ext->native = native;

At least for udev the device object has to be released. It looks like the logic
is currently missing (also in your draft patches). In order to fix that it will
probably be necessary to store additional information in each device object.
> +    ext->vid = vid;
> +    ext->pid = pid;
> +    ext->uid = uid;
> +    ext->is_gamepad = is_gamepad;
> +    ext->version = version;
> +    ext->busidW = busidW;
> +    if (serial)
> +        lstrcpyW(ext->serial, serial);
> +    else
> +        lstrcpyW(ext->serial, zero_serialW);
> +
> +    add_device(device, MAKELONG(vid, pid));
> +
> +    devinfo = SetupDiGetClassDevsW(class, NULL, NULL, DIGCF_DEVICEINTERFACE);
> +    if (!devinfo)
> +        FIXME( "failed to get ClassDevs %x\n", GetLastError());
> +    else
> +    {
> +        SP_DEVINFO_DATA Data;
> +        WCHAR id[MAX_DEVICE_ID_LEN];
> +
> +        get_instance_id(device, id);
> +        Data.cbSize = sizeof(Data);
> +        SetupDiCreateDeviceInfoW(devinfo, id, class, NULL, NULL, DICD_INHERIT_CLASSDRVS, &Data);
> +        if (!SetupDiRegisterDeviceInfo(devinfo, &Data, 0, NULL, NULL, NULL ))
> +            FIXME( "failed to Register Device Info %x\n", GetLastError());
> +        SetupDiDestroyDeviceInfoList(devinfo);

How does this correspond to similar code in hidclass.sys/device.c. Do we really need both? Why does
hidclass contain a call to SetupDiCreateDeviceInterfaceW but not this version?
> +    }
> +
> +    IoInvalidateDeviceRelations(device, BusRelations);
> +
> +    return device;
> +}
> +
>  NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp)
>  {
>      NTSTATUS status = irp->IoStatus.u.Status;
> @@ -59,6 +189,32 @@ NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp)
>      return status;
>  }
>
> +DWORD check_bus_option(UNICODE_STRING *registry_path, const UNICODE_STRING *option)
> +{
> +    OBJECT_ATTRIBUTES attr;
> +    HANDLE key;
> +    DWORD output = 0;
> +
> +    InitializeObjectAttributes(&attr, registry_path, OBJ_CASE_INSENSITIVE | OBJ_KERNEL_HANDLE, NULL, NULL);
> +    if (ZwOpenKey(&key, KEY_ALL_ACCESS, &attr) == STATUS_SUCCESS)
> +    {
> +        DWORD size;
> +        char buffer[FIELD_OFFSET(KEY_VALUE_PARTIAL_INFORMATION, Data[sizeof(DWORD)])];
> +
> +        KEY_VALUE_PARTIAL_INFORMATION *info = (KEY_VALUE_PARTIAL_INFORMATION*)buffer;
> +
> +        if (ZwQueryValueKey(key, option, KeyValuePartialInformation, info, sizeof(buffer), &size) == STATUS_SUCCESS)
> +        {
> +            if (info->Type == REG_DWORD)
> +                output = *(DWORD*)info->Data;
> +        }
> +
> +        ZwClose(key);
> +    }
> +
> +    return output;
> +}

I think it would be better to add this in a later patch. Currently there is only support for one subsystem,
so it would not make much sense to disable it.
> +
>  NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path )
>  {
>      static const WCHAR udevW[] = {'\\','D','r','i','v','e','r','\\','U','D','E','V',0};
> diff --git a/loader/wine.inf.in b/loader/wine.inf.in
> index 4e29026..a29b99f 100644
> --- a/loader/wine.inf.in
> +++ b/loader/wine.inf.in
> @@ -48,7 +48,8 @@ AddReg=\
>      SessionMgr,\
>      Tapi,\
>      Timezones,\
> -    LicenseInformation
> +    LicenseInformation,\
> +    PlatformBus
>
> [DefaultInstall.NT]
>  RegisterDlls=RegisterDllsSection
> @@ -73,7 +74,8 @@ AddReg=\
>      Tapi,\
>      Timezones,\
>      VersionInfo,\
> -    LicenseInformation
> +    LicenseInformation,\
> +    PlatformBus
>
> [DefaultInstall.ntamd64]
>  RegisterDlls=RegisterDllsSection
> @@ -100,7 +102,8 @@ AddReg=\
>      Tapi,\
>      Timezones,\
>      VersionInfo.ntamd64,\
> -    LicenseInformation
> +    LicenseInformation,\
> +    PlatformBus
>
> [Wow64Install]
>  RegisterDlls=RegisterDllsSection
> @@ -115,7 +118,8 @@ AddReg=\
>      Misc,\
>      Tapi,\
>      VersionInfo.ntamd64,\
> -    LicenseInformation
> +    LicenseInformation,\
> +    PlatformBus
>
> [DefaultInstall.Services]
>  AddService=BITS,0,BITSService
> @@ -3383,3 +3387,6 @@ HKLM,Software\Wine\LicenseInformation,"Shell-InBoxGames-Shanghai-EnableGame",0x1
>  HKLM,Software\Wine\LicenseInformation,"Shell-InBoxGames-Solitaire-EnableGame",0x10001,0x00000001
>  HKLM,Software\Wine\LicenseInformation,"Shell-InBoxGames-SpiderSolitaire-EnableGame",0x10001,0x00000001
>  HKLM,Software\Wine\LicenseInformation,"Shell-PremiumInBoxGames-Chess-EnableGame",0x10001,0x00000001
> +
> +[PlatformBus]
> +HKLM,System\CurrentControlSet\Services\UDEV,"Disable hidraw",0x10001,0x00000000

It does not make sense to add registry keys for user-defined settings in wine.inf.
This would have the disadvantage that they will get reset on each wine prefix update.
The correct solution is to assume default settings when registry keys are not present.
>

[PATCH v7] winebus.sys: Build initial hidraw device set

[Aric Stewart]

Thanks for the review!

On 9/12/16 3:24 AM, Sebastian Lackner wrote:
> On 08.09.2016 20:07, Aric Stewart wrote:
> > v2: style changes
> > v4: changes required by previous updates
> > v6: removed disableing busses
> > v7: Rebased, suggestions from Sebastian Lackner
> >
> > Includes implementation of common HID_PNP_CreateDevice
> > +static INT get_vidpid_index(DEVICE_OBJECT *device, DWORD vidpid)
> > +{
> > +    pnp_device *ptr;
> > +    int index = 0;
> > +    LIST_FOR_EACH_ENTRY(ptr, &pnp_devset, pnp_device, entry)
> > +    {
> > +        if (ptr->vidpid == vidpid)
> > +            index++;
> > +        if (ptr->device == device)
> > +            return index;
> > +    }
> > +    ERR("Device not found in list\n");
> > +    return 0;
> > +}
>
> Does this work properly when elements are deleted? You might return the same index
> for different devices.

As far as I understand, this index does not need to be unique to a device, just unique for in given device set of active devices, so this should be ok.
> > +
> > +static int get_instance_id(DEVICE_OBJECT *device, WCHAR *id)
> > +{
> > +    int idx;
> > +    WCHAR index_string[256];
> > +
> > +    extension *ext = (extension*)device->DeviceExtension;
> > +
> > +    idx = get_vidpid_index(device, MAKELONG(ext->vid,ext->pid));
> > +
> > +    if (ext->is_gamepad)
> > +        sprintfW(index_string, ig_fmtW, idx);
> > +    else
> > +        sprintfW(index_string, im_fmtW, idx);
> > +
> > +    return sprintfW(id, hwidfmtW, ext->busidW, ext->vid, ext->pid, index_string, ext->version, ext->serial, ext->uid);
>
> It looks like you use %p in the format string, but uid has type DWORD?
> > +}
> > +
> > +DEVICE_OBJECT* bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, LPVOID native, WORD vid, WORD pid, DWORD version, DWORD uid, const WCHAR *serial, BOOL is_gamepad, const GUID *class)
> > +{
>
> Please note that LPVOID types should be avoided in new wine code.
> > +    NTSTATUS status;
> > +    DEVICE_OBJECT *device;
> > +    static const WCHAR device_name_fmtW[] = {'\\','D','e','v','i','c','e',
> > +    '\\','%','s','#','%','p',0};
> > +    static const WCHAR zero_serialW[]= {'0','0','0','0',0};
> > +    WCHAR dev_name[255];
> > +    UNICODE_STRING nameW;
> > +    HDEVINFO devinfo;
> > +    extension *ext;
> > +
> > +    TRACE("Create HID bus device for native device %p\n", native);
> > +
> > +    sprintfW(dev_name, device_name_fmtW, busidW, native);
> > +    RtlInitUnicodeString(&nameW, dev_name);
> > +    status = IoCreateDevice(driver, sizeof(extension), &nameW, 0, 0, FALSE, &device);
> > +    if (status)
> > +    {
> > +        FIXME( "failed to create device error %x\n", status );
> > +        return NULL;
> > +    }
> > +
> > +    ext = (extension*)device->DeviceExtension;
> > +    ext->native = native;
>
> At least for udev the device object has to be released. It looks like the logic
> is currently missing (also in your draft patches). In order to fix that it will
> probably be necessary to store additional information in each device object.

That is coming in a later patch where I handle device removals. You are right that part of that will need to be moved here for the error case.
> > +    ext->vid = vid;
> > +    ext->pid = pid;
> > +    ext->uid = uid;
> > +    ext->is_gamepad = is_gamepad;
> > +    ext->version = version;
> > +    ext->busidW = busidW;
> > +    if (serial)
> > +        lstrcpyW(ext->serial, serial);
> > +    else
> > +        lstrcpyW(ext->serial, zero_serialW);
> > +
> > +    add_device(device, MAKELONG(vid, pid));
> > +
> > +    devinfo = SetupDiGetClassDevsW(class, NULL, NULL, DIGCF_DEVICEINTERFACE);
> > +    if (!devinfo)
> > +        FIXME( "failed to get ClassDevs %x\n", GetLastError());
> > +    else
> > +    {
> > +        SP_DEVINFO_DATA Data;
> > +        WCHAR id[MAX_DEVICE_ID_LEN];
> > +
> > +        get_instance_id(device, id);
> > +        Data.cbSize = sizeof(Data);
> > +        SetupDiCreateDeviceInfoW(devinfo, id, class, NULL, NULL, DICD_INHERIT_CLASSDRVS, &Data);
> > +        if (!SetupDiRegisterDeviceInfo(devinfo, &Data, 0, NULL, NULL, NULL ))
> > +            FIXME( "failed to Register Device Info %x\n", GetLastError());
> > +        SetupDiDestroyDeviceInfoList(devinfo);
>
> How does this correspond to similar code in hidclass.sys/device.c. Do we really need both? Why does
> hidclass contain a call to SetupDiCreateDeviceInterfaceW but not this version?

These part of registry manipulations are done on windows via device installation. So setupapi is used to make these keys and then they are just expected to be there for installed devices. Since we are trying to handle native devices that have not been installed in this method we have to add the proper registry information more dynamically. That both areas have to do similar things that is why the code is similar.

These bus devices do not have any Interfaces, that is why we have no call to SetupDiCreateDeviceInterfaceW.


I will work on integrating all the other changes. thanks!

-aric
> > +    }
> > +
> > +    IoInvalidateDeviceRelations(device, BusRelations);
> > +
> > +    return device;
> > +}
> > +
> >  NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp)
> >  {
> >      NTSTATUS status = irp->IoStatus.u.Status;
> > @@ -59,6 +189,32 @@ NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp)
> >      return status;
> >  }
> >
> > +DWORD check_bus_option(UNICODE_STRING *registry_path, const UNICODE_STRING *option)
> > +{
> > +    OBJECT_ATTRIBUTES attr;
> > +    HANDLE key;
> > +    DWORD output = 0;
> > +
> > +    InitializeObjectAttributes(&attr, registry_path, OBJ_CASE_INSENSITIVE | OBJ_KERNEL_HANDLE, NULL, NULL);
> > +    if (ZwOpenKey(&key, KEY_ALL_ACCESS, &attr) == STATUS_SUCCESS)
> > +    {
> > +        DWORD size;
> > +        char buffer[FIELD_OFFSET(KEY_VALUE_PARTIAL_INFORMATION, Data[sizeof(DWORD)])];
> > +
> > +        KEY_VALUE_PARTIAL_INFORMATION *info = (KEY_VALUE_PARTIAL_INFORMATION*)buffer;
> > +
> > +        if (ZwQueryValueKey(key, option, KeyValuePartialInformation, info, sizeof(buffer), &size) == STATUS_SUCCESS)
> > +        {
> > +            if (info->Type == REG_DWORD)
> > +                output = *(DWORD*)info->Data;
> > +        }
> > +
> > +        ZwClose(key);
> > +    }
> > +
> > +    return output;
> > +}
>
> I think it would be better to add this in a later patch. Currently there is only support for one subsystem,
> so it would not make much sense to disable it.
> > +
> >  NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path )
> >  {
> >      static const WCHAR udevW[] = {'\\','D','r','i','v','e','r','\\','U','D','E','V',0};
> > diff --git a/loader/wine.inf.in b/loader/wine.inf.in
> > index 4e29026..a29b99f 100644
> > --- a/loader/wine.inf.in
> > +++ b/loader/wine.inf.in
> > @@ -48,7 +48,8 @@ AddReg=\
> >      SessionMgr,\
> >      Tapi,\
> >      Timezones,\
> > -    LicenseInformation
> > +    LicenseInformation,\
> > +    PlatformBus
> >
> > [DefaultInstall.NT]
> >  RegisterDlls=RegisterDllsSection
> > @@ -73,7 +74,8 @@ AddReg=\
> >      Tapi,\
> >      Timezones,\
> >      VersionInfo,\
> > -    LicenseInformation
> > +    LicenseInformation,\
> > +    PlatformBus
> >
> > [DefaultInstall.ntamd64]
> >  RegisterDlls=RegisterDllsSection
> > @@ -100,7 +102,8 @@ AddReg=\
> >      Tapi,\
> >      Timezones,\
> >      VersionInfo.ntamd64,\
> > -    LicenseInformation
> > +    LicenseInformation,\
> > +    PlatformBus
> >
> > [Wow64Install]
> >  RegisterDlls=RegisterDllsSection
> > @@ -115,7 +118,8 @@ AddReg=\
> >      Misc,\
> >      Tapi,\
> >      VersionInfo.ntamd64,\
> > -    LicenseInformation
> > +    LicenseInformation,\
> > +    PlatformBus
> >
> > [DefaultInstall.Services]
> >  AddService=BITS,0,BITSService
> > @@ -3383,3 +3387,6 @@ HKLM,Software\Wine\LicenseInformation,"Shell-InBoxGames-Shanghai-EnableGame",0x1
> >  HKLM,Software\Wine\LicenseInformation,"Shell-InBoxGames-Solitaire-EnableGame",0x10001,0x00000001
> >  HKLM,Software\Wine\LicenseInformation,"Shell-InBoxGames-SpiderSolitaire-EnableGame",0x10001,0x00000001
> >  HKLM,Software\Wine\LicenseInformation,"Shell-PremiumInBoxGames-Chess-EnableGame",0x10001,0x00000001
> > +
> > +[PlatformBus]
> > +HKLM,System\CurrentControlSet\Services\UDEV,"Disable hidraw",0x10001,0x00000000
>
> It does not make sense to add registry keys for user-defined settings in wine.inf.
> This would have the disadvantage that they will get reset on each wine prefix update.
> The correct solution is to assume default settings when registry keys are not present.
> >

[PATCH v7] winebus.sys: Build initial hidraw device set

[Sebastian Lackner]

On 12.09.2016 14:41, Aric Stewart wrote:
> > > Does this work properly when elements are deleted? You might return the same index
> > > for different devices.
> >
> > As far as I understand, this index does not need to be unique to a device, just unique for in given device set of active devices, so this should be ok.

I don't think its guaranteed to be unique at all. Lets assume we have three
devices with the same vidpid (index 1, 2, 3). The second one gets removed
(index 1, 3) and a fourth one gets added. It will then attempt to use index
3 which is still in use. Is that not a problem?
> > > At least for udev the device object has to be released. It looks like the logic
> > > is currently missing (also in your draft patches). In order to fix that it will
> > > probably be necessary to store additional information in each device object.
> >
> > That is coming in a later patch where I handle device removals. You are right that part of that will need to be moved here for the error case.

If it was part of your draft series I must have missed it. How exactly are you
planning to implement it? Does it make sense to store a release function as part
of each device, or maybe even a Vtable?
> > > How does this correspond to similar code in hidclass.sys/device.c. Do we really need both? Why does
> > > hidclass contain a call to SetupDiCreateDeviceInterfaceW but not this version?
> >
> > These part of registry manipulations are done on windows via device installation. So setupapi is used to make these keys and then they are just expected to be there for installed devices. Since we are trying to handle native devices that have not been installed in this method we have to add the proper registry information more dynamically. That both areas have to do similar things that is why the code is similar.
>
> These bus devices do not have any Interfaces, that is why we have no call to SetupDiCreateDeviceInterfaceW.

Okay, thanks for the explanation. I still find it a bit odd when we have all
this code with a similar purpose at different places though, especially when
it looks very different. The hidclass version for example will leak the devinfo
handle on various error paths and returns ERROR_* codes instead of NTSTATUS
values. This version here might have other issues (at least missing error
handling for some of the functions). If the code duplication cannot be avoided,
it might at least be useful to keep the code a bit more synchronized to these
other versions (after fixing them, of course).
> I will work on integrating all the other changes. thanks!
>
> -aric

Regards,
Sebastian

[PATCH v7] winebus.sys: Build initial hidraw device set

[Aric Stewart]

On 9/12/16 8:08 AM, Sebastian Lackner wrote:
> On 12.09.2016 14:41, Aric Stewart wrote:
> > > > Does this work properly when elements are deleted? You might return the same index
> > > > for different devices.
> > >
> > > As far as I understand, this index does not need to be unique to a device, just unique for in given device set of active devices, so this should be ok.
>
> I don't think its guaranteed to be unique at all. Lets assume we have three
> devices with the same vidpid (index 1, 2, 3). The second one gets removed
> (index 1, 3) and a fourth one gets added. It will then attempt to use index
> 3 which is still in use. Is that not a problem?

Yes, my logic is clearly flawed. I will re-works it, thanks for noticing!
> > > > At least for udev the device object has to be released. It looks like the logic
> > > > is currently missing (also in your draft patches). In order to fix that it will
> > > > probably be necessary to store additional information in each device object.
> > >
> > > That is coming in a later patch where I handle device removals. You are right that part of that will need to be moved here for the error case.
>
> If it was part of your draft series I must have missed it. How exactly are you
> planning to implement it? Does it make sense to store a release function as part
> of each device, or maybe even a Vtable?

Yeah, I was avoiding vtables as much as possible. You can see it in patch 126373. But clearly my error handling here needs work which I am giving it.
> > > > How does this correspond to similar code in hidclass.sys/device.c. Do we really need both? Why does
> > > > hidclass contain a call to SetupDiCreateDeviceInterfaceW but not this version?
> > >
> > > These part of registry manipulations are done on windows via device installation. So setupapi is used to make these keys and then they are just expected to be there for installed devices. Since we are trying to handle native devices that have not been installed in this method we have to add the proper registry information more dynamically. That both areas have to do similar things that is why the code is similar.
> >
> > These bus devices do not have any Interfaces, that is why we have no call to SetupDiCreateDeviceInterfaceW.
>
> Okay, thanks for the explanation. I still find it a bit odd when we have all
> this code with a similar purpose at different places though, especially when
> it looks very different. The hidclass version for example will leak the devinfo
> handle on various error paths and returns ERROR_* codes instead of NTSTATUS
> values. This version here might have other issues (at least missing error
> handling for some of the functions). If the code duplication cannot be avoided,
> it might at least be useful to keep the code a bit more synchronized to these
> other versions (after fixing them, of course).

I will work on the error handling in this chunk of code. Patches to fix up the hidclass stuff are welcome! :) I will try to circle back to that also since you have identified issues. 

I have not found a good way to avoid this duplication. There are functions like DiInstallDriver or UpdateDriverForPlugAndPlayDevices, but they expect an INF file with the driver information specified. I have not found a clean way to just do these step programmatically in the windows API. 

-aric
> > I will work on integrating all the other changes. thanks!
> >
> > -aric
>
> Regards,
> Sebastian