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

["Shi, Steven" <steven.shi@intel.com>]

This patch can fix the Bug 405 issue (https://bugzilla.tianocore.org/show_bug.cgi?id=405). Thank you!



Steven Shi
Intel\SSG\STO\UEFI Firmware

Tel: +86 021-61166522
iNet: 821-6522


> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, November 7, 2017 1:36 PM
> To: Laszlo Ersek <lersek@redhat.com>; Shi, Steven <steven.shi@intel.com>
> Cc: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [PATCH] PcAtChipsetPkg/IsaAcpiDxe: Restore PCI attributes
> correctly
> 
> Laszlo,
> Sure I will add the Bugzilla url in the commit message.
> 
> Steven,
> Could you please check whether this patch can fix your "reconnect -r" hang?
> 
> Thanks/Ray
> 
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Tuesday, November 7, 2017 7:23 AM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.zeng@intel.com>; Shi, Steven <steven.shi@intel.com>
> > Subject: Re: [PATCH] PcAtChipsetPkg/IsaAcpiDxe: Restore PCI attributes
> > correctly
> >
> > Hi Ray,
> >
> > 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(-)
> >
> > Is this for <https://bugzilla.tianocore.org/show_bug.cgi?id=405>?
> >
> > If so, can you please add the reference to the commit message?
> >
> > Also, I think we should ask Steven to test the patch. (CC'd.)
> >
> > I'll try to comment more later.
> >
> > Thanks
> > Laszlo
> >
> >
> > >
> > > 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
> > > @@ -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
> > >                                      );
> > >    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)
> > >