[07/14] powernv/eeh: Use generic code to handle hot resets

When we reset PCI devices managed by a hotplug driver the reset may
generate spurious hotplug events that cause the PCI device we're resetting
to be torn down accidently. This is a problem for EEH (when the driver is
EEH aware) since we want to leave the OS PCI device state intact so that
the device can be re-set without losing any resources (network, disks,
etc) provided by the driver.

Generic PCI code provides the pci_bus_error_reset() function to handle
resetting a PCI Device (or bus) by using the reset method provided by the
hotplug slot driver. We can use this function if the EEH core has
requested a hot reset (common case) without tripping over the hotplug
driver.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
I know that include is a bit gross, but:

a) We're already doing it in pci-ioda.c, and in pseries/pci.
b) It's pci_bus_error_reset() isn't really a function that
   should be provided to non-pci core code.
---
 arch/powerpc/platforms/powernv/eeh-powernv.c | 38 ++++++++++++++++++--
 1 file changed, 35 insertions(+), 3 deletions(-)

On Tue, Sep 03, 2019 at 08:15:58PM +1000, Oliver O'Halloran wrote:
> When we reset PCI devices managed by a hotplug driver the reset may
> generate spurious hotplug events that cause the PCI device we're resetting
> to be torn down accidently. This is a problem for EEH (when the driver is
> EEH aware) since we want to leave the OS PCI device state intact so that
> the device can be re-set without losing any resources (network, disks,
> etc) provided by the driver.
> 
> Generic PCI code provides the pci_bus_error_reset() function to handle
> resetting a PCI Device (or bus) by using the reset method provided by the
> hotplug slot driver. We can use this function if the EEH core has
> requested a hot reset (common case) without tripping over the hotplug
> driver.

Could you explain a bit more about how this change solves the problem?
Is it that the hotplug driver's reset method doesn't cause spurious
hotplug events?

(Some other comments below)

> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> I know that include is a bit gross, but:
> 
> a) We're already doing it in pci-ioda.c, and in pseries/pci.
> b) It's pci_bus_error_reset() isn't really a function that
>    should be provided to non-pci core code.
> ---
>  arch/powerpc/platforms/powernv/eeh-powernv.c | 38 ++++++++++++++++++--
>  1 file changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
> index 94e26d56ecd2..6bc24a47e9ef 100644
> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> @@ -34,6 +34,7 @@
>  
>  #include "powernv.h"
>  #include "pci.h"
> +#include "../../../../drivers/pci/pci.h"
>  
>  static int eeh_event_irq = -EINVAL;
>  
> @@ -849,7 +850,7 @@ static int __pnv_eeh_bridge_reset(struct pci_dev *dev, int option)
>  	int aer = edev ? edev->aer_cap : 0;
>  	u32 ctrl;
>  
> -	pr_debug("%s: Reset PCI bus %04x:%02x with option %d\n",
> +	pr_debug("%s: Secondary Reset PCI bus %04x:%02x with option %d\n",
>  		 __func__, pci_domain_nr(dev->bus),
>  		 dev->bus->number, option);
>  
> @@ -907,6 +908,10 @@ static int pnv_eeh_bridge_reset(struct pci_dev *pdev, int option)
>  	if (!dn || !of_get_property(dn, "ibm,reset-by-firmware", NULL))
>  		return __pnv_eeh_bridge_reset(pdev, option);
>  
> +	pr_debug("%s: FW reset PCI bus %04x:%02x with option %d\n",
> +		 __func__, pci_domain_nr(pdev->bus),
> +		 pdev->bus->number, option);
> +
>  	switch (option) {
>  	case EEH_RESET_FUNDAMENTAL:
>  		scope = OPAL_RESET_PCI_FUNDAMENTAL;
> @@ -1125,10 +1130,37 @@ static int pnv_eeh_reset(struct eeh_pe *pe, int option)
>  		return -EIO;
>  	}
>  
> -	if (pci_is_root_bus(bus) ||
> -	    pci_is_root_bus(bus->parent))
> +	if (pci_is_root_bus(bus))
>  		return pnv_eeh_root_reset(hose, option);
>  
> +	/*
> +	 * For hot resets try use the generic PCI error recovery reset
> +	 * functions. These correctly handles the case where the secondary
> +	 * bus is behind a hotplug slot and it will use the slot provided
> +	 * reset methods to prevent spurious hotplug events during the reset.
> +	 *
> +	 * Fundemental resets need to be handled internally to EEH since the
> +	 * PCI core doesn't really have a concept of a fundemental reset,
> +	 * mainly because there's no standard way to generate one. Only a
> +	 * few devices require an FRESET so it should be fine.
> +	 */
> +	if (option != EEH_RESET_FUNDAMENTAL) {
> +		/*
> +		 * NB: Skiboot and pnv_eeh_bridge_reset() also no-op the
> +		 *     de-assert step. It's like the OPAL reset API was
> +		 *     poorly designed or something...
> +		 */
> +		if (option == EEH_RESET_DEACTIVATE)
> +			return 0;

It looks like this will prevent pnv_eeh_root_reset(bus->parent) (below)
from being called for EEH_RESET_DEACTIVATE, when it was before the
patch. Is that right?

> +
> +		rc = pci_bus_error_reset(bus->self);
> +		if (!rc)
> +			return 0;

Is it correct to fall through and try a different reset if this fails?

> +	}
> +
> +	/* otherwise, use the generic bridge reset. this might call into FW */
> +	if (pci_is_root_bus(bus->parent))
> +		return pnv_eeh_root_reset(hose, option);
>  	return pnv_eeh_bridge_reset(bus->self, option);
>  }
>  
> -- 
> 2.21.0
>

