[PATCH] PCI AER: handle pci_cleanup_aer_uncorrect_error_status() in firmware first mode

[PATCH] PCI AER: handle pci_cleanup_aer_uncorrect_error_status() in firmware first mode

[Betty Dall]

There are three functions exported from aerdrv_core.c that could be
called when the system is in firmware first mode:
pci_enable_pcie_error_reporting(), pci_disable_pcie_error_reporting, and
pci_cleanup_aer_uncorrect_error_status(). The first two functions check if
we are in firmware first mode and return immediately.
pci_cleanup_aer_uncorrect_error_status() does not check firmware first
mode. The problem is that all of these functions should not access the AER
registers in firmware first mode because the firmware has not granted OS
control of the AER registers through the _OSC. Many drivers call this
function in their pci_error_handlers in firmware first mode.

The fix is to change pci_cleanup_aer_uncorrect_error_status() to check
firmware first mode before accessing the AER registers. If it is in firmware
first mode, return 0. I considered returning -EIO, but decided the status
has been cleaned up appropriately for firmware first. Returning 0 also avoids
an error message. Not many places check the return of this function, and the
ones that do, print an error message and continue such as:
   err = pci_cleanup_aer_uncorrect_error_status(pdev);
   if (err) {
       dev_err(&pdev->dev,
           "pci_cleanup_aer_uncorrect_error_status failed 0x%0x\n",
            err); /* non-fatal, continue */
   }
That error message is how I found this problem, and it is not applicable
for the firmware first recovery path.

Signed-off-by: Betty Dall <betty.dall@hp.com>
---

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


diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index b2c8881..1f60408 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -62,6 +62,9 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
 	int pos;
 	u32 status;
 
+	if (pcie_aer_get_firmware_first(dev))
+		return 0;
+
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
 	if (!pos)
 		return -EIO;

Re: [PATCH] PCI AER: handle pci_cleanup_aer_uncorrect_error_status() in firmware first mode

[Bjorn Helgaas]

On Fri, Dec 13, 2013 at 8:41 AM, Betty Dall <betty.dall@hp.com> wrote:
> There are three functions exported from aerdrv_core.c that could be
> called when the system is in firmware first mode:
> pci_enable_pcie_error_reporting(), pci_disable_pcie_error_reporting, and
> pci_cleanup_aer_uncorrect_error_status(). The first two functions check if
> we are in firmware first mode and return immediately.
> pci_cleanup_aer_uncorrect_error_status() does not check firmware first
> mode. The problem is that all of these functions should not access the AER
> registers in firmware first mode because the firmware has not granted OS
> control of the AER registers through the _OSC.

This looks like a good fix to me.  If I read aer_acpi_firmware_first()
correctly, we don't even *ask* for control of AER if
ACPI_HEST_FIRMWARE_FIRST appears anywhere in the HEST.  Does that
match your understanding?

> Many drivers call this
> function in their pci_error_handlers in firmware first mode.

Drivers don't have any idea whether their device is in firmware-first
mode, do they?

> The fix is to change pci_cleanup_aer_uncorrect_error_status() to check
> firmware first mode before accessing the AER registers. If it is in firmware
> first mode, return 0. I considered returning -EIO, but decided the status
> has been cleaned up appropriately for firmware first. Returning 0 also avoids
> an error message. Not many places check the return of this function, and the
> ones that do, print an error message and continue such as:
>    err = pci_cleanup_aer_uncorrect_error_status(pdev);
>    if (err) {
>        dev_err(&pdev->dev,
>            "pci_cleanup_aer_uncorrect_error_status failed 0x%0x\n",
>             err); /* non-fatal, continue */
>    }
> That error message is how I found this problem, and it is not applicable
> for the firmware first recovery path.

I'm curious -- did you find this problem because you saw a message
when pci_cleanup_aer_uncorrect_error_status() returned failure?  The
only way it can return failure is if there is no AER capability, and
that should be completely independent of whether we're in
firmware-first mode.

