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 <ooh...@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
>
signature.asc
Description: PGP signature
