[V11,08/17] powrepc/pci: Refactor pci_dn

From: Gavin Shan <gwshan@linux.vnet.ibm.com>

pci_dn is the extension of PCI device node and it's created from
device node. Unfortunately, VFs that are enabled dynamically by
PF's driver and they don't have corresponding device nodes, and
pci_dn. The patch refactors pci_dn to support VFs:

   * pci_dn is organized as a hierarchy tree. VF's pci_dn is put
     to the child list of pci_dn of PF's bridge. pci_dn of other
     device put to the child list of pci_dn of its upstream bridge.

   * VF's pci_dn is expected to be created dynamically when PF
     enabling VFs. VF's pci_dn will be destroyed when PF disabling
     VFs. pci_dn of other device is still created from device node
     as before.

   * For one particular PCI device (VF or not), its pci_dn can be
     found from pdev->dev.archdata.firmware_data, PCI_DN(devnode),
     or parent's list. The fast path (fetching pci_dn through PCI
     device instance) is populated during early fixup time.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/device.h         |    3 +
 arch/powerpc/include/asm/pci-bridge.h     |   14 +-
 arch/powerpc/kernel/pci_dn.c              |  242 ++++++++++++++++++++++++++++-
 arch/powerpc/platforms/powernv/pci-ioda.c |   16 ++
 4 files changed, 270 insertions(+), 5 deletions(-)

On Thu, Jan 15, 2015 at 10:27:58AM +0800, Wei Yang wrote:
> From: Gavin Shan <gwshan@linux.vnet.ibm.com>
> 
> pci_dn is the extension of PCI device node and it's created from
> device node. Unfortunately, VFs that are enabled dynamically by
> PF's driver and they don't have corresponding device nodes, and
> pci_dn. The patch refactors pci_dn to support VFs:
> 
>    * pci_dn is organized as a hierarchy tree. VF's pci_dn is put
>      to the child list of pci_dn of PF's bridge. pci_dn of other
>      device put to the child list of pci_dn of its upstream bridge.
> 
>    * VF's pci_dn is expected to be created dynamically when PF
>      enabling VFs. VF's pci_dn will be destroyed when PF disabling
>      VFs. pci_dn of other device is still created from device node
>      as before.
> 
>    * For one particular PCI device (VF or not), its pci_dn can be
>      found from pdev->dev.archdata.firmware_data, PCI_DN(devnode),
>      or parent's list. The fast path (fetching pci_dn through PCI
>      device instance) is populated during early fixup time.
> 
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/device.h         |    3 +
>  arch/powerpc/include/asm/pci-bridge.h     |   14 +-
>  arch/powerpc/kernel/pci_dn.c              |  242 ++++++++++++++++++++++++++++-
>  arch/powerpc/platforms/powernv/pci-ioda.c |   16 ++
>  4 files changed, 270 insertions(+), 5 deletions(-)
> ...