> Signed-off-by: Betty Dall <betty.dall@hp.com>
> ---
>
>  drivers/pci/pcie/aer/aerdrv_core.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
>
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> index b2c8881..1f60408 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -62,6 +62,9 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
>         int pos;
>         u32 status;
>
> +       if (pcie_aer_get_firmware_first(dev))
> +               return 0;
> +
>         pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
>         if (!pos)
>                 return -EIO;

Re: [PATCH] PCI AER: handle pci_cleanup_aer_uncorrect_error_status() in firmware first mode

[Betty Dall]

On Fri, 2013-12-13 at 15:35 -0700, Bjorn Helgaas wrote:
> On Fri, Dec 13, 2013 at 8:41 AM, Betty Dall <betty.dall@hp.com> wrote:
> > There are three functions exported from aerdrv_core.c that could be
> > called when the system is in firmware first mode:
> > pci_enable_pcie_error_reporting(), pci_disable_pcie_error_reporting, and
> > pci_cleanup_aer_uncorrect_error_status(). The first two functions check if
> > we are in firmware first mode and return immediately.
> > pci_cleanup_aer_uncorrect_error_status() does not check firmware first
> > mode. The problem is that all of these functions should not access the AER
> > registers in firmware first mode because the firmware has not granted OS
> > control of the AER registers through the _OSC.
> 
> This looks like a good fix to me.  If I read aer_acpi_firmware_first()
> correctly, we don't even *ask* for control of AER if
> ACPI_HEST_FIRMWARE_FIRST appears anywhere in the HEST.  Does that
> match your understanding?

Yes, when the system is in firmware first mode the code setting the _OSC
control register does not ask for AER control. 

> 
> > Many drivers call this
> > function in their pci_error_handlers in firmware first mode.
> 
> Drivers don't have any idea whether their device is in firmware-first
> mode, do they?

Right. And I think we want to keep it that way. Having this function is
a good thing so that the firmware first can be abstracted from the
drivers.

> 
> > The fix is to change pci_cleanup_aer_uncorrect_error_status() to check
> > firmware first mode before accessing the AER registers. If it is in firmware
> > first mode, return 0. I considered returning -EIO, but decided the status
> > has been cleaned up appropriately for firmware first. Returning 0 also avoids
> > an error message. Not many places check the return of this function, and the
> > ones that do, print an error message and continue such as:
> >    err = pci_cleanup_aer_uncorrect_error_status(pdev);
> >    if (err) {
> >        dev_err(&pdev->dev,
> >            "pci_cleanup_aer_uncorrect_error_status failed 0x%0x\n",
> >             err); /* non-fatal, continue */
> >    }
> > That error message is how I found this problem, and it is not applicable
> > for the firmware first recovery path.
> 
> I'm curious -- did you find this problem because you saw a message
> when pci_cleanup_aer_uncorrect_error_status() returned failure?  The
> only way it can return failure is if there is no AER capability, and
> that should be completely independent of whether we're in
> firmware-first mode.

Yes, I saw the error message during error injection testing and using a
firmware that denies access to AER control because it is firmware first.
You are right that it would only print out when there is no AER
capability. I was reading code looking for places that might access the
AER registers in firmware first mode. This is the only one I found.

-Betty

> 
> > Signed-off-by: Betty Dall <betty.dall@hp.com>
> > ---
> >
> >  drivers/pci/pcie/aer/aerdrv_core.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> >
> > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> > index b2c8881..1f60408 100644
> > --- a/drivers/pci/pcie/aer/aerdrv_core.c
> > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> > @@ -62,6 +62,9 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
> >         int pos;
> >         u32 status;
> >
> > +       if (pcie_aer_get_firmware_first(dev))
> > +               return 0;
> > +
> >         pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> >         if (!pos)
> >                 return -EIO;

Re: [PATCH] PCI AER: handle pci_cleanup_aer_uncorrect_error_status() in firmware first mode

[Bjorn Helgaas]

