[PATCH] pci/iov: return a reference to PF on destroying VF

[PATCH] pci/iov: return a reference to PF on destroying VF

[Wei Yang]

Each VF will get a reference to its PF, while it is not returned back in
all cases and leave a removed PF's pci_dev un-released.

As commit ac205b7b ("PCI: make sriov work with hotplug remove") indicates,
when removing devices on a bus, we do it in the reverse order. This means
we would remove VFs first, then PFs. After doing so, VF's removal is done
with pci_stop_and_remove_bus_device() instead of virtfn_remove().
virtfn_remove() returns the reference of its PF, while
pci_stop_and_remove_bus_device() doesn't.

This patches moves the return of PF's reference to pci_destroy_dev() to
make sure the PF's pci_dev is released in any case.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
 drivers/pci/iov.c    |    1 -
 drivers/pci/remove.c |    5 +++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 4b3a4ea..9b04bde 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -167,7 +167,6 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)
 
 	/* balance pci_get_domain_bus_and_slot() */
 	pci_dev_put(virtfn);
-	pci_dev_put(dev);
 }
 
 static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 8bd76c9..836ddf6 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -41,6 +41,11 @@ static void pci_destroy_dev(struct pci_dev *dev)
 	list_del(&dev->bus_list);
 	up_write(&pci_bus_sem);
 
+#ifdef CONFIG_PCI_IOV
+	if (dev->is_virtfn)
+		pci_dev_put(dev->physfn);
+#endif
+
 	pci_free_resources(dev);
 	put_device(&dev->dev);
 }
-- 
1.7.9.5

Re: [PATCH] pci/iov: return a reference to PF on destroying VF

[Bjorn Helgaas]

On Fri, Apr 10, 2015 at 04:53:04PM +0800, Wei Yang wrote:
> Each VF will get a reference to its PF, while it is not returned back in
> all cases and leave a removed PF's pci_dev un-released.
> 
> As commit ac205b7b ("PCI: make sriov work with hotplug remove") indicates,
> when removing devices on a bus, we do it in the reverse order. This means
> we would remove VFs first, then PFs. After doing so, VF's removal is done
> with pci_stop_and_remove_bus_device() instead of virtfn_remove().
> virtfn_remove() returns the reference of its PF, while
> pci_stop_and_remove_bus_device() doesn't.

Please use conventional citation style (12-char SHA1).

ac205b7bb72f appeared in v3.4.  Did that commit cause a regression?
Should this patch be marked for stable?

"After doing so, VF removal is done with pci_stop_and_remove_device() ..."
After doing what?  After removing the VFs and PFs?  After commit
ac205b7bb72f?

Prior to your patch, the VF reference was released in virtfn_remove(),
which is only called via pci_disable_sriov().  This typically happens in
a driver .remove() method.  The reference is *not* released if we call
pci_stop_and_remove_bus_device(VF) directly, as we would via the
remove_store() (sysfs "remove" file) or hot unplug paths, e.g.,
pciehp_unconfigure_device().

After your patch, the VF reference is released in pci_destroy_dev().  This
is called from pci_disable_sriov(), so it happens in that path as before.
But pci_destroy_dev() is called from pci_stop_and_remove_bus_device(), so
the reference is now released for all the paths that use
pci_stop_and_remove_bus_device().

What about the other things done in virtfn_remove(), e.g., the sysfs link
removal?  Your patch fixes a reference count leak, but don't we still have
a sysfs link leak?

It would be useful to mention a way to cause the leak.  I suspect writing
to a VF's sysfs "remove" file is the easiest.

> This patches moves the return of PF's reference to pci_destroy_dev() to
> make sure the PF's pci_dev is released in any case.
> 
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> ---
>  drivers/pci/iov.c    |    1 -
>  drivers/pci/remove.c |    5 +++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 4b3a4ea..9b04bde 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -167,7 +167,6 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>  
>  	/* balance pci_get_domain_bus_and_slot() */
>  	pci_dev_put(virtfn);
> -	pci_dev_put(dev);
>  }
>  
>  static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 8bd76c9..836ddf6 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -41,6 +41,11 @@ static void pci_destroy_dev(struct pci_dev *dev)
>  	list_del(&dev->bus_list);
>  	up_write(&pci_bus_sem);
>  
> +#ifdef CONFIG_PCI_IOV
> +	if (dev->is_virtfn)
> +		pci_dev_put(dev->physfn);
> +#endif
> +
>  	pci_free_resources(dev);
>  	put_device(&dev->dev);
>  }
> -- 
> 1.7.9.5
> 

Re: [PATCH] pci/iov: return a reference to PF on destroying VF

[Wei Yang]

On Tue, May 05, 2015 at 04:29:05PM -0500, Bjorn Helgaas wrote:
>On Fri, Apr 10, 2015 at 04:53:04PM +0800, Wei Yang wrote:
>> Each VF will get a reference to its PF, while it is not returned back in
>> all cases and leave a removed PF's pci_dev un-released.
>> 
>> As commit ac205b7b ("PCI: make sriov work with hotplug remove") indicates,
>> when removing devices on a bus, we do it in the reverse order. This means
>> we would remove VFs first, then PFs. After doing so, VF's removal is done
>> with pci_stop_and_remove_bus_device() instead of virtfn_remove().
>> virtfn_remove() returns the reference of its PF, while
>> pci_stop_and_remove_bus_device() doesn't.

