Re: [PATCH 6/6] powerpc/eeh: Rework eeh_ops->probe()

[Sam Bobroff <sbobroff@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 9210 bytes --]

On Mon, Feb 03, 2020 at 07:35:21PM +1100, Oliver O'Halloran wrote:
> With the EEH early probe now being pseries specific there's no need for
> eeh_ops->probe() to take a pci_dn. Instead, we can make it take a pci_dev
> and use the probe function to map a pci_dev to an eeh_dev. This allows
> the platform to implement it's own method for finding (or creating) an
> eeh_dev for a given pci_dev which also removes a use of pci_dn in
> generic EEH code.
> 
> This patch also renames eeh_device_add_late() to eeh_device_probe(). This
> better reflects what it does does and removes the last vestiges of the
> early/late EEH probe split.

Nice!
Just one nit, below.

Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>


> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
>  arch/powerpc/include/asm/eeh.h               |  6 ++--
>  arch/powerpc/kernel/eeh.c                    | 42 +++++++++++++++-------------
>  arch/powerpc/platforms/powernv/eeh-powernv.c | 30 ++++++++++----------
>  arch/powerpc/platforms/pseries/eeh_pseries.c | 23 ++++++++++++++-
>  4 files changed, 61 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 8580238..964a542 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -215,7 +215,7 @@ enum {
>  struct eeh_ops {
>  	char *name;
>  	int (*init)(void);
> -	void* (*probe)(struct pci_dn *pdn, void *data);
> +	struct eeh_dev *(*probe)(struct pci_dev *pdev);
>  	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);
> @@ -301,7 +301,7 @@ int __exit eeh_ops_unregister(const char *name);
>  int eeh_check_failure(const volatile void __iomem *token);
>  int eeh_dev_check_failure(struct eeh_dev *edev);
>  void eeh_addr_cache_init(void);
> -void eeh_add_device_late(struct pci_dev *);
> +void eeh_probe_device(struct pci_dev *pdev);
>  void eeh_remove_device(struct pci_dev *);
>  int eeh_unfreeze_pe(struct eeh_pe *pe);
>  int eeh_pe_reset_and_recover(struct eeh_pe *pe);
> @@ -356,7 +356,7 @@ static inline int eeh_check_failure(const volatile void __iomem *token)
>  
>  static inline void eeh_addr_cache_init(void) { }
>  
> -static inline void eeh_add_device_late(struct pci_dev *dev) { }
> +static inline void eeh_probe_device(struct pci_dev *dev) { }
>  
>  static inline void eeh_remove_device(struct pci_dev *dev) { }
>  
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 55d3ef6..2c5f7a6 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1113,29 +1113,37 @@ core_initcall_sync(eeh_init);
>   * This routine must be used to complete EEH initialization for PCI
>   * devices that were added after system boot (e.g. hotplug, dlpar).
>   */

You can't see it in the patch but up a few lines in the comment block,
there's a leftover "eeh_add_device_late".

> -void eeh_add_device_late(struct pci_dev *dev)
> +void eeh_probe_device(struct pci_dev *dev)
>  {
> -	struct pci_dn *pdn;
>  	struct eeh_dev *edev;
>  
> -	if (!dev)
> +	pr_debug("EEH: Adding device %s\n", pci_name(dev));
> +
> +	/*
> +	 * pci_dev_to_eeh_dev() can only work if eeh_probe_dev() was
> +	 * already called for this device.
> +	 */
> +	if (WARN_ON_ONCE(pci_dev_to_eeh_dev(dev))) {
> +		eeh_edev_dbg(edev, "Already bound to an eeh_dev!\n");
>  		return;
> +	}
>  
> -	pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
> -	edev = pdn_to_eeh_dev(pdn);
> -	eeh_edev_dbg(edev, "Adding device\n");
> -	if (edev->pdev == dev) {
> -		eeh_edev_dbg(edev, "Device already referenced!\n");
> +	edev = eeh_ops->probe(dev);
> +	if (!edev) {
> +		pr_debug("EEH: Adding device failed\n");
>  		return;
>  	}
>  
>  	/*
> -	 * The EEH cache might not be removed correctly because of
> -	 * unbalanced kref to the device during unplug time, which
> -	 * relies on pcibios_release_device(). So we have to remove
> -	 * that here explicitly.
> +	 * FIXME: We rely on pcibios_release_device() to remove the
> +	 * existing EEH state. The release function is only called if
> +	 * the pci_dev's refcount drops to zero so if something is
> +	 * keeping a ref to a device (e.g. a filesystem) we need to
> +	 * remove the old EEH state.
> +	 *
> +	 * FIXME: HEY MA, LOOK AT ME, NO LOCKING!
>  	 */
> -	if (edev->pdev) {
> +	if (edev->pdev && edev->pdev != dev) {
>  		eeh_rmv_from_parent_pe(edev);
>  		eeh_addr_cache_rmv_dev(edev->pdev);
>  		eeh_sysfs_remove_device(edev->pdev);
> @@ -1146,17 +1154,11 @@ void eeh_add_device_late(struct pci_dev *dev)
>  		 * into error handler afterwards.
>  		 */
>  		edev->mode |= EEH_DEV_NO_HANDLER;
> -
> -		edev->pdev = NULL;
> -		dev->dev.archdata.edev = NULL;
>  	}
>  
> -	if (eeh_has_flag(EEH_PROBE_MODE_DEV))
> -		eeh_ops->probe(pdn, NULL);
> -
> +	/* bind the pdev and the edev together */
>  	edev->pdev = dev;
>  	dev->dev.archdata.edev = edev;
> -
>  	eeh_addr_cache_insert_dev(dev);
>  	eeh_sysfs_add_device(dev);
>  }
> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
> index eaa8dfe..79409e0 100644
> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> @@ -41,7 +41,7 @@ static int eeh_event_irq = -EINVAL;
>  void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
>  {
>  	dev_dbg(&pdev->dev, "EEH: Setting up device\n");
> -	eeh_add_device_late(pdev);
> +	eeh_probe_device(pdev);
>  }
>  
>  static int pnv_eeh_init(void)
> @@ -340,23 +340,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.
> + * Create, or find the existing, eeh_dev for this pci_dev.
>   */
> -static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
> +static struct eeh_dev *pnv_eeh_probe(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);
> @@ -373,6 +363,14 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
>  	if (!edev || edev->pe)
>  		return NULL;
>  
> +	/* already configured? */
> +	if (edev->pdev) {
> +		pr_debug("%s: found existing edev for %04x:%02x:%02x.%01x\n",
> +			__func__, hose->global_number, config_addr >> 8,
> +			PCI_SLOT(config_addr), PCI_FUNC(config_addr));
> +		return edev;
> +	}
> +
>  	/* Skip for PCI-ISA bridge */
>  	if ((pdn->class_code >> 8) == PCI_CLASS_BRIDGE_ISA)
>  		return NULL;
> @@ -464,7 +462,7 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
>  
>  	eeh_edev_dbg(edev, "EEH enabled on device\n");
>  
> -	return NULL;
> +	return edev;
>  }
>  
>  /**
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index 1ca7cf0..8453428 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -77,7 +77,7 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
>  		eeh_add_to_parent_pe(edev);   /* Add as VF PE type */
>  	}
>  #endif
> -	eeh_add_device_late(pdev);
> +	eeh_probe_device(pdev);
>  }
>  
>  /*
> @@ -335,6 +335,26 @@ void pseries_eeh_init_edev(struct pci_dn *pdn)
>  	eeh_save_bars(edev);
>  }
>  
> +static struct eeh_dev *pseries_eeh_probe(struct pci_dev *pdev)
> +{
> +	struct eeh_dev *edev;
> +	struct pci_dn *pdn;
> +
> +	pdn = pci_get_pdn_by_devfn(pdev->bus, pdev->devfn);
> +	if (!pdn)
> +		return NULL;
> +
> +	/*
> +	 * If the system supports EEH on this device then the eeh_dev was
> +	 * configured and inserted into a PE in pseries_eeh_init_edev()
> +	 */
> +	edev = pdn_to_eeh_dev(pdn);
> +	if (!edev || !edev->pe)
> +		return NULL;
> +
> +	return edev;
> +}
> +
>  /**
>   * pseries_eeh_init_edev_recursive - Enable EEH for the indicated device
>   * @pdn: PCI device node
> @@ -813,6 +833,7 @@ 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,
>  	.set_option		= pseries_eeh_set_option,
>  	.get_pe_addr		= pseries_eeh_get_pe_addr,
>  	.get_state		= pseries_eeh_get_state,
> -- 
> 2.9.5
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]