Re: [PATCH] PcAtChipsetPkg/IsaAcpiDxe: Restore PCI attributes correctly

[Laszlo Ersek <lersek@redhat.com>]

On 11/03/17 09:28, Ruiyu Ni wrote:
> The original code enables some BITs in PCI attributes in Start(),
> but wrongly to disable these BITs in Stop().
> 
> The correct behavior is to save the original PCI attributes before
> enables some BITs in Start(), and restore to original value
> in Stop().
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c | 44 +++++++++++++++++----------------
>  PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.h |  3 ++-
>  2 files changed, 25 insertions(+), 22 deletions(-)

I see that Star commented already; I agree with his points. Beyond them,
I have some comments of my own:

> diff --git a/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c b/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c
> index 32381b112d..60d2fb5a5b 100644
> --- a/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c
> +++ b/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.c

(1) The copyright notice is updated in the header file. For consistency,
I think you might want to do the same in the C file.

> @@ -172,6 +172,7 @@ PcatIsaAcpiDriverBindingStart (
>    EFI_PCI_IO_PROTOCOL  *PciIo;
>    PCAT_ISA_ACPI_DEV    *PcatIsaAcpiDev;
>    UINT64               Supports;
> +  UINT64               OriginalAttributes;
>    BOOLEAN              Enabled;
>  
>    Enabled = FALSE;
> @@ -210,9 +211,18 @@ PcatIsaAcpiDriverBindingStart (
>    if (Supports == 0 || Supports == (EFI_PCI_IO_ATTRIBUTE_ISA_IO | EFI_PCI_IO_ATTRIBUTE_ISA_IO_16)) {
>      Status = EFI_UNSUPPORTED;
>      goto Done;
> -  }  
> +  }
> +
> +  Status = PciIo->Attributes (
> +                    PciIo,
> +                    EfiPciIoAttributeOperationGet,
> +                    0,
> +                    &OriginalAttributes
> +                    );
> +  if (EFI_ERROR (Status)) {
> +    goto Done;
> +  }
>  
> -  Enabled = TRUE;
>    Status = PciIo->Attributes (
>                      PciIo, 
>                      EfiPciIoAttributeOperationEnable, 
> @@ -222,7 +232,8 @@ PcatIsaAcpiDriverBindingStart (
>    if (EFI_ERROR (Status)) {
>      goto Done;
>    }
> -  
> +
> +  Enabled = TRUE;
>    //
>    // Allocate memory for the PCAT ISA ACPI Device structure
>    //
> @@ -239,9 +250,10 @@ PcatIsaAcpiDriverBindingStart (
>    //
>    // Initialize the PCAT ISA ACPI Device structure
>    //
> -  PcatIsaAcpiDev->Signature = PCAT_ISA_ACPI_DEV_SIGNATURE;
> -  PcatIsaAcpiDev->Handle    = Controller;
> -  PcatIsaAcpiDev->PciIo     = PciIo;
> +  PcatIsaAcpiDev->Signature         = PCAT_ISA_ACPI_DEV_SIGNATURE;
> +  PcatIsaAcpiDev->Handle            = Controller;
> +  PcatIsaAcpiDev->PciIo             = PciIo;
> +  PcatIsaAcpiDev->OriginalAttribute = OriginalAttributes;
>  
>    //
>    // Initialize PcatIsaAcpiDeviceList
> @@ -274,8 +286,8 @@ Done:
>      if (PciIo != NULL && Enabled) {
>        PciIo->Attributes (
>                 PciIo, 
> -               EfiPciIoAttributeOperationDisable, 
> -               EFI_PCI_DEVICE_ENABLE | Supports | EFI_PCI_IO_ATTRIBUTE_ISA_MOTHERBOARD_IO,
> +               EfiPciIoAttributeOperationSet, 
> +               OriginalAttributes,
>                 NULL 
>                 );
>      }
> @@ -321,7 +333,6 @@ PcatIsaAcpiDriverBindingStop (
>    EFI_STATUS             Status;
>    EFI_ISA_ACPI_PROTOCOL  *IsaAcpi;
>    PCAT_ISA_ACPI_DEV      *PcatIsaAcpiDev;
> -  UINT64                 Supports;
>    
>    //
>    // Get the ISA ACPI Protocol Interface
> @@ -348,23 +359,14 @@ PcatIsaAcpiDriverBindingStop (
>    //
>    Status = PcatIsaAcpiDev->PciIo->Attributes (
>                                      PcatIsaAcpiDev->PciIo,
> -                                    EfiPciIoAttributeOperationSupported,
> -                                    0,
> -                                    &Supports
> +                                    EfiPciIoAttributeOperationSet,
> +                                    PcatIsaAcpiDev->OriginalAttribute,
> +                                    0
>                                      );

(2) For better edk2 style, the last argument should be NULL, not 0.

(3) (Mentioned earlier, also by Star) -- please add the TianoCore BZ
reference (#405).

With those fixed,

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
>  
> -  Supports &= (UINT64) (EFI_PCI_IO_ATTRIBUTE_ISA_IO | EFI_PCI_IO_ATTRIBUTE_ISA_IO_16);
> -
> -  PcatIsaAcpiDev->PciIo->Attributes (
> -                           PcatIsaAcpiDev->PciIo, 
> -                           EfiPciIoAttributeOperationDisable, 
> -                           EFI_PCI_DEVICE_ENABLE | Supports | EFI_PCI_IO_ATTRIBUTE_ISA_MOTHERBOARD_IO,
> -                           NULL 
> -                           );
> - 
>    //
>    // Uninstall protocol interface: EFI_ISA_ACPI_PROTOCOL
>    //
> diff --git a/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.h b/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.h
> index 0671127644..3ad3a3f313 100644
> --- a/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.h
> +++ b/PcAtChipsetPkg/IsaAcpiDxe/PcatIsaAcpi.h
> @@ -1,7 +1,7 @@
>  /** @file
>    EFI PCAT ISA ACPI Driver for a Generic PC Platform
>  
> -Copyright (c) 2006 - 2011, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials                          
>  are licensed and made available under the terms and conditions of the BSD License         
>  which accompanies this distribution.  The full text of the license may be found at        
> @@ -43,6 +43,7 @@ typedef struct {
>    EFI_HANDLE             Handle;    
>    EFI_ISA_ACPI_PROTOCOL  IsaAcpi;
>    EFI_PCI_IO_PROTOCOL    *PciIo;
> +  UINT64                 OriginalAttribute;
>  } PCAT_ISA_ACPI_DEV;
>  
>  #define PCAT_ISA_ACPI_DEV_FROM_THIS(a) BASE_CR(a, PCAT_ISA_ACPI_DEV, IsaAcpi)
>