> +#ifdef CONFIG_PCI_IOV
> +static struct pci_dn *add_one_dev_pci_info(struct pci_dn *parent,
> +					   struct pci_dev *pdev,
> +					   int busno, int devfn)
> +{
> +	struct pci_dn *pdn;
> +
> +	/* Except PHB, we always have parent firmware data */
> +	if (!parent)
> +		return NULL;
> +
> +	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
> +	if (!pdn) {
> +		pr_warn("%s: Out of memory !\n", __func__);
> +		return NULL;
> +	}
> +
> +	pdn->phb = parent->phb;
> +	pdn->parent = parent;
> +	pdn->busno = busno;
> +	pdn->devfn = devfn;
> +#ifdef CONFIG_PPC_POWERNV
> +	pdn->pe_number = IODA_INVALID_PE;
> +#endif
> +	INIT_LIST_HEAD(&pdn->child_list);
> +	INIT_LIST_HEAD(&pdn->list);
> +	list_add_tail(&pdn->list, &parent->child_list);
> +
> +	/*
> +	 * If we already have PCI device instance, lets
> +	 * bind them.
> +	 */
> +	if (pdev)
> +		pdev->dev.archdata.firmware_data = pdn;
> +
> +	return pdn;

I'd like to see this done in pcibios_add_device(), as I mentioned in
response to "[PATCH V11 01/17] PCI/IOV: Export interface for retrieve VF's
BDF".  Maybe that's not feasible for some reason, but it would be a nicer
design if it's possible.

The remove_dev_pci_info() work would be done in pcibios_release_device()
then, of course.

> +}
> +#endif // CONFIG_PCI_IOV
> +
> +struct pci_dn *add_dev_pci_info(struct pci_dev *pdev, u16 vf_num)
> +{
> +#ifdef CONFIG_PCI_IOV
> +	struct pci_dn *parent, *pdn;
> +	int i;
> +
> +	/* Only support IOV for now */
> +	if (!pdev->is_physfn)
> +		return pci_get_pdn(pdev);
> +
> +	/* Check if VFs have been populated */
> +	pdn = pci_get_pdn(pdev);
> +	if (!pdn || (pdn->flags & PCI_DN_FLAG_IOV_VF))
> +		return NULL;
> +
> +	pdn->flags |= PCI_DN_FLAG_IOV_VF;
> +	parent = pci_bus_to_pdn(pdev->bus);
> +	if (!parent)
>  		return NULL;
> -	return PCI_DN(dn);
> +
> +	for (i = 0; i < vf_num; i++) {
> +		pdn = add_one_dev_pci_info(parent, NULL,
> +					   pci_iov_virtfn_bus(pdev, i),
> +					   pci_iov_virtfn_devfn(pdev, i));
> +		if (!pdn) {
> +			pr_warn("%s: Cannot create firmware data "
> +				"for VF#%d of %s\n",
> +				__func__, i, pci_name(pdev));
> +			return NULL;
> +		}
> +	}
> +#endif
> +
> +	return pci_get_pdn(pdev);
> +}
> +
> +void remove_dev_pci_info(struct pci_dev *pdev, u16 vf_num)
> +{
> +#ifdef CONFIG_PCI_IOV
> +	struct pci_dn *parent;
> +	struct pci_dn *pdn, *tmp;
> +	int i;
> +
> +	/* Only support IOV PF for now */
> +	if (!pdev->is_physfn)
> +		return;
> +
> +	/* Check if VFs have been populated */
> +	pdn = pci_get_pdn(pdev);
> +	if (!pdn || !(pdn->flags & PCI_DN_FLAG_IOV_VF))
> +		return;
> +
> +	pdn->flags &= ~PCI_DN_FLAG_IOV_VF;
> +	parent = pci_bus_to_pdn(pdev->bus);
> +	if (!parent)
> +		return;
> +
> +	/*
> +	 * We might introduce flag to pci_dn in future
> +	 * so that we can release VF's firmware data in
> +	 * a batch mode.
> +	 */
> +	for (i = 0; i < vf_num; i++) {
> +		list_for_each_entry_safe(pdn, tmp,
> +			&parent->child_list, list) {
> +			if (pdn->busno != pci_iov_virtfn_bus(pdev, i) ||
> +			    pdn->devfn != pci_iov_virtfn_devfn(pdev, i))
> +				continue;
> +
> +			if (!list_empty(&pdn->list))
> +				list_del(&pdn->list);
> +			kfree(pdn);
> +		}
> +	}
> +#endif
>  }

