TailLight: Replace timer for clearing the tail-light with a PnP-trigg…

…ered work item

Done to fix #44.

TailLight/SetBlack.h

@@ -0,0 +1,8 @@
+#pragma once
+
+struct SET_BLACK_WORKITEM_CONTEXT {
+    UNICODE_STRING symLink;
+};
+WDF_DECLARE_CONTEXT_TYPE(SET_BLACK_WORKITEM_CONTEXT);
+
+NTSTATUS CreateWorkItemForIoTargetOpenDevice(WDFDEVICE device, CONST UNICODE_STRING& symLink);

TailLight/SetTaillightBlack.cpp

@@ -0,0 +1,189 @@
+#include "driver.h"
+#include <Hidport.h>
+#include "SetBlack.h"
+
+EVT_WDF_REQUEST_COMPLETION_ROUTINE  SetBlackCompletionRoutine;
+EVT_WDF_WORKITEM                    SetBlackWorkItem;
+
+
+/*++
+Routine Description:
+    Creates a WDF workitem to do the SetBlack() call after the driver
+    stack has initialized.
+
+Arguments:
+    Device - Handle to a pre-allocated WDF work item.
+
+Requirements:
+    Must be synchronized to the device.
+--*/
+NTSTATUS CreateWorkItemForIoTargetOpenDevice(WDFDEVICE device, CONST UNICODE_STRING& symLink)
+{    
+    WDFWORKITEM hWorkItem = 0;
+    {
+        WDF_OBJECT_ATTRIBUTES workItemAttributes = {};
+        WDF_OBJECT_ATTRIBUTES_INIT(&workItemAttributes);
+        WDF_OBJECT_ATTRIBUTES_INIT_CONTEXT_TYPE(&workItemAttributes, SET_BLACK_WORKITEM_CONTEXT);
+        workItemAttributes.ParentObject = device;
+
+        DEVICE_CONTEXT* pDeviceContext = WdfObjectGet_DEVICE_CONTEXT(device);
+
+        // It's possible to get called twice. Please refer to 
+        // https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-ioregisterplugplaynotification under "Remarks"
+        if ((!pDeviceContext) || pDeviceContext->SetBlackSuccess)
+            return STATUS_SUCCESS;
+
+        WDF_WORKITEM_CONFIG workItemConfig = {};
+        WDF_WORKITEM_CONFIG_INIT(&workItemConfig, SetBlackWorkItem);
+
+        NTSTATUS status = WdfWorkItemCreate(&workItemConfig, &workItemAttributes, &hWorkItem);
+        if (!NT_SUCCESS(status)) {
+            KdPrint(("TailLight: Workitem creation failure 0x%x\n", status));
+            return status; // Maybe better luck next time.
+        }
+
+        SET_BLACK_WORKITEM_CONTEXT* workItemContext = WdfObjectGet_SET_BLACK_WORKITEM_CONTEXT(hWorkItem);
+        workItemContext->symLink = symLink;
+    }
+
+    WdfWorkItemEnqueue(hWorkItem);
+
+    return STATUS_SUCCESS;
+}
+
+void SetBlackCompletionRoutine(_In_ WDFREQUEST Request, _In_ WDFIOTARGET Target, _In_ PWDF_REQUEST_COMPLETION_PARAMS Params, _In_ WDFCONTEXT Context)
+{
+    UNREFERENCED_PARAMETER(Target);
+    UNREFERENCED_PARAMETER(Params);
+    UNREFERENCED_PARAMETER(Context);
+
+    NTSTATUS status = WdfRequestGetStatus(Request);
+    KdPrint(("TailLight: %s WdfRequestSend status: 0x%x\n", __func__, status));
+
+    WdfObjectDelete(Request);
+}
+
+
+/*++
+Routine Description:
+    Sets the taillight to black using an asynchronous request send. This
+    code is called from multiple system worker threads and is thus
+    multithreaded.
+
+Arguments:
+    symLink - an opaque name representing a symbolic link.
+--*/
+NTSTATUS SetBlackAsync(WDFDEVICE device, CONST UNICODE_STRING& symLink) {
+    NTSTATUS         status = STATUS_UNSUCCESSFUL;
+
+    WDFIOTARGET_Wrap target;
+    {
+        WDF_OBJECT_ATTRIBUTES attributes = {};
+        WDF_OBJECT_ATTRIBUTES_INIT(&attributes);
+
+        status = WdfIoTargetCreate(device, &attributes, &target);
+        if (!NT_SUCCESS(status)) {
+            KdPrint(("TailLight: 0x%x while creating I/O target from worker thread\n", status));
+            return status;
+        }
+
+        // We're warned not to process a symlink path name returned from 
+        // IoRegisterDeviceInterface so we need to use a unique device property
+        //  - the device object name - to see if we can send the IRP down with
+        //  minimal bus traffic.
+        WDF_IO_TARGET_OPEN_PARAMS openParams = {};
+        WDF_IO_TARGET_OPEN_PARAMS_INIT_OPEN_BY_NAME(&openParams, &symLink, STANDARD_RIGHTS_ALL);
+        openParams.ShareAccess = FILE_SHARE_WRITE | FILE_SHARE_READ;
+
+        status = WdfIoTargetOpen(target, &openParams);
+        if (!NT_SUCCESS(status)) {
+            KdPrint(("TailLight: 0x%x while opening the I/O target from worker thread\n", status));
+            return status;
+        }
+    }
+
+    {
+        // The PDO name will be attached to the IO target and thus deleted at the end of the function.
+        UNICODE_STRING theirPDOName = GetTargetPropertyString(target, DevicePropertyPhysicalDeviceObjectName);
+
+        if (theirPDOName.MaximumLength > 0) {
+            DEVICE_CONTEXT* pDeviceContext = WdfObjectGet_DEVICE_CONTEXT(device);
+            if (!RtlEqualUnicodeString(&pDeviceContext->PdoName, &theirPDOName, TRUE))
+                return STATUS_NOT_FOUND;
+        }
+    }
+
+    WDFREQUEST request = NULL;
+    status = WdfRequestCreate(WDF_NO_OBJECT_ATTRIBUTES, target, &request);
+    if (!NT_SUCCESS(status)) {
+        KdPrint(("TailLight: WdfRequestCreate failed 0x%x\n", status));
+        return status;
+    }
+
+    WdfRequestSetCompletionRoutine(request, SetBlackCompletionRoutine, WDF_NO_CONTEXT);
+
+    WDFMEMORY InBuffer = 0;
+    {
+        // Set up a WDF memory object for the IOCTL request
+        WDF_OBJECT_ATTRIBUTES mem_attrib = {};
+        WDF_OBJECT_ATTRIBUTES_INIT(&mem_attrib);
+        mem_attrib.ParentObject = request; // auto-delete with request
+
+        TailLightReport* pInBuffer = nullptr;
+        status = WdfMemoryCreate(&mem_attrib, NonPagedPoolNx, POOL_TAG, sizeof(TailLightReport), &InBuffer, (void**)&pInBuffer);
+        if (!NT_SUCCESS(status)) {
+            KdPrint(("TailLight: WdfMemoryCreate failed: 0x%x\n", status));
+            WdfObjectDelete(request);
+            request = 0;
+            return status;
+        }
+
+        TailLightReport  report = {};
+        report.Blue = 0x0;
+        report.Green = 0x0;
+        report.Red = 0x0;
+
+        *pInBuffer = report;
+    }
+
+    // Format the request as write operation
+    status = WdfIoTargetFormatRequestForIoctl(target, request, IOCTL_HID_SET_FEATURE, InBuffer, NULL, 0, 0);
+    if (!NT_SUCCESS(status)) {
+        KdPrint(("TailLight: WdfIoTargetFormatRequestForIoctl failed: 0x%x\n", status));
+        return status;
+    }
+
+    if (!WdfRequestSend(request, target, WDF_NO_SEND_OPTIONS)) {
+        WdfObjectDelete(request);
+        status = STATUS_UNSUCCESSFUL;
+    }
+
+    return status;
+}
+
+/*++
+Routine Description:
+    Creates a WDF workitem to do the SetBlack() call after the driver
+    stack has initialized.
+
+Arguments:
+    workItem - Handle to a pre-allocated WDF work item.
+--*/
+VOID SetBlackWorkItem(WDFWORKITEM workItem)
+{
+    WDFDEVICE device = static_cast<WDFDEVICE>(WdfWorkItemGetParentObject(workItem));
+    SET_BLACK_WORKITEM_CONTEXT* workItemContext = WdfObjectGet_SET_BLACK_WORKITEM_CONTEXT(workItem);
+
+    NTSTATUS status = SetBlackAsync(device, workItemContext->symLink);
+    if (!NT_SUCCESS(status)) {
+        KdPrint(("TailLight: SetBlackAsync failed: 0x%x\n", status));
+        // don't return yet
+    }
+
+    if (NT_SUCCESS(status)) {
+        DEVICE_CONTEXT* pDeviceContext = WdfObjectGet_DEVICE_CONTEXT(device);
+        InterlockedExchange(&pDeviceContext->SetBlackSuccess, TRUE);
+    }
+
+    WdfObjectDelete(workItem);
+}

