[PATCH 2/2] hidclass.sys: Create Driver reg key for HID devices.

[Zebediah Figura (she/her)]

On 4/14/21 8:59 AM, Arkadiusz Hiler wrote:
> On Tue, Apr 13, 2021 at 02:29:48PM -0500, Zebediah Figura (she/her) wrote:
>> On 4/13/21 2:03 PM, Arkadiusz Hiler wrote:
>>> On Tue, Apr 13, 2021 at 10:54:12AM -0500, Zebediah Figura (she/her) wrote:
>>>> On 4/13/21 10:31 AM, Arkadiusz Hiler wrote:
>>>>> SDL doesn't use HID devices directly if they don't have SPDRP_DRIVER
>>>>> registry property set. It uses another input method as a fallback instead.
>>>>>
>>>>> Since SDL_Joystick GUIDs are dependant on the input method this can mess
>>>>> with mappings and controller type detection.
>>>>>
>>>>> The change fixes Hades wrongly displaying Xbox prompts with Sony controllers
>>>>> over winebus.sys/bus_udev.c.
>>>>
>>>> It seems that Windows actually installs a null function driver (i.e. one
>>>> with the SPSVCINST_ASSOCSERVICE flag set) for HID devices, so setupapi will
>>>> create the key automatically in this case.
>>>
>>> I was just following what we already do for the display drivers
>>> (winex11.drv, winemac.drv).
>>>
>>> So we would need to add an .inf with an appropriate AddService directive
>>> and install it instead of just adding the driver property pointing
>>> to an empty driver key?
>>
>> Right. On Windows this is c:\windows\input.inf.
>>
>>>> I'm not sure how that interacts with drivers that also want to layer onto
>>>> the HID device; probably it means our SetupDiSelectBestCompatDrv()
>>>> implementation needs to be less stubby.
>>>
>>> I don't understand this comment. I tried to follow the function's usage
>>> + see how we end up creating a HID device out of the underlaying one but
>>> the call chains are long and it gets confusing quickly.
>>>
>>> Looks like SetupDiSelectBestCompatDrv() is used by
>>> SetupDiCallClassInstaller(DIF_SELECTBESTCOMPATDRV) and that in turn is
>>> used by ntoskrnl.exe with each function when adding a new PNP device.
>>>
>>> It seems that whenever a specific driver (one of the implemenations in
>>> winebus.sys) detects a new hardware it creates a device on it's own bus
>>> (e.g. HIDRAW for udev) and then calls IoInvalidateDeviceRelations()
>>>
>>> IoInvalidateDeviceRelations() -> handle_bus_relations() ->
>>> enumerate_new_device() -> install_device_driver() ->
>>> SetupDiCallClassInstaller(DIF_REGISTER_COINSTLLERS) ->
>>> SetupDiRegisterCoDeviceInstllers() -> create_driver_key().
>>>
>>> Which installs the winehid.ini for the device on the underlaying HIDRAW bus.
>>>
>>> Then enumerate_new_device() ends up calling start_device() which creates
>>> HID device by invoking hidclass.sys/pnp.c:PNP_AddDevice().
>>>
>>> Right now hidclass.sys (just like the display drivers) seems to be in
>>> charge of its own SetupAPI entries.
>>>
>>> How exactly does SetupDiSelectBestCompatDrv() tie into that?
>>>
>>
>> It is long and complicated ;-)
>>
>> The basic point of .inf files is to detect a layered driver for a device.
>> input.inf contains an section that says "the layered driver for this device
>> is no driver at all", which isn't the same as a driver not being present.
>> But it's possible for a third-party driver to layer on top of HID (or even a
>> first-party driver, such as mouhid.sys), in which case that driver *should*
>> take precedence.
>>
>> SetupDiSelectBestCompatDrv() is supposed to pick the "best" driver.
>> According to [1] it should try to match hardware IDs in order, so that a
>> driver targeting a specific hardware ID "HID\VID_1234&PID_5678&MI_00" is
>> selected instead of a driver targeting a less specific "HID_DEVICE" ID.
>>
>> DIF_SELECTBESTCOMPATDRV is done by UpdateDriverForPlugAndPlayDevices(), or
>> the ntoskrnl equivalent install_device_driver(). Basically the order goes:
>>
>> 1. Call SetupDiBuildDriverInfoList(), which scans all installed .inf files
>> for matching drivers and stores them in a list.
>> 2. Call SetupDiCallClassInstaller(DIF_SELECTBESTCOMPATDRV), which picks the
>> best driver. In Wine this currently picks the first driver, since we don't
>> really expect there to be more than one. We should probably print an
>> explicit FIXME instead of a WARN if there are.
>> 3. Call SetupDiCallClassInstaller with other DIF_* values, these populate
>> various registry keys and copy files, etc.
>>
>> Honestly, I think with the aforementioned FIXME, it's probably not necessary
>> to improve SetupDiSelectBestCompatDrv() yet. On the other hand, it probably
>> wouldn't be that hard.
>>
>> [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifier-score--windows-vista-and-later-
> 
> Windows driver world is completely new to me but I thing I start to
> understand the control flow thanks to your explanations and reading
> through MSDN.
> 
> AFAIU:
> 
> 1. winebus.sys does:
>     bus_create_hid_device("HIDRAW") which adds it to the pnp devset
>     followed by IoInvalidateDeviceRelations(bus_pdo);
> 
> 2. IoInvalidateDeviceRelations(bus_pdo) does
>     IRP_MN_QUERY_DEVICE_RELATIONS/BusRelations on the winebus.sys
>     through the bus_pdo which gets all the devices from the pnp devset.
>     Then it checks what's a new child and does enumerate_new_device(child).
>     Then it checks for what was removed and calls remove_device(child).
>      
> 3. enumerate_new_device(child) does install_device_driver() that you
>     have explained above (thanks again!), which picks up winehid.inf, as
>     it has the entry for HIDRAW.
> 
> 4. it then calls start_device() which does AddDevice() on the function
>     driver selected using SPDRP_SERVICE, which is winehid as set by
>     winehid.inf.
> 
> 5. winehid is registered as HID Minidriver with hidclass.sys, which
>     overrides AddDevice with PNP_AddDevice, so we end up calling it.
> 
> 
> Now, I think we want to introduce hidclass.inf that would be installed for
> HID devices. The questions is what is the correct way to do this. I see
> two options:
> 
> 1. Turn hidclass.sys into something similar to winebus.sys - it would
>     then call IoInvalidateDeviceRelations() and install the NULL driver.
> 
> 2. Keep it as is but call SetupDiCreateDevRegKeyW() with InfHandle and
>     InfSectionName (I am not 100% sure it does what we want it to do).
> 
> 
> I assume it's #1, but there may be a third, correct option :-P
> 

The answer is broadly #1, assuming I understand you correctly, and I 
have patches to do this queued. We should be creating devices the normal 
PnP way in hidclass, not calling into setupapi directly.