On Fri, Feb 20, 2015 at 05:19:17PM -0600, Bjorn Helgaas wrote:
>On Thu, Jan 15, 2015 at 10:27:58AM +0800, Wei Yang wrote:
>> From: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> 
>> pci_dn is the extension of PCI device node and it's created from
>> device node. Unfortunately, VFs that are enabled dynamically by
>> PF's driver and they don't have corresponding device nodes, and
>> pci_dn. The patch refactors pci_dn to support VFs:
>> 
>>    * pci_dn is organized as a hierarchy tree. VF's pci_dn is put
>>      to the child list of pci_dn of PF's bridge. pci_dn of other
>>      device put to the child list of pci_dn of its upstream bridge.
>> 
>>    * VF's pci_dn is expected to be created dynamically when PF
>>      enabling VFs. VF's pci_dn will be destroyed when PF disabling
>>      VFs. pci_dn of other device is still created from device node
>>      as before.
>> 
>>    * For one particular PCI device (VF or not), its pci_dn can be
>>      found from pdev->dev.archdata.firmware_data, PCI_DN(devnode),
>>      or parent's list. The fast path (fetching pci_dn through PCI
>>      device instance) is populated during early fixup time.
>> 
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/include/asm/device.h         |    3 +
>>  arch/powerpc/include/asm/pci-bridge.h     |   14 +-
>>  arch/powerpc/kernel/pci_dn.c              |  242 ++++++++++++++++++++++++++++-
>>  arch/powerpc/platforms/powernv/pci-ioda.c |   16 ++
>>  4 files changed, 270 insertions(+), 5 deletions(-)
>> ...
>
>> +#ifdef CONFIG_PCI_IOV
>> +static struct pci_dn *add_one_dev_pci_info(struct pci_dn *parent,
>> +					   struct pci_dev *pdev,
>> +					   int busno, int devfn)
>> +{
>> +	struct pci_dn *pdn;
>> +
>> +	/* Except PHB, we always have parent firmware data */
>> +	if (!parent)
>> +		return NULL;
>> +
>> +	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
>> +	if (!pdn) {
>> +		pr_warn("%s: Out of memory !\n", __func__);
>> +		return NULL;
>> +	}
>> +
>> +	pdn->phb = parent->phb;
>> +	pdn->parent = parent;
>> +	pdn->busno = busno;
>> +	pdn->devfn = devfn;
>> +#ifdef CONFIG_PPC_POWERNV
>> +	pdn->pe_number = IODA_INVALID_PE;
>> +#endif
>> +	INIT_LIST_HEAD(&pdn->child_list);
>> +	INIT_LIST_HEAD(&pdn->list);
>> +	list_add_tail(&pdn->list, &parent->child_list);
>> +
>> +	/*
>> +	 * If we already have PCI device instance, lets
>> +	 * bind them.
>> +	 */
>> +	if (pdev)
>> +		pdev->dev.archdata.firmware_data = pdn;
>> +
>> +	return pdn;
>
>I'd like to see this done in pcibios_add_device(), as I mentioned in
>response to "[PATCH V11 01/17] PCI/IOV: Export interface for retrieve VF's
>BDF".  Maybe that's not feasible for some reason, but it would be a nicer
>design if it's possible.
>
>The remove_dev_pci_info() work would be done in pcibios_release_device()
>then, of course.
>

Yes, it's not feasible. PCI config accessors rely on VF's pci_dn. Before
calling pcibios_add_device(), we need access VF's config space. That means
we need VF's pci_dn before pci_setup_device() as follows:

    sriov_enable()
        pcibios_sriov_enable();     /* Currently, VF's pci_dn is created at this point */
        virtfn_add();
            virtfn_add_bus();       /* Create virtual bus if necessary */
                                    /* ---> A */
            pci_alloc_dev();        /* ---> B */
            pci_setup_device(vf);   /* Access VF's config space */
                pci_read_config_byte(vf, PCI_HEADER_TYPE);
                pci_read_config_dword(vf, PCI_CLASS_REVISION);
                pci_fixup_device(pci_fixup_early, vf);
                pci_read_irq();
                pci_read_bases();
            pci_device_add(vf);
                device_initialize(&vf->dev);
                pci_fixup_device(pci_fixup_header, vf);
                pci_init_capabilities(vf);
                pcibios_add_device(vf);

We have couple of options here:

1) Keep current code. VF's pci_dn is going to be destroyed in
   pcibios_sriov_disable() as we're doing currently.
2) Introduce pcibios_iov_virtfn_add() (at A) for platform to override.
   VF's pci_dn is going to be destroyed in pcibios_release_device().
3) Introduce pcibios_alloc_dev() (at B) for platform to override. The
   VF's pci_dn is going to be destroyed in pcibios_release_device().

Thanks,
Gavin