Hi, Bjorn

Nice to see you again :-)

>
>Please use conventional citation style (12-char SHA1).

sure, I'd like to change it.

>
>ac205b7bb72f appeared in v3.4.  Did that commit cause a regression?
>Should this patch be marked for stable?
>

Hmm... regression.

I think commit ac205b7bb72f is not a complete fix for the problem. Before
commit ac205b7bb72f, system would crash, and after that, at least we can use
the machine.

While yes, I prefer this could be in stable tree.

>"After doing so, VF removal is done with pci_stop_and_remove_device() ..."
>After doing what?  After removing the VFs and PFs?  After commit
>ac205b7bb72f?

Sorry for my poor expression.

Here I mean, after commit ac205b7bb72f.
Before this commit ac205b7bb72f, VFs are destroyed in sriov_disable() called 
by the PF's driver. After this commit ac205b7bb72f, since we reverse the
order, VFs are destroyed by the pci_stop_and_remove_bus_device() in the loop.

>
>Prior to your patch, the VF reference was released in virtfn_remove(),
>which is only called via pci_disable_sriov().  This typically happens in
>a driver .remove() method.  The reference is *not* released if we call
>pci_stop_and_remove_bus_device(VF) directly, as we would via the
>remove_store() (sysfs "remove" file) or hot unplug paths, e.g.,
>pciehp_unconfigure_device().

You want to say the reference for VF or PF?

VF's reference is still released in virtfn_remove.
pci_stop_and_remove_bus_device() will call pci_destroy_dev() which will put
the dev's reference.

Maybe I don't get your point.

>
>After your patch, the VF reference is released in pci_destroy_dev().  This
>is called from pci_disable_sriov(), so it happens in that path as before.
>But pci_destroy_dev() is called from pci_stop_and_remove_bus_device(), so
>the reference is now released for all the paths that use
>pci_stop_and_remove_bus_device().
>

The change in this patch is the reference release of the PF.

Before my patch, PF's reference is released in virtfn_remove(). After my
patch, PF's reference is released in the pci_destroy_dev() of the VF.

>What about the other things done in virtfn_remove(), e.g., the sysfs link
>removal?  Your patch fixes a reference count leak, but don't we still have
>a sysfs link leak?
>

Agree, I am afraid the sysfs would have a leak too.

While I am not that familiar with the sysfs part, I don't dare to move that to
pci_destroy_dev(). Need more investigation.

>It would be useful to mention a way to cause the leak.  I suspect writing
>to a VF's sysfs "remove" file is the easiest.
>

I use the EEH hotplug case to see the leak, sounds your way is more general.
I will do some tests, and if it is true, I will put it in the change log.

>> This patches moves the return of PF's reference to pci_destroy_dev() to
>> make sure the PF's pci_dev is released in any case.
>> 
>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>> ---
>>  drivers/pci/iov.c    |    1 -
>>  drivers/pci/remove.c |    5 +++++
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index 4b3a4ea..9b04bde 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -167,7 +167,6 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>>  
>>  	/* balance pci_get_domain_bus_and_slot() */
>>  	pci_dev_put(virtfn);
>> -	pci_dev_put(dev);
>>  }
>>  
>>  static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>> index 8bd76c9..836ddf6 100644
>> --- a/drivers/pci/remove.c
>> +++ b/drivers/pci/remove.c
>> @@ -41,6 +41,11 @@ static void pci_destroy_dev(struct pci_dev *dev)
>>  	list_del(&dev->bus_list);
>>  	up_write(&pci_bus_sem);
>>  
>> +#ifdef CONFIG_PCI_IOV
>> +	if (dev->is_virtfn)
>> +		pci_dev_put(dev->physfn);
>> +#endif
>> +
>>  	pci_free_resources(dev);
>>  	put_device(&dev->dev);
>>  }
>> -- 
>> 1.7.9.5
>> 

-- 
Richard Yang
Help you, Help me

Re: [PATCH] pci/iov: return a reference to PF on destroying VF

[Wei Yang]

On Tue, May 05, 2015 at 04:29:05PM -0500, Bjorn Helgaas wrote:
>On Fri, Apr 10, 2015 at 04:53:04PM +0800, Wei Yang wrote:
>> Each VF will get a reference to its PF, while it is not returned back in
>> all cases and leave a removed PF's pci_dev un-released.
>> 
>> As commit ac205b7b ("PCI: make sriov work with hotplug remove") indicates,
>> when removing devices on a bus, we do it in the reverse order. This means
>> we would remove VFs first, then PFs. After doing so, VF's removal is done
>> with pci_stop_and_remove_bus_device() instead of virtfn_remove().
>> virtfn_remove() returns the reference of its PF, while
>> pci_stop_and_remove_bus_device() doesn't.
>
>Please use conventional citation style (12-char SHA1).
>
>ac205b7bb72f appeared in v3.4.  Did that commit cause a regression?
>Should this patch be marked for stable?
>
>"After doing so, VF removal is done with pci_stop_and_remove_device() ..."
>After doing what?  After removing the VFs and PFs?  After commit
>ac205b7bb72f?
>
>Prior to your patch, the VF reference was released in virtfn_remove(),
>which is only called via pci_disable_sriov().  This typically happens in
>a driver .remove() method.  The reference is *not* released if we call
>pci_stop_and_remove_bus_device(VF) directly, as we would via the
>remove_store() (sysfs "remove" file) or hot unplug paths, e.g.,
>pciehp_unconfigure_device().
>
>After your patch, the VF reference is released in pci_destroy_dev().  This
>is called from pci_disable_sriov(), so it happens in that path as before.
>But pci_destroy_dev() is called from pci_stop_and_remove_bus_device(), so
>the reference is now released for all the paths that use
>pci_stop_and_remove_bus_device().
>
>What about the other things done in virtfn_remove(), e.g., the sysfs link
>removal?  Your patch fixes a reference count leak, but don't we still have
>a sysfs link leak?
>
>It would be useful to mention a way to cause the leak.  I suspect writing
>to a VF's sysfs "remove" file is the easiest.
>

