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

[Sebastian Lackner]

On 12.09.2016 16:01, Aric Stewart wrote:
> v2: style changes
> v4: changes required by previous updates
> v6: removed disableing busses
> v7: Rebased, suggestions from Sebastian Lackner
> v8: More suggestions from Sebastian Lackner

Because of the size of the patch I've decided to split it. I've already sent an improved version
of the first part including my signoff. An improved version of the second part is already in my
stash, but first I would like to discuss a few remaining things.
> Includes implementation of common HID_PNP_CreateDevice
> Signed-off-by: Aric Stewart
>
> dlls/winebus.sys/Makefile.in |   4 +-
>  dlls/winebus.sys/bus.h       |   1 +
>  dlls/winebus.sys/bus_udev.c  | 135 ++++++++++++++++++++++++++++++++++++++++
>  dlls/winebus.sys/main.c      | 144 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 283 insertions(+), 1 deletion(-)
>
> v8-0001-winebus.sys-Build-initial-hidraw-device-set.txt
>
> diff --git a/dlls/winebus.sys/Makefile.in b/dlls/winebus.sys/Makefile.in
> index 99ff4d2..9d3a0c2 100644
> --- a/dlls/winebus.sys/Makefile.in
> +++ b/dlls/winebus.sys/Makefile.in
> @@ -1,5 +1,7 @@
>  MODULE    = winebus.sys
> -IMPORTS   = ntoskrnl
> +IMPORTS   = ntoskrnl setupapi
> +EXTRALIBS = $(UDEV_LIBS)
> +EXTRAINCL = $(UDEV_CFLAGS)
>  EXTRADLLFLAGS = -Wb,--subsystem,native
>
> C_SRCS = \
> diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h
> index 3d53a46..47d146e 100644
> --- a/dlls/winebus.sys/bus.h
> +++ b/dlls/winebus.sys/bus.h
> @@ -21,3 +21,4 @@ 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;
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> index f040e60..792422c 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,6 +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);

