[janderwald] 47197: [PORTCLS] - Don't request initializing delayed service request as this is the task of the miniport driver - Reimplement the service group object: - Use the initialized timer object when RequestService is called - Fix possible race conditions when adding / removing a service sink by protecting it with a lock - Acquire the service group list lock when executing the shared dpc routine

Author: janderwald
Date: Fri May 14 17:47:00 2010
New Revision: 47197

URL: http://svn.reactos.org/svn/reactos?rev=47197&view=rev
Log:
[PORTCLS]
- Don't request initializing delayed service request as this is the task of the
miniport driver
- Reimplement the service group object:
- Use the initialized timer object when RequestService is called
- Fix possible race conditions when adding / removing a service sink by protecting it with
a lock
- Acquire the service group list lock when executing the shared dpc routine

Modified:
    trunk/reactos/drivers/wdm/audio/backpln/portcls/pin_dmus.cpp
    trunk/reactos/drivers/wdm/audio/backpln/portcls/pin_wavecyclic.cpp
    trunk/reactos/drivers/wdm/audio/backpln/portcls/pin_wavepci.cpp
    trunk/reactos/drivers/wdm/audio/backpln/portcls/service_group.cpp

Modified: trunk/reactos/drivers/wdm/audio/backpln/portcls/pin_dmus.cpp
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/wdm/audio/backpln/…
==============================================================================
--- trunk/reactos/drivers/wdm/audio/backpln/portcls/pin_dmus.cpp [iso-8859-1] (original)
+++ trunk/reactos/drivers/wdm/audio/backpln/portcls/pin_dmus.cpp [iso-8859-1] Fri May 14
17:47:00 2010
@@ -602,7 +602,6 @@
             DPRINT("Failed to add pin to service group\n");
             return Status;
         }
-        m_ServiceGroup->SupportDelayedService();
     }
 
     Status = m_IrpQueue->Init(ConnectDetails, 0, 0, NULL);

Modified: trunk/reactos/drivers/wdm/audio/backpln/portcls/pin_wavecyclic.cpp
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/wdm/audio/backpln/…
==============================================================================
--- trunk/reactos/drivers/wdm/audio/backpln/portcls/pin_wavecyclic.cpp [iso-8859-1]
(original)
+++ trunk/reactos/drivers/wdm/audio/backpln/portcls/pin_wavecyclic.cpp [iso-8859-1] Fri
May 14 17:47:00 2010
@@ -1213,7 +1213,6 @@
         return Status;
     }
 
-    m_ServiceGroup->SupportDelayedService();
     m_Stream->SetState(KSSTATE_STOP);
     m_State = KSSTATE_STOP;
     m_CommonBufferOffset = 0;
@@ -1224,6 +1223,8 @@
     m_Delay = Int32x32To64(10, -10000);
 
     Status = m_Stream->SetNotificationFreq(10, &m_FrameSize);
+    PC_ASSERT(NT_SUCCESS(Status));
+    PC_ASSERT(m_FrameSize);
 
     SilenceBuffer = AllocateItem(NonPagedPool, m_FrameSize, TAG_PORTCLASS);
     if (!SilenceBuffer)

Modified: trunk/reactos/drivers/wdm/audio/backpln/portcls/pin_wavepci.cpp
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/wdm/audio/backpln/…
==============================================================================
--- trunk/reactos/drivers/wdm/audio/backpln/portcls/pin_wavepci.cpp [iso-8859-1]
(original)
+++ trunk/reactos/drivers/wdm/audio/backpln/portcls/pin_wavepci.cpp [iso-8859-1] Fri May
14 17:47:00 2010
@@ -815,7 +815,6 @@
             DPRINT("Failed to add pin to service group\n");
             return Status;
         }
-        m_ServiceGroup->SupportDelayedService();
     }
 
     // delay of 10 milisec

Modified: trunk/reactos/drivers/wdm/audio/backpln/portcls/service_group.cpp
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/wdm/audio/backpln/…
==============================================================================
--- trunk/reactos/drivers/wdm/audio/backpln/portcls/service_group.cpp [iso-8859-1]
(original)
+++ trunk/reactos/drivers/wdm/audio/backpln/portcls/service_group.cpp [iso-8859-1] Fri May
14 17:47:00 2010
@@ -54,12 +54,10 @@
 
     LIST_ENTRY m_ServiceSinkHead;
 
-    BOOL m_Initialized;
-    BOOL m_TimerActive;
+    BOOL m_TimerInitialized;
     KTIMER m_Timer;
     KDPC m_Dpc;
-    KEVENT m_Event;
-    LONG m_ThreadActive;
+    KSPIN_LOCK m_Lock;
 
     friend VOID NTAPI IServiceGroupDpc(IN struct _KDPC  *Dpc, IN PVOID  DeferredContext,
IN PVOID  SystemArgument1, IN PVOID  SystemArgument2);
 