>> +}
>> +#endif // CONFIG_PCI_IOV
>> +
>> +struct pci_dn *add_dev_pci_info(struct pci_dev *pdev, u16 vf_num)
>> +{
>> +#ifdef CONFIG_PCI_IOV
>> +	struct pci_dn *parent, *pdn;
>> +	int i;
>> +
>> +	/* Only support IOV for now */
>> +	if (!pdev->is_physfn)
>> +		return pci_get_pdn(pdev);
>> +
>> +	/* Check if VFs have been populated */
>> +	pdn = pci_get_pdn(pdev);
>> +	if (!pdn || (pdn->flags & PCI_DN_FLAG_IOV_VF))
>> +		return NULL;
>> +
>> +	pdn->flags |= PCI_DN_FLAG_IOV_VF;
>> +	parent = pci_bus_to_pdn(pdev->bus);
>> +	if (!parent)
>>  		return NULL;
>> -	return PCI_DN(dn);
>> +
>> +	for (i = 0; i < vf_num; i++) {
>> +		pdn = add_one_dev_pci_info(parent, NULL,
>> +					   pci_iov_virtfn_bus(pdev, i),
>> +					   pci_iov_virtfn_devfn(pdev, i));
>> +		if (!pdn) {
>> +			pr_warn("%s: Cannot create firmware data "
>> +				"for VF#%d of %s\n",
>> +				__func__, i, pci_name(pdev));
>> +			return NULL;
>> +		}
>> +	}
>> +#endif
>> +
>> +	return pci_get_pdn(pdev);
>> +}
>> +
>> +void remove_dev_pci_info(struct pci_dev *pdev, u16 vf_num)
>> +{
>> +#ifdef CONFIG_PCI_IOV
>> +	struct pci_dn *parent;
>> +	struct pci_dn *pdn, *tmp;
>> +	int i;
>> +
>> +	/* Only support IOV PF for now */
>> +	if (!pdev->is_physfn)
>> +		return;
>> +
>> +	/* Check if VFs have been populated */
>> +	pdn = pci_get_pdn(pdev);
>> +	if (!pdn || !(pdn->flags & PCI_DN_FLAG_IOV_VF))
>> +		return;
>> +
>> +	pdn->flags &= ~PCI_DN_FLAG_IOV_VF;
>> +	parent = pci_bus_to_pdn(pdev->bus);
>> +	if (!parent)
>> +		return;
>> +
>> +	/*
>> +	 * We might introduce flag to pci_dn in future
>> +	 * so that we can release VF's firmware data in
>> +	 * a batch mode.
>> +	 */
>> +	for (i = 0; i < vf_num; i++) {
>> +		list_for_each_entry_safe(pdn, tmp,
>> +			&parent->child_list, list) {
>> +			if (pdn->busno != pci_iov_virtfn_bus(pdev, i) ||
>> +			    pdn->devfn != pci_iov_virtfn_devfn(pdev, i))
>> +				continue;
>> +
>> +			if (!list_empty(&pdn->list))
>> +				list_del(&pdn->list);
>> +			kfree(pdn);
>> +		}
>> +	}
>> +#endif
>>  }
>