On Tue, Sep 17, 2019 at 11:15 AM Sam Bobroff <sbobroff@linux.ibm.com> wrote:
>
> On Tue, Sep 03, 2019 at 08:15:58PM +1000, Oliver O'Halloran wrote:
> > When we reset PCI devices managed by a hotplug driver the reset may
> > generate spurious hotplug events that cause the PCI device we're resetting
> > to be torn down accidently. This is a problem for EEH (when the driver is
> > EEH aware) since we want to leave the OS PCI device state intact so that
> > the device can be re-set without losing any resources (network, disks,
> > etc) provided by the driver.
> >
> > Generic PCI code provides the pci_bus_error_reset() function to handle
> > resetting a PCI Device (or bus) by using the reset method provided by the
> > hotplug slot driver. We can use this function if the EEH core has
> > requested a hot reset (common case) without tripping over the hotplug
> > driver.

> Could you explain a bit more about how this change solves the problem?
> Is it that the hotplug driver's reset method doesn't cause spurious
> hotplug events?

Yes, see the comment below.

> > -     if (pci_is_root_bus(bus) ||
> > -         pci_is_root_bus(bus->parent))
> > +     if (pci_is_root_bus(bus))
> >               return pnv_eeh_root_reset(hose, option);
> >
> > +     /*
> > +      * For hot resets try use the generic PCI error recovery reset
> > +      * functions. These correctly handles the case where the secondary
> > +      * bus is behind a hotplug slot and it will use the slot provided
> > +      * reset methods to prevent spurious hotplug events during the reset.
> > +      *
> > +      * Fundemental resets need to be handled internally to EEH since the
> > +      * PCI core doesn't really have a concept of a fundemental reset,
> > +      * mainly because there's no standard way to generate one. Only a
> > +      * few devices require an FRESET so it should be fine.
> > +      */
> > +     if (option != EEH_RESET_FUNDAMENTAL) {
> > +             /*
> > +              * NB: Skiboot and pnv_eeh_bridge_reset() also no-op the
> > +              *     de-assert step. It's like the OPAL reset API was
> > +              *     poorly designed or something...
> > +              */
> > +             if (option == EEH_RESET_DEACTIVATE)
> > +                     return 0;
>
> It looks like this will prevent pnv_eeh_root_reset(bus->parent) (below)
> from being called for EEH_RESET_DEACTIVATE, when it was before the
> patch. Is that right?

I agree it's a little awkward, but it works fine. OPAL has always
treated the resets defined by opal_pci_reset() as being edge-triggered
rather than level triggered since the de-assert step has always been
implemented as a no-op. This behaviour is effectively part of the ABI
between OPAL and the kernel since the kernel skips the de-assert step
in pnv_eeh_bridge_reset(). Although pnv_eeh_reset() uses
pnv_eeh_reset_root() to reset the secondary bus of the root port
pnv_pci_reset_secondary_bus() still uses the bridge reset.

I should probably update the OPAL API docs to mention that. Oh well.

> > +             rc = pci_bus_error_reset(bus->self);
> > +             if (!rc)
> > +                     return 0;
>
> Is it correct to fall through and try a different reset if this fails?

The only reason I can see for the generic code failing is when config
space to the bridge is blocked by the EEH core. The internal
pnv_eeh_bridge_reset() function has the option of calling
opal_pci_reset() or using the internal EEH config accessors (which
aren't filtered) so falling back makes sense to me.