From patchwork Tue Jun 4 13:13:24 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 2659061 Return-Path: X-Original-To: patchwork-linux-acpi@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 3A31FDFF66 for ; Tue, 4 Jun 2013 13:13:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751812Ab3FDNN3 (ORCPT ); Tue, 4 Jun 2013 09:13:29 -0400 Received: from mail-ie0-f169.google.com ([209.85.223.169]:51945 "EHLO mail-ie0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752182Ab3FDNN1 (ORCPT ); Tue, 4 Jun 2013 09:13:27 -0400 Received: by mail-ie0-f169.google.com with SMTP id 10so418705ied.14 for ; Tue, 04 Jun 2013 06:13:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=A7878PcDfg9t2sQUKuzJKhDiCM/DwrulYXkhEzUcpTk=; b=Q5D0fK/VtaNo7Y+ArpBL8Ck/VnvuEawA+j9VCsxYOHbJ6Bvup7/YosFuMowkegQVF9 Z8GOuZLyulHT9ck/+BomoMmRGotNOiiSmW858HQCgSLwelBhGNa6Sb6LJC3o2QmsiizQ L1tLTGYkMVhtW4HVsH1AF5qRD+Qu2wgYaEnl7WwdHX32hHno8CC7Te+PVxqjvw/3rp5n +VFDiYSVxR1DF9J5cmbkf2AOqgiUl/ana+0b/924PHHLSLASux/lmwqmvasyxUaXEvLP +YE1IrqnljhbRM8f2v5c+hqFN/K3/hQ5mDpmHCbSo6bR6iGP5VjBel0+khhAtOM+PhdF ZOXA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent :x-gm-message-state; bh=A7878PcDfg9t2sQUKuzJKhDiCM/DwrulYXkhEzUcpTk=; b=fZ2/tGXJazed+/hxv1beePOu1K79pLt1L35zwtp/RG8sZzlRqoHELKJh2Ew8v3wRPV oP11BaVpEQ9OljffFlTCeyuUEHXVlI/RZ0Y7d5oxrrPnXdbtMlezlDUaSpIVdNlOHA0f WJEo/aZggPa0ZjeIC19RmCwW2duwPRJgyvE2nLtLJ8nLdmt1a5F7+4Hp1lOBBWh6tmNR gapeAnt5Ru6Ys48wJySH2ROXZ3vYITBvq+0FStxuMppB+bJp8Bs0l9zVEWc6LDbJ6/IX cC46WQfwEK002elgSsO0tRtw2kMOwZhmTyJpxRsufHXEIYa30I03IH1Nx9uCNHmymHbz 9/eA== X-Received: by 10.50.22.106 with SMTP id c10mr784514igf.14.1370351607275; Tue, 04 Jun 2013 06:13:27 -0700 (PDT) Received: from google.com ([172.16.54.244]) by mx.google.com with ESMTPSA id l14sm1757145igf.9.2013.06.04.06.13.26 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Tue, 04 Jun 2013 06:13:26 -0700 (PDT) Date: Tue, 4 Jun 2013 07:13:24 -0600 From: Bjorn Helgaas To: Betty Dall Cc: rjw@sisk.pl, ying.huang@intel.com, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, gong.chen@linux.intel.com Subject: Re: [PATCH v2 1/3] PCI/AER: Fix incorrect return from aer_hest_parse() Message-ID: <20130604131324.GA4829@google.com> References: <1369924769-17183-1-git-send-email-betty.dall@hp.com> <1369924769-17183-2-git-send-email-betty.dall@hp.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1369924769-17183-2-git-send-email-betty.dall@hp.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Gm-Message-State: ALoCoQmlKFBhNw1PL6zoE6VEhvUBrgDRROR348h63Txn1r0b3pAzmhD+KcRany/OfCqP4OsExRXv37yzdTfytwEoj1Qad48ldVdREDQbLWFYU1V5pGxwIwptlnVRoqcsFVL4aal6CmpNmXMBqekf+v5u9856Fe2VWNgBArPlYs5GZJiKr1z4bzNqwiTKeyuUFiYivZDN+1ugrlmVrvc+iMVR1XEK5Ovthg== Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org On Thu, May 30, 2013 at 08:39:27AM -0600, Betty Dall wrote: > The function aer_hest_parse() is called to determine if the given > PCI device is firmware first or not. The code loops through each > section of the HEST table to look for a match. The bug is that > the function always returns whether the last HEST section is firmware > first. The fix stops the iteration once the info.firmware_first > variable is set. This is similar to how the function aer_hest_parse_aff() > stops the iteration. > > Signed-off-by: Betty Dall > --- > > drivers/pci/pcie/aer/aerdrv_acpi.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > > diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c > index 5194a7d..39b8671 100644 > --- a/drivers/pci/pcie/aer/aerdrv_acpi.c > +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c > @@ -42,6 +42,9 @@ static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data) > u8 bridge = 0; > int ff = 0; > > + if (info->firmware_first) > + return 0; > + > switch (hest_hdr->type) { > case ACPI_HEST_TYPE_AER_ROOT_PORT: > pcie_type = PCI_EXP_TYPE_ROOT_PORT; 1) I think dev->__aer_firmware_first should be initialized somewhere in the pci_scan_single_device() path, e.g., maybe pci_init_capabilities(). It's known at device add-time and never changes, so there's no point in doing the lazy setup we do now. That would let us get rid of __aer_firmware_first_valid, too (along with the pointless "__" prefix). This is just an observation, not a requirement for this patch set. 2) This is a band-aid that covers up the real problem, which is that we update info->firmware_first even for non-matching devices. I think we should do something like the following instead: commit c67612f272f1792a08f012f1b5ca37d5cfde5de4 Author: Bjorn Helgaas Date: Mon Jun 3 16:49:12 2013 -0600 PCI/AER: Don't parse HEST table for non-PCIe devices AER is a PCIe-only capability, so there's no point in trying to match a HEST PCIe structure with a non-PCIe device. Previously, a HEST global AER bridge entry (type 8) could incorrectly match *any* bridge, even a legacy PCI-PCI bridge, and a non-global HEST entry could match a legacy PCI device. Signed-off-by: Bjorn Helgaas Reviewed-by: Chen Gong Reviewed-by: Chen Gong Reviewed-by: Chen Gong --- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c index 5194a7d..4f798ab 100644 --- a/drivers/pci/pcie/aer/aerdrv_acpi.c +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c @@ -59,8 +59,7 @@ static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data) p = (struct acpi_hest_aer_common *)(hest_hdr + 1); if (p->flags & ACPI_HEST_GLOBAL) { - if ((pci_is_pcie(info->pci_dev) && - pci_pcie_type(info->pci_dev) == pcie_type) || bridge) + if ((pci_pcie_type(info->pci_dev) == pcie_type) || bridge) ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); } else if (hest_match_pci(p, info->pci_dev)) @@ -89,6 +88,9 @@ static void aer_set_firmware_first(struct pci_dev *pci_dev) int pcie_aer_get_firmware_first(struct pci_dev *dev) { + if (!pci_is_pcie(dev)) + return 0; + if (!dev->__aer_firmware_first_valid) aer_set_firmware_first(dev); return dev->__aer_firmware_first; commit 947da50270686b0d70f4bc2f7323ef7229489ecb Author: Bjorn Helgaas Date: Mon Jun 3 15:42:00 2013 -0600 PCI/AER: Factor out HEST device type matching This factors out the matching of HEST structure type and PCIe device type to improve readability. No functional change. Signed-off-by: Bjorn Helgaas diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c index 4f798ab..56e2d94 100644 --- a/drivers/pci/pcie/aer/aerdrv_acpi.c +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c @@ -29,6 +29,22 @@ static inline int hest_match_pci(struct acpi_hest_aer_common *p, p->function == PCI_FUNC(pci->devfn)); } +static inline bool hest_match_type(struct acpi_hest_header *hest_hdr, + struct pci_dev *dev) +{ + u16 hest_type = hest_hdr->type; + u8 pcie_type = pci_pcie_type(dev); + + if ((hest_type == ACPI_HEST_TYPE_AER_ROOT_PORT && + pcie_type == PCI_EXP_TYPE_ROOT_PORT) || + (hest_type == ACPI_HEST_TYPE_AER_ENDPOINT && + pcie_type == PCI_EXP_TYPE_ENDPOINT) || + (hest_type == ACPI_HEST_TYPE_AER_BRIDGE && + (dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)) + return true; + return false; +} + struct aer_hest_parse_info { struct pci_dev *pci_dev; int firmware_first; @@ -38,28 +54,11 @@ static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data) { struct aer_hest_parse_info *info = data; struct acpi_hest_aer_common *p; - u8 pcie_type = 0; - u8 bridge = 0; int ff = 0; - switch (hest_hdr->type) { - case ACPI_HEST_TYPE_AER_ROOT_PORT: - pcie_type = PCI_EXP_TYPE_ROOT_PORT; - break; - case ACPI_HEST_TYPE_AER_ENDPOINT: - pcie_type = PCI_EXP_TYPE_ENDPOINT; - break; - case ACPI_HEST_TYPE_AER_BRIDGE: - if ((info->pci_dev->class >> 16) == PCI_BASE_CLASS_BRIDGE) - bridge = 1; - break; - default: - return 0; - } - p = (struct acpi_hest_aer_common *)(hest_hdr + 1); if (p->flags & ACPI_HEST_GLOBAL) { - if ((pci_pcie_type(info->pci_dev) == pcie_type) || bridge) + if (hest_match_type(hest_hdr, info->pci_dev)) ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); } else if (hest_match_pci(p, info->pci_dev)) commit e9f977a04d96a54c4f6aa0b831e725dab2154364 Author: Bjorn Helgaas Date: Mon Jun 3 19:47:27 2013 -0600 PCI/AER: Set dev->__aer_firmware_first only for matching devices Previously, we always updated info->firmware_first, even for HEST entries that didn't match the device. Therefore, if the last HEST descriptor was a PCIe structure that didn't match the device, we always cleared dev->__aer_firmware_first. Based-on-patch-by: Betty Dall Signed-off-by: Bjorn Helgaas diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c index 56e2d94..2bedad8 100644 --- a/drivers/pci/pcie/aer/aerdrv_acpi.c +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c @@ -54,16 +54,16 @@ static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data) { struct aer_hest_parse_info *info = data; struct acpi_hest_aer_common *p; - int ff = 0; + int ff; p = (struct acpi_hest_aer_common *)(hest_hdr + 1); - if (p->flags & ACPI_HEST_GLOBAL) { + ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); + if (p->flags & ACPI_HEST_GLOBAL) if (hest_match_type(hest_hdr, info->pci_dev)) - ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); - } else + info->firmware_first = ff; + else if (hest_match_pci(p, info->pci_dev)) - ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); - info->firmware_first = ff; + info->firmware_first = ff; return 0; }