On Thu, Oct 11, 2018 at 09:57:16AM -0600, Keith Busch wrote: > On Thu, Oct 11, 2018 at 08:26:18AM -0700, Bjorn Helgaas wrote: > > From: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > > > > Previously we enabled AER error reporting only for Switch Ports that were > > enumerated prior to registering the AER service driver. Switch Ports > > enumerated after AER driver registration were left with error reporting > > disabled. > > > > A common order, which works correctly, is that we enumerate devices before > > registering portdrv and the AER driver: > > > > - Enumerate all the devices at boot-time > > > > - Register portdrv and bind it to all Root Ports and Switch Ports, which > > disables error reporting for these Ports > > > > - Register AER service driver and bind it to all Root Ports, which > > enables error reporting for the Root Ports and any Switch Ports below > > them > > > > But if we enumerate devices *after* registering portdrv and the AER driver, > > e.g., if a host bridge driver is loaded as a module, error reporting is not > > enabled correctly: > > > > - Register portdrv and AER driver (this happens at boot-time) > > > > - Enumerate a Root Port > > > > - Bind portdrv to Root Port, disabling its error reporting > > > > - Bind AER service driver to Root Port, enabling error reporting for it > > and its children (there are no children, since we haven't enumerated > > them yet) > > > > - Enumerate Switch Port below the Root Port > > > > - Bind portdrv to Switch Port, disabling its error reporting > > > > - AER service driver doesn't bind to Switch Ports, so error reporting > > remains disabled > > > > Hot-adding a Switch fails similarly: error reporting is enabled correctly > > for the Root Port, but when the Switch is enumerated, the AER service > > driver doesn't claim it, so there's nothing to enable error reporting for > > the Switch Ports. > > > > Change the AER service driver so it binds to *all* PCIe Ports, including > > Switch Upstream and Downstream Ports. Enable AER error reporting for all > > these Ports, but not for any children. > > > > Link: https://lore.kernel.org/linux-pci/1536085989-2956-1-git-send-email-jonathan.derrick@xxxxxxxxx > > Based-on-patch-by: Jon Derrick <jonathan.derrick@xxxxxxxxx> > > Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > > --- > > drivers/pci/pcie/aer.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > index 90b53abf621d..c40c6607849b 100644 > > --- a/drivers/pci/pcie/aer.c > > +++ b/drivers/pci/pcie/aer.c > > @@ -1316,12 +1316,6 @@ static void aer_enable_rootport(struct aer_rpc *rpc) > > pci_read_config_dword(pdev, aer_pos + PCI_ERR_UNCOR_STATUS, ®32); > > pci_write_config_dword(pdev, aer_pos + PCI_ERR_UNCOR_STATUS, reg32); > > > > - /* > > - * Enable error reporting for the root port device and downstream port > > - * devices. > > - */ > > - set_downstream_devices_error_reporting(pdev, true); > > - > > /* Enable Root Port's interrupt in response to error messages */ > > pci_read_config_dword(pdev, aer_pos + PCI_ERR_ROOT_COMMAND, ®32); > > reg32 |= ROOT_PORT_INTR_ON_MESG_MASK; > > @@ -1378,10 +1372,17 @@ static void aer_remove(struct pcie_device *dev) > > */ > > static int aer_probe(struct pcie_device *dev) > > { > > + struct pci_dev *pdev = dev->port; > > + int type = pci_pcie_type(pdev); > > int status; > > struct aer_rpc *rpc; > > struct device *device = &dev->device; > > > > + if (type == PCI_EXP_TYPE_UPSTREAM || type == PCI_EXP_TYPE_DOWNSTREAM) { > > + pci_enable_pcie_error_reporting(pdev); > > + return 0; > > + } > > I think we need to either return an error in this case so that the > pcie_device won't be eligable for the .remove() callback, or add a > similiar type check in aer_remove(). Indeed, thanks! I think a check in aer_remove() seems nicer. It doesn't seem right to return an error here, since everything is working correctly. Bjorn