Looks a VF don't support the remove now.

static umode_t pci_dev_hp_attrs_are_visible(struct kobject *kobj,
					    struct attribute *a, int n)
{
	struct device *dev = container_of(kobj, struct device, kobj);
	struct pci_dev *pdev = to_pci_dev(dev);

	if (pdev->is_virtfn)
		return 0;

	return a->mode;
}

static struct attribute_group pci_dev_hp_attr_group = {
	.attrs = pci_dev_hp_attrs,
	.is_visible = pci_dev_hp_attrs_are_visible,
};


>> This patches moves the return of PF's reference to pci_destroy_dev() to
>> make sure the PF's pci_dev is released in any case.
>> 
>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>> ---
>>  drivers/pci/iov.c    |    1 -
>>  drivers/pci/remove.c |    5 +++++
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index 4b3a4ea..9b04bde 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -167,7 +167,6 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>>  
>>  	/* balance pci_get_domain_bus_and_slot() */
>>  	pci_dev_put(virtfn);
>> -	pci_dev_put(dev);
>>  }
>>  
>>  static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>> index 8bd76c9..836ddf6 100644
>> --- a/drivers/pci/remove.c
>> +++ b/drivers/pci/remove.c
>> @@ -41,6 +41,11 @@ static void pci_destroy_dev(struct pci_dev *dev)
>>  	list_del(&dev->bus_list);
>>  	up_write(&pci_bus_sem);
>>  
>> +#ifdef CONFIG_PCI_IOV
>> +	if (dev->is_virtfn)
>> +		pci_dev_put(dev->physfn);
>> +#endif
>> +
>>  	pci_free_resources(dev);
>>  	put_device(&dev->dev);
>>  }
>> -- 
>> 1.7.9.5
>> 

-- 
Richard Yang
Help you, Help me

Re: [PATCH] pci/iov: return a reference to PF on destroying VF

[Bjorn Helgaas]

On Wed, May 6, 2015 at 2:25 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> On Tue, May 05, 2015 at 04:29:05PM -0500, Bjorn Helgaas wrote:

>>It would be useful to mention a way to cause the leak.  I suspect writing
>>to a VF's sysfs "remove" file is the easiest.
>>
>
> Looks a VF don't support the remove now.
>
> static umode_t pci_dev_hp_attrs_are_visible(struct kobject *kobj,
>                                             struct attribute *a, int n)
> {
>         struct device *dev = container_of(kobj, struct device, kobj);
>         struct pci_dev *pdev = to_pci_dev(dev);
>
>         if (pdev->is_virtfn)
>                 return 0;
>
>         return a->mode;
> }
>
> static struct attribute_group pci_dev_hp_attr_group = {
>         .attrs = pci_dev_hp_attrs,
>         .is_visible = pci_dev_hp_attrs_are_visible,
> };

Right, I forgot about this.  A VF has no "remove" file, so it can't be
used to cause the leak.

Re: [PATCH] pci/iov: return a reference to PF on destroying VF

[Bjorn Helgaas]

On Wed, May 6, 2015 at 1:09 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> On Tue, May 05, 2015 at 04:29:05PM -0500, Bjorn Helgaas wrote:

>>Prior to your patch, the VF reference was released in virtfn_remove(),
>>which is only called via pci_disable_sriov().  This typically happens in
>>a driver .remove() method.  The reference is *not* released if we call
>>pci_stop_and_remove_bus_device(VF) directly, as we would via the
>>remove_store() (sysfs "remove" file) or hot unplug paths, e.g.,
>>pciehp_unconfigure_device().
>
> You want to say the reference for VF or PF?

Yes, I meant the PF reference.

The hot unplug paths call pci_stop_and_remove_bus_device() for the VFs
first, then the PF.  Calling it for the VF releases the VF reference
but not the PF one.  Calling it for the PF will call virtfn_remove()
via the driver's .remove() method, but it probably does nothing
because when it calls pci_get_domain_bus_and_slot() to get the virtfn,
it gets a NULL because the VF has already been removed.

>>What about the other things done in virtfn_remove(), e.g., the sysfs link
>>removal?  Your patch fixes a reference count leak, but don't we still have
>>a sysfs link leak?
>
> Agree, I am afraid the sysfs would have a leak too.
>
> While I am not that familiar with the sysfs part, I don't dare to move that to
> pci_destroy_dev(). Need more investigation.

OK.  I don't want to fix half of the leak problem.  I want to fix the
whole thing at once.

