From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 411CE2034CF7D for ; Tue, 7 Nov 2017 09:46:01 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9BACDC0587C9; Tue, 7 Nov 2017 17:50:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9BACDC0587C9 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-130.rdu2.redhat.com [10.10.123.130]) by smtp.corp.redhat.com (Postfix) with ESMTP id 973A25D756; Tue, 7 Nov 2017 17:49:59 +0000 (UTC) To: Ruiyu Ni , edk2-devel@lists.01.org Cc: Star Zeng References: <20171103082836.125696-1-ruiyu.ni@intel.com> From: Laszlo Ersek Message-ID: <4c0f9b23-a0ab-7f8d-11be-579dddf8ec3a@redhat.com> Date: Tue, 7 Nov 2017 18:49:58 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171103082836.125696-1-ruiyu.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Tue, 07 Nov 2017 17:50:00 +0000 (UTC) Subject: Re: [PATCH] PcAtChipsetPkg/IsaAcpiDxe: Restore PCI attributes correctly X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Nov 2017 17:46:01 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Star Zeng > Cc: Laszlo Ersek > --- > 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 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.
> +Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
> 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) >