@@ -105,9 +103,16 @@
 
 CServiceGroup::CServiceGroup(IUnknown * OuterUnknown)
 {
+    // initialize dpc
     KeInitializeDpc(&m_Dpc, IServiceGroupDpc, (PVOID)this);
+
+    // set highest importance
     KeSetImportanceDpc(&m_Dpc, HighImportance);
-    KeInitializeEvent(&m_Event, NotificationEvent, FALSE);
+
+    // initialize service group list lock
+    KeInitializeSpinLock(&m_Lock);
+
+    // initialize service group list
     InitializeListHead(&m_ServiceSinkHead);
 }
 
@@ -119,15 +124,34 @@
 
     DPRINT("CServiceGroup::RequestService() Dpc at Level %u\n",
KeGetCurrentIrql());
 
-    if (KeGetCurrentIrql() > DISPATCH_LEVEL)
-    {
-        KeInsertQueueDpc(&m_Dpc, NULL, NULL);
-        return;
-    }
-
-    KeRaiseIrql(DISPATCH_LEVEL, &OldIrql);
-    KeInsertQueueDpc(&m_Dpc, NULL, NULL);
-    KeLowerIrql(OldIrql);
+    if (m_TimerInitialized)
+    {
+        LARGE_INTEGER DueTime;
+
+        // no due time
+        DueTime.QuadPart = 0LL;
+
+        // delayed service requested
+        KeSetTimer(&m_Timer, DueTime, &m_Dpc);
+    }
+    else
+    {
+        // check curent irql
+        if (KeGetCurrentIrql() > DISPATCH_LEVEL)
+        {
+            //insert dpc to queue
+            KeInsertQueueDpc(&m_Dpc, NULL, NULL);
+        }
+        else
+        {
+            // raise irql to dispatch level to make dpc fire immediately
+            KeRaiseIrql(DISPATCH_LEVEL, &OldIrql);
+            // insert dpc to queue
+            KeInsertQueueDpc(&m_Dpc, NULL, NULL);
+            // lower irql to old level
+            KeLowerIrql(OldIrql);
+        }
+    }
 }
 
 //---------------------------------------------------------------
@@ -140,17 +164,32 @@
     IN PSERVICESINK pServiceSink)
 {
     PGROUP_ENTRY Entry;
-
+    KIRQL OldLevel;
+
+    // sanity check
     PC_ASSERT_IRQL_EQUAL(PASSIVE_LEVEL);
 
+    // allocate service sink entry
     Entry = (PGROUP_ENTRY)AllocateItem(NonPagedPool, sizeof(GROUP_ENTRY),
TAG_PORTCLASS);
     if (!Entry)
+    {
+        // out of memory
         return STATUS_INSUFFICIENT_RESOURCES;
-
+    }
+
+    // initialize service sink entry
     Entry->pServiceSink = pServiceSink;
+    // increment reference count
     pServiceSink->AddRef();
 
+    // acquire service group list lock
+    KeAcquireSpinLock(&m_Lock, &OldLevel);
+
+    // insert into service sink list
     InsertTailList(&m_ServiceSinkHead, &Entry->Entry);
+
+    // release service group list lock
+    KeReleaseSpinLock(&m_Lock, OldLevel);
 
     return STATUS_SUCCESS;
 }
@@ -162,22 +201,44 @@
 {
     PLIST_ENTRY CurEntry;
     PGROUP_ENTRY Entry;
-
+    KIRQL OldLevel;
+
+    // sanity check
     PC_ASSERT_IRQL_EQUAL(PASSIVE_LEVEL);
 
+    // acquire service group list lock
+    KeAcquireSpinLock(&m_Lock, &OldLevel);
+
+    // grab first entry
     CurEntry = m_ServiceSinkHead.Flink;
+
+    // loop list until the passed entry is found
     while (CurEntry != &m_ServiceSinkHead)
     {
+        // grab entry
         Entry = CONTAINING_RECORD(CurEntry, GROUP_ENTRY, Entry);
+
+        // check if it matches the passed entry
         if (Entry->pServiceSink == pServiceSink)
         {
+            // remove entry from list
             RemoveEntryList(&Entry->Entry);
+
+            // release service sink reference
             pServiceSink->Release();
+
+            // free service sink entry
             FreeItem(Entry, TAG_PORTCLASS);
-            return;
+
+            // leave loop
+            break;
         }
+        // move to next entry
         CurEntry = CurEntry->Flink;
     }
+
+    // release service group list lock
+    KeReleaseSpinLock(&m_Lock, OldLevel);
 
 }
 
@@ -194,73 +255,40 @@
     PGROUP_ENTRY Entry;
     CServiceGroup * This = (CServiceGroup*)DeferredContext;
 