On Mon, Feb 23, 2015 at 11:13:49AM +1100, Gavin Shan wrote:
> On Fri, Feb 20, 2015 at 05:19:17PM -0600, Bjorn Helgaas wrote:
> >On Thu, Jan 15, 2015 at 10:27:58AM +0800, Wei Yang wrote:
> >> From: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> 
> >> pci_dn is the extension of PCI device node and it's created from
> >> device node. Unfortunately, VFs that are enabled dynamically by
> >> PF's driver and they don't have corresponding device nodes, and
> >> pci_dn. The patch refactors pci_dn to support VFs:
> >> 
> >>    * pci_dn is organized as a hierarchy tree. VF's pci_dn is put
> >>      to the child list of pci_dn of PF's bridge. pci_dn of other
> >>      device put to the child list of pci_dn of its upstream bridge.
> >> 
> >>    * VF's pci_dn is expected to be created dynamically when PF
> >>      enabling VFs. VF's pci_dn will be destroyed when PF disabling
> >>      VFs. pci_dn of other device is still created from device node
> >>      as before.
> >> 
> >>    * For one particular PCI device (VF or not), its pci_dn can be
> >>      found from pdev->dev.archdata.firmware_data, PCI_DN(devnode),
> >>      or parent's list. The fast path (fetching pci_dn through PCI
> >>      device instance) is populated during early fixup time.
> >> 
> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> ---
> >>  arch/powerpc/include/asm/device.h         |    3 +
> >>  arch/powerpc/include/asm/pci-bridge.h     |   14 +-
> >>  arch/powerpc/kernel/pci_dn.c              |  242 ++++++++++++++++++++++++++++-
> >>  arch/powerpc/platforms/powernv/pci-ioda.c |   16 ++
> >>  4 files changed, 270 insertions(+), 5 deletions(-)
> >> ...
> >
> >> +#ifdef CONFIG_PCI_IOV
> >> +static struct pci_dn *add_one_dev_pci_info(struct pci_dn *parent,
> >> +					   struct pci_dev *pdev,
> >> +					   int busno, int devfn)
> >> +{
> >> +	struct pci_dn *pdn;
> >> +
> >> +	/* Except PHB, we always have parent firmware data */
> >> +	if (!parent)
> >> +		return NULL;
> >> +
> >> +	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
> >> +	if (!pdn) {
> >> +		pr_warn("%s: Out of memory !\n", __func__);
> >> +		return NULL;
> >> +	}
> >> +
> >> +	pdn->phb = parent->phb;
> >> +	pdn->parent = parent;
> >> +	pdn->busno = busno;
> >> +	pdn->devfn = devfn;
> >> +#ifdef CONFIG_PPC_POWERNV
> >> +	pdn->pe_number = IODA_INVALID_PE;
> >> +#endif
> >> +	INIT_LIST_HEAD(&pdn->child_list);
> >> +	INIT_LIST_HEAD(&pdn->list);
> >> +	list_add_tail(&pdn->list, &parent->child_list);
> >> +
> >> +	/*
> >> +	 * If we already have PCI device instance, lets
> >> +	 * bind them.
> >> +	 */
> >> +	if (pdev)
> >> +		pdev->dev.archdata.firmware_data = pdn;
> >> +
> >> +	return pdn;
> >
> >I'd like to see this done in pcibios_add_device(), as I mentioned in
> >response to "[PATCH V11 01/17] PCI/IOV: Export interface for retrieve VF's
> >BDF".  Maybe that's not feasible for some reason, but it would be a nicer
> >design if it's possible.
> >
> >The remove_dev_pci_info() work would be done in pcibios_release_device()
> >then, of course.
> >
> 
> Yes, it's not feasible. PCI config accessors rely on VF's pci_dn. Before
> calling pcibios_add_device(), we need access VF's config space.  That means
> we need VF's pci_dn before pci_setup_device() as follows:
> 
>     sriov_enable()
>         pcibios_sriov_enable();     /* Currently, VF's pci_dn is created at this point */
>         virtfn_add();
>             virtfn_add_bus();       /* Create virtual bus if necessary */
>                                     /* ---> A */
>             pci_alloc_dev();        /* ---> B */
>             pci_setup_device(vf);   /* Access VF's config space */
>                 pci_read_config_byte(vf, PCI_HEADER_TYPE);
>                 pci_read_config_dword(vf, PCI_CLASS_REVISION);
>                 pci_fixup_device(pci_fixup_early, vf);
>                 pci_read_irq();
>                 pci_read_bases();
>             pci_device_add(vf);
>                 device_initialize(&vf->dev);
>                 pci_fixup_device(pci_fixup_header, vf);
>                 pci_init_capabilities(vf);
>                 pcibios_add_device(vf);
> 
> We have couple of options here:
> 
> 1) Keep current code. VF's pci_dn is going to be destroyed in
>    pcibios_sriov_disable() as we're doing currently.
> 2) Introduce pcibios_iov_virtfn_add() (at A) for platform to override.
>    VF's pci_dn is going to be destroyed in pcibios_release_device().
> 3) Introduce pcibios_alloc_dev() (at B) for platform to override. The
>    VF's pci_dn is going to be destroyed in pcibios_release_device().

