On Fri, Nov 20, 2020 at 04:10:28PM -0800, Sean V Kelley wrote:
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 7a5af873d8bc..46a5b84f8842 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -151,24 +151,27 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> pci_ers_result_t (*reset_subordinates)(struct pci_dev *pdev))
> {
> int type = pci_pcie_type(dev);
> - pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
> + struct pci_dev *bridge;
> struct pci_bus *bus;
> + pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
>
> /*
> - * Error recovery runs on all subordinates of the first downstream port.
> - * If the downstream port detected the error, it is cleared at the end.
> + * Error recovery runs on all subordinates of the bridge. If the
> + * bridge detected the error, it is cleared at the end.
> */
> if (!(type == PCI_EXP_TYPE_ROOT_PORT ||
> type == PCI_EXP_TYPE_DOWNSTREAM))
> - dev = pci_upstream_bridge(dev);
> - bus = dev->subordinate;
> + bridge = pci_upstream_bridge(dev);
> + else
> + bridge = dev;
I think there's a bug here even before your series. We started with:
pcie_do_recovery(struct pci_dev *dev, ..., pci_ers_result_t (*reset_link)())
{
if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
dev = dev->bus->self;
...
reset_link(dev);
so if we called pcie_do_recovery() with an Endpoint, we set "dev" to
the upstream bridge, either a Root Port or a Switch Downstream Port,
which we then pass on to reset_link(). For native AER and APEI,
that's aer_root_reset(), which assumes it gets a Root Port.
If we pass a Switch Downstream Port, aer_root_reset() writes to the
*switch port's* PCI_ERR_ROOT_COMMAND and PCI_ERR_ROOT_STATUS, which
are reserved since it's not a Root Port or an RCEC.
The writes probably don't *break* anything since those registers are
reserved, but they also don't disable the interrupt or clear the Root
Error Status.
Bjorn
