On 20/11/2019 12:28, Oliver O'Halloran wrote:
> The EEH core has a concept of "early probe" and "late probe." When the
> EEH_PROBE_MODE_DEVTREE flag is set (i.e pseries) we call the eeh_ops->probe()
> function in eeh_add_device_early() so the eeh_dev state is initialised based
> on
> the pci_dn. It's important to realise that this happens *long* before the PCI
> device has been probed and a pci_dev structure created. This is necessary due
> to a PAPR requirement that EEH be enabled before to OS starts interacting
> with the device.
>
> The late probe is done in eeh_add_device_late() when the EEH_PROBE_MODE_DEV
> flag is set (i.e. PowerNV). The main difference is the late probe happens
> after the pci_dev has been created. As a result there is no actual dependency
> on a pci_dn in the late probe case. Splitting the single eeh_ops->probe()
> function into seperate functions allows us to simplify the late probe case
> since we have access to a pci_dev at that point. Having access to a pci_dev
> means that we can use the functions provided by the PCI core for finding
> capabilities, etc rather than doing it manually.
>
> It also changes the prototype for the probe functions to be void. Currently
> they return a void *, but both implementations always return NULL so there's
> not much point to it.
>
> Signed-off-by: Oliver O'Halloran <ooh...@gmail.com>
> ---
> arch/powerpc/include/asm/eeh.h | 3 +-
> arch/powerpc/kernel/eeh.c | 6 ++--
> arch/powerpc/platforms/powernv/eeh-powernv.c | 29 ++++++--------------
> arch/powerpc/platforms/pseries/eeh_pseries.c | 13 ++++-----
> 4 files changed, 20 insertions(+), 31 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 67847f8dfe71..466b0165fbcf 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -215,7 +215,8 @@ enum {
> struct eeh_ops {
> char *name;
> int (*init)(void);
> - void* (*probe)(struct pci_dn *pdn, void *data);
> + void (*probe_pdn)(struct pci_dn *pdn); /* used on pseries */
> + void (*probe_pdev)(struct pci_dev *pdev); /* used on powernv */
> int (*set_option)(struct eeh_pe *pe, int option);
> int (*get_pe_addr)(struct eeh_pe *pe);
> int (*get_state)(struct eeh_pe *pe, int *delay);
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index c8039fdb23ba..087a98b42a8c 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1066,7 +1066,7 @@ void eeh_add_device_early(struct pci_dn *pdn)
> (eeh_has_flag(EEH_PROBE_MODE_DEVTREE) && 0 == phb->buid))
> return;
>
> - eeh_ops->probe(pdn, NULL);
> + eeh_ops->probe_pdn(pdn);
> }
>
> /**
> @@ -1135,8 +1135,8 @@ void eeh_add_device_late(struct pci_dev *dev)
This guy is called directly from pseries and powernv so it feels like
you do not really need these probe/probe_pdev() as eeh_ops hooks and can
just call them directly.
eeh_add_device_early() is even simpler and only used for pseries (not
now but after 14/46), unless I missed something. Thanks,
> dev->dev.archdata.edev = NULL;
> }
>
> - if (eeh_has_flag(EEH_PROBE_MODE_DEV))
> - eeh_ops->probe(pdn, NULL);
> + if (eeh_ops->probe_pdev && eeh_has_flag(EEH_PROBE_MODE_DEV))
> + eeh_ops->probe_pdev(dev);
>
> edev->pdev = dev;
> dev->dev.archdata.edev = edev;
> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c
> b/arch/powerpc/platforms/powernv/eeh-powernv.c
> index 6c5d9f1bc378..8bd5317aa878 100644
> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> @@ -346,23 +346,13 @@ static int pnv_eeh_find_ecap(struct pci_dn *pdn, int
> cap)
>
> /**
> * pnv_eeh_probe - Do probe on PCI device
> - * @pdn: PCI device node
> - * @data: unused
> + * @pdev: pci_dev to probe
> *
> - * When EEH module is installed during system boot, all PCI devices
> - * are checked one by one to see if it supports EEH. The function
> - * is introduced for the purpose. By default, EEH has been enabled
> - * on all PCI devices. That's to say, we only need do necessary
> - * initialization on the corresponding eeh device and create PE
> - * accordingly.
> - *
> - * It's notable that's unsafe to retrieve the EEH device through
> - * the corresponding PCI device. During the PCI device hotplug, which
> - * was possiblly triggered by EEH core, the binding between EEH device
> - * and the PCI device isn't built yet.
> + * Creates (or finds an existing) edev for this pci_dev.
> */
> -static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
> +static void pnv_eeh_probe_pdev(struct pci_dev *pdev)
> {
> + struct pci_dn *pdn = pci_get_pdn(pdev);
> struct pci_controller *hose = pdn->phb;
> struct pnv_phb *phb = hose->private_data;
> struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
> @@ -377,11 +367,11 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void
> *data)
> * the probing.
> */
> if (!edev || edev->pe)
> - return NULL;
> + return;
>
> /* Skip for PCI-ISA bridge */
> if ((pdn->class_code >> 8) == PCI_CLASS_BRIDGE_ISA)
> - return NULL;
> + return;
>
> eeh_edev_dbg(edev, "Probing device\n");
>
> @@ -411,7 +401,7 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
> ret = eeh_add_to_parent_pe(edev);
> if (ret) {
> eeh_edev_warn(edev, "Failed to add device to PE (code %d)\n",
> ret);
> - return NULL;
> + return;
> }
>
> /*
> @@ -469,8 +459,6 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
> eeh_save_bars(edev);
>
> eeh_edev_dbg(edev, "EEH enabled on device\n");
> -
> - return NULL;
> }
>
> /**
> @@ -1673,7 +1661,8 @@ static int pnv_eeh_restore_config(struct eeh_dev *edev)
> static struct eeh_ops pnv_eeh_ops = {
> .name = "powernv",
> .init = pnv_eeh_init,
> - .probe = pnv_eeh_probe,
> + .probe_pdn = NULL,
> + .probe_pdev = pnv_eeh_probe_pdev,
> .set_option = pnv_eeh_set_option,
> .get_pe_addr = pnv_eeh_get_pe_addr,
> .get_state = pnv_eeh_get_state,
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c
> b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index 6f911a048339..3ac23c884f4e 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -229,7 +229,7 @@ static int pseries_eeh_find_ecap(struct pci_dn *pdn, int
> cap)
> * are checked one by one to see if it supports EEH. The function
> * is introduced for the purpose.
> */
> -static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
> +static void pseries_eeh_probe_pdn(struct pci_dn *pdn)
> {
> struct eeh_dev *edev;
> struct eeh_pe pe;
> @@ -240,15 +240,15 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void
> *data)
> /* Retrieve OF node and eeh device */
> edev = pdn_to_eeh_dev(pdn);
> if (!edev || edev->pe)
> - return NULL;
> + return;
>
> /* Check class/vendor/device IDs */
> if (!pdn->vendor_id || !pdn->device_id || !pdn->class_code)
> - return NULL;
> + return;
>
> /* Skip for PCI-ISA bridge */
> if ((pdn->class_code >> 8) == PCI_CLASS_BRIDGE_ISA)
> - return NULL;
> + return;
>
> eeh_edev_dbg(edev, "Probing device\n");
>
> @@ -315,8 +315,6 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void
> *data)
>
> /* Save memory bars */
> eeh_save_bars(edev);
> -
> - return NULL;
> }
>
> /**
> @@ -755,7 +753,8 @@ static int pseries_notify_resume(struct pci_dn *pdn)
> static struct eeh_ops pseries_eeh_ops = {
> .name = "pseries",
> .init = pseries_eeh_init,
> - .probe = pseries_eeh_probe,
> + .probe_pdn = pseries_eeh_probe_pdn,
> + .probe_pdev = NULL,
> .set_option = pseries_eeh_set_option,
> .get_pe_addr = pseries_eeh_get_pe_addr,
> .get_state = pseries_eeh_get_state,
>
--
Alexey
