Re: [PATCH] PCI/portdrv: Enable error reporting on managed ports

[Derrick, Jonathan]

Hi Bjorn,

On Tue, 2018-10-09 at 12:56 -0500, Bjorn Helgaas wrote:
> On Tue, Sep 04, 2018 at 12:33:09PM -0600, Jon Derrick wrote:
> > During probe, the port driver will disable error reporting and
> > assumes
> > it will be enabled later by the AER driver's pci_walk_bus()
> > sequence.
> > This may not be the case for host-bridge enabled root ports, who
> > will
> > enable first error reporting on the bus during the root port probe,
> > and
> > then disable error reporting on downstream devices during
> > subsequent
> > probing of the bus.
> 
> I understand the hotplug case (see below), but help me understand
> this
> "host-bridge enabled root ports" thing.  I'm not sure what that
> means.
Sorry for the confusion. I meant a device which doesn't expose the root
ports with firmware but has to expose the root ports using
pci_create_root_bus or similar methods. These methods use a host bridge
aperture on the backend.


> 
> We run pcie_portdrv_probe() for every root port, switch upstream
> port,
> and switch downstream port, and it always disables error reporting
> for
> the port:
> 
>   pcie_portdrv_probe          # pci_driver .probe
>     pcie_port_device_register
>       get_port_device_capability
>         services |= PCIE_PORT_SERVICE_AER
>         pci_disable_pcie_error_reporting
>           # clear DEVCTL Error Reporting Enables
> 
> For root ports, we call aer_probe(), and it enables error reporting
> for the entire tree below the root port:
> 
>   aer_probe                   # pcie_port_service .probe
>     aer_enable_rootport
>       set_downstream_devices_error_reporting(dev, true)
>         pci_walk_bus(dev->subordinate, set_device_error_reporting)
>           set_device_error_reporting
>             if (Root Port || Upstream Port || Downstream Port)
>               pci_enable_pcie_error_reporting
>                 # set DEVCTL Error Reporting Enables
> 
> This is definitely broken for hot-added switches because aer_probe()
> is the only place we enable error reporting, and it's only run when
> we
> enumerate a root port, not when we hot-add things below that root
> port.

I don't currently have the hardware to test hotplugging a switch,
although I think it should be possible to test with Thunderbolt. Mika? 
:)

> 
> > A hotplugged port device may also fail to enable error reporting as
> > the
> > AER driver has already run on the root bus.
> > Check for these conditions and enable error reporting during
> > portdrv
> > probing.
> > 
> > Example case:
> 
> pcie_portdrv_probe(10000:00:00.0):
> > [  343.790573] pcieport 10000:00:00.0:
> > pci_disable_pcie_error_reporting
> 
> aer_probe(10000:00:00.0):
> > [  343.809812] pcieport 10000:00:00.0:
> > pci_enable_pcie_error_reporting
> > [  343.819506] pci 10000:01:00.0: pci_enable_pcie_error_reporting
> > [  343.828814] pci 10000:02:00.0: pci_enable_pcie_error_reporting
> > [  343.838089] pci 10000:02:01.0: pci_enable_pcie_error_reporting
> > [  343.847478] pci 10000:02:02.0: pci_enable_pcie_error_reporting
> > [  343.856659] pci 10000:02:03.0: pci_enable_pcie_error_reporting
> > [  343.865794] pci 10000:02:04.0: pci_enable_pcie_error_reporting
> > [  343.874875] pci 10000:02:05.0: pci_enable_pcie_error_reporting
> > [  343.883918] pci 10000:02:06.0: pci_enable_pcie_error_reporting
> > [  343.892922] pci 10000:02:07.0: pci_enable_pcie_error_reporting
> 
> pcie_portdrv_probe(10000:01:00.0):
> > [  343.918900] pcieport 10000:01:00.0:
> > pci_disable_pcie_error_reporting
> 
> pcie_portdrv_probe(10000:02:00.0):
> > [  343.968426] pcieport 10000:02:00.0:
> > pci_disable_pcie_error_reporting
> 
> ...
> > [  344.028179] pcieport 10000:02:01.0:
> > pci_disable_pcie_error_reporting
> > [  344.091269] pcieport 10000:02:02.0:
> > pci_disable_pcie_error_reporting
> > [  344.156473] pcieport 10000:02:03.0:
> > pci_disable_pcie_error_reporting
> > [  344.238042] pcieport 10000:02:04.0:
> > pci_disable_pcie_error_reporting
> > [  344.321864] pcieport 10000:02:05.0:
> > pci_disable_pcie_error_reporting
> > [  344.411601] pcieport 10000:02:06.0:
> > pci_disable_pcie_error_reporting
> > [  344.505332] pcieport 10000:02:07.0:
> > pci_disable_pcie_error_reporting
> > [  344.621824] nvme 10000:06:00.0: pci_enable_pcie_error_reporting

