On Wed, Aug 07, 2013 at 06:49:25PM -0700, Yinghai Lu wrote: > On Tue, Aug 6, 2013 at 11:34 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote: > > On Tue, Aug 6, 2013 at 9:01 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote: > >> > >> Huh. I wish we didn't have virtfn_remove() at all. I wish the > >> normal device removal path, i.e., pci_stop_and_remove_bus_device(), > >> could deal with VFs directly. I don't know all the history there, > >> so maybe there's some reason that's not feasible. > > > > I had one draft version, but looks more confusing. > > please check attached that make pci_stop_and_remove_bus_device() > could be used with VF. > > It could replace > https://patchwork.kernel.org/patch/2834638/ > https://patchwork.kernel.org/patch/2834639/ > > Please choose one of the solutions. > > but we still need > https://patchwork.kernel.org/patch/2834640/ > as VFs that does not use ARI could be on other virtual bus. > so they will not be removed directly. [I inlined your patch below for convenience] > Subject: [PATCH] PCI: Release PF ref during removing VF > From: Yinghai Lu <yinghai@xxxxxxxxxx> > > commit dc087f2f6a2925e81831f3016b9cbb6e470e7423 > (PCI: Simplify IOV implementation and fix reference count races) > broke the pcie native hotplug with SRIOV enabled: PF is not freed > during hot-remove, and later hot-add do not work as old pci_dev > is still around, and can not create new pci_dev. > > That commit need VFs to be removed via pci_disable_sriov/virtfn_remove > to make sure ref to PF is put back. > > So we can not call stop_and_remove for VF before PF, that will > make VF get removed directly before PF's driver try to use > pci_disable_sriov/virtfn_remove to do it. > > Solution would be > 1. separating stop and remove in two iterations. > 2. or make pci_stop_and_remove_bus_device could be used on VF. > > This patch is second solution: > Separate PF ref releasing from virtfn_remove, all that during > pci_destroy_dev for VFs. I like the second approach better, but I think the call path and locking is way too messy: virtfn_remove mutex_lock(&iov->dev->sriov->lock) <-- pci_stop_and_remove_bus_device mutex_trylock(&iov->dev->sriov->lock) <-- pci_stop_bus_device pci_remove_bus_device pci_destroy_dev virtfn_release virtfn_remove_bus pci_dev_put I think it could be fixed, but it would require significant SR-IOV rework, and I'm dubious that we can get it done in time for v3.11. What would happen if we just reverted dc087f2f6a2 for now? Jiang? > Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx> > > --- > drivers/pci/iov.c | 34 +++++++++++++++++++++++++++++----- > drivers/pci/pci.h | 4 ++++ > drivers/pci/remove.c | 17 +++++++++++++++++ > 3 files changed, 50 insertions(+), 5 deletions(-) > > Index: linux-2.6/drivers/pci/iov.c > =================================================================== > --- linux-2.6.orig/drivers/pci/iov.c > +++ linux-2.6/drivers/pci/iov.c > @@ -132,9 +132,37 @@ failed: > return rc; > } > > -static void virtfn_remove(struct pci_dev *dev, int id, int reset) > +void virtfn_release(struct pci_dev *virtfn) > { > + int i; > + struct pci_dev *dev; > char buf[VIRTFN_ID_LEN]; > + > + if (!virtfn->is_virtfn) > + return; > + > + dev = virtfn->physfn; > + > + /* > + * Remove link to virtfn for PF. > + * pci_disable_sriov() could be called after pci_stop_dev() is > + * called for PF. So check sd to avoid spurious sfsfs warning. > + */ > + if (dev->dev.kobj.sd) > + for (i = 0; i < dev->sriov->num_VFs; i++) > + if ((virtfn_bus(dev, i) == virtfn->bus->number) && > + (virtfn_devfn(dev, i) == virtfn->devfn)) { > + sprintf(buf, "virtfn%u", i); > + sysfs_remove_link(&dev->dev.kobj, buf); > + break; > + } > + > + virtfn_remove_bus(dev->bus, virtfn->bus); > + pci_dev_put(dev); > +} > + > +static void virtfn_remove(struct pci_dev *dev, int id, int reset) > +{ > struct pci_dev *virtfn; > struct pci_sriov *iov = dev->sriov; > > @@ -149,8 +177,6 @@ static void virtfn_remove(struct pci_dev > __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. > @@ -161,12 +187,10 @@ static void virtfn_remove(struct pci_dev > > 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); > } > > static int sriov_migration(struct pci_dev *dev) > Index: linux-2.6/drivers/pci/pci.h > =================================================================== > --- linux-2.6.orig/drivers/pci/pci.h > +++ linux-2.6/drivers/pci/pci.h > @@ -262,6 +262,7 @@ int pci_iov_resource_bar(struct pci_dev > resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno); > void pci_restore_iov_state(struct pci_dev *dev); > int pci_iov_bus_range(struct pci_bus *bus); > +void virtfn_release(struct pci_dev *dev); > > #else > static inline int pci_iov_init(struct pci_dev *dev) > @@ -284,6 +285,9 @@ static inline int pci_iov_bus_range(stru > { > return 0; > } > +void virtfn_release(struct pci_dev *dev) > +{ > +} > > #endif /* CONFIG_PCI_IOV */ > > Index: linux-2.6/drivers/pci/remove.c > =================================================================== > --- linux-2.6.orig/drivers/pci/remove.c > +++ linux-2.6/drivers/pci/remove.c > @@ -38,6 +38,8 @@ static void pci_destroy_dev(struct pci_d > list_del(&dev->bus_list); > up_write(&pci_bus_sem); > > + virtfn_release(dev); > + > pci_free_resources(dev); > put_device(&dev->dev); > } > @@ -107,8 +109,23 @@ static void pci_remove_bus_device(struct > */ > void pci_stop_and_remove_bus_device(struct pci_dev *dev) > { > +#ifdef CONFIG_PCI_ATS > + struct pci_sriov *iov = NULL; > + int locked = 0; > + > + if (dev->is_virtfn) { > + iov = dev->physfn->sriov; > + locked = mutex_trylock(&iov->dev->sriov->lock); > + } > +#endif > + > pci_stop_bus_device(dev); > pci_remove_bus_device(dev); > + > +#ifdef CONFIG_PCI_ATS > + if (locked) > + mutex_unlock(&iov->dev->sriov->lock); > +#endif > } > EXPORT_SYMBOL(pci_stop_and_remove_bus_device); > -- 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