On Fri, Dec 13, 2013 at 4:16 PM, Betty Dall <betty.dall@hp.com> wrote:
> On Fri, 2013-12-13 at 15:35 -0700, Bjorn Helgaas wrote:
>> On Fri, Dec 13, 2013 at 8:41 AM, Betty Dall <betty.dall@hp.com> wrote:
>> > There are three functions exported from aerdrv_core.c that could be
>> > called when the system is in firmware first mode:
>> > pci_enable_pcie_error_reporting(), pci_disable_pcie_error_reporting, and
>> > pci_cleanup_aer_uncorrect_error_status(). The first two functions check if
>> > we are in firmware first mode and return immediately.
>> > pci_cleanup_aer_uncorrect_error_status() does not check firmware first
>> > mode. The problem is that all of these functions should not access the AER
>> > registers in firmware first mode because the firmware has not granted OS
>> > control of the AER registers through the _OSC.
>>
>> This looks like a good fix to me.  If I read aer_acpi_firmware_first()
>> correctly, we don't even *ask* for control of AER if
>> ACPI_HEST_FIRMWARE_FIRST appears anywhere in the HEST.  Does that
>> match your understanding?
>
> Yes, when the system is in firmware first mode the code setting the _OSC
> control register does not ask for AER control.
>
>>
>> > Many drivers call this
>> > function in their pci_error_handlers in firmware first mode.
>>
>> Drivers don't have any idea whether their device is in firmware-first
>> mode, do they?
>
> Right. And I think we want to keep it that way. Having this function is
> a good thing so that the firmware first can be abstracted from the
> drivers.
>
>>
>> > The fix is to change pci_cleanup_aer_uncorrect_error_status() to check
>> > firmware first mode before accessing the AER registers. If it is in firmware
>> > first mode, return 0. I considered returning -EIO, but decided the status
>> > has been cleaned up appropriately for firmware first. Returning 0 also avoids
>> > an error message. Not many places check the return of this function, and the
>> > ones that do, print an error message and continue such as:
>> >    err = pci_cleanup_aer_uncorrect_error_status(pdev);
>> >    if (err) {
>> >        dev_err(&pdev->dev,
>> >            "pci_cleanup_aer_uncorrect_error_status failed 0x%0x\n",
>> >             err); /* non-fatal, continue */
>> >    }
>> > That error message is how I found this problem, and it is not applicable
>> > for the firmware first recovery path.
>>
>> I'm curious -- did you find this problem because you saw a message
>> when pci_cleanup_aer_uncorrect_error_status() returned failure?  The
>> only way it can return failure is if there is no AER capability, and
>> that should be completely independent of whether we're in
>> firmware-first mode.
>
> Yes, I saw the error message during error injection testing and using a
> firmware that denies access to AER control because it is firmware first.
> You are right that it would only print out when there is no AER
> capability. I was reading code looking for places that might access the
> AER registers in firmware first mode. This is the only one I found.

I see why you added a pcie_aer_get_firmware_first() test, because
that's what pci_enable_pcie_error_reporting() and
pci_disable_pcie_error_reporting() do.

But I think we implemented the firmware-first stuff wrong by elevating
the firmware-first concept to the arch-independent level.  The
connection between this and the _OSC negotiation is pretty convoluted,
even on x86.  It's hard to verify by reading the code that we avoid
touching AER if we haven't asked for control or the BIOS declined to
grant it.

I think it would be better if the pci_dev.__aer_firmware_first stuff
were replaced by a more generic "can we use AER?" flag.  That flag
should be set at device enumeration time, so we wouldn't need anything
like the __aer_firmware_first_valid flag.

Bjorn

>> > Signed-off-by: Betty Dall <betty.dall@hp.com>
>> > ---
>> >
>> >  drivers/pci/pcie/aer/aerdrv_core.c |    3 +++
>> >  1 files changed, 3 insertions(+), 0 deletions(-)
>> >
>> >
>> > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
>> > index b2c8881..1f60408 100644
>> > --- a/drivers/pci/pcie/aer/aerdrv_core.c
>> > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
>> > @@ -62,6 +62,9 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
>> >         int pos;
>> >         u32 status;
>> >
>> > +       if (pcie_aer_get_firmware_first(dev))
>> > +               return 0;
>> > +
>> >         pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
>> >         if (!pos)
>> >                 return -EIO;
>
>

