[v2,1/3] PCI/AER: Fix incorrect return from aer_hest_parse()

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 <betty.dall@hp.com>
---

 drivers/pci/pcie/aer/aerdrv_acpi.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[+cc Bob for ACPI HEST spec questions]

On Thu, May 30, 2013 at 8:39 AM, Betty Dall <betty.dall@hp.com> 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 <betty.dall@hp.com>
> ---
>
>  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;

Not related directly to your patch, Betty, but I can't figure out why
the ACPI spec defines the HEST structures for PCIe as it does.  I'm
looking at ACPI 5.0, sec 18.3.2.3 - 18.3.2.5.

1) The PCIe Root Port, PCIe Device, and PCIe/PCI-X Bridge structures
all include Bus, Device, and Function fields.  But there's no Segment.
 The current Linux code (hest_match_pci()) assumes HEST records can
only apply to PCI domain 0.  Is Linux missing something, or is the
HEST really this limited?

2) Doesn't the fact that the HEST structures include a bus number mean
that the OS cannot reassign bus numbers?  I guess maybe we could still
reassign them if we kept track of the mapping back to the original
values.

3) It's interesting that the only choices seem to be "global" or "list
every device."  There's no "apply to this device and the subtree under
it," so I guess if you want HEST to apply to hot-added devices,
"global" is the only thing that makes sense?

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, 2013-06-01 at 18:38 -0600, Bjorn Helgaas wrote:
> [+cc Bob for ACPI HEST spec questions]
> 
> On Thu, May 30, 2013 at 8:39 AM, Betty Dall <betty.dall@hp.com> 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 <betty.dall@hp.com>
> > ---
> >
> >  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;
> 
> Not related directly to your patch, Betty, but I can't figure out why
> the ACPI spec defines the HEST structures for PCIe as it does.  I'm
> looking at ACPI 5.0, sec 18.3.2.3 - 18.3.2.5. 

> 1) The PCIe Root Port, PCIe Device, and PCIe/PCI-X Bridge structures
> all include Bus, Device, and Function fields.  But there's no Segment.
>  The current Linux code (hest_match_pci()) assumes HEST records can
> only apply to PCI domain 0.  Is Linux missing something, or is the
> HEST really this limited?
You are right that the HEST table does not have the Segment for the PCIe
sources. The Linux code uses the Generic Source type that points to a
UEFI CPER record. Those do have the Segment. The code in
acpi/apei/ghes.c that parses the HEST and invokes the
aer_recover_queue() is using the segment from the CPER record. 

The hest_match_pci() code is only looking at the PCIe error source types
because that is where the flags field is defined to indicate "firmware
first". Flags is not defined in the Generic error source.  Firmware
first platforms today must be using GLOBAL error sources because the
code is written in such a way that any specific PCIe match will cause
the entire platform to be firmware first. Linux only supports either AER
or firmware first right now, and it would be an enhancement to do both. 

> 2) Doesn't the fact that the HEST structures include a bus number mean
> that the OS cannot reassign bus numbers?  I guess maybe we could still
> reassign them if we kept track of the mapping back to the original
> values.
Yes, I think there needs to be a mapping between the HEST/CPER bus
number and the OS assigned bus numbers, if the OS is reassigning bus
numbers.  

> 3) It's interesting that the only choices seem to be "global" or "list
> every device."  There's no "apply to this device and the subtree under
> it," so I guess if you want HEST to apply to hot-added devices,
> "global" is the only thing that makes sense?
"Global" is the one one that makes sense to me too. The size of the HEST
table is fixed at boot and firmware strives to keep it as small as
possible because firmware memory is a scarce resource. 

Betty

> Bjorn


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, May 30, 2013 at 08:39:27AM -0600, Betty Dall wrote:
> Date:	Thu, 30 May 2013 08:39:27 -0600
> From: Betty Dall <betty.dall@hp.com>
> To: rjw@sisk.pl, bhelgaas@google.com
> Cc: ying.huang@intel.com, linux-acpi@vger.kernel.org,
>  linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, Betty Dall
>  <betty.dall@hp.com>
> Subject: [PATCH v2 1/3] PCI/AER: Fix incorrect return from aer_hest_parse()
> X-Mailer: git-send-email 1.7.7.6
> 
> 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 <betty.dall@hp.com>

The patch is good. But I have further concern based on your patch.
1) aer_hest_parse never checks the 2nd input parameter (void *data),
which means once it is NULL, it will crash the kernel.

2) both aer_hest_parse and aer_hest_parse_aff utilize some flag
as shortcut, if so, why not adding similar logic in apei_hest_parse
to stop meaningless loops once FFM is confirmed as enabled.

> ---
> 
>  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;
> --
> 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

On Mon, Jun 3, 2013 at 3:18 PM, Betty Dall <betty.dall@hp.com> wrote:
> On Sat, 2013-06-01 at 18:38 -0600, Bjorn Helgaas wrote:
>> [+cc Bob for ACPI HEST spec questions]
>>
>> On Thu, May 30, 2013 at 8:39 AM, Betty Dall <betty.dall@hp.com> 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 <betty.dall@hp.com>
>> > ---
>> >
>> >  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;
>>
>> Not related directly to your patch, Betty, but I can't figure out why
>> the ACPI spec defines the HEST structures for PCIe as it does.  I'm
>> looking at ACPI 5.0, sec 18.3.2.3 - 18.3.2.5.
>
>> 1) The PCIe Root Port, PCIe Device, and PCIe/PCI-X Bridge structures
>> all include Bus, Device, and Function fields.  But there's no Segment.
>>  The current Linux code (hest_match_pci()) assumes HEST records can
>> only apply to PCI domain 0.  Is Linux missing something, or is the
>> HEST really this limited?
> You are right that the HEST table does not have the Segment for the PCIe
> sources. The Linux code uses the Generic Source type that points to a
> UEFI CPER record. Those do have the Segment. The code in
> acpi/apei/ghes.c that parses the HEST and invokes the
> aer_recover_queue() is using the segment from the CPER record.

My question has nothing to do with CPER.  Drivers can enable error
reporting for their devices with pci_enable_pcie_error_reporting().
If the device is marked "firmware-first" in the HEST, this call fails
without enabling reporting.  The HEST can only mark devices in domain
0 as "firmware-first."  Therefore pci_enable_pcie_error_reporting()
will enable reporting for:

  - domain 0 devices not marked "firmware-first" and
  - all devices in other domains.

This inconsistency seems like a hole in the spec, but maybe I'm
missing something.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html