Bjorn

Re: [PATCH] pci/iov: return a reference to PF on destroying VF

[Wei Yang]

On Wed, May 06, 2015 at 10:30:39AM -0500, Bjorn Helgaas wrote:
>On Wed, May 6, 2015 at 1:09 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>> On Tue, May 05, 2015 at 04:29:05PM -0500, Bjorn Helgaas wrote:
>
>>>Prior to your patch, the VF reference was released in virtfn_remove(),
>>>which is only called via pci_disable_sriov().  This typically happens in
>>>a driver .remove() method.  The reference is *not* released if we call
>>>pci_stop_and_remove_bus_device(VF) directly, as we would via the
>>>remove_store() (sysfs "remove" file) or hot unplug paths, e.g.,
>>>pciehp_unconfigure_device().
>>
>> You want to say the reference for VF or PF?
>
>Yes, I meant the PF reference.
>
>The hot unplug paths call pci_stop_and_remove_bus_device() for the VFs
>first, then the PF.  Calling it for the VF releases the VF reference
>but not the PF one.  Calling it for the PF will call virtfn_remove()
>via the driver's .remove() method, but it probably does nothing
>because when it calls pci_get_domain_bus_and_slot() to get the virtfn,
>it gets a NULL because the VF has already been removed.
>
>>>What about the other things done in virtfn_remove(), e.g., the sysfs link
>>>removal?  Your patch fixes a reference count leak, but don't we still have
>>>a sysfs link leak?
>>
>> Agree, I am afraid the sysfs would have a leak too.
>>
>> While I am not that familiar with the sysfs part, I don't dare to move that to
>> pci_destroy_dev(). Need more investigation.
>
>OK.  I don't want to fix half of the leak problem.  I want to fix the
>whole thing at once.
>

Sure, I will do some investigation and repost it.

BTW, seems we also face the leak of the virtual bus. Will fix it in next
version too.

>Bjorn

-- 
Richard Yang
Help you, Help me

Re: [PATCH] pci/iov: return a reference to PF on destroying VF

[Wei Yang]

On Wed, May 06, 2015 at 10:23:26AM -0500, Bjorn Helgaas wrote:
>On Wed, May 6, 2015 at 2:25 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>> On Tue, May 05, 2015 at 04:29:05PM -0500, Bjorn Helgaas wrote:
>
>>>It would be useful to mention a way to cause the leak.  I suspect writing
>>>to a VF's sysfs "remove" file is the easiest.
>>>
>>
>> Looks a VF don't support the remove now.
>>
>> static umode_t pci_dev_hp_attrs_are_visible(struct kobject *kobj,
>>                                             struct attribute *a, int n)
>> {
>>         struct device *dev = container_of(kobj, struct device, kobj);
>>         struct pci_dev *pdev = to_pci_dev(dev);
>>
>>         if (pdev->is_virtfn)
>>                 return 0;
>>
>>         return a->mode;
>> }
>>
>> static struct attribute_group pci_dev_hp_attr_group = {
>>         .attrs = pci_dev_hp_attrs,
>>         .is_visible = pci_dev_hp_attrs_are_visible,
>> };
>
>Right, I forgot about this.  A VF has no "remove" file, so it can't be
>used to cause the leak.

Hmm, I see this is introduced in commit dfab88beda88, to prevent some memory
leak.

Could we say, if we do the release properly like in this patch, we could
re-enable this "remove"?

-- 
Richard Yang
Help you, Help me

[PATCH V2] pci/iov: fix resource leak on destroying VF

[Wei Yang]

Beside pci_dev resources, VF has other resources between its PF, like
refcount to PF, sysfs link between them and the virtual bus. When a VF is
released in virtfn_remove(), those resources are released properly. But
in the hotplug case, they are not.

In hotplug case, VFs are removed by pci_stop_and_remove_bus_device()
instead of virtfn_remove(). This leads to some leak for resources.

This patch moves the resource release to pci_destroy_dev() to make sure
those resources are released when VF is destroyed.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
 drivers/pci/iov.c    |   42 +++++++++++++++++++++++-------------------
 drivers/pci/pci.h    |   10 ++++++++++
 drivers/pci/remove.c |   31 +++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index ee0ebff..47daf2f 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -17,8 +17,6 @@
 #include <linux/pci-ats.h>
 #include "pci.h"
 
-#define VIRTFN_ID_LEN	16
-
 int pci_iov_virtfn_bus(struct pci_dev *dev, int vf_id)
 {
 	if (!dev->is_physfn)
@@ -94,7 +92,7 @@ static struct pci_bus *virtfn_add_bus(struct pci_bus *bus, int busnr)
 	return child;
 }
 
-static void virtfn_remove_bus(struct pci_bus *physbus, struct pci_bus *virtbus)
+void virtfn_remove_bus(struct pci_bus *physbus, struct pci_bus *virtbus)
 {
 	if (physbus != virtbus && list_empty(&virtbus->devices))
 		pci_remove_bus(virtbus);
@@ -185,9 +183,7 @@ failed:
 
 static void virtfn_remove(struct pci_dev *dev, int id, int reset)
 {
-	char buf[VIRTFN_ID_LEN];
 	struct pci_dev *virtfn;
-	struct pci_sriov *iov = dev->sriov;
 
 	virtfn = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus),
 					     pci_iov_virtfn_bus(dev, id),
@@ -200,24 +196,10 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)
 		__pci_reset_function(virtfn);
 	}
 
