From: Paul Durrant Date: Wed, 5 Aug 2015 09:19:04 +0000 (+0100) Subject: Revert commit 632cc904 "Remove PDO set/is-missing logic from... X-Git-Tag: 8.1.0-rc1~3 X-Git-Url: http://xenbits.xensource.com/gitweb?a=commitdiff_plain;h=ed3c9abd34faed88ae8bc05ff5e96de2df1458ae;p=pvdrivers%2Fwin%2Fxenbus.git Revert commit 632cc904 "Remove PDO set/is-missing logic from... ... XENFILT" and re-work PnP code again. In WHQL testing I suspect the removal and re-creation of filter objects when IRP_MN_REMOVE_DEVICE is processed in the case that underlying PDO is not actually going away may cause problems. By reverting 632cc904 this bouncing is prevented but the code needs more work to fix the hanging object references from filtDO to PDO that were the motivation for 632cc904 in the first place. Signed-off-by: Paul Durrant --- diff --git a/src/xenbus/fdo.c b/src/xenbus/fdo.c index 92634cc..68d6e76 100644 --- a/src/xenbus/fdo.c +++ b/src/xenbus/fdo.c @@ -1074,41 +1074,43 @@ FdoEnumerate( while (ListEntry != &Fdo->List) { PLIST_ENTRY Next = ListEntry->Flink; PXENBUS_DX Dx = CONTAINING_RECORD(ListEntry, XENBUS_DX, ListEntry); - PCHAR Name = Dx->Name; PXENBUS_PDO Pdo = Dx->Pdo; - BOOLEAN Missing; - Name = PdoGetName(Pdo); - Missing = TRUE; + if (!PdoIsMissing(Pdo) && PdoGetDevicePnpState(Pdo) != Deleted) { + PCHAR Name; + BOOLEAN Missing; - // If the PDO already exists ans its name is in the class list then - // we don't want to remove it. - for (Index = 0; Classes[Index].Buffer != NULL; Index++) { - PANSI_STRING Class = &Classes[Index]; + Name = PdoGetName(Pdo); + Missing = TRUE; - if (Class->Length == 0) - continue; + // If the PDO already exists and its name is in the class list + // then we don't want to remove it. + for (Index = 0; Classes[Index].Buffer != NULL; Index++) { + PANSI_STRING Class = &Classes[Index]; - if (strcmp(Name, Class->Buffer) == 0) { - Missing = FALSE; - Class->Length = 0; // avoid duplication - break; + if (Class->Length == 0) + continue; + + if (strcmp(Name, Class->Buffer) == 0) { + Missing = FALSE; + Class->Length = 0; // avoid duplication + break; + } } - } - if (Missing && - !PdoIsMissing(Pdo) && - PdoGetDevicePnpState(Pdo) != Deleted) { - PdoSetMissing(Pdo, "device disappeared"); - - // If the PDO has not yet been enumerated then we can go ahead - // and mark it as deleted, otherwise we need to notify PnP manager and - // wait for the REMOVE_DEVICE IRP. - if (PdoGetDevicePnpState(Pdo) == Present) { - PdoSetDevicePnpState(Pdo, Deleted); - PdoDestroy(Pdo); - } else { - NeedInvalidate = TRUE; + if (Missing) { + PdoSetMissing(Pdo, "device disappeared"); + + // If the PDO has not yet been enumerated then we can + // go ahead and mark it as deleted, otherwise we need + // to notify PnP manager and wait for the REMOVE_DEVICE + // IRP. + if (PdoGetDevicePnpState(Pdo) == Present) { + PdoSetDevicePnpState(Pdo, Deleted); + PdoDestroy(Pdo); + } else { + NeedInvalidate = TRUE; + } } } @@ -3624,10 +3626,6 @@ FdoQueryDeviceRelations( ASSERT3U(Dx->Type, ==, PHYSICAL_DEVICE_OBJECT); - if (PdoGetDevicePnpState(Pdo) == Deleted && - !PdoIsMissing(Pdo)) - PdoSetMissing(Pdo, "surprise remove"); - if (PdoIsMissing(Pdo)) continue; @@ -3655,17 +3653,19 @@ FdoQueryDeviceRelations( __FdoAcquireMutex(Fdo); - for (ListEntry = Fdo->List.Flink; - ListEntry != &Fdo->List; - ListEntry = ListEntry->Flink) { + ListEntry = Fdo->List.Flink; + while (ListEntry != &Fdo->List) { PXENBUS_DX Dx = CONTAINING_RECORD(ListEntry, XENBUS_DX, ListEntry); PXENBUS_PDO Pdo = Dx->Pdo; + PLIST_ENTRY Next = ListEntry->Flink; ASSERT3U(Dx->Type, ==, PHYSICAL_DEVICE_OBJECT); if (PdoGetDevicePnpState(Pdo) == Deleted && PdoIsMissing(Pdo)) PdoDestroy(Pdo); + + ListEntry = Next; } __FdoReleaseMutex(Fdo); diff --git a/src/xenbus/pdo.c b/src/xenbus/pdo.c index e85732e..88c82a7 100644 --- a/src/xenbus/pdo.c +++ b/src/xenbus/pdo.c @@ -848,6 +848,7 @@ PdoRemoveDevice( ) { PXENBUS_FDO Fdo = __PdoGetFdo(Pdo); + BOOLEAN NeedInvalidate; NTSTATUS status; if (__PdoGetDevicePowerState(Pdo) != PowerDeviceD0) @@ -856,24 +857,29 @@ PdoRemoveDevice( PdoD0ToD3(Pdo); done: + NeedInvalidate = FALSE; + FdoAcquireMutex(Fdo); - if (__PdoIsMissing(Pdo) || - __PdoGetDevicePnpState(Pdo) == SurpriseRemovePending) + if (__PdoIsMissing(Pdo)) { + DEVICE_PNP_STATE State = __PdoGetDevicePnpState(Pdo); + __PdoSetDevicePnpState(Pdo, Deleted); - else - __PdoSetDevicePnpState(Pdo, Enumerated); - if (__PdoIsMissing(Pdo)) { - if (__PdoGetDevicePnpState(Pdo) == Deleted) + if (State == SurpriseRemovePending) PdoDestroy(Pdo); else - IoInvalidateDeviceRelations(FdoGetPhysicalDeviceObject(Fdo), - BusRelations); + NeedInvalidate = TRUE; + } else { + __PdoSetDevicePnpState(Pdo, Enumerated); } FdoReleaseMutex(Fdo); + if (NeedInvalidate) + IoInvalidateDeviceRelations(FdoGetPhysicalDeviceObject(Fdo), + BusRelations); + status = STATUS_SUCCESS; Irp->IoStatus.Status = status; @@ -1559,10 +1565,11 @@ PdoEject( __PdoSetDevicePnpState(Pdo, Deleted); __PdoSetMissing(Pdo, "device ejected"); - PdoDestroy(Pdo); - FdoReleaseMutex(Fdo); + IoInvalidateDeviceRelations(FdoGetPhysicalDeviceObject(Fdo), + BusRelations); + status = STATUS_SUCCESS; Irp->IoStatus.Status = status; diff --git a/src/xenfilt/fdo.c b/src/xenfilt/fdo.c index 0c48090..94e285b 100644 --- a/src/xenfilt/fdo.c +++ b/src/xenfilt/fdo.c @@ -126,6 +126,16 @@ __FdoGetDevicePnpState( return Dx->DevicePnpState; } +static FORCEINLINE DEVICE_PNP_STATE +__FdoGetPreviousDevicePnpState( + IN PXENFILT_FDO Fdo + ) +{ + PXENFILT_DX Dx = Fdo->Dx; + + return Dx->PreviousDevicePnpState; +} + static FORCEINLINE VOID __FdoSetDevicePowerState( IN PXENFILT_FDO Fdo, @@ -542,10 +552,32 @@ FdoEnumerate( PXENFILT_DX Dx = CONTAINING_RECORD(ListEntry, XENFILT_DX, ListEntry); PXENFILT_PDO Pdo = Dx->Pdo; - for (Index = 0; Index < Count; Index++) { - if (PdoGetPhysicalDeviceObject(Pdo) == PhysicalDeviceObject[Index]) { - PhysicalDeviceObject[Index] = NULL; // avoid duplication - break; + if (!PdoIsMissing(Pdo) && PdoGetDevicePnpState(Pdo) != Deleted) { + BOOLEAN Missing; + + Missing = TRUE; + + for (Index = 0; Index < Count; Index++) { + if (PdoGetPhysicalDeviceObject(Pdo) == PhysicalDeviceObject[Index]) { + Missing = FALSE; +#pragma prefast(suppress:6387) // PhysicalDeviceObject[Index] could be NULL + ObDereferenceObject(PhysicalDeviceObject[Index]); + PhysicalDeviceObject[Index] = NULL; // avoid duplication + break; + } + } + + if (Missing) { + PdoSetMissing(Pdo, "device disappeared"); + + // If the PDO has not yet been enumerated then we can + // go ahead and mark it as deleted, otherwise we need + // to notify PnP manager and wait for the REMOVE_DEVICE + // IRP. + if (PdoGetDevicePnpState(Pdo) == Present) { + PdoSetDevicePnpState(Pdo, Deleted); + PdoDestroy(Pdo); + } } } @@ -555,8 +587,10 @@ FdoEnumerate( // Walk the list and create PDO filters for any new devices for (Index = 0; Index < Count; Index++) { #pragma warning(suppress:6385) // Reading invalid data from 'PhysicalDeviceObject' - if (PhysicalDeviceObject[Index] != NULL) + if (PhysicalDeviceObject[Index] != NULL) { (VOID) FdoAddDevice(Fdo, PhysicalDeviceObject[Index]); + ObDereferenceObject(PhysicalDeviceObject[Index]); + } } __FdoSetEnumerated(Fdo); @@ -807,19 +841,22 @@ FdoStopDevice( IN PIRP Irp ) { - POWER_STATE PowerState; NTSTATUS status; status = IoAcquireRemoveLock(&Fdo->Dx->RemoveLock, Irp); if (!NT_SUCCESS(status)) goto fail1; - PowerState.DeviceState = PowerDeviceD3; - PoSetPowerState(Fdo->Dx->DeviceObject, - DevicePowerState, - PowerState); + if (__FdoGetDevicePowerState(Fdo) == PowerDeviceD0) { + POWER_STATE PowerState; - __FdoSetDevicePowerState(Fdo, PowerDeviceD3); + PowerState.DeviceState = PowerDeviceD3; + PoSetPowerState(Fdo->Dx->DeviceObject, + DevicePowerState, + PowerState); + + __FdoSetDevicePowerState(Fdo, PowerDeviceD3); + } __FdoSetDevicePnpState(Fdo, Stopped); Irp->IoStatus.Status = STATUS_SUCCESS; @@ -1014,20 +1051,49 @@ FdoRemoveDevice( IN PIRP Irp ) { - POWER_STATE PowerState; + PLIST_ENTRY ListEntry; NTSTATUS status; status = IoAcquireRemoveLock(&Fdo->Dx->RemoveLock, Irp); if (!NT_SUCCESS(status)) goto fail1; - PowerState.DeviceState = PowerDeviceD3; - PoSetPowerState(Fdo->Dx->DeviceObject, - DevicePowerState, - PowerState); + if (__FdoGetPreviousDevicePnpState(Fdo) != Started) + goto done; + + __FdoAcquireMutex(Fdo); + + ListEntry = Fdo->List.Flink; + while (ListEntry != &Fdo->List) { + PLIST_ENTRY Flink = ListEntry->Flink; + PXENFILT_DX Dx = CONTAINING_RECORD(ListEntry, XENFILT_DX, ListEntry); + PXENFILT_PDO Pdo = Dx->Pdo; + + ASSERT3U(Dx->Type, ==, PHYSICAL_DEVICE_OBJECT); + + if (!PdoIsMissing(Pdo)) + PdoSetMissing(Pdo, "FDO removed"); - __FdoSetDevicePowerState(Fdo, PowerDeviceD3); + PdoSetDevicePnpState(Pdo, Deleted); + PdoDestroy(Pdo); + ListEntry = Flink; + } + + __FdoReleaseMutex(Fdo); + + if (__FdoGetDevicePowerState(Fdo) == PowerDeviceD0) { + POWER_STATE PowerState; + + PowerState.DeviceState = PowerDeviceD3; + PoSetPowerState(Fdo->Dx->DeviceObject, + DevicePowerState, + PowerState); + + __FdoSetDevicePowerState(Fdo, PowerDeviceD3); + } + +done: __FdoSetDevicePnpState(Fdo, Deleted); IoReleaseRemoveLockAndWait(&Fdo->Dx->RemoveLock, Irp); @@ -1083,8 +1149,8 @@ FdoQueryDeviceRelations( KEVENT Event; PIO_STACK_LOCATION StackLocation; ULONG Size; - PDEVICE_RELATIONS LowerRelations; PDEVICE_RELATIONS Relations; + PLIST_ENTRY ListEntry; XENFILT_FILTER_STATE State; ULONG Count; NTSTATUS status; @@ -1122,16 +1188,24 @@ FdoQueryDeviceRelations( if (StackLocation->Parameters.QueryDeviceRelations.Type != BusRelations) goto done; - LowerRelations = (PDEVICE_RELATIONS)Irp->IoStatus.Information; - __FdoAcquireMutex(Fdo); - if (LowerRelations->Count != 0) - FdoEnumerate(Fdo, LowerRelations); + Relations = (PDEVICE_RELATIONS)Irp->IoStatus.Information; + + if (Relations->Count != 0) + FdoEnumerate(Fdo, Relations); + + ExFreePool(Relations); State = DriverGetFilterState(); + Count = 0; - Count = (State == XENFILT_FILTER_DISABLED) ? LowerRelations->Count : 0; + if (State == XENFILT_FILTER_DISABLED) { + for (ListEntry = Fdo->List.Flink; + ListEntry != &Fdo->List; + ListEntry = ListEntry->Flink) + Count++; + } Size = FIELD_OFFSET(DEVICE_RELATIONS, Objects) + (sizeof (PDEVICE_OBJECT) * __max(Count, 1)); @@ -1145,40 +1219,45 @@ FdoQueryDeviceRelations( if (State == XENFILT_FILTER_DISABLED) { PLIST_ENTRY ListEntry; - for (ListEntry = Fdo->List.Flink; - ListEntry != &Fdo->List; - ListEntry = ListEntry->Flink) { + ListEntry = Fdo->List.Flink; + while (ListEntry != &Fdo->List) { PXENFILT_DX Dx = CONTAINING_RECORD(ListEntry, XENFILT_DX, ListEntry); PXENFILT_PDO Pdo = Dx->Pdo; + PLIST_ENTRY Next = ListEntry->Flink; ASSERT3U(Dx->Type, ==, PHYSICAL_DEVICE_OBJECT); + if (PdoIsMissing(Pdo)) { + if (PdoGetDevicePnpState(Pdo) == Deleted) + PdoDestroy(Pdo); + + continue; + } + if (PdoGetDevicePnpState(Pdo) == Present) PdoSetDevicePnpState(Pdo, Enumerated); + + ObReferenceObject(PdoGetPhysicalDeviceObject(Pdo)); + Relations->Objects[Relations->Count++] = PdoGetPhysicalDeviceObject(Pdo); + + ListEntry = Next; } - RtlCopyMemory(Relations, LowerRelations, Size); + ASSERT3U(Relations->Count, <=, Count); Trace("%s: %d PDO(s)\n", __FdoGetName(Fdo), Relations->Count); } else { - ULONG Index; - Trace("%s: FILTERED\n", __FdoGetName(Fdo)); - for (Index = 0; Index < LowerRelations->Count; Index++) - ObDereferenceObject(LowerRelations->Objects[Index]); - IoInvalidateDeviceRelations(__FdoGetPhysicalDeviceObject(Fdo), BusRelations); } __FdoReleaseMutex(Fdo); - ExFreePool(LowerRelations); - Irp->IoStatus.Information = (ULONG_PTR)Relations; status = STATUS_SUCCESS; @@ -1191,16 +1270,12 @@ done: return status; fail3: - Error("fail3\n"); + __FdoReleaseMutex(Fdo); fail2: - Error("fail2\n"); - IoReleaseRemoveLock(&Fdo->Dx->RemoveLock, Irp); fail1: - Error("fail1 (%08x)\n", status); - Irp->IoStatus.Status = status; IoCompleteRequest(Irp, IO_NO_INCREMENT); diff --git a/src/xenfilt/pdo.c b/src/xenfilt/pdo.c index 57502f3..e2c8c81 100644 --- a/src/xenfilt/pdo.c +++ b/src/xenfilt/pdo.c @@ -63,6 +63,8 @@ struct _XENFILT_PDO { PIRP DevicePowerIrp; PXENFILT_FDO Fdo; + BOOLEAN Missing; + const CHAR *Reason; XENFILT_EMULATED_OBJECT_TYPE Type; PXENFILT_EMULATED_OBJECT EmulatedObject; @@ -190,6 +192,41 @@ PdoGetPhysicalDeviceObject( return Pdo->PhysicalDeviceObject; } +static FORCEINLINE VOID +__PdoSetMissing( + IN PXENFILT_PDO Pdo, + IN const CHAR *Reason + ) +{ + Pdo->Reason = Reason; + Pdo->Missing = TRUE; +} + +VOID +PdoSetMissing( + IN PXENFILT_PDO Pdo, + IN const CHAR *Reason + ) +{ + __PdoSetMissing(Pdo, Reason); +} + +static FORCEINLINE BOOLEAN +__PdoIsMissing( + IN PXENFILT_PDO Pdo + ) +{ + return Pdo->Missing; +} + +BOOLEAN +PdoIsMissing( + IN PXENFILT_PDO Pdo + ) +{ + return __PdoIsMissing(Pdo); +} + static FORCEINLINE PDEVICE_OBJECT __PdoGetDeviceObject( IN PXENFILT_PDO Pdo @@ -766,6 +803,7 @@ PdoRemoveDevice( { PXENFILT_FDO Fdo = __PdoGetFdo(Pdo); POWER_STATE PowerState; + BOOLEAN NeedInvalidate; NTSTATUS status; status = IoAcquireRemoveLock(&Pdo->Dx->RemoveLock, Irp); @@ -783,21 +821,34 @@ PdoRemoveDevice( __PdoSetDevicePowerState(Pdo, PowerDeviceD3); done: + status = PdoForwardIrpSynchronously(Pdo, Irp); + FdoAcquireMutex(Fdo); - __PdoSetDevicePnpState(Pdo, Deleted); - FdoReleaseMutex(Fdo); - IoReleaseRemoveLockAndWait(&Pdo->Dx->RemoveLock, Irp); + NeedInvalidate = FALSE; - status = PdoForwardIrpSynchronously(Pdo, Irp); - IoCompleteRequest(Irp, IO_NO_INCREMENT); + if (__PdoIsMissing(Pdo)) { + DEVICE_PNP_STATE State = __PdoGetDevicePnpState(Pdo); + + __PdoSetDevicePnpState(Pdo, Deleted); + IoReleaseRemoveLockAndWait(&Pdo->Dx->RemoveLock, Irp); + + if (State == SurpriseRemovePending) + PdoDestroy(Pdo); + else + NeedInvalidate = TRUE; + } else { + __PdoSetDevicePnpState(Pdo, Enumerated); + IoReleaseRemoveLock(&Pdo->Dx->RemoveLock, Irp); + } - FdoAcquireMutex(Fdo); - PdoDestroy(Pdo); FdoReleaseMutex(Fdo); - IoInvalidateDeviceRelations(FdoGetPhysicalDeviceObject(Fdo), - BusRelations); + if (NeedInvalidate) + IoInvalidateDeviceRelations(FdoGetPhysicalDeviceObject(Fdo), + BusRelations); + + IoCompleteRequest(Irp, IO_NO_INCREMENT); return status; @@ -1051,7 +1102,10 @@ PdoEject( PXENFILT_FDO Fdo = __PdoGetFdo(Pdo); NTSTATUS status; + FdoAcquireMutex(Fdo); + __PdoSetMissing(Pdo, "Ejected"); __PdoSetDevicePnpState(Pdo, Deleted); + FdoReleaseMutex(Fdo); status = PdoForwardIrpSynchronously(Pdo, Irp); IoCompleteRequest(Irp, IO_NO_INCREMENT); @@ -2050,13 +2104,18 @@ PdoDestroy( ASSERT3U(__PdoGetDevicePnpState(Pdo), ==, Deleted); + ASSERT(__PdoIsMissing(Pdo)); + Pdo->Missing = FALSE; + FdoRemovePhysicalDeviceObject(Fdo, Pdo); Dx->Pdo = NULL; - Info("%p (%s)\n", + Info("%p (%s) (%s)\n", FilterDeviceObject, - __PdoGetName(Pdo)); + __PdoGetName(Pdo), + Pdo->Reason); + Pdo->Reason = NULL; RtlZeroMemory(Pdo->Name, sizeof (Pdo->Name)); diff --git a/src/xenfilt/pdo.h b/src/xenfilt/pdo.h index aac5702..0dfd384 100644 --- a/src/xenfilt/pdo.h +++ b/src/xenfilt/pdo.h @@ -56,6 +56,22 @@ PdoGetPhysicalDeviceObject( IN PXENFILT_PDO Pdo ); +extern BOOLEAN +PdoIsMissing( + IN PXENFILT_PDO Pdo + ); + +extern VOID +PdoSetMissing( + IN PXENFILT_PDO Pdo, + IN const CHAR *Reason + ); + +extern BOOLEAN +PdoIsMasked( + IN PXENFILT_PDO Pdo + ); + extern PDEVICE_OBJECT PdoGetDeviceObject( IN PXENFILT_PDO Pdo