diff mbox series

[Very,RFC,43/46] powernv/pci: Do not set pdn->pe_number for NPU/CAPI devices

Message ID 20191120012859.23300-44-oohall@gmail.com (mailing list archive)
State RFC
Headers show
Series [Very,RFC,01/46] powerpc/eeh: Don't attempt to restore VF config space after reset | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch warning Failed to apply on branch powerpc/merge (784eee1cc44801366d4f197e0ade7739ee8e1e83)
snowpatch_ozlabs/apply_patch warning Failed to apply on branch powerpc/next (0695f8bca93ea0c57f0e8e21b4b4db70183b3d1c)
snowpatch_ozlabs/apply_patch warning Failed to apply on branch linus/master (c74386d50fbaf4a54fd3fe560f1abc709c0cff4b)
snowpatch_ozlabs/apply_patch warning Failed to apply on branch powerpc/fixes (7d6475051fb3d9339c5c760ed9883bc0a9048b21)
snowpatch_ozlabs/apply_patch warning Failed to apply on branch linux-next (5d1131b4d61e52e5702e0fa4bcbec81ac7d6ef52)
snowpatch_ozlabs/apply_patch fail Failed to apply to any branch

Commit Message

Oliver O'Halloran Nov. 20, 2019, 1:28 a.m. UTC
The only thing we need the pdn for in this function is setting the pe_number
field, which we don't use anymore. Fix the weird refcounting behaviour while
we're here.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
Either Fred, or Reza also fixed this in some patch lately and that'll probably get
merged before this one does.
---
 arch/powerpc/platforms/powernv/pci-ioda.c | 27 +++++++++--------------
 1 file changed, 10 insertions(+), 17 deletions(-)

Comments

Alexey Kardashevskiy Nov. 27, 2019, 10:49 p.m. UTC | #1
cc: Greg.


On 20/11/2019 12:28, Oliver O'Halloran wrote:
> The only thing we need the pdn for in this function is setting the pe_number
> field, which we don't use anymore. Fix the weird refcounting behaviour while
> we're here.
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> Either Fred, or Reza also fixed this in some patch lately and that'll probably get
> merged before this one does.
> ---
>  arch/powerpc/platforms/powernv/pci-ioda.c | 27 +++++++++--------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 45d940730c30..2a9201306543 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1066,16 +1066,13 @@ static int pnv_pci_vf_resource_shift(struct pci_dev *dev, int offset)
>  static struct pnv_ioda_pe *pnv_ioda_setup_dev_PE(struct pci_dev *dev)
>  {
>  	struct pnv_phb *phb = pci_bus_to_pnvhb(dev->bus);
> -	struct pci_dn *pdn = pci_get_pdn(dev);
> -	struct pnv_ioda_pe *pe;
> +	struct pnv_ioda_pe *pe = pnv_ioda_get_pe(dev);
>  
> -	if (!pdn) {
> -		pr_err("%s: Device tree node not associated properly\n",
> -			   pci_name(dev));
> +	/* Already has a PE assigned? huh? */
> +	if (pe) {
> +		WARN_ON(1);
>  		return NULL;
>  	}
> -	if (pdn->pe_number != IODA_INVALID_PE)
> -		return NULL;
>  
>  	pe = pnv_ioda_alloc_pe(phb);
>  	if (!pe) {
> @@ -1084,29 +1081,25 @@ static struct pnv_ioda_pe *pnv_ioda_setup_dev_PE(struct pci_dev *dev)
>  		return NULL;
>  	}
>  
> -	/* NOTE: We get only one ref to the pci_dev for the pdn, not for the
> -	 * pointer in the PE data structure, both should be destroyed at the
> -	 * same time. However, this needs to be looked at more closely again
> -	 * once we actually start removing things (Hotplug, SR-IOV, ...)
> +	/*
> +	 * NB: We **do not** hold a pci_dev ref for pe->pdev.
>  	 *
> -	 * At some point we want to remove the PDN completely anyways
> +	 * The pci_dev's release function cleans up the ioda_pe state, so:
> +	 *  a) We can't take a ref otherwise the release function is never called
> +	 *  b) The pe->pdev pointer will always point to valid pci_dev (or NULL)
>  	 */
> -	pci_dev_get(dev);
> -	pdn->pe_number = pe->pe_number;
>  	pe->flags = PNV_IODA_PE_DEV;
>  	pe->pdev = dev;
>  	pe->pbus = NULL;
>  	pe->mve_number = -1;
> -	pe->rid = dev->bus->number << 8 | pdn->devfn;
> +	pe->rid = dev->bus->number << 8 | dev->devfn;
>  
>  	pe_info(pe, "Associated device to PE\n");
>  
>  	if (pnv_ioda_configure_pe(phb, pe)) {
>  		/* XXX What do we do here ? */
>  		pnv_ioda_free_pe(pe);
> -		pdn->pe_number = IODA_INVALID_PE;
>  		pe->pdev = NULL;
> -		pci_dev_put(dev);
>  		return NULL;
>  	}
>  
>
diff mbox series

Patch

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 45d940730c30..2a9201306543 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1066,16 +1066,13 @@  static int pnv_pci_vf_resource_shift(struct pci_dev *dev, int offset)
 static struct pnv_ioda_pe *pnv_ioda_setup_dev_PE(struct pci_dev *dev)
 {
 	struct pnv_phb *phb = pci_bus_to_pnvhb(dev->bus);
-	struct pci_dn *pdn = pci_get_pdn(dev);
-	struct pnv_ioda_pe *pe;
+	struct pnv_ioda_pe *pe = pnv_ioda_get_pe(dev);
 
-	if (!pdn) {
-		pr_err("%s: Device tree node not associated properly\n",
-			   pci_name(dev));
+	/* Already has a PE assigned? huh? */
+	if (pe) {
+		WARN_ON(1);
 		return NULL;
 	}
-	if (pdn->pe_number != IODA_INVALID_PE)
-		return NULL;
 
 	pe = pnv_ioda_alloc_pe(phb);
 	if (!pe) {
@@ -1084,29 +1081,25 @@  static struct pnv_ioda_pe *pnv_ioda_setup_dev_PE(struct pci_dev *dev)
 		return NULL;
 	}
 
-	/* NOTE: We get only one ref to the pci_dev for the pdn, not for the
-	 * pointer in the PE data structure, both should be destroyed at the
-	 * same time. However, this needs to be looked at more closely again
-	 * once we actually start removing things (Hotplug, SR-IOV, ...)
+	/*
+	 * NB: We **do not** hold a pci_dev ref for pe->pdev.
 	 *
-	 * At some point we want to remove the PDN completely anyways
+	 * The pci_dev's release function cleans up the ioda_pe state, so:
+	 *  a) We can't take a ref otherwise the release function is never called
+	 *  b) The pe->pdev pointer will always point to valid pci_dev (or NULL)
 	 */
-	pci_dev_get(dev);
-	pdn->pe_number = pe->pe_number;
 	pe->flags = PNV_IODA_PE_DEV;
 	pe->pdev = dev;
 	pe->pbus = NULL;
 	pe->mve_number = -1;
-	pe->rid = dev->bus->number << 8 | pdn->devfn;
+	pe->rid = dev->bus->number << 8 | dev->devfn;
 
 	pe_info(pe, "Associated device to PE\n");
 
 	if (pnv_ioda_configure_pe(phb, pe)) {
 		/* XXX What do we do here ? */
 		pnv_ioda_free_pe(pe);
-		pdn->pe_number = IODA_INVALID_PE;
 		pe->pdev = NULL;
-		pci_dev_put(dev);
 		return NULL;
 	}