I haven't found this definition in header files, so I assume this is Wine specific, right? Does
Windows have an equivalent which we could use here?
> -
>
> +static const WCHAR hidraw_busidW[] = {'H','I','D','R','A','W',0};
> -
>
> +/* udev based PNP support */
> +static struct udev *udev = NULL;
> -
>
> +static int try_add_device(struct udev_device *dev)
> +{
> - const char *devnode;
> - int fd;
> - DWORD vid=0, pid=0, uid=0, version=0;
> - WCHAR serial[255] = {'0','0','0','0','0',0};

Here you use a default serial with 5 digits "0", but in bus_create_hid_device() only 4 digits.
Which of both makes more sense?
> - DEVICE_OBJECT *device = NULL;
> - struct udev_device *usbdev;
> - const char *subsystem;
> -
> - devnode = udev_device_get_devnode(dev);
> - if (!devnode || ((fd = open(devnode, O_RDWR)) == -1))
> - {
> -
>    WARN("Unable to open udev device %s\n", debugstr_a(devnode));
> -
>    return 0;
> - }
> - close(fd);
> -
> - TRACE("Got udev device %s\n", debugstr_a(devnode));
> -
> - usbdev = udev_device_get_parent_with_subsystem_devtype(dev, "usb",
> -
>         "usb_device");
> - if (usbdev)
> - {
> -
>    char *endptr;
> -
>    const char *attr;
> -
>    attr = udev_device_get_sysattr_value(usbdev, "idVendor");
> -
>    if (attr)
> -
>        vid = strtol(attr, &endptr, 16);
> -
>    else
> -
>        WARN("Could not get idVendor from usb device\n");
> -
>    attr = udev_device_get_sysattr_value(usbdev, "idProduct");
> -
>    if (attr)
> -
>        pid = strtol(attr, &endptr, 16);
> -
>    else
> -
>        WARN("Could not get idProduct from usb device\n");
> -
>    attr = udev_device_get_sysattr_value(usbdev, "version");
> -
>    if (attr)
> -
>        version = strtol(attr, &endptr, 10);
> -
>    else
> -
>        WARN("Could not get version from usb device\n");
> -
>    attr = udev_device_get_sysattr_value(usbdev, "serial");
> -
>    if (attr)
> -
>        MultiByteToWideChar(CP_UNIXCP, 0, attr, -1, serial, 255);
> - }
> - else
> -
>    WARN("Could not get underlying usb device to query VID, PID, Version and Serial\n");
> -
> - 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);
> -
>    return 0;
> - }
> -
> - if (!device)
> - {
> -
>    ERR("Failed to create device\n");
> -
>    return 0;
> - }
> -
> - return 1;
>
> +}
> -
>
> +static void build_initial_deviceset( void )
> +{
> - struct udev_enumerate *enumerate;
> - struct udev_list_entry *udevices, *dev_list_entry;
> -
> - enumerate = udev_enumerate_new(udev);
> - if (!enumerate)
> - {
> -
>    WARN("Unable to start UDEV enumeration\n");
> -
>    return;
> - }
> -
> - if (udev_enumerate_add_match_subsystem(enumerate, "hidraw") != 0)
> -
>    WARN("Failed to add subsystem 'hidraw' to enumeration\n");
> -
> - if (udev_enumerate_scan_devices(enumerate) != 0)
> -
>    WARN("Enumeration scan failed\n");
> -
> - 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);
> -
>    if (!dev)
> -
>        WARN("Unable to create device %s\n", debugstr_a(path));
> -
>    else if (!try_add_device(dev))
> -
>        udev_device_unref(dev);
> - }
> -
> - udev_enumerate_unref(enumerate);
>
> +}
> -
>
> NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry_path)
>  {
>      TRACE("(%p, %s)\n", driver, debugstr_w(registry_path->Buffer));
> @@ -46,6 +170,17 @@ NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry
>      udev_driver_obj = driver;
>      driver->MajorFunction[IRP_MJ_PNP] = common_pnp_dispatch;
> - udev = udev_new();
> -
> - if (!udev)
> - {
> -
>    ERR("Can't create udev\n");
> -
>    return STATUS_UNSUCCESSFUL;
> - }
> -
> - /* PNP Bus initialization */
> - build_initial_deviceset();
> - return STATUS_SUCCESS;
>
> }
>
> diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c
> index 43de25a..f06d5c3 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,157 @@
>  #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','&','%','x',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;
>
> +} pnp_device;
> -
>
> +typedef struct {
> - void *native;  /* Must be the first member of the structure */

So far I do not see any code which depends on the fact that native is the first
struct member. Is this comment still valid? If you plan to add code which depends
on this in following patches, I would suggest to evaluate if there isn't a better
solution (like moving the structs to the header file for example).
> -
> - DWORD vid, pid, version, uid, index;
> - BOOL is_gamepad;
> - WCHAR serial[255];
> - WCHAR const *busidW; /* Expected to be a static constant */
>
> +} extension;
> -
>
> +static CRITICAL_SECTION device_list_cs;
> +static CRITICAL_SECTION_DEBUG critsect_debug =
> +{
> - 0, 0, &device_list_cs,
> - { &critsect_debug.ProcessLocksList, &critsect_debug.ProcessLocksList },
> -
>  0, 0, { (DWORD_PTR)(__FILE__ ": device_list_cs") }
>
> +};
> +static CRITICAL_SECTION device_list_cs = { &critsect_debug, -1, 0, 0, 0, 0 };
> -
>
> +static struct list pnp_devset = LIST_INIT(pnp_devset);
> -
>
> +static void add_device(DEVICE_OBJECT *device)
> +{
> - pnp_device *pnp_dev;
> -
> - pnp_dev = HeapAlloc(GetProcessHeap(), 0, sizeof(*pnp_dev));
> - pnp_dev->device = device;
> - EnterCriticalSection(&device_list_cs);
> - list_add_tail(&pnp_devset, &pnp_dev->entry);
> - LeaveCriticalSection(&device_list_cs);
>
> +}
> -
>
> +static INT get_vidpid_index(DEVICE_OBJECT *device)
> +{
> - pnp_device *ptr;
> - int index = 1;
> - extension *ext = (extension*)device->DeviceExtension;
> - DWORD vidpid = MAKELONG(ext->vid, ext->pid);
> -
> - EnterCriticalSection(&device_list_cs);
> - LIST_FOR_EACH_ENTRY(ptr, &pnp_devset, pnp_device, entry)
> - {
> -
>    DWORD device_vidpid;
> -
> -
>    ext = (extension*)ptr->device->DeviceExtension;
> -
>    device_vidpid = MAKELONG(ext->vid, ext->pid);
> -
> -
>    if (device_vidpid == vidpid)
> -
>        index = max(ext->index + 1, index);
> - }
> - LeaveCriticalSection(&device_list_cs);

As mentioned on IRC there is a risk of race-conditions when two threads try to
add a device with the same vid/pid. I've already fixed that in my local version, I'm
just mentioning it for completeness.
> - return index;
>
> +}
> -
>
> +static int get_instance_id(DEVICE_OBJECT *device, WCHAR *id)
> +{
> - WCHAR index_string[256];
> - extension *ext = (extension*)device->DeviceExtension;
> -
> - if (ext->is_gamepad)
> -
>    sprintfW(index_string, ig_fmtW, ext->index);
> - else
> -
>    sprintfW(index_string, im_fmtW, ext->index);
> -
> - return sprintfW(id, hwidfmtW, ext->busidW, ext->vid, ext->pid, index_string, ext->version, ext->serial, ext->uid);

Wouldn't it be better to put together the string at once, without intermediate results?
> +}
> -
>
> +DEVICE_OBJECT* bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, void *native, WORD vid, WORD pid, DWORD version, DWORD uid, const WCHAR *serial, BOOL is_gamepad, const GUID *class)
> +{
> - 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;
> - ext->vid = vid;
> - ext->pid = pid;
> - ext->uid = uid;
> - ext->is_gamepad = is_gamepad;
> - ext->version = version;
> - ext->busidW = busidW;
> - ext->index = get_vidpid_index(device);
> - if (serial)
> -
>    lstrcpyW(ext->serial, serial);
> - else
> -
>    lstrcpyW(ext->serial, zero_serialW);
> -
> - add_device(device);
> -
> - 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);
> -
>    if (!SetupDiCreateDeviceInfoW(devinfo, id, class, NULL, NULL, DICD_INHERIT_CLASSDRVS, &Data))
> -
>        FIXME("Failed to create device info: %x\n", GetLastError());
> -
>    else if (!SetupDiRegisterDeviceInfo(devinfo, &Data, 0, NULL, NULL, NULL ))
> -
>        FIXME("Failed to Register Device Info: %x\n", GetLastError());
> -
>    SetupDiDestroyDeviceInfoList(devinfo);
> - }
> -
> - IoInvalidateDeviceRelations(device, BusRelations);
> -
> - return device;
>
> +}
> -
>
> NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp)
>  {
>      NTSTATUS status = irp->IoStatus.u.Status;