[PATCH 1/5] ntoskrnl: Implement IoSetDevicePropertyData().

[Rémi Bernon]

On 4/26/21 6:28 PM, Zebediah Figura (she/her) wrote:
> On 4/26/21 5:37 AM, Rémi Bernon wrote:
>> From: Arkadiusz Hiler <ahiler at codeweavers.com>
>>
>> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
>> ---
>>
>> In PATCH 1, the return GetLastError() look wrong to me but the same is
>> done in IoGetDeviceProperty so I kept them. I guess we would need to
>> convert the setupapi error back to NTSTATUS, or implement the higher
>> level setupapi on top of these lower level primitives intead.
> 
> It's maybe not exactly ideal, but it's also not clear how to map 
> setupapi errors to NTSTATUS, and moreover it probably doesn't matter 
> anyway (we don't expect those functions to fail).
> 
> I don't think we can implement setupapi functions on top of ntoskrnl 
> ones; that crosses the kernel boundary (and there's no clear syscall 
> interface for it). Probably on Windows the code is just duplicated.
> 
> FWIW, we have a PnP test driver now, so it's actually possible to write 
> tests for this function.
> 
>>
>> In PATCH 2, I moved the error case before IoInvalidateDeviceRelations,
>> because I don't know what the call is actually expecting. Maybe it was
>> unnecessary.
> 
> I think it's correct as-is to move it—as soon as we call 
> IoInvalidateDeviceRelations() the device can be exposed.
> 
> Actually, to be completely correct (i.e. if we were running this driver 
> in Windows), I think we should treat it as possible to receive 
> IRP_MN_QUERY_DEVICE_RELATIONS at any time. What we probably should be 
> doing is to initialize everything necessary *before* the statement 
> "fdo_ext->u.fdo.child_pdo = child_pdo", and then everything else at 
> IRP_MN_START_DEVICE (on the child PDO).
> 

It's actually causing IoSetDevicePropertyData to fail the first time a 
device is detected (so on prefix creation for both mouse and keyboard 
devices), as setupapi doesn't know about it until we call 
IoInvalidateDeviceRelations.

Note however, although it fails to set the property, as 
SetupDiDestroyDeviceInfoList clears the last error, it still returns 
success, and the device initialization continues.

I think it may be better to move it after, as it's less likely that 
anything happens in between, while missing the property is going to make 
rawinput device enumeration skip the device. But it's just a workaround.
-- 
Rémi Bernon <rbernon at codeweavers.com>