TailLight/TailLight.vcxproj

@@ -137,6 +137,7 @@ copy "$(WDKBinRoot)\$(Platform)\certmgr.exe" $(PackageDir)</Command>
     <ClCompile Include="device.cpp" />
     <ClCompile Include="driver.cpp" />
     <ClCompile Include="eventlog.cpp" />
+    <ClCompile Include="SetTaillightBlack.cpp" />
     <ClCompile Include="vfeature.cpp" />
     <ClCompile Include="wmi.cpp" />
     <ResourceCompile Include="module.rc" />
@@ -150,6 +151,7 @@ copy "$(WDKBinRoot)\$(Platform)\certmgr.exe" $(PackageDir)</Command>
     <ClInclude Include="device.h" />
     <ClInclude Include="driver.h" />
     <ClInclude Include="eventlog.h" />
+    <ClInclude Include="SetBlack.h" />
     <ClInclude Include="TailLight.h" />
     <ClInclude Include="vfeature.h" />
     <ClInclude Include="wmi.h" />

TailLight/device.cpp

@@ -1,46 +1,61 @@
 #include "driver.h"
 #include <Hidport.h>
+#include "SetBlack.h"
 
 EVT_WDF_IO_QUEUE_IO_DEVICE_CONTROL EvtIoDeviceControlFilter;
 
 
