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

[Arkadiusz Hiler]

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

-- 
Cheers,
Arek