Re: [PATCH] PCI AER: handle pci_cleanup_aer_uncorrect_error_status() in firmware first mode

[Bjorn Helgaas]

On Mon, Dec 16, 2013 at 12:51 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Dec 13, 2013 at 4:16 PM, Betty Dall <betty.dall@hp.com> wrote:
>> On Fri, 2013-12-13 at 15:35 -0700, Bjorn Helgaas wrote:
>>> On Fri, Dec 13, 2013 at 8:41 AM, Betty Dall <betty.dall@hp.com> wrote:
>>> > There are three functions exported from aerdrv_core.c that could be
>>> > called when the system is in firmware first mode:
>>> > pci_enable_pcie_error_reporting(), pci_disable_pcie_error_reporting, and
>>> > pci_cleanup_aer_uncorrect_error_status(). The first two functions check if
>>> > we are in firmware first mode and return immediately.
>>> > pci_cleanup_aer_uncorrect_error_status() does not check firmware first
>>> > mode. The problem is that all of these functions should not access the AER
>>> > registers in firmware first mode because the firmware has not granted OS
>>> > control of the AER registers through the _OSC.
>>>
>>> This looks like a good fix to me.  If I read aer_acpi_firmware_first()
>>> correctly, we don't even *ask* for control of AER if
>>> ACPI_HEST_FIRMWARE_FIRST appears anywhere in the HEST.  Does that
>>> match your understanding?
>>
>> Yes, when the system is in firmware first mode the code setting the _OSC
>> control register does not ask for AER control.
>>
>>>
>>> > Many drivers call this
>>> > function in their pci_error_handlers in firmware first mode.
>>>
>>> Drivers don't have any idea whether their device is in firmware-first
>>> mode, do they?
>>
>> Right. And I think we want to keep it that way. Having this function is
>> a good thing so that the firmware first can be abstracted from the
>> drivers.
>>
>>>
>>> > The fix is to change pci_cleanup_aer_uncorrect_error_status() to check
>>> > firmware first mode before accessing the AER registers. If it is in firmware
>>> > first mode, return 0. I considered returning -EIO, but decided the status
>>> > has been cleaned up appropriately for firmware first. Returning 0 also avoids
>>> > an error message. Not many places check the return of this function, and the
>>> > ones that do, print an error message and continue such as:
>>> >    err = pci_cleanup_aer_uncorrect_error_status(pdev);
>>> >    if (err) {
>>> >        dev_err(&pdev->dev,
>>> >            "pci_cleanup_aer_uncorrect_error_status failed 0x%0x\n",
>>> >             err); /* non-fatal, continue */
>>> >    }
>>> > That error message is how I found this problem, and it is not applicable
>>> > for the firmware first recovery path.
>>>
>>> I'm curious -- did you find this problem because you saw a message
>>> when pci_cleanup_aer_uncorrect_error_status() returned failure?  The
>>> only way it can return failure is if there is no AER capability, and
>>> that should be completely independent of whether we're in
>>> firmware-first mode.
>>
>> Yes, I saw the error message during error injection testing and using a
>> firmware that denies access to AER control because it is firmware first.
>> You are right that it would only print out when there is no AER
>> capability. I was reading code looking for places that might access the
>> AER registers in firmware first mode. This is the only one I found.
>
> I see why you added a pcie_aer_get_firmware_first() test, because
> that's what pci_enable_pcie_error_reporting() and
> pci_disable_pcie_error_reporting() do.
>
> But I think we implemented the firmware-first stuff wrong by elevating
> the firmware-first concept to the arch-independent level.  The
> connection between this and the _OSC negotiation is pretty convoluted,
> even on x86.  It's hard to verify by reading the code that we avoid
> touching AER if we haven't asked for control or the BIOS declined to
> grant it.
>
> I think it would be better if the pci_dev.__aer_firmware_first stuff
> were replaced by a more generic "can we use AER?" flag.  That flag
> should be set at device enumeration time, so we wouldn't need anything
> like the __aer_firmware_first_valid flag.

