From: Laszlo Ersek <lersek@redhat.com>
To: Ruiyu Ni <ruiyu.ni@intel.com>, edk2-devel@lists.01.org
Cc: Star Zeng <star.zeng@intel.com>, "Shi, Steven" <steven.shi@intel.com>
Subject: Re: [PATCH] PcAtChipsetPkg/IsaAcpiDxe: Restore PCI attributes correctly
Date: Tue, 7 Nov 2017 00:23:22 +0100 [thread overview]
Message-ID: <6925f265-26e3-90fb-10f4-f794a9c925e5@redhat.com> (raw)
In-Reply-To: <20171103082836.125696-1-ruiyu.ni@intel.com>
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)
>
next prev parent reply other threads:[~2017-11-06 23:19 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-03 8:28 [PATCH] PcAtChipsetPkg/IsaAcpiDxe: Restore PCI attributes correctly Ruiyu Ni
2017-11-06 23:23 ` Laszlo Ersek [this message]
2017-11-07 5:35 ` Ni, Ruiyu
2017-11-07 8:14 ` Shi, Steven
2017-11-07 8:43 ` Zeng, Star
2017-11-07 17:49 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6925f265-26e3-90fb-10f4-f794a9c925e5@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox