Re: [PATCH v3] PCI/AER: Enable reporting for ports enumerated after AER driver registration

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Oct 12, 2018 at 04:16:04PM +0800, Dongdong Liu wrote:
> 在 2018/10/11 23:57, Keith Busch 写道:
> > On Thu, Oct 11, 2018 at 08:26:18AM -0700, Bjorn Helgaas wrote:
> > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > 
> > > 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@intel.com
> > > Based-on-patch-by: Jon Derrick <jonathan.derrick@intel.com>
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >  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, &reg32);
> > >  	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, &reg32);
> > >  	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().
> 
> It seems aer_root_reset() also will be called for downstream port(err.c driver->reset_link(dev)),
> but aer_root_reset is only for root port.

Also right, thanks!

I think it will do the right thing if we make aer_root_reset() check
the port type and only disable/restore the Root Port interrupt
settings and status when called for a Root Port.  The
pci_bus_error_reset() it does is exactly what default_reset_link()
does for devices that aren't claimed by the AER driver.

Bjorn