I guess I'm assuming that firmware-first means "the OS should not use
AER at all for this device; any errors will be reported via HEST."  Is
that true?

The term "firmware-first" definitely suggests that the firmware gets
to process an error before the OS does anything.  It *could* also
suggest that after the firmware gets first chance, the OS can do its
normal error handling with AER.  But I hope that's not the case
because I think it would be hard to coordinate.

Bjorn

>>> > Signed-off-by: Betty Dall <betty.dall@hp.com>
>>> > ---
>>> >
>>> >  drivers/pci/pcie/aer/aerdrv_core.c |    3 +++
>>> >  1 files changed, 3 insertions(+), 0 deletions(-)
>>> >
>>> >
>>> > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
>>> > index b2c8881..1f60408 100644
>>> > --- a/drivers/pci/pcie/aer/aerdrv_core.c
>>> > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
>>> > @@ -62,6 +62,9 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
>>> >         int pos;
>>> >         u32 status;
>>> >
>>> > +       if (pcie_aer_get_firmware_first(dev))
>>> > +               return 0;
>>> > +
>>> >         pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
>>> >         if (!pos)
>>> >                 return -EIO;
>>
>>

Re: [PATCH] PCI AER: handle pci_cleanup_aer_uncorrect_error_status() in firmware first mode

[Betty Dall]

On Mon, 2013-12-16 at 12:51 -0700, Bjorn Helgaas wrote:
> On Fri, Dec 13, 2013 at 4:16 PM, Betty Dall <betty.dall@hp.com> wrote:
> > On Fri, 2013-12-13 at 15:35 -0700, Bjorn Helgaas wrote:
> >> On Fri, Dec 13, 2013 at 8:41 AM, Betty Dall <betty.dall@hp.com> wrote:
> >> > There are three functions exported from aerdrv_core.c that could be
> >> > called when the system is in firmware first mode:
> >> > pci_enable_pcie_error_reporting(), pci_disable_pcie_error_reporting, and
> >> > pci_cleanup_aer_uncorrect_error_status(). The first two functions check if
> >> > we are in firmware first mode and return immediately.
> >> > pci_cleanup_aer_uncorrect_error_status() does not check firmware first
> >> > mode. The problem is that all of these functions should not access the AER
> >> > registers in firmware first mode because the firmware has not granted OS
> >> > control of the AER registers through the _OSC.
> >>
> >> This looks like a good fix to me.  If I read aer_acpi_firmware_first()
> >> correctly, we don't even *ask* for control of AER if
> >> ACPI_HEST_FIRMWARE_FIRST appears anywhere in the HEST.  Does that
> >> match your understanding?
> >
> > Yes, when the system is in firmware first mode the code setting the _OSC
> > control register does not ask for AER control.
> >
> >>
> >> > Many drivers call this
> >> > function in their pci_error_handlers in firmware first mode.
> >>
> >> Drivers don't have any idea whether their device is in firmware-first
> >> mode, do they?
> >
> > Right. And I think we want to keep it that way. Having this function is
> > a good thing so that the firmware first can be abstracted from the
> > drivers.
> >
> >>
> >> > The fix is to change pci_cleanup_aer_uncorrect_error_status() to check
> >> > firmware first mode before accessing the AER registers. If it is in firmware
> >> > first mode, return 0. I considered returning -EIO, but decided the status
> >> > has been cleaned up appropriately for firmware first. Returning 0 also avoids
> >> > an error message. Not many places check the return of this function, and the
> >> > ones that do, print an error message and continue such as:
> >> >    err = pci_cleanup_aer_uncorrect_error_status(pdev);
> >> >    if (err) {
> >> >        dev_err(&pdev->dev,
> >> >            "pci_cleanup_aer_uncorrect_error_status failed 0x%0x\n",
> >> >             err); /* non-fatal, continue */
> >> >    }
> >> > That error message is how I found this problem, and it is not applicable
> >> > for the firmware first recovery path.
> >>
> >> I'm curious -- did you find this problem because you saw a message
> >> when pci_cleanup_aer_uncorrect_error_status() returned failure?  The
> >> only way it can return failure is if there is no AER capability, and
> >> that should be completely independent of whether we're in
> >> firmware-first mode.
> >
> > Yes, I saw the error message during error injection testing and using a
> > firmware that denies access to AER control because it is firmware first.
> > You are right that it would only print out when there is no AER
> > capability. I was reading code looking for places that might access the
> > AER registers in firmware first mode. This is the only one I found.
> 
> I see why you added a pcie_aer_get_firmware_first() test, because
> that's what pci_enable_pcie_error_reporting() and
> pci_disable_pcie_error_reporting() do.
> 
> But I think we implemented the firmware-first stuff wrong by elevating
> the firmware-first concept to the arch-independent level.  The
> connection between this and the _OSC negotiation is pretty convoluted,
> even on x86.  It's hard to verify by reading the code that we avoid
> touching AER if we haven't asked for control or the BIOS declined to
> grant it.
> 
> I think it would be better if the pci_dev.__aer_firmware_first stuff
> were replaced by a more generic "can we use AER?" flag.  That flag
> should be set at device enumeration time, so we wouldn't need anything
> like the __aer_firmware_first_valid flag.
> 
> Bjorn

