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@xxxxxx> 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@xxxxxx> 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@xxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/