+    // acquire service group list lock
+    KeAcquireSpinLockAtDpcLevel(&This->m_Lock);
+
+    // grab first entry
     CurEntry = This->m_ServiceSinkHead.Flink;
+
+    // loop the list and call the attached service sink/group
     while (CurEntry != &This->m_ServiceSinkHead)
     {
+        //grab current entry
         Entry = (PGROUP_ENTRY)CONTAINING_RECORD(CurEntry, GROUP_ENTRY, Entry);
+
+        // call service sink/group
         Entry->pServiceSink->RequestService();
+
+        // move to next entry
         CurEntry = CurEntry->Flink;
     }
-}
-
-
-#if 0
-VOID
-NTAPI
-ServiceGroupThread(IN PVOID StartContext)
-{
-    NTSTATUS Status;
-    KWAIT_BLOCK WaitBlockArray[2];
-    PVOID WaitObjects[2];
-    CServiceGroup * This = (CServiceGroup*)StartContext;
-
-    // Set thread state
-    InterlockedIncrement(&This->m_ThreadActive);
-
-    // Setup the wait objects
-    WaitObjects[0] = &m_Timer;
-    WaitObjects[1] = &m_Event;
-
-    do
-    {
-        // Wait on our objects
-        Status = KeWaitForMultipleObjects(2, WaitObjects, WaitAny, Executive, KernelMode,
FALSE, NULL, WaitBlockArray);
-
-        switch(Status)
-        {
-            case STATUS_WAIT_0:
-                IServiceGroupDpc(&This->m_Dpc, (PVOID)This, NULL, NULL);
-                break;
-            case STATUS_WAIT_1:
-                PsTerminateSystemThread(STATUS_SUCCESS);
-                return;
-        }
-    }while(TRUE);
-}
-
-#endif
+
+    // release service group list lock
+    KeReleaseSpinLockFromDpcLevel(&This->m_Lock);
+}
+
 VOID
 NTAPI
 CServiceGroup::SupportDelayedService()
 {
-    //NTSTATUS Status;
-    //HANDLE ThreadHandle;
-
     PC_ASSERT_IRQL(DISPATCH_LEVEL);
 
-    if (m_Initialized)
-        return;
-
-    KeInitializeTimerEx(&m_Timer, NotificationTimer);
-
-#if 0
-    Status = PsCreateSystemThread(&ThreadHandle, THREAD_ALL_ACCESS, NULL, 0, NULL,
ServiceGroupThread, (PVOID)This);
-    if (NT_SUCCESS(Status))
-    {
-        ZwClose(ThreadHandle);
-        m_Initialized = TRUE;
-    }
-#endif
+    // initialize the timer
+    KeInitializeTimer(&m_Timer);
+
+    // use the timer to perform service requests
+    m_TimerInitialized = TRUE;
 }
 
 VOID
@@ -270,17 +298,14 @@
 {
     LARGE_INTEGER DueTime;
 
+    // sanity check
     PC_ASSERT_IRQL(DISPATCH_LEVEL);
+    PC_ASSERT(m_TimerInitialized);
 
     DueTime.QuadPart = ullDelay;
 
-    if (m_Initialized)
-    {
-        if (KeGetCurrentIrql() <= DISPATCH_LEVEL)
-            KeSetTimer(&m_Timer, DueTime, &m_Dpc);
-        else
-            KeInsertQueueDpc(&m_Dpc, NULL, NULL);
-    }
+    // set the timer
+    KeSetTimer(&m_Timer, DueTime, &m_Dpc);
 }
 
 VOID
@@ -288,11 +313,10 @@
 CServiceGroup::CancelDelayedService()
 {
     PC_ASSERT_IRQL(DISPATCH_LEVEL);
-
-    if (m_Initialized)
-    {
-        KeCancelTimer(&m_Timer);
-    }
+    PC_ASSERT(m_TimerInitialized);
+
+    // cancel the timer
+    KeCancelTimer(&m_Timer);
 }
 
 NTSTATUS
@@ -303,19 +327,31 @@
 {
     CServiceGroup * This;
     NTSTATUS Status;
+
     DPRINT("PcNewServiceGroup entered\n");
 
+    //FIXME support aggregation
+    PC_ASSERT(OuterUnknown == NULL);
+
+    // allocate a service group object
     This = new(NonPagedPool, TAG_PORTCLASS)CServiceGroup(OuterUnknown);
+
     if (!This)
+    {
+        // out of memory
         return STATUS_INSUFFICIENT_RESOURCES;
-
+    }
+
+    // request IServiceSink interface
     Status = This->QueryInterface(IID_IServiceSink, (PVOID*)OutServiceGroup);
 
     if (!NT_SUCCESS(Status))
     {
+        // failed to acquire service sink interface
         delete This;
         return Status;
     }
 
+    // done
     return Status;
 }