Hi Bjorn,

I see what you are saying about the interaction of firmware first and
_OSC AER control being convoluted. I will work on some patches to
address this. 

I am thinking about a new flag __aer_control_granted that would be set
if _OSC control is granted to the OS for AER. We already account for
firmware first in negotiate_os_control(). An new function
aer_control_granted(struct pci_dev) could be used instead of
pcie_aer_get_firmware_first() in functions like
pci_enable_pcie_error_reporting(), pci_disable_pcie_error_reporting(),
and pci_cleanup_aer_uncorrect_error_status(). 

Another idea is to put a check for aer_control_granted() in
pci_find_ext_capability() so that any driver requesting the AER extended
capability would not even get the capability pointer if AER control has
not been granted. 

-Betty

Re: [PATCH] PCI AER: handle pci_cleanup_aer_uncorrect_error_status() in firmware first mode

[Bjorn Helgaas]

On Tue, Dec 17, 2013 at 11:33 AM, Betty Dall <betty.dall@hp.com> wrote:
> On Mon, 2013-12-16 at 12:51 -0700, Bjorn Helgaas wrote:
>> On Fri, Dec 13, 2013 at 4:16 PM, Betty Dall <betty.dall@hp.com> wrote:
>> > On Fri, 2013-12-13 at 15:35 -0700, Bjorn Helgaas wrote:
>> >> On Fri, Dec 13, 2013 at 8:41 AM, Betty Dall <betty.dall@hp.com> wrote:
>> >> > There are three functions exported from aerdrv_core.c that could be
>> >> > called when the system is in firmware first mode:
>> >> > pci_enable_pcie_error_reporting(), pci_disable_pcie_error_reporting, and
>> >> > pci_cleanup_aer_uncorrect_error_status(). The first two functions check if
>> >> > we are in firmware first mode and return immediately.
>> >> > pci_cleanup_aer_uncorrect_error_status() does not check firmware first
>> >> > mode. The problem is that all of these functions should not access the AER
>> >> > registers in firmware first mode because the firmware has not granted OS
>> >> > control of the AER registers through the _OSC.
>> >>
>> >> This looks like a good fix to me.  If I read aer_acpi_firmware_first()
>> >> correctly, we don't even *ask* for control of AER if
>> >> ACPI_HEST_FIRMWARE_FIRST appears anywhere in the HEST.  Does that
>> >> match your understanding?
>> >
>> > Yes, when the system is in firmware first mode the code setting the _OSC
>> > control register does not ask for AER control.
>> >
>> >>
>> >> > Many drivers call this
>> >> > function in their pci_error_handlers in firmware first mode.
>> >>
>> >> Drivers don't have any idea whether their device is in firmware-first
>> >> mode, do they?
>> >
>> > Right. And I think we want to keep it that way. Having this function is
>> > a good thing so that the firmware first can be abstracted from the
>> > drivers.
>> >
>> >>
>> >> > The fix is to change pci_cleanup_aer_uncorrect_error_status() to check
>> >> > firmware first mode before accessing the AER registers. If it is in firmware
>> >> > first mode, return 0. I considered returning -EIO, but decided the status
>> >> > has been cleaned up appropriately for firmware first. Returning 0 also avoids
>> >> > an error message. Not many places check the return of this function, and the
>> >> > ones that do, print an error message and continue such as:
>> >> >    err = pci_cleanup_aer_uncorrect_error_status(pdev);
>> >> >    if (err) {
>> >> >        dev_err(&pdev->dev,
>> >> >            "pci_cleanup_aer_uncorrect_error_status failed 0x%0x\n",
>> >> >             err); /* non-fatal, continue */
>> >> >    }
>> >> > That error message is how I found this problem, and it is not applicable
>> >> > for the firmware first recovery path.
>> >>
>> >> I'm curious -- did you find this problem because you saw a message
>> >> when pci_cleanup_aer_uncorrect_error_status() returned failure?  The
>> >> only way it can return failure is if there is no AER capability, and
>> >> that should be completely independent of whether we're in
>> >> firmware-first mode.
>> >
>> > Yes, I saw the error message during error injection testing and using a
>> > firmware that denies access to AER control because it is firmware first.
>> > You are right that it would only print out when there is no AER
>> > capability. I was reading code looking for places that might access the
>> > AER registers in firmware first mode. This is the only one I found.
>>
>> I see why you added a pcie_aer_get_firmware_first() test, because
>> that's what pci_enable_pcie_error_reporting() and
>> pci_disable_pcie_error_reporting() do.
>>
>> But I think we implemented the firmware-first stuff wrong by elevating
>> the firmware-first concept to the arch-independent level.  The
>> connection between this and the _OSC negotiation is pretty convoluted,
>> even on x86.  It's hard to verify by reading the code that we avoid
>> touching AER if we haven't asked for control or the BIOS declined to
>> grant it.
>>
>> I think it would be better if the pci_dev.__aer_firmware_first stuff
>> were replaced by a more generic "can we use AER?" flag.  That flag
>> should be set at device enumeration time, so we wouldn't need anything
>> like the __aer_firmware_first_valid flag.
>>
>> Bjorn
>
> Hi Bjorn,
>
> I see what you are saying about the interaction of firmware first and
> _OSC AER control being convoluted. I will work on some patches to
> address this.
>
> I am thinking about a new flag __aer_control_granted that would be set
> if _OSC control is granted to the OS for AER. We already account for
> firmware first in negotiate_os_control(). An new function
> aer_control_granted(struct pci_dev) could be used instead of
> pcie_aer_get_firmware_first() in functions like
> pci_enable_pcie_error_reporting(), pci_disable_pcie_error_reporting(),
> and pci_cleanup_aer_uncorrect_error_status().
>
> Another idea is to put a check for aer_control_granted() in
> pci_find_ext_capability() so that any driver requesting the AER extended
> capability would not even get the capability pointer if AER control has
> not been granted.

I kind of like the idea of a check in pci_find_ext_capability().  I
looked at all the users of PCI_EXT_CAP_ID_ERR, and it's not very clear
to me that they all keep their mitts off the AER capability when
firmware-first is enabled.

At least one (atl1c_reset_pcie()) requires a fix because it doesn't
check for failure of pci_find_ext_capability().

I'm not sure drivers like ixgbe will work correctly with
firmware-first.  I think the firmware-first path feeds into AER
(ghes_do_proc() calls aer_recover_queue()), which ultimately calls a
driver's .error_detected() method, such as ixgbe_io_error_detected(),
which reads the error info directly from the AER capability.  But I
assume that with firmware-first, the error info should come from the
firmware, not from the AER capability.  Right?

And I'm not sure what the effect of skipping other AER accesses is.
E.g., myhri10ge enables ECRC and masks link down events, netxen masks
AER correctable errors, pci_configure_slot() sets hotplug settings,
etc.  What happens if we skip all these things when firmware-first is
enabled?  What happens if we *don't* skip them; will we mess up things
set by the firmware?

Bjorn