-	sprintf(buf, "virtfn%u", id);
-	sysfs_remove_link(&dev->dev.kobj, buf);
-	/*
-	 * pci_stop_dev() could have been called for this virtfn already,
-	 * so the directory for the virtfn may have been removed before.
-	 * Double check to avoid spurious sysfs warnings.
-	 */
-	if (virtfn->dev.kobj.sd)
-		sysfs_remove_link(&virtfn->dev.kobj, "physfn");
-
-	mutex_lock(&iov->dev->sriov->lock);
 	pci_stop_and_remove_bus_device(virtfn);
-	virtfn_remove_bus(dev->bus, virtfn->bus);
-	mutex_unlock(&iov->dev->sriov->lock);
 
 	/* balance pci_get_domain_bus_and_slot() */
 	pci_dev_put(virtfn);
-	pci_dev_put(dev);
 }
 
 int __weak pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
@@ -760,3 +742,25 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
 	return dev->sriov->total_VFs;
 }
 EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
+
+void pci_sriov_lock(struct pci_dev *dev)
+{
+	struct pci_sriov *iov;
+
+	if (!dev->is_physfn)
+		return;
+
+	iov = dev->sriov;
+	mutex_lock(&iov->dev->sriov->lock);
+}
+
+void pci_sriov_unlock(struct pci_dev *dev)
+{
+	struct pci_sriov *iov;
+
+	if (!dev->is_physfn)
+		return;
+
+	iov = dev->sriov;
+	mutex_unlock(&iov->dev->sriov->lock);
+}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9bd762c2..a0323a2 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -260,6 +260,12 @@ static inline void pci_restore_ats_state(struct pci_dev *dev)
 #endif /* CONFIG_PCI_ATS */
 
 #ifdef CONFIG_PCI_IOV
+
+#define VIRTFN_ID_LEN	16
+void pci_sriov_lock(struct pci_dev *dev);
+void pci_sriov_unlock(struct pci_dev *dev);
+void virtfn_remove_bus(struct pci_bus *physbus, struct pci_bus *virtbus);
+
 int pci_iov_init(struct pci_dev *dev);
 void pci_iov_release(struct pci_dev *dev);
 int pci_iov_resource_bar(struct pci_dev *dev, int resno);
@@ -268,6 +274,10 @@ void pci_restore_iov_state(struct pci_dev *dev);
 int pci_iov_bus_range(struct pci_bus *bus);
 
 #else
+static inline void pci_sriov_lock(struct pci_dev *dev) { }
+static inline void pci_sriov_unlock(struct pci_dev *dev) { }
+static inline void virtfn_remove_bus(struct pci_bus *physbus,
+		struct pci_bus *virtbus) { }
 static inline int pci_iov_init(struct pci_dev *dev)
 {
 	return -ENODEV;
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 8a280e9..f2a07bf 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -41,6 +41,37 @@ static void pci_destroy_dev(struct pci_dev *dev)
 	list_del(&dev->bus_list);
 	up_write(&pci_bus_sem);
 
+#ifdef CONFIG_PCI_IOV
+	if (dev->is_virtfn) {
+		char buf[VIRTFN_ID_LEN];
+		int id;
+		struct pci_dev *pdev = dev->physfn;
+
+		for (id = 0; id < pci_sriov_get_totalvfs(pdev); id++) {
+			if ((dev->bus->number == pci_iov_virtfn_bus(pdev, id))
+			   && (dev->devfn == pci_iov_virtfn_devfn(pdev, id))) {
+				sprintf(buf, "virtfn%u", id);
+				sysfs_remove_link(&pdev->dev.kobj, buf);
+				break;
+			}
+		}
+		/*
+		 * pci_stop_dev() could have been called for this virtfn
+		 * already, so the directory for the virtfn may have been
+		 * removed before.  Double check to avoid spurious sysfs
+		 * warnings.
+		 */
+		if (dev->dev.kobj.sd)
+			sysfs_remove_link(&dev->dev.kobj, "physfn");
+
+		pci_sriov_lock(pdev);
+		virtfn_remove_bus(pdev->bus, dev->bus);
+		pci_sriov_unlock(pdev);
+
+		pci_dev_put(dev->physfn);
+	}
+#endif
+
 	pci_free_resources(dev);
 	put_device(&dev->dev);
 }
-- 
1.7.9.5

Re: [PATCH V2] pci/iov: fix resource leak on destroying VF

[Bjorn Helgaas]

On Fri, May 08, 2015 at 11:53:27AM +0800, Wei Yang wrote:
> Beside pci_dev resources, VF has other resources between its PF, like
> refcount to PF, sysfs link between them and the virtual bus. When a VF is
> released in virtfn_remove(), those resources are released properly. But
> in the hotplug case, they are not.
> 
> In hotplug case, VFs are removed by pci_stop_and_remove_bus_device()
> instead of virtfn_remove(). This leads to some leak for resources.
> 
> This patch moves the resource release to pci_destroy_dev() to make sure
> those resources are released when VF is destroyed.

