[PATCH v8] winebus.sys: Build initial hidraw device set
Aric Stewart
aric at codeweavers.com
Tue Sep 13 06:59:41 CDT 2016
On 9/13/16 4:28 AM, Sebastian Lackner wrote:
> 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 <aric at codeweavers.com>
>> ---
>> 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?
>
Correct this is Wine specific. I dont feel like it is appropriate to use any other class guid because hidraw devices are hidraw devices, they are not anything else. We do not want any application to search for a know class guid and get these devices unexpectedly.
>> +
>> +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?
>
Either, they are place holders. I must have just miscounted on one of them. I have not counted how many "0" are in the windows placeholder as really it makes no material difference.
>> + 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).
>
This becomes important in the winehid.sys driver. We could put this in a common header somewhere but I always find that putting something non-windows in a header file to be a real fight.
>> +
>> + 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.
>
Thanks, we discussed this on IRC.
>> + 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?
>
Either way. Either you have 2 hwidfmtW versions or 2 im_fmtW versions. I am not attached to one or the other.
-aric
>> +}
>> +
>> +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;
>>
>>
>>
>
More information about the wine-devel
mailing list