Re: [win-pv-devel] [PATCH 3/9] Refactor Pnp/Power handlers

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel [mailto:win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On
> Behalf Of Owen Smith
> Sent: 22 April 2016 15:16
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Owen Smith
> Subject: [win-pv-devel] [PATCH 3/9] Refactor Pnp/Power handlers
> 
> Moves the mapping of DeviceObject to Pdo into the Fdo code
> 
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>

Acked-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

> ---
>  src/xenvbd/driver.c | 127 +++++----------------------
>  src/xenvbd/fdo.c    | 248 ++++++++++++++++++++++++------------------------
> ----
>  src/xenvbd/fdo.h    |  20 ++---
>  3 files changed, 138 insertions(+), 257 deletions(-)
> 
> diff --git a/src/xenvbd/driver.c b/src/xenvbd/driver.c
> index cff6034..c5352d0 100644
> --- a/src/xenvbd/driver.c
> +++ b/src/xenvbd/driver.c
> @@ -42,10 +42,6 @@
>  #include <xencrsh_interface.h>
>  #include <xenvbd-ntstrsafe.h>
> 
> -#define IS_NULL         ((ULONG)'llun')
> -#define IS_FDO          ((ULONG)'odf')
> -#define IS_PDO          ((ULONG)'odp')
> -
> //=========================================================
> ====================
>  XENVBD_PARAMETERS   DriverParameters;
>  HANDLE              DriverStatusKey;
> 
> @@ -261,65 +257,26 @@ DriverUnlinkFdo(
>      KeReleaseSpinLock(&__XenvbdLock, Irql);
>  }
> 
> -__checkReturn
> -static FORCEINLINE ULONG
> -DriverGetFdoOrPdo(
> -    __in PDEVICE_OBJECT          DeviceObject,
> -    __out PXENVBD_FDO*           _Fdo,
> -    __out PXENVBD_PDO*           _Pdo
> -    )
> -{
> -    KIRQL       Irql;
> -    ULONG       Result = IS_NULL;
> -
> -    *_Fdo = NULL;
> -    *_Pdo = NULL;
> -
> -    KeAcquireSpinLock(&__XenvbdLock, &Irql);
> -    if (__XenvbdFdo) {
> -        PXENVBD_FDO Fdo = __XenvbdFdo;
> -        if (FdoReference(Fdo) > 0) {
> -            if (FdoGetDeviceObject(Fdo) == DeviceObject) {
> -                *_Fdo = Fdo;
> -                Result = IS_FDO;
> -            } else {
> -                KeReleaseSpinLock(&__XenvbdLock, Irql);
> -
> -                *_Pdo = FdoGetPdoFromDeviceObject(Fdo, DeviceObject);
> -                FdoDereference(Fdo);
> -                return IS_PDO;
> -            }
> -        }
> -    }
> -    KeReleaseSpinLock(&__XenvbdLock, Irql);
> -
> -    return Result;
> -}
> -__checkReturn
> -static FORCEINLINE NTSTATUS
> -DriverMapPdo(
> -    __in PDEVICE_OBJECT          DeviceObject,
> -    __in PIRP                    Irp
> +static FORCEINLINE BOOLEAN
> +__DriverGetFdo(
> +    IN  PDEVICE_OBJECT      DeviceObject,
> +    OUT PXENVBD_FDO         *Fdo
>      )
>  {
>      KIRQL       Irql;
> -    NTSTATUS    Status;
> +    BOOLEAN     IsFdo = FALSE;
> 
>      KeAcquireSpinLock(&__XenvbdLock, &Irql);
> -    if (__XenvbdFdo && FdoGetDeviceObject(__XenvbdFdo) !=
> DeviceObject) {
> -        PXENVBD_FDO Fdo = __XenvbdFdo;
> -        if (FdoReference(Fdo) > 0) {
> -            KeReleaseSpinLock(&__XenvbdLock, Irql);
> -            Status = FdoMapDeviceObjectToPdo(Fdo, DeviceObject, Irp);
> -            FdoDereference(Fdo);
> -            goto done;
> +    *Fdo = __XenvbdFdo;
> +    if (*Fdo) {
> +        FdoReference(*Fdo);
> +        if (FdoGetDeviceObject(*Fdo) == DeviceObject) {
> +            IsFdo = TRUE;
>          }
>      }
>      KeReleaseSpinLock(&__XenvbdLock, Irql);
> -    Status = DriverDispatchPnp(DeviceObject, Irp);
> 
> -done:
> -    return Status;
> +    return IsFdo;
>  }
> 
>  VOID
> @@ -563,9 +520,6 @@ HwStartIo(
>      return FdoStartIo((PXENVBD_FDO)HwDeviceExtension, Srb);
>  }
> 
> -
> //=========================================================
> ====================
> -// Driver Redirections
> -
>  __drv_dispatchType(IRP_MJ_PNP)
>  DRIVER_DISPATCH             DispatchPnp;
> 
> @@ -575,34 +529,15 @@ DispatchPnp(
>      IN PIRP             Irp
>      )
>  {
> -    NTSTATUS            Status;
> -    ULONG               IsFdo;
>      PXENVBD_FDO         Fdo;
> -    PXENVBD_PDO         Pdo;
> -
> -    IsFdo = DriverGetFdoOrPdo(DeviceObject, &Fdo, &Pdo);
> 
> -    switch (IsFdo) {
> -    case IS_FDO:
> -        Status = FdoDispatchPnp(Fdo, DeviceObject, Irp); // drops Fdo
> reference
> -        break;
> +    if (__DriverGetFdo(DeviceObject, &Fdo))
> +        return FdoDispatchPnp(Fdo, DeviceObject, Irp);
> 
> -    case IS_PDO:
> -        if (Pdo) {
> -            Status = PdoDispatchPnp(Pdo, DeviceObject, Irp); // drops Pdo
> reference
> -        } else {
> -            Status = DriverMapPdo(DeviceObject, Irp);
> -        }
> -        break;
> +    if (Fdo != NULL)
> +        return FdoForwardPnp(Fdo, DeviceObject, Irp);
> 
> -    case IS_NULL:
> -    default:
> -        Warning("DeviceObject 0x%p is not FDO (0x%p) or a PDO\n",
> DeviceObject, __XenvbdFdo);
> -        Status = DriverDispatchPnp(DeviceObject, Irp);
> -        break;
> -    }
> -
> -    return Status;
> +    return DriverDispatchPnp(DeviceObject, Irp);
>  }
> 
>  __drv_dispatchType(IRP_MJ_POWER)
> @@ -614,35 +549,15 @@ DispatchPower(
>      IN PIRP             Irp
>      )
>  {
> -    NTSTATUS            Status;
> -    ULONG               IsFdo;
>      PXENVBD_FDO         Fdo;
> -    PXENVBD_PDO         Pdo;
> -
> -    IsFdo = DriverGetFdoOrPdo(DeviceObject, &Fdo, &Pdo);
> 
> -    switch (IsFdo) {
> -    case IS_FDO:
> -        ASSERT3P(Fdo, !=, NULL);
> -        ASSERT3P(Pdo, ==, NULL);
> -        Status = FdoDispatchPower(Fdo, DeviceObject, Irp); // drops Fdo
> reference
> -        break;
> -
> -    case IS_PDO:
> -        if (Pdo) {
> -            PdoDereference(Pdo); // drops Pdo reference
> -        }
> -        Status = DriverDispatchPower(DeviceObject, Irp);
> -        break;
> +    if (__DriverGetFdo(DeviceObject, &Fdo))
> +        return FdoDispatchPower(Fdo, DeviceObject, Irp);
> 
> -    case IS_NULL:
> -    default:
> -        Warning("DeviceObject 0x%p is not FDO (0x%p) or a PDO\n",
> DeviceObject, __XenvbdFdo);
> -        Status = DriverDispatchPower(DeviceObject, Irp);
> -        break;
> -    }
> +    if (Fdo != NULL)
> +        FdoDereference(Fdo);
> 
> -    return Status;
> +    return DriverDispatchPower(DeviceObject, Irp);
>  }
> 
>  DRIVER_UNLOAD               DriverUnload;
> diff --git a/src/xenvbd/fdo.c b/src/xenvbd/fdo.c
> index 247ef51..939fb47 100644
> --- a/src/xenvbd/fdo.c
> +++ b/src/xenvbd/fdo.c
> @@ -1879,70 +1879,7 @@ FdoStartIo(
>      return TRUE;
>  }
> 
> -
> //=========================================================
> ====================
> -// PnP Handler
> -__checkReturn
> -NTSTATUS
> -FdoDispatchPnp(
> -    __in PXENVBD_FDO                 Fdo,
> -    __in PDEVICE_OBJECT              DeviceObject,
> -    __in PIRP                        Irp
> -    )
> -{
> -    PIO_STACK_LOCATION  Stack = IoGetCurrentIrpStackLocation(Irp);
> -    UCHAR               Minor = Stack->MinorFunction;
> -    NTSTATUS            Status;
> -
> -    switch (Stack->MinorFunction) {
> -    case IRP_MN_REMOVE_DEVICE:
> -        Verbose("FDO:IRP_MN_REMOVE_DEVICE\n");
> -        FdoD0ToD3(Fdo);
> -        FdoUnplugRequest(Fdo, FALSE);
> -        // drop ref-count acquired in DriverGetFdo *before* destroying Fdo
> -        FdoDereference(Fdo);
> -        __FdoTerminate(Fdo);
> -        break;
> -
> -    case IRP_MN_QUERY_DEVICE_RELATIONS:
> -        if (Stack->Parameters.QueryDeviceRelations.Type == BusRelations) {
> -            KIRQL   Irql;
> -            BOOLEAN NeedInvalidate;
> -            BOOLEAN NeedReboot;
> -
> -            KeAcquireSpinLock(&Fdo->Lock, &Irql);
> -
> -            if (Fdo->DevicePower == PowerDeviceD0) {
> -                FdoScanTargets(Fdo, &NeedInvalidate, &NeedReboot);
> -            } else {
> -                NeedInvalidate = FALSE;
> -                NeedReboot = FALSE;
> -            }
> -
> -            KeReleaseSpinLock(&Fdo->Lock, Irql);
> -
> -            if (NeedInvalidate)
> -                FdoLogTargets("QUERY_RELATIONS", Fdo);
> -
> -            if (NeedReboot)
> -                DriverNotifyInstaller();
> -        }
> -        FdoDereference(Fdo);
> -        break;
> -
> -    default:
> -        FdoDereference(Fdo);
> -        break;
> -    }
> -
> -    Status = DriverDispatchPnp(DeviceObject, Irp);
> -    if (!NT_SUCCESS(Status)) {
> -        Verbose("%02x:%s -> %08x\n", Minor, PnpMinorFunctionName(Minor),
> Status);
> -    }
> -    return Status;
> -}
> -
> -__checkReturn
> -PXENVBD_PDO
> +static PXENVBD_PDO
>  FdoGetPdoFromDeviceObject(
>      __in PXENVBD_FDO                 Fdo,
>      __in PDEVICE_OBJECT              DeviceObject
> @@ -1964,24 +1901,27 @@ FdoGetPdoFromDeviceObject(
>      return NULL;
>  }
> 
> -__checkReturn
> -static FORCEINLINE NTSTATUS
> -__FdoSendQueryId(
> -    __in PDEVICE_OBJECT              DeviceObject,
> -    __out PWCHAR*                    _String
> +static PXENVBD_PDO
> +FdoMapDeviceObjectToPdo(
> +    __in PXENVBD_FDO                Fdo,
> +    __in PDEVICE_OBJECT             DeviceObject
>      )
>  {
> -    KEVENT              Complete;
> -    PIRP                Irp;
> -    IO_STATUS_BLOCK     StatusBlock;
> -    PIO_STACK_LOCATION  Stack;
> -    NTSTATUS            Status;
> +    PXENVBD_PDO                 Pdo;
> +    KEVENT                      Complete;
> +    PIRP                        Irp;
> +    IO_STATUS_BLOCK             StatusBlock;
> +    PIO_STACK_LOCATION          Stack;
> +    NTSTATUS                    Status;
> +    PWCHAR                      String;
> +    ULONG                       TargetId;
> +    DECLARE_UNICODE_STRING_SIZE(UniStr, 4);
> 
>      KeInitializeEvent(&Complete, NotificationEvent, FALSE);
> 
>      Irp = IoBuildSynchronousFsdRequest(IRP_MJ_PNP, DeviceObject, NULL, 0,
> NULL, &Complete, &StatusBlock);
> -    if (!Irp)
> -        return STATUS_INSUFFICIENT_RESOURCES;
> +    if (Irp == NULL)
> +        goto fail1;
> 
>      Stack = IoGetNextIrpStackLocation(Irp);
>      Stack->MinorFunction = IRP_MN_QUERY_ID;
> @@ -1994,21 +1934,10 @@ __FdoSendQueryId(
>          (VOID) KeWaitForSingleObject(&Complete, Executive, KernelMode,
> FALSE, NULL);
>          Status = StatusBlock.Status;
>      }
> -    if (NT_SUCCESS(Status)) {
> -        *_String = (PWCHAR)StatusBlock.Information;
> -    }
> -    return Status;
> -}
> -
> -__checkReturn
> -static FORCEINLINE NTSTATUS
> -__FdoExtractTargetId(
> -    __in PWCHAR                      String,
> -    __out PULONG                     TargetId
> -    )
> -{
> -    DECLARE_UNICODE_STRING_SIZE(UniStr, 4);
> -
> +    if (!NT_SUCCESS(Status))
> +        goto fail2;
> +
> +    String = (PWCHAR)StatusBlock.Information;
>      switch (wcslen(String)) {
>      case 3:
>          UniStr.Length = 1 * sizeof(WCHAR);
> @@ -2022,75 +1951,122 @@ __FdoExtractTargetId(
>          UniStr_buffer[2] = UNICODE_NULL;
>          break;
>      default:
> -        return STATUS_INVALID_PARAMETER;
> +        goto fail3;
>      }
> 
> -    return RtlUnicodeStringToInteger(&UniStr, 16, TargetId);
> +    Status = RtlUnicodeStringToInteger(&UniStr, 16, &TargetId);
> +    if (!NT_SUCCESS(Status))
> +        goto fail4;
> +
> +    Pdo = __FdoGetPdo(Fdo, TargetId);
> +    if (Pdo == NULL)
> +        goto fail5;
> +
> +    PdoSetDeviceObject(Pdo, DeviceObject);
> +    ExFreePool(String);
> +
> +    return Pdo;
> +
> +fail5:
> +fail4:
> +fail3:
> +    ExFreePool(String);
> +fail2:
> +fail1:
> +    return NULL;
>  }
> 
> -static FORCEINLINE VOID
> -__FdoSetDeviceObject(
> -    __in PXENVBD_FDO                 Fdo,
> -    __in ULONG                       TargetId,
> -    __in PDEVICE_OBJECT              DeviceObject
> +__checkReturn
> +NTSTATUS
> +FdoForwardPnp(
> +    __in PXENVBD_FDO                Fdo,
> +    __in PDEVICE_OBJECT             DeviceObject,
> +    __in PIRP                       Irp
>      )
>  {
> -    PXENVBD_PDO Pdo;
> +    PIO_STACK_LOCATION  Stack;
> +    PXENVBD_PDO         Pdo;
> 
> -    Pdo = __FdoGetPdo(Fdo, TargetId);
> -    if (Pdo) {
> -        PdoSetDeviceObject(Pdo, DeviceObject);
> -        PdoDereference(Pdo);
> +    ASSERT3P(DeviceObject, !=, Fdo->DeviceObject);
> +
> +    Pdo = FdoGetPdoFromDeviceObject(Fdo, DeviceObject);
> +    if (Pdo != NULL) {
> +        FdoDereference(Fdo);
> +        return PdoDispatchPnp(Pdo, DeviceObject, Irp);
>      }
> +
> +    Stack = IoGetCurrentIrpStackLocation(Irp);
> +    if (Stack->MinorFunction == IRP_MN_QUERY_ID &&
> +        Stack->Parameters.QueryId.IdType == BusQueryDeviceID) {
> +        Pdo = FdoMapDeviceObjectToPdo(Fdo, DeviceObject);
> +        if (Pdo != NULL) {
> +            FdoDereference(Fdo);
> +            return PdoDispatchPnp(Pdo, DeviceObject, Irp);
> +        }
> +    }
> +
> +    FdoDereference(Fdo);
> +    return DriverDispatchPnp(DeviceObject, Irp);
>  }
> 
>  __checkReturn
>  NTSTATUS
> -FdoMapDeviceObjectToPdo(
> +FdoDispatchPnp(
>      __in PXENVBD_FDO                 Fdo,
>      __in PDEVICE_OBJECT              DeviceObject,
>      __in PIRP                        Irp
>      )
>  {
> -    PWCHAR              String;
> -    NTSTATUS            Status;
> -    ULONG               TargetId;
> -    PIO_STACK_LOCATION  StackLocation;
> -    UCHAR               Minor;
> +    PIO_STACK_LOCATION  Stack;
> 
> -    StackLocation = IoGetCurrentIrpStackLocation(Irp);
> -    Minor = StackLocation->MinorFunction;
> +    ASSERT3P(DeviceObject, ==, Fdo->DeviceObject);
> 
> -    if (!(StackLocation->MinorFunction == IRP_MN_QUERY_ID &&
> -          StackLocation->Parameters.QueryId.IdType == BusQueryDeviceID)) {
> -        goto done;
> -    }
> +    Stack = IoGetCurrentIrpStackLocation(Irp);
> 
> -    Status = __FdoSendQueryId(DeviceObject, &String);
> -    if (!NT_SUCCESS(Status)) {
> -        goto done;
> -    }
> +    switch (Stack->MinorFunction) {
> +    case IRP_MN_REMOVE_DEVICE:
> +        Verbose("FDO:IRP_MN_REMOVE_DEVICE\n");
> +        FdoD0ToD3(Fdo);
> +        FdoUnplugRequest(Fdo, FALSE);
> +        // drop ref-count acquired in DriverGetFdo *before* destroying Fdo
> +        FdoDereference(Fdo);
> +        __FdoTerminate(Fdo);
> +        break;
> 
> -    Status = __FdoExtractTargetId(String, &TargetId);
> -    if (NT_SUCCESS(Status)) {
> -        __FdoSetDeviceObject(Fdo, TargetId, DeviceObject);
> -        Verbose("0x%p --> Target %d (%ws)\n", DeviceObject, TargetId, 
> String);
> -    }
> +    case IRP_MN_QUERY_DEVICE_RELATIONS:
> +        if (Stack->Parameters.QueryDeviceRelations.Type == BusRelations) {
> +            KIRQL   Irql;
> +            BOOLEAN NeedInvalidate;
> +            BOOLEAN NeedReboot;
> 
> -    // String is PagedPool, allocated by lower driver
> -    ASSERT3U(KeGetCurrentIrql(), <=, APC_LEVEL);
> -    ExFreePool(String);
> +            KeAcquireSpinLock(&Fdo->Lock, &Irql);
> 
> -done:
> -    Status = DriverDispatchPnp(DeviceObject, Irp);;
> -    if (!NT_SUCCESS(Status)) {
> -        Verbose("%02x:%s -> %08x\n", Minor, PnpMinorFunctionName(Minor),
> Status);
> +            if (Fdo->DevicePower == PowerDeviceD0) {
> +                FdoScanTargets(Fdo, &NeedInvalidate, &NeedReboot);
> +            } else {
> +                NeedInvalidate = FALSE;
> +                NeedReboot = FALSE;
> +            }
> +
> +            KeReleaseSpinLock(&Fdo->Lock, Irql);
> +
> +            if (NeedInvalidate)
> +                FdoLogTargets("QUERY_RELATIONS", Fdo);
> +
> +            if (NeedReboot)
> +                DriverNotifyInstaller();
> +        }
> +        FdoDereference(Fdo);
> +        break;
> +
> +    default:
> +        FdoDereference(Fdo);
> +        break;
>      }
> -    return Status;
> +
> +    return DriverDispatchPnp(DeviceObject, Irp);
>  }
> 
> -
> //=========================================================
> ====================
> -// Power Handler
>  __checkReturn
>  NTSTATUS
>  FdoDispatchPower(
> @@ -2103,6 +2079,8 @@ FdoDispatchPower(
>      POWER_STATE_TYPE    PowerType;
>      NTSTATUS            status;
> 
> +    ASSERT3P(DeviceObject, ==, Fdo->DeviceObject);
> +
>      Stack = IoGetCurrentIrpStackLocation(Irp);
>      PowerType = Stack->Parameters.Power.Type;
> 
> @@ -2136,8 +2114,6 @@ FdoDispatchPower(
>      return status;
>  }
> 
> -
> //=========================================================
> ====================
> -// Interfaces
>  PXENBUS_STORE_INTERFACE
>  FdoAcquireStore(
>      __in PXENVBD_FDO    Fdo
> diff --git a/src/xenvbd/fdo.h b/src/xenvbd/fdo.h
> index a9e36c2..96c66a5 100644
> --- a/src/xenvbd/fdo.h
> +++ b/src/xenvbd/fdo.h
> @@ -127,31 +127,22 @@ FdoStartIo(
>      __in PSCSI_REQUEST_BLOCK         Srb
>      );
> 
> -// PnP Handler
>  __checkReturn
>  extern NTSTATUS
> -FdoDispatchPnp(
> -    __in PXENVBD_FDO                 Fdo,
> -    __in PDEVICE_OBJECT              DeviceObject,
> -    __in PIRP                        Irp
> -    );
> -
> -__checkReturn
> -extern PXENVBD_PDO
> -FdoGetPdoFromDeviceObject(
> -    __in PXENVBD_FDO                 Fdo,
> -    __in PDEVICE_OBJECT              DeviceObject
> +FdoForwardPnp(
> +    __in PXENVBD_FDO                Fdo,
> +    __in PDEVICE_OBJECT             DeviceObject,
> +    __in PIRP                       Irp
>      );
> 
>  __checkReturn
>  extern NTSTATUS
> -FdoMapDeviceObjectToPdo(
> +FdoDispatchPnp(
>      __in PXENVBD_FDO                 Fdo,
>      __in PDEVICE_OBJECT              DeviceObject,
>      __in PIRP                        Irp
>      );
> 
> -// Power Handler
>  __checkReturn
>  extern NTSTATUS
>  FdoDispatchPower(
> @@ -160,7 +151,6 @@ FdoDispatchPower(
>      __in PIRP                        Irp
>      );
> 
> -// Interfaces
>  extern PXENBUS_STORE_INTERFACE
>  FdoAcquireStore(
>      __in PXENVBD_FDO                 Fdo
> --
> 1.9.4.msysgit.1
> 
> 
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel
_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel