[PATCH v7] winebus.sys: Build initial hidraw device set
Aric Stewart
aric at codeweavers.com
Mon Sep 12 07:41:24 CDT 2016
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.
>
>>
>>
>>
>
More information about the wine-devel
mailing list