Ah, yes, now I see the problem.  I don't really like having to export
pci_iov_virtfn_bus() and pci_iov_virtfn_devfn(), but it's probably not
worth the hassle of changing it, and I think adding more pcibios interfaces
would be even worse.

So let's leave it as-is for now.

> >> +}
> >> +#endif // CONFIG_PCI_IOV
> >> +
> >> +struct pci_dn *add_dev_pci_info(struct pci_dev *pdev, u16 vf_num)
> >> +{
> >> +#ifdef CONFIG_PCI_IOV
> >> +	struct pci_dn *parent, *pdn;
> >> +	int i;
> >> +
> >> +	/* Only support IOV for now */
> >> +	if (!pdev->is_physfn)
> >> +		return pci_get_pdn(pdev);
> >> +
> >> +	/* Check if VFs have been populated */
> >> +	pdn = pci_get_pdn(pdev);
> >> +	if (!pdn || (pdn->flags & PCI_DN_FLAG_IOV_VF))
> >> +		return NULL;
> >> +
> >> +	pdn->flags |= PCI_DN_FLAG_IOV_VF;
> >> +	parent = pci_bus_to_pdn(pdev->bus);
> >> +	if (!parent)
> >>  		return NULL;
> >> -	return PCI_DN(dn);
> >> +
> >> +	for (i = 0; i < vf_num; i++) {
> >> +		pdn = add_one_dev_pci_info(parent, NULL,
> >> +					   pci_iov_virtfn_bus(pdev, i),
> >> +					   pci_iov_virtfn_devfn(pdev, i));
> >> +		if (!pdn) {
> >> +			pr_warn("%s: Cannot create firmware data "
> >> +				"for VF#%d of %s\n",
> >> +				__func__, i, pci_name(pdev));
> >> +			return NULL;
> >> +		}
> >> +	}
> >> +#endif
> >> +
> >> +	return pci_get_pdn(pdev);
> >> +}
> >> +
> >> +void remove_dev_pci_info(struct pci_dev *pdev, u16 vf_num)
> >> +{
> >> +#ifdef CONFIG_PCI_IOV
> >> +	struct pci_dn *parent;
> >> +	struct pci_dn *pdn, *tmp;
> >> +	int i;
> >> +
> >> +	/* Only support IOV PF for now */
> >> +	if (!pdev->is_physfn)
> >> +		return;
> >> +
> >> +	/* Check if VFs have been populated */
> >> +	pdn = pci_get_pdn(pdev);
> >> +	if (!pdn || !(pdn->flags & PCI_DN_FLAG_IOV_VF))
> >> +		return;
> >> +
> >> +	pdn->flags &= ~PCI_DN_FLAG_IOV_VF;
> >> +	parent = pci_bus_to_pdn(pdev->bus);
> >> +	if (!parent)
> >> +		return;
> >> +
> >> +	/*
> >> +	 * We might introduce flag to pci_dn in future
> >> +	 * so that we can release VF's firmware data in
> >> +	 * a batch mode.
> >> +	 */
> >> +	for (i = 0; i < vf_num; i++) {
> >> +		list_for_each_entry_safe(pdn, tmp,
> >> +			&parent->child_list, list) {
> >> +			if (pdn->busno != pci_iov_virtfn_bus(pdev, i) ||
> >> +			    pdn->devfn != pci_iov_virtfn_devfn(pdev, i))
> >> +				continue;
> >> +
> >> +			if (!list_empty(&pdn->list))
> >> +				list_del(&pdn->list);
> >> +			kfree(pdn);
> >> +		}
> >> +	}
> >> +#endif
> >>  }
> >
>

On Tue, 2015-02-24 at 02:13 -0600, Bjorn Helgaas wrote:
> 
> Ah, yes, now I see the problem.  I don't really like having to export
> pci_iov_virtfn_bus() and pci_iov_virtfn_devfn(), but it's probably not
> worth the hassle of changing it, and I think adding more pcibios
> interfaces
> would be even worse.

Aren't we going to eventually turn them all into host bridge ops ? :-)

Cheers,
Ben.