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
>
> 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.
>