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@xxxxxxxxxxxxxxxxxx>
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