-VOID EvtSetBlackTimer(_In_ WDFTIMER  Timer) {
-    KdPrint(("TailLight: EvtSetBlackTimer begin\n"));
+/*++
+Routine Description:
+    Filters out HID class device interface arrivals.
 
-    WDFDEVICE device = (WDFDEVICE)WdfTimerGetParentObject(Timer);
-    NT_ASSERTMSG("EvtSetBlackTimer device NULL\n", device);
+Arguments:
+    PVOID - One of several possible notification structures. All we care about
+            for this implementation is DEVICE_INTERFACE_CHANGE_NOTIFICATION.
 
-    NTSTATUS status = SetFeatureColor(device, 0);
-    if (!NT_SUCCESS(status)) {
-        KdPrint(("TailLight: EvtSetBlackTimer failure NTSTATUS=0x%x\n", status));
-        return;
+    PVOID - The WDFDEVICE that we received from EvtDriverDeviceAdd.
+--*/
+NTSTATUS PnpNotifyDeviceInterfaceChange(_In_ PVOID pvNotificationStructure, _Inout_opt_ PVOID pvContext)
+{
+    //KdPrint(("TailLight: PnpNotifyDeviceInterfaceChange enter\n"));
+    NT_ASSERTMSG("WDFDEVICE not passed in!", pvContext);
+
+    if (!pvNotificationStructure)
+        return STATUS_SUCCESS;
+
+    auto* pDevInterface = (DEVICE_INTERFACE_CHANGE_NOTIFICATION*)pvNotificationStructure;
+
+    ASSERT(IsEqualGUID(pDevInterface->InterfaceClassGuid, GUID_DEVINTERFACE_HID));
+
+    // Assumption: Device will arrive before removal.
+    if (IsEqualGUID(pDevInterface->Event, GUID_DEVICE_INTERFACE_ARRIVAL)) {
+        // Opening a device may trigger PnP operations. Ensure that either a
+        // timer or a work item is used when opening up a device.
+        // Refer to page 356 of "Programming The Microsoft Windows Driver
+        // Model", 2nd edition by Walter Oney and IoGetDeviceObjectPointer.
+        // 
+        // Also note that eventually several system threads will be running
+        // in parallel. As a PnP system thread must not block, serializing
+        // would require a queue.
+        return CreateWorkItemForIoTargetOpenDevice((WDFDEVICE)pvContext, *pDevInterface->SymbolicLinkName);
     }
 
-    KdPrint(("TailLight: EvtSetBlackTimer end\n"));
+    return STATUS_SUCCESS;
 }
 
 NTSTATUS EvtSelfManagedIoInit(WDFDEVICE device) {
-    // Initialize tail-light to black to have control over HW state
-    WDF_TIMER_CONFIG timerCfg = {};
-    WDF_TIMER_CONFIG_INIT(&timerCfg, EvtSetBlackTimer);
-
-    WDF_OBJECT_ATTRIBUTES attribs = {};
-    WDF_OBJECT_ATTRIBUTES_INIT(&attribs);
-    attribs.ParentObject = device;
-    attribs.ExecutionLevel = WdfExecutionLevelPassive; // required to access HID functions
-
-    WDFTIMER timer = nullptr;
-    NTSTATUS status = WdfTimerCreate(&timerCfg, &attribs, &timer);
-    if (!NT_SUCCESS(status)) {
-        KdPrint(("WdfTimerCreate failed 0x%x\n", status));
-        return status;
-    }
-
-    status = WdfTimerStart(timer, 0); // no wait
-    if (!NT_SUCCESS(status)) {
-        KdPrint(("WdfTimerStart failed 0x%x\n", status));
-        return status;
-    }
+    WDFDRIVER driver = WdfDeviceGetDriver(device);
+    DEVICE_CONTEXT* pDeviceContext = WdfObjectGet_DEVICE_CONTEXT(device);
+
+    NTSTATUS status = IoRegisterPlugPlayNotification(
+        EventCategoryDeviceInterfaceChange,
+        PNPNOTIFY_DEVICE_INTERFACE_INCLUDE_EXISTING_INTERFACES,
+        (PVOID)&GUID_DEVINTERFACE_HID,
+        WdfDriverWdmGetDriverObject(driver),
+        PnpNotifyDeviceInterfaceChange,
+        (PVOID)device,
+        &pDeviceContext->pnpDevInterfaceChangedHandle
+    );
 
     return status;
 }
@@ -71,6 +86,38 @@ UNICODE_STRING GetTargetPropertyString(WDFIOTARGET target, DEVICE_REGISTRY_PROPE
 }
 
 
+/*++
+Routine Description:
+    Removes the plug and play notification callback if registered.
+    Called near the end of processing an IRP_MN_REMOVE_DEVICE.
+    This work could also be done at EvtDeviceSelfManagedIoCleanup, which
+    comes before this callback. However using the lifetime of the device
+    context means that the PnP notification handle will be around for all users
+    of the device context, while the EvtDeviceSelfManagedIoCleanup would
+    present a small time where the handle is "dangling." In addition,
+    IoRegisterPlugPlayNotification adds a reference to our device object. Thus,
+    if the PnP notification handle is not removed, our driver will not unload
+    and DriverUnload will not be called. Please see page 355 of "Programming
+    The Microsoft Windows Driver Model", 2nd edition, by Walter Oney.
+
+Arguments:
+    WDFOBJECT - Handle to a framework device object from AddDevice
+--*/
+VOID EvtCleanupCallback(_In_ WDFOBJECT object)
+{
+    DEVICE_CONTEXT* pDeviceContext = WdfObjectGet_DEVICE_CONTEXT(object);
+    if (!pDeviceContext->pnpDevInterfaceChangedHandle)
+        return; // not registered
+
+    NTSTATUS status = IoUnregisterPlugPlayNotificationEx(pDeviceContext->pnpDevInterfaceChangedHandle);
+    if (!NT_SUCCESS(status)) {
+        KdPrint(("TailLight: IoUnregisterPlugPlayNotification failed with 0x%x\n", status));
+    }
+
+    pDeviceContext->pnpDevInterfaceChangedHandle = NULL;
+}
+
+
 NTSTATUS EvtDriverDeviceAdd(_In_ WDFDRIVER Driver, _Inout_ PWDFDEVICE_INIT DeviceInit)
 /*++
 Routine Description:
@@ -102,6 +149,7 @@ Routine Description:
         // create device
         WDF_OBJECT_ATTRIBUTES attributes = {};
         WDF_OBJECT_ATTRIBUTES_INIT_CONTEXT_TYPE(&attributes, DEVICE_CONTEXT);
+        attributes.EvtCleanupCallback = EvtCleanupCallback;
 
         NTSTATUS status = WdfDeviceCreate(&DeviceInit, &attributes, &device);
         if (!NT_SUCCESS(status)) {

TailLight/device.h

@@ -3,10 +3,16 @@
 /** Driver-specific struct for storing instance-specific data. */
 struct DEVICE_CONTEXT {
     UNICODE_STRING PdoName;
+    LONG           SetBlackSuccess;
     WDFWMIINSTANCE WmiInstance;
+    PVOID          pnpDevInterfaceChangedHandle;
 };
 WDF_DECLARE_CONTEXT_TYPE(DEVICE_CONTEXT)
 
 WDF_DECLARE_CONTEXT_TYPE(TailLightDeviceInformation)
 
 EVT_WDF_DEVICE_CONTEXT_CLEANUP EvtDeviceContextCleanup;
+
+NTSTATUS PnpNotifyDeviceInterfaceChange(_In_ PVOID NotificationStructure, _Inout_opt_ PVOID Context);
+
+UNICODE_STRING GetTargetPropertyString(WDFIOTARGET target, DEVICE_REGISTRY_PROPERTY DeviceProperty);