* [PATCH] PCI AER: Handle non-AER HEST error sources in aer_hest_parse() @ 2013-12-13 15:23 Betty Dall 2013-12-13 22:16 ` Bjorn Helgaas 0 siblings, 1 reply; 6+ messages in thread From: Betty Dall @ 2013-12-13 15:23 UTC (permalink / raw) To: lenb, rjw, bhelgaas; +Cc: linux-acpi, linux-kernel, linux-pci, Betty Dall aer_hest_parse() could pass a non-AER HEST error source to the function hest_match_pci(). hest_match_pci() assumes that the HEST error source is type ACPI_HEST_TYPE_AER_ROOT_PORT, ACPI_HEST_TYPE_AER_ENDPOINT, or ACPI_HEST_TYPE_AER_BRIDGE. I have a platform that has some ACPI_HEST_TYPE_GENERIC error sources where hest_match_pci() was trying to access the structure as if it had the acpi_hest_aer_common fields. aer_hest_parse() is only ever interested in the AER type HEST error sources. Add a check on the type of the error souce at the beginning of aer_hest_parse(). Signed-off-by: Betty Dall <betty.dall@hp.com> --- drivers/pci/pcie/aer/aerdrv_acpi.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c index cf611ab..39186e3 100644 --- a/drivers/pci/pcie/aer/aerdrv_acpi.c +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c @@ -56,6 +56,10 @@ static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data) struct acpi_hest_aer_common *p; int ff; + if (hest_hdr->type != ACPI_HEST_TYPE_AER_ROOT_PORT && + hest_hdr->type != ACPI_HEST_TYPE_AER_ENDPOINT && + hest_hdr->type != ACPI_HEST_TYPE_AER_BRIDGE) + return 0; p = (struct acpi_hest_aer_common *)(hest_hdr + 1); ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); if (p->flags & ACPI_HEST_GLOBAL) { ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI AER: Handle non-AER HEST error sources in aer_hest_parse() 2013-12-13 15:23 [PATCH] PCI AER: Handle non-AER HEST error sources in aer_hest_parse() Betty Dall @ 2013-12-13 22:16 ` Bjorn Helgaas 2013-12-13 22:47 ` Betty Dall 0 siblings, 1 reply; 6+ messages in thread From: Bjorn Helgaas @ 2013-12-13 22:16 UTC (permalink / raw) To: Betty Dall; +Cc: lenb, rjw, linux-acpi, linux-kernel, linux-pci On Fri, Dec 13, 2013 at 08:23:16AM -0700, Betty Dall wrote: > aer_hest_parse() could pass a non-AER HEST error source to the function > hest_match_pci(). hest_match_pci() assumes that the HEST error source is > type ACPI_HEST_TYPE_AER_ROOT_PORT, ACPI_HEST_TYPE_AER_ENDPOINT, or > ACPI_HEST_TYPE_AER_BRIDGE. I have a platform that has some > ACPI_HEST_TYPE_GENERIC error sources where hest_match_pci() was trying to > access the structure as if it had the acpi_hest_aer_common fields. > > aer_hest_parse() is only ever interested in the AER type HEST error > sources. Add a check on the type of the error souce at the beginning of > aer_hest_parse(). > > Signed-off-by: Betty Dall <betty.dall@hp.com> > --- > > drivers/pci/pcie/aer/aerdrv_acpi.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > > diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c > index cf611ab..39186e3 100644 > --- a/drivers/pci/pcie/aer/aerdrv_acpi.c > +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c > @@ -56,6 +56,10 @@ static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data) > struct acpi_hest_aer_common *p; > int ff; > > + if (hest_hdr->type != ACPI_HEST_TYPE_AER_ROOT_PORT && > + hest_hdr->type != ACPI_HEST_TYPE_AER_ENDPOINT && > + hest_hdr->type != ACPI_HEST_TYPE_AER_BRIDGE) > + return 0; > p = (struct acpi_hest_aer_common *)(hest_hdr + 1); > ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); > if (p->flags & ACPI_HEST_GLOBAL) { Hi Betty, I propose the following tweaked version. Will this work for you? PCI/AER: Ingore non-PCIe AER error sources in aer_hest_parse() From: Betty Dall <betty.dall@hp.com> aer_set_firmware_first() searches the HEST for an error source descriptor matching the specified PCI device. It uses the apei_hest_parse() iterator to call aer_hest_parse() for every descriptor in the HEST. Previously, aer_hest_parse() incorrectly assumed every descriptor was for a PCIe error source. This patch adds a check to avoid that error. [bhelgaas: factor check into helper function, changelog] Signed-off-by: Betty Dall <betty.dall@hp.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/pcie/aer/aerdrv_acpi.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c index cf611ab2193a..f670313479ee 100644 --- a/drivers/pci/pcie/aer/aerdrv_acpi.c +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c @@ -50,12 +50,24 @@ struct aer_hest_parse_info { int firmware_first; }; +static int hest_source_is_pcie_aer(struct acpi_hest_header *hest_hdr) +{ + if (hest_hdr->type == ACPI_HEST_TYPE_AER_ROOT_PORT || + hest_hdr->type == ACPI_HEST_TYPE_AER_ENDPOINT || + hest_hdr->type == ACPI_HEST_TYPE_AER_BRIDGE) + return 1; + return 0; +} + 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; + if (!hest_source_is_pcie_aer(hest_hdr)) + return 0; + p = (struct acpi_hest_aer_common *)(hest_hdr + 1); ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); if (p->flags & ACPI_HEST_GLOBAL) { ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI AER: Handle non-AER HEST error sources in aer_hest_parse() 2013-12-13 22:16 ` Bjorn Helgaas @ 2013-12-13 22:47 ` Betty Dall 2013-12-13 23:06 ` Bjorn Helgaas 0 siblings, 1 reply; 6+ messages in thread From: Betty Dall @ 2013-12-13 22:47 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: lenb, rjw, linux-acpi, linux-kernel, linux-pci On Fri, 2013-12-13 at 15:16 -0700, Bjorn Helgaas wrote: > On Fri, Dec 13, 2013 at 08:23:16AM -0700, Betty Dall wrote: > > aer_hest_parse() could pass a non-AER HEST error source to the function > > hest_match_pci(). hest_match_pci() assumes that the HEST error source is > > type ACPI_HEST_TYPE_AER_ROOT_PORT, ACPI_HEST_TYPE_AER_ENDPOINT, or > > ACPI_HEST_TYPE_AER_BRIDGE. I have a platform that has some > > ACPI_HEST_TYPE_GENERIC error sources where hest_match_pci() was trying to > > access the structure as if it had the acpi_hest_aer_common fields. > > > > aer_hest_parse() is only ever interested in the AER type HEST error > > sources. Add a check on the type of the error souce at the beginning of > > aer_hest_parse(). > > > > Signed-off-by: Betty Dall <betty.dall@hp.com> > > --- > > > > drivers/pci/pcie/aer/aerdrv_acpi.c | 4 ++++ > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c > > index cf611ab..39186e3 100644 > > --- a/drivers/pci/pcie/aer/aerdrv_acpi.c > > +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c > > @@ -56,6 +56,10 @@ static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data) > > struct acpi_hest_aer_common *p; > > int ff; > > > > + if (hest_hdr->type != ACPI_HEST_TYPE_AER_ROOT_PORT && > > + hest_hdr->type != ACPI_HEST_TYPE_AER_ENDPOINT && > > + hest_hdr->type != ACPI_HEST_TYPE_AER_BRIDGE) > > + return 0; > > p = (struct acpi_hest_aer_common *)(hest_hdr + 1); > > ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); > > if (p->flags & ACPI_HEST_GLOBAL) { > > Hi Betty, > > I propose the following tweaked version. Will this work for you? > > > PCI/AER: Ingore non-PCIe AER error sources in aer_hest_parse() > > From: Betty Dall <betty.dall@hp.com> > > aer_set_firmware_first() searches the HEST for an error source descriptor > matching the specified PCI device. It uses the apei_hest_parse() iterator > to call aer_hest_parse() for every descriptor in the HEST. > > Previously, aer_hest_parse() incorrectly assumed every descriptor was for a > PCIe error source. This patch adds a check to avoid that error. > > [bhelgaas: factor check into helper function, changelog] > Signed-off-by: Betty Dall <betty.dall@hp.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/pcie/aer/aerdrv_acpi.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c > index cf611ab2193a..f670313479ee 100644 > --- a/drivers/pci/pcie/aer/aerdrv_acpi.c > +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c > @@ -50,12 +50,24 @@ struct aer_hest_parse_info { > int firmware_first; > }; > > +static int hest_source_is_pcie_aer(struct acpi_hest_header *hest_hdr) > +{ > + if (hest_hdr->type == ACPI_HEST_TYPE_AER_ROOT_PORT || > + hest_hdr->type == ACPI_HEST_TYPE_AER_ENDPOINT || > + hest_hdr->type == ACPI_HEST_TYPE_AER_BRIDGE) > + return 1; > + return 0; > +} > + > 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; > > + if (!hest_source_is_pcie_aer(hest_hdr)) > + return 0; > + > p = (struct acpi_hest_aer_common *)(hest_hdr + 1); > ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); > if (p->flags & ACPI_HEST_GLOBAL) { Hi Bjorn, Yes, that is more readable code. Thanks for revising it. I tested it on my system that has non-AER error sources and it works fine. One nit is that "Ignore" is misspelled in the subject line. -Betty ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI AER: Handle non-AER HEST error sources in aer_hest_parse() 2013-12-13 22:47 ` Betty Dall @ 2013-12-13 23:06 ` Bjorn Helgaas 2013-12-16 15:15 ` Betty Dall 0 siblings, 1 reply; 6+ messages in thread From: Bjorn Helgaas @ 2013-12-13 23:06 UTC (permalink / raw) To: Betty Dall; +Cc: lenb, rjw, linux-acpi, linux-kernel, linux-pci On Fri, Dec 13, 2013 at 03:47:33PM -0700, Betty Dall wrote: > Yes, that is more readable code. Thanks for revising it. I tested it on > my system that has non-AER error sources and it works fine. One nit is > that "Ignore" is misspelled in the subject line. Thanks, fixed. I should have thought longer before hitting send. I think it's worthwhile to use the same helper in aer_hest_parse_aff(), and once I did that, it became more obvious that aer_hest_parse() and aer_hest_parse_aff() are essentially similar, so I wonder if it's worth consolidating them, as below. It doesn't reduce the number of lines of code, but maybe it's simpler for the reader? Bjorn commit 8e7f8d0b30d4e3e30007b10eb2916d970b5f8c93 Author: Betty Dall <betty.dall@hp.com> Date: Fri Dec 13 08:23:16 2013 -0700 PCI/AER: Ignore non-PCIe AER error sources in aer_hest_parse() aer_set_firmware_first() searches the HEST for an error source descriptor matching the specified PCI device. It uses the apei_hest_parse() iterator to call aer_hest_parse() for every descriptor in the HEST. Previously, aer_hest_parse() incorrectly assumed every descriptor was for a PCIe error source. This patch adds a check to avoid that error. [bhelgaas: factor check into helper, use in aer_hest_parse_aff(), changelog] Signed-off-by: Betty Dall <betty.dall@hp.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c index cf611ab2193a..a23995749f1d 100644 --- a/drivers/pci/pcie/aer/aerdrv_acpi.c +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c @@ -50,12 +50,24 @@ struct aer_hest_parse_info { int firmware_first; }; +static int hest_source_is_pcie_aer(struct acpi_hest_header *hest_hdr) +{ + if (hest_hdr->type == ACPI_HEST_TYPE_AER_ROOT_PORT || + hest_hdr->type == ACPI_HEST_TYPE_AER_ENDPOINT || + hest_hdr->type == ACPI_HEST_TYPE_AER_BRIDGE) + return 1; + return 0; +} + 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; + if (!hest_source_is_pcie_aer(hest_hdr)) + return 0; + p = (struct acpi_hest_aer_common *)(hest_hdr + 1); ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); if (p->flags & ACPI_HEST_GLOBAL) { @@ -104,15 +116,12 @@ static int aer_hest_parse_aff(struct acpi_hest_header *hest_hdr, void *data) if (aer_firmware_first) return 0; - switch (hest_hdr->type) { - case ACPI_HEST_TYPE_AER_ROOT_PORT: - case ACPI_HEST_TYPE_AER_ENDPOINT: - case ACPI_HEST_TYPE_AER_BRIDGE: - p = (struct acpi_hest_aer_common *)(hest_hdr + 1); - aer_firmware_first = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); - default: + if (!hest_source_is_pcie_aer(hest_hdr)) return 0; - } + + p = (struct acpi_hest_aer_common *)(hest_hdr + 1); + aer_firmware_first = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); + return 0; } /** commit 01600a1b034f93dab6f855c81626b8030b85aec0 Author: Bjorn Helgaas <bhelgaas@google.com> Date: Fri Dec 13 15:42:53 2013 -0700 PCI/AER: Consolidate HEST error source parsers aer_hest_parse() and aer_hest_parse_aff() are almost identical. We use aer_hest_parse() to check the ACPI_HEST_FIRMWARE_FIRST flag for a specific device, and we use aer_hest_parse_aff() to check to see if any device sets the flag. This drops aer_hest_parse_aff() and enhances aer_hest_parse() so it collects the union of the ACPI_HEST_FIRMWARE_FIRST flag setting when no specific device is supplied. No functional change. Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c index a23995749f1d..4d6991794fa2 100644 --- a/drivers/pci/pcie/aer/aerdrv_acpi.c +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c @@ -70,6 +70,17 @@ static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data) p = (struct acpi_hest_aer_common *)(hest_hdr + 1); ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); + + /* + * If no specific device is supplied, determine whether + * FIRMWARE_FIRST is set for *any* PCIe device. + */ + if (!info->pci_dev) { + info->firmware_first |= ff; + return 0; + } + + /* Otherwise, check the specific device */ if (p->flags & ACPI_HEST_GLOBAL) { if (hest_match_type(hest_hdr, info->pci_dev)) info->firmware_first = ff; @@ -109,30 +120,20 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev) static bool aer_firmware_first; -static int aer_hest_parse_aff(struct acpi_hest_header *hest_hdr, void *data) -{ - struct acpi_hest_aer_common *p; - - if (aer_firmware_first) - return 0; - - if (!hest_source_is_pcie_aer(hest_hdr)) - return 0; - - p = (struct acpi_hest_aer_common *)(hest_hdr + 1); - aer_firmware_first = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); - return 0; -} - /** * aer_acpi_firmware_first - Check if APEI should control AER. */ bool aer_acpi_firmware_first(void) { static bool parsed = false; + struct aer_hest_parse_info info = { + .pci_dev = NULL, /* Check all PCIe devices */ + .firmware_first = 0, + }; if (!parsed) { - apei_hest_parse(aer_hest_parse_aff, NULL); + apei_hest_parse(aer_hest_parse, &info); + aer_firmware_first = info.firmware_first; parsed = true; } return aer_firmware_first; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI AER: Handle non-AER HEST error sources in aer_hest_parse() 2013-12-13 23:06 ` Bjorn Helgaas @ 2013-12-16 15:15 ` Betty Dall 2013-12-16 19:12 ` Bjorn Helgaas 0 siblings, 1 reply; 6+ messages in thread From: Betty Dall @ 2013-12-16 15:15 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: lenb, rjw, linux-acpi, linux-kernel, linux-pci On Fri, 2013-12-13 at 16:06 -0700, Bjorn Helgaas wrote: > I should have thought longer before hitting send. I think it's worthwhile > to use the same helper in aer_hest_parse_aff(), and once I did that, it > became more obvious that aer_hest_parse() and aer_hest_parse_aff() are > essentially similar, so I wonder if it's worth consolidating them, as > below. It doesn't reduce the number of lines of code, but maybe it's > simpler for the reader? Hi Bjorn, That is a nice cleanup. I tested it on my firmware first system. Also, I double checked back through the ACPI spec, and there are FIRMWARE_FIRST flags in the IA-32 Architecture Machine Check Exception and IA-32 Architecture Corrected Machine Check error sources too. These functions we are changing are specifically looking just for the AER error sources. That is correct because AER firmware first is separate from MCE/CMC firmware first. So, this patch looks good. -Betty ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI AER: Handle non-AER HEST error sources in aer_hest_parse() 2013-12-16 15:15 ` Betty Dall @ 2013-12-16 19:12 ` Bjorn Helgaas 0 siblings, 0 replies; 6+ messages in thread From: Bjorn Helgaas @ 2013-12-16 19:12 UTC (permalink / raw) To: Betty Dall Cc: Len Brown, Rafael J. Wysocki, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org On Mon, Dec 16, 2013 at 8:15 AM, Betty Dall <betty.dall@hp.com> wrote: > On Fri, 2013-12-13 at 16:06 -0700, Bjorn Helgaas wrote: > >> I should have thought longer before hitting send. I think it's worthwhile >> to use the same helper in aer_hest_parse_aff(), and once I did that, it >> became more obvious that aer_hest_parse() and aer_hest_parse_aff() are >> essentially similar, so I wonder if it's worth consolidating them, as >> below. It doesn't reduce the number of lines of code, but maybe it's >> simpler for the reader? > > > Hi Bjorn, > > That is a nice cleanup. I tested it on my firmware first system. > > Also, I double checked back through the ACPI spec, and there are > FIRMWARE_FIRST flags in the IA-32 Architecture Machine Check Exception > and IA-32 Architecture Corrected Machine Check error sources too. These > functions we are changing are specifically looking just for the AER > error sources. That is correct because AER firmware first is separate > from MCE/CMC firmware first. So, this patch looks good. Thanks, I added your "Reviewed-by". Bjorn ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-12-16 19:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-12-13 15:23 [PATCH] PCI AER: Handle non-AER HEST error sources in aer_hest_parse() Betty Dall 2013-12-13 22:16 ` Bjorn Helgaas 2013-12-13 22:47 ` Betty Dall 2013-12-13 23:06 ` Bjorn Helgaas 2013-12-16 15:15 ` Betty Dall 2013-12-16 19:12 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).