Messages in this thread | | | From | "Sean V Kelley" <> | Subject | Re: [PATCH v7 07/13] PCI/AER: Extend AER error handling to RCECs | Date | Thu, 01 Oct 2020 21:15:10 -0700 |
| |
On 1 Oct 2020, at 16:14, Bjorn Helgaas wrote:
> On Wed, Sep 30, 2020 at 02:58:14PM -0700, Sean V Kelley wrote: >> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> >> Currently the kernel does not handle AER errors for Root Complex >> integrated End Points (RCiEPs)[0]. These devices sit on a root bus >> within >> the Root Complex (RC). AER handling is performed by a Root Complex >> Event >> Collector (RCEC) [1] which is a effectively a type of RCiEP on the >> same >> root bus. >> >> For an RCEC (technically not a Bridge), error messages "received" >> from >> associated RCiEPs must be enabled for "transmission" in order to >> cause a >> System Error via the Root Control register or (when the Advanced >> Error >> Reporting Capability is present) reporting via the Root Error Command >> register and logging in the Root Error Status register and Error >> Source >> Identification register. >> >> In addition to the defined OS level handling of the reset flow for >> the >> associated RCiEPs of an RCEC, it is possible to also have non-native >> handling. In that case there is no need to take any actions on the >> RCEC >> because the firmware is responsible for them. This is true where APEI >> [2] >> is used to report the AER errors via a GHES[v2] HEST entry [3] and >> relevant AER CPER record [4] and non-native handling is in use. >> >> We effectively end up with two different types of discovery for >> purposes of handling AER errors: >> >> 1) Normal bus walk - we pass the downstream port above a bus to which >> the device is attached and it walks everything below that point. >> >> 2) An RCiEP with no visible association with an RCEC as there is no >> need >> to walk devices. In that case, the flow is to just call the callbacks >> for >> the actual device, which in turn references its associated RCEC. >> >> A new walk function pci_walk_bridge(), similar to pci_walk_bus(), >> is provided that takes a pci_dev instead of a bus. If that bridge >> corresponds to a downstream port it will walk the subordinate bus of >> that bridge. If the device does not then it will call the function on >> that device alone. >> >> [0] ACPI PCI Express Base Specification 5.0-1 1.3.2.3 Root Complex >> Integrated Endpoint Rules. >> [1] ACPI PCI Express Base Specification 5.0-1 6.2 Error Signalling >> and >> Logging >> [2] ACPI Specification 6.3 Chapter 18 ACPI Platform Error Interface >> (APEI) >> [3] ACPI Specification 6.3 18.2.3.7 Generic Hardware Error Source >> [4] UEFI Specification 2.8, N.2.7 PCI Express Error Section >> >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com> >> --- >> drivers/pci/pcie/err.c | 52 >> +++++++++++++++++++++++++++++++++--------- >> 1 file changed, 41 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c >> index 9e552330155b..c4ceca42a3bf 100644 >> --- a/drivers/pci/pcie/err.c >> +++ b/drivers/pci/pcie/err.c >> @@ -146,44 +146,73 @@ static int report_resume(struct pci_dev *dev, >> void *data) >> return 0; >> } >> >> +/** >> + * pci_walk_bridge - walk bridges potentially AER affected >> + * @bridge bridge which may be an RCEC with associated RCiEPs, >> + * an RCiEP associated with an RCEC, or a Port. >> + * @cb callback to be called for each device found >> + * @userdata arbitrary pointer to be passed to callback. >> + * >> + * If the device provided is a bridge, walk the subordinate bus, >> + * including any bridged devices on buses under this bus. >> + * Call the provided callback on each device found. >> + * >> + * If the device provided has no subordinate bus, call the provided >> + * callback on the device itself. >> + */ >> +static void pci_walk_bridge(struct pci_dev *bridge, int (*cb)(struct >> pci_dev *, void *), >> + void *userdata) >> +{ >> + if (bridge->subordinate) >> + pci_walk_bus(bridge->subordinate, cb, userdata); >> + else >> + cb(bridge, userdata); >> +} >> + >> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, >> pci_channel_state_t state, >> pci_ers_result_t (*reset_subordinate_devices)(struct pci_dev >> *pdev)) >> { >> pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER; >> - struct pci_bus *bus; >> struct pci_dev *bridge; >> int type; >> >> /* >> * Error recovery runs on all subordinates of the first downstream >> * bridge. If the downstream bridge detected the error, it is >> - * cleared at the end. >> + * cleared at the end. For RCiEPs we should reset just the RCiEP >> itself. >> */ >> type = pci_pcie_type(dev); >> if (type == PCI_EXP_TYPE_ROOT_PORT || >> - type == PCI_EXP_TYPE_DOWNSTREAM) >> + type == PCI_EXP_TYPE_DOWNSTREAM || >> + type == PCI_EXP_TYPE_RC_EC || >> + type == PCI_EXP_TYPE_RC_END) >> bridge = dev; >> else >> bridge = pci_upstream_bridge(dev); >> >> - bus = bridge->subordinate; >> pci_dbg(dev, "broadcast error_detected message\n"); >> if (state == pci_channel_io_frozen) { >> - pci_walk_bus(bus, report_frozen_detected, &status); >> - status = reset_subordinate_device(dev); >> + pci_walk_bridge(bridge, report_frozen_detected, &status); > > Wonder if it would be worth splitting out the pci_walk_bus() to > pci_walk_bridge() change -- initially pci_walk_bridge() would do only > this: > > if (bridge->subordinate) > pci_walk_bus(bridge->subordinate, cb, userdata); > > so basically just rename it and move the bridge->subordinate > dereference out.
Sure, that’s fine. It was actually something that crossed my mind when I was doing this prior splitting out because I realized I still needed to dereference the bus and was disappointed to keep it here.
> > Then the next patch would be a lot smaller and would add the > !bridge->subordinate case (which I think is only for RC_EC & RC_END?)
Correct the check on bridge && bridge->subordinate comes in with the RC_EC & RC_END
> >> + if (type == PCI_EXP_TYPE_RC_END) { >> + pci_warn(dev, "subordinate device reset not possible for >> RCiEP\n"); >> + status = PCI_ERS_RESULT_NONE; >> + goto failed; >> + } >> + >> + status = reset_subordinate_devices(bridge); > > I missed the reason for this change: > > - status = reset_subordinate_device(dev); > + status = reset_subordinate_devices(bridge);
This should have happened in the ‘bridge’ renaming patch. This was going to be either a reset of dev or dev->bus->self depending on the type via the assignment of dev = prior to renaming in (5/13). I should move this change back to the bridge renaming patch.
Thanks,
Sean
> >> if (status != PCI_ERS_RESULT_RECOVERED) { >> pci_warn(dev, "subordinate device reset failed\n"); >> goto failed; >> } >> } else { >> - pci_walk_bus(bus, report_normal_detected, &status); >> + pci_walk_bridge(bridge, report_normal_detected, &status); >> } >> >> if (status == PCI_ERS_RESULT_CAN_RECOVER) { >> status = PCI_ERS_RESULT_RECOVERED; >> pci_dbg(dev, "broadcast mmio_enabled message\n"); >> - pci_walk_bus(bus, report_mmio_enabled, &status); >> + pci_walk_bridge(bridge, report_mmio_enabled, &status); >> } >> >> if (status == PCI_ERS_RESULT_NEED_RESET) { >> @@ -194,17 +223,18 @@ pci_ers_result_t pcie_do_recovery(struct >> pci_dev *dev, >> */ >> status = PCI_ERS_RESULT_RECOVERED; >> pci_dbg(dev, "broadcast slot_reset message\n"); >> - pci_walk_bus(bus, report_slot_reset, &status); >> + pci_walk_bridge(bridge, report_slot_reset, &status); >> } >> >> if (status != PCI_ERS_RESULT_RECOVERED) >> goto failed; >> >> pci_dbg(dev, "broadcast resume message\n"); >> - pci_walk_bus(bus, report_resume, &status); >> + pci_walk_bridge(bridge, report_resume, &status); >> >> if (type == PCI_EXP_TYPE_ROOT_PORT || >> - type == PCI_EXP_TYPE_DOWNSTREAM) { >> + type == PCI_EXP_TYPE_DOWNSTREAM || >> + type == PCI_EXP_TYPE_RC_EC) { >> if (pcie_aer_is_native(bridge)) >> pcie_clear_device_status(bridge); >> pci_aer_clear_nonfatal_status(bridge); >> -- >> 2.28.0 >>
| |