The main issue leading to this sequence of events is that the root port
and tree is enumerated, then the tree has drivers attached in-order.
During driver probe, the root port services are attached which enables
AER on the entire bus. Then the port's service drivers are attached,
which disables AER during initialization.


It looks like I don't see this issue in the non-host-bridge case
because this statement isn't executed:
#ifdef CONFIG_PCIEAER
        if (dev->aer_cap && pci_aer_available() &&
            (pcie_ports_native || host->native_aer)) {
                services |= PCIE_PORT_SERVICE_AER;

                /*
                 * Disable AER on this port in case it's been enabled
by the
                 * BIOS (the AER service driver will enable it when
necessary).
                 */         
                pci_disable_pcie_error_reporting(dev);
        }                                                              
#endif


So I think re-enabling or strictly enabling error reporting is the
correct course of action for hotplug support at the least. And doing it
after port service driver probe is done and IRQs are requested should
eliminate or reduce races with initialization, which is what above code
snippet looks to be doing in the first place along with
pcie_init_services.

> > 
> > Signed-off-by: Jon Derrick <jonathan.derrick@xxxxxxxxx>
> > ---
> >  drivers/pci/pcie/portdrv_core.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/pci/pcie/portdrv_core.c
> > b/drivers/pci/pcie/portdrv_core.c
> > index 7c37d81..fdd953a 100644
> > --- a/drivers/pci/pcie/portdrv_core.c
> > +++ b/drivers/pci/pcie/portdrv_core.c
> > @@ -343,6 +343,16 @@ int pcie_port_device_register(struct pci_dev
> > *dev)
> >       if (!nr_service)
> >               goto error_cleanup_irqs;
> >  
> > +#ifdef CONFIG_PCIEAER
> > +     /*
> > +      * Enable error reporting for this port in case AER probing
> > has already
> > +      * run on the root bus or this port device is hot-inserted
> > +      */
> > +     if (dev->aer_cap && pci_aer_available() &&
> > +         (pcie_ports_native || pci_find_host_bridge(dev->bus)-
> > >native_aer))
> > +             pci_enable_pcie_error_reporting(dev);
> > +#endif
> 
> I plan to apply this after we clarify the changelog a bit, but I
> don't
> really like this patch because it (and the corresponding code added
> by
> 2bd50dd800b5 ("PCI: PCIe: Disable PCIe port services during port
> initialization")) seem a little out of place.
> 
> The way I think this *should* work is that the PCI core should
> arrange to
> handle AER interrupts when it enumerates the devices that can
> generate
> them (Root Ports and Root Complex Event Collectors), even before it
> enumerates the devices below the Root Port.
To add to this, I'm not the biggest fan of the current way of having
AER enable notification for ports on the tree, but then having every
other PCI driver enable their own notification. It seems inconsistent
for the port driver to not do it.


> 
> Then the PCI core could directly enable the AER interrupts on all
> devices
> as it enumerates them.  I would envision both cases being handled
> somewhere
> like pci_aer_init() in pci_init_capabilities().
> 
> This would also allow us to get rid of the
> pci_enable_pcie_error_reporting()
> calls that are currently sprinkled around in drivers, because that
> would be
> handled by the core for all devices.
I agree with you here. It seems to make the most sense to handle it
inclusively and exclude devices as necessary.


> 
> Bjorn

Attachment: smime.p7s
Description: S/MIME cryptographic signature