Earlier you mentioned a related commit, but you didn't reference it here.
I thought you said this fixed a regression.  If so, we need information
about it.  I know you said you'd like this patch to go to the stable
kernel.  We need justification for that, too, e.g., a way to reproduce a
problem and what the effect of the problem is.  I guess "hot-remove of
an SR-IOV device" is probably the way to reproduce it, but I don't know
what the effect is.  Does a subsequent hot-add fail?  Do we silently leak
some memory?  If you could open a bugzilla with a dmesg log showing the
problem, that would be great.

> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> ---
>  drivers/pci/iov.c    |   42 +++++++++++++++++++++++-------------------
>  drivers/pci/pci.h    |   10 ++++++++++
>  drivers/pci/remove.c |   31 +++++++++++++++++++++++++++++++
>  3 files changed, 64 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index ee0ebff..47daf2f 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -17,8 +17,6 @@
>  #include <linux/pci-ats.h>
>  #include "pci.h"
>  
> -#define VIRTFN_ID_LEN	16
> -
>  int pci_iov_virtfn_bus(struct pci_dev *dev, int vf_id)
>  {
>  	if (!dev->is_physfn)
> @@ -94,7 +92,7 @@ static struct pci_bus *virtfn_add_bus(struct pci_bus *bus, int busnr)
>  	return child;
>  }
>  
> -static void virtfn_remove_bus(struct pci_bus *physbus, struct pci_bus *virtbus)
> +void virtfn_remove_bus(struct pci_bus *physbus, struct pci_bus *virtbus)
>  {
>  	if (physbus != virtbus && list_empty(&virtbus->devices))
>  		pci_remove_bus(virtbus);
> @@ -185,9 +183,7 @@ failed:
>  
>  static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>  {
> -	char buf[VIRTFN_ID_LEN];
>  	struct pci_dev *virtfn;
> -	struct pci_sriov *iov = dev->sriov;
>  
>  	virtfn = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus),
>  					     pci_iov_virtfn_bus(dev, id),
> @@ -200,24 +196,10 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>  		__pci_reset_function(virtfn);
>  	}
>  
> -	sprintf(buf, "virtfn%u", id);
> -	sysfs_remove_link(&dev->dev.kobj, buf);
> -	/*
> -	 * pci_stop_dev() could have been called for this virtfn already,
> -	 * so the directory for the virtfn may have been removed before.
> -	 * Double check to avoid spurious sysfs warnings.
> -	 */
> -	if (virtfn->dev.kobj.sd)
> -		sysfs_remove_link(&virtfn->dev.kobj, "physfn");
> -
> -	mutex_lock(&iov->dev->sriov->lock);
>  	pci_stop_and_remove_bus_device(virtfn);
> -	virtfn_remove_bus(dev->bus, virtfn->bus);
> -	mutex_unlock(&iov->dev->sriov->lock);
>  
>  	/* balance pci_get_domain_bus_and_slot() */
>  	pci_dev_put(virtfn);
> -	pci_dev_put(dev);
>  }
>  
>  int __weak pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
> @@ -760,3 +742,25 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
>  	return dev->sriov->total_VFs;
>  }
>  EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
> +
> +void pci_sriov_lock(struct pci_dev *dev)
> +{
> +	struct pci_sriov *iov;
> +
> +	if (!dev->is_physfn)
> +		return;
> +
> +	iov = dev->sriov;
> +	mutex_lock(&iov->dev->sriov->lock);
> +}
> +
> +void pci_sriov_unlock(struct pci_dev *dev)
> +{
> +	struct pci_sriov *iov;
> +
> +	if (!dev->is_physfn)
> +		return;
> +
> +	iov = dev->sriov;
> +	mutex_unlock(&iov->dev->sriov->lock);
> +}
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 9bd762c2..a0323a2 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -260,6 +260,12 @@ static inline void pci_restore_ats_state(struct pci_dev *dev)
>  #endif /* CONFIG_PCI_ATS */
>  
>  #ifdef CONFIG_PCI_IOV
> +
> +#define VIRTFN_ID_LEN	16
> +void pci_sriov_lock(struct pci_dev *dev);
> +void pci_sriov_unlock(struct pci_dev *dev);
> +void virtfn_remove_bus(struct pci_bus *physbus, struct pci_bus *virtbus);
> +
>  int pci_iov_init(struct pci_dev *dev);
>  void pci_iov_release(struct pci_dev *dev);
>  int pci_iov_resource_bar(struct pci_dev *dev, int resno);
> @@ -268,6 +274,10 @@ void pci_restore_iov_state(struct pci_dev *dev);
>  int pci_iov_bus_range(struct pci_bus *bus);
>  
>  #else
> +static inline void pci_sriov_lock(struct pci_dev *dev) { }
> +static inline void pci_sriov_unlock(struct pci_dev *dev) { }
> +static inline void virtfn_remove_bus(struct pci_bus *physbus,
> +		struct pci_bus *virtbus) { }

This doesn't make sense to me.  You added calls to pci_sriov_lock(), etc.,
but they are all under #ifdef CONFIG_PCI_IOV.  So why do you need stubs
when CONFIG_PCI_IOV is not defined?

>  static inline int pci_iov_init(struct pci_dev *dev)
>  {
>  	return -ENODEV;
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 8a280e9..f2a07bf 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -41,6 +41,37 @@ static void pci_destroy_dev(struct pci_dev *dev)
>  	list_del(&dev->bus_list);
>  	up_write(&pci_bus_sem);
>  
> +#ifdef CONFIG_PCI_IOV
> +	if (dev->is_virtfn) {
> +		char buf[VIRTFN_ID_LEN];
> +		int id;
> +		struct pci_dev *pdev = dev->physfn;
> +
> +		for (id = 0; id < pci_sriov_get_totalvfs(pdev); id++) {
> +			if ((dev->bus->number == pci_iov_virtfn_bus(pdev, id))
> +			   && (dev->devfn == pci_iov_virtfn_devfn(pdev, id))) {
> +				sprintf(buf, "virtfn%u", id);
> +				sysfs_remove_link(&pdev->dev.kobj, buf);
> +				break;
> +			}
> +		}
> +		/*
> +		 * pci_stop_dev() could have been called for this virtfn
> +		 * already, so the directory for the virtfn may have been
> +		 * removed before.  Double check to avoid spurious sysfs
> +		 * warnings.
> +		 */
> +		if (dev->dev.kobj.sd)
> +			sysfs_remove_link(&dev->dev.kobj, "physfn");
> +
> +		pci_sriov_lock(pdev);
> +		virtfn_remove_bus(pdev->bus, dev->bus);
> +		pci_sriov_unlock(pdev);
> +
> +		pci_dev_put(dev->physfn);
> +	}

All this IOV-related code looks really out of place in pci_destroy_dev().
Isn't there some way this code can be put in drivers/pci/iov.c?

> +#endif
> +
>  	pci_free_resources(dev);
>  	put_device(&dev->dev);
>  }
> -- 
> 1.7.9.5
> 

Re: [PATCH V2] pci/iov: fix resource leak on destroying VF

[Wei Yang]

On Tue, May 19, 2015 at 06:00:20PM -0500, Bjorn Helgaas wrote:
>On Fri, May 08, 2015 at 11:53:27AM +0800, Wei Yang wrote:
>> Beside pci_dev resources, VF has other resources between its PF, like
>> refcount to PF, sysfs link between them and the virtual bus. When a VF is
>> released in virtfn_remove(), those resources are released properly. But
>> in the hotplug case, they are not.
>> 
>> In hotplug case, VFs are removed by pci_stop_and_remove_bus_device()
>> instead of virtfn_remove(). This leads to some leak for resources.
>> 
>> This patch moves the resource release to pci_destroy_dev() to make sure
>> those resources are released when VF is destroyed.
>
>Earlier you mentioned a related commit, but you didn't reference it here.
>I thought you said this fixed a regression.  If so, we need information
>about it.  I know you said you'd like this patch to go to the stable
>kernel.  We need justification for that, too, e.g., a way to reproduce a
>problem and what the effect of the problem is.  I guess "hot-remove of
>an SR-IOV device" is probably the way to reproduce it, but I don't know
>what the effect is.  Does a subsequent hot-add fail?  Do we silently leak
>some memory?  If you could open a bugzilla with a dmesg log showing the
>problem, that would be great.
>

I don't find an easy way to do "hot-remove" of an SR-IOV device. What I can
think is to export the "remove" sysfs for a VF, then see the effect. While
this needs to change the kernel to see the effect. Not sure this is
acceptable.

The effect of the "hot-remove" is, (I guess, not tested yet)
1. after VF removed, PF still has a link to VF in sysfs
2. PF is not release cleanly when all VF and PF itself is removed

Let me try to gather those information, if I could create this.

>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>> ---
>>  drivers/pci/iov.c    |   42 +++++++++++++++++++++++-------------------
>>  drivers/pci/pci.h    |   10 ++++++++++
>>  drivers/pci/remove.c |   31 +++++++++++++++++++++++++++++++
>>  3 files changed, 64 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index ee0ebff..47daf2f 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -17,8 +17,6 @@
>>  #include <linux/pci-ats.h>
>>  #include "pci.h"
>>  
>> -#define VIRTFN_ID_LEN	16
>> -
>>  int pci_iov_virtfn_bus(struct pci_dev *dev, int vf_id)
>>  {
>>  	if (!dev->is_physfn)
>> @@ -94,7 +92,7 @@ static struct pci_bus *virtfn_add_bus(struct pci_bus *bus, int busnr)
>>  	return child;
>>  }
>>  
>> -static void virtfn_remove_bus(struct pci_bus *physbus, struct pci_bus *virtbus)
>> +void virtfn_remove_bus(struct pci_bus *physbus, struct pci_bus *virtbus)
>>  {
>>  	if (physbus != virtbus && list_empty(&virtbus->devices))
>>  		pci_remove_bus(virtbus);
>> @@ -185,9 +183,7 @@ failed:
>>  
>>  static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>>  {
>> -	char buf[VIRTFN_ID_LEN];
>>  	struct pci_dev *virtfn;
>> -	struct pci_sriov *iov = dev->sriov;
>>  
>>  	virtfn = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus),
>>  					     pci_iov_virtfn_bus(dev, id),
>> @@ -200,24 +196,10 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>>  		__pci_reset_function(virtfn);
>>  	}
>>  
>> -	sprintf(buf, "virtfn%u", id);
>> -	sysfs_remove_link(&dev->dev.kobj, buf);
>> -	/*
>> -	 * pci_stop_dev() could have been called for this virtfn already,
>> -	 * so the directory for the virtfn may have been removed before.
>> -	 * Double check to avoid spurious sysfs warnings.
>> -	 */
>> -	if (virtfn->dev.kobj.sd)
>> -		sysfs_remove_link(&virtfn->dev.kobj, "physfn");
>> -
>> -	mutex_lock(&iov->dev->sriov->lock);
>>  	pci_stop_and_remove_bus_device(virtfn);
>> -	virtfn_remove_bus(dev->bus, virtfn->bus);
>> -	mutex_unlock(&iov->dev->sriov->lock);
>>  
>>  	/* balance pci_get_domain_bus_and_slot() */
>>  	pci_dev_put(virtfn);
>> -	pci_dev_put(dev);
>>  }
>>  
>>  int __weak pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
>> @@ -760,3 +742,25 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
>>  	return dev->sriov->total_VFs;
>>  }
>>  EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
>> +
>> +void pci_sriov_lock(struct pci_dev *dev)
>> +{
>> +	struct pci_sriov *iov;
>> +
>> +	if (!dev->is_physfn)
>> +		return;
>> +
>> +	iov = dev->sriov;
>> +	mutex_lock(&iov->dev->sriov->lock);
>> +}
>> +
>> +void pci_sriov_unlock(struct pci_dev *dev)
>> +{
>> +	struct pci_sriov *iov;
>> +
>> +	if (!dev->is_physfn)
>> +		return;
>> +
>> +	iov = dev->sriov;
>> +	mutex_unlock(&iov->dev->sriov->lock);
>> +}
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 9bd762c2..a0323a2 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -260,6 +260,12 @@ static inline void pci_restore_ats_state(struct pci_dev *dev)
>>  #endif /* CONFIG_PCI_ATS */
>>  
>>  #ifdef CONFIG_PCI_IOV
>> +
>> +#define VIRTFN_ID_LEN	16
>> +void pci_sriov_lock(struct pci_dev *dev);
>> +void pci_sriov_unlock(struct pci_dev *dev);
>> +void virtfn_remove_bus(struct pci_bus *physbus, struct pci_bus *virtbus);
>> +
>>  int pci_iov_init(struct pci_dev *dev);
>>  void pci_iov_release(struct pci_dev *dev);
>>  int pci_iov_resource_bar(struct pci_dev *dev, int resno);
>> @@ -268,6 +274,10 @@ void pci_restore_iov_state(struct pci_dev *dev);
>>  int pci_iov_bus_range(struct pci_bus *bus);
>>  
>>  #else
>> +static inline void pci_sriov_lock(struct pci_dev *dev) { }
>> +static inline void pci_sriov_unlock(struct pci_dev *dev) { }
>> +static inline void virtfn_remove_bus(struct pci_bus *physbus,
>> +		struct pci_bus *virtbus) { }
>
>This doesn't make sense to me.  You added calls to pci_sriov_lock(), etc.,
>but they are all under #ifdef CONFIG_PCI_IOV.  So why do you need stubs
>when CONFIG_PCI_IOV is not defined?
>

Hmm, you are right.

>>  static inline int pci_iov_init(struct pci_dev *dev)
>>  {
>>  	return -ENODEV;
>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>> index 8a280e9..f2a07bf 100644
>> --- a/drivers/pci/remove.c
>> +++ b/drivers/pci/remove.c
>> @@ -41,6 +41,37 @@ static void pci_destroy_dev(struct pci_dev *dev)
>>  	list_del(&dev->bus_list);
>>  	up_write(&pci_bus_sem);
>>  
>> +#ifdef CONFIG_PCI_IOV
>> +	if (dev->is_virtfn) {
>> +		char buf[VIRTFN_ID_LEN];
>> +		int id;
>> +		struct pci_dev *pdev = dev->physfn;
>> +
>> +		for (id = 0; id < pci_sriov_get_totalvfs(pdev); id++) {
>> +			if ((dev->bus->number == pci_iov_virtfn_bus(pdev, id))
>> +			   && (dev->devfn == pci_iov_virtfn_devfn(pdev, id))) {
>> +				sprintf(buf, "virtfn%u", id);
>> +				sysfs_remove_link(&pdev->dev.kobj, buf);
>> +				break;
>> +			}
>> +		}
>> +		/*
>> +		 * pci_stop_dev() could have been called for this virtfn
>> +		 * already, so the directory for the virtfn may have been
>> +		 * removed before.  Double check to avoid spurious sysfs
>> +		 * warnings.
>> +		 */
>> +		if (dev->dev.kobj.sd)
>> +			sysfs_remove_link(&dev->dev.kobj, "physfn");
>> +
>> +		pci_sriov_lock(pdev);
>> +		virtfn_remove_bus(pdev->bus, dev->bus);
>> +		pci_sriov_unlock(pdev);
>> +
>> +		pci_dev_put(dev->physfn);
>> +	}
>
>All this IOV-related code looks really out of place in pci_destroy_dev().
>Isn't there some way this code can be put in drivers/pci/iov.c?
>

Currently, I didn't find a better solution.
Let me take a look again.

>> +#endif
>> +
>>  	pci_free_resources(dev);
>>  	put_device(&dev->dev);
>>  }
>> -- 
>> 1.7.9.5
>> 

-- 
Richard Yang
Help you, Help me