Hi Alex,
Thanks, this definitely clears up some problems. Two minor questions
below.
On Tue, Oct 27, 2015 at 01:52:27PM -0700, Alexander Duyck wrote:
> >From what I can tell there were several errors in the sriov_enable
> exception handling path. Below is a brief list of what I believe I am
> fixing:
>
> 1. If pcibios_enable_sriov failed, we returned without disabling SR-IOV on
> the device.
> 2. If virtfn_add failed we didn't call pcibios_disable_sriov to undo
> pcibios_enable_sriov.
> 3. We were resetting numvfs to 0 before a second had passed for the VFs to
> quiesce.
> 4. Minor coding style issues for white space and for assignment in
> conditional check.
>
> Beyond addressing these 4 issues there were also 2 other minor issues in
> that retval was a redundant variable with rc, and j wasn't actually needed
> as we could simply reverse the loop we were running when setting up i. As
> such I have updated the code to address those two items.
>
> Signed-off-by: Alexander Duyck <adu...@mirantis.com>
> ---
> drivers/pci/iov.c | 31 +++++++++++++++++--------------
> 1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 238950412de0..cecc242c1af0 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -231,13 +231,18 @@ static void virtfn_remove(struct pci_dev *dev, int id,
> int reset)
>
> int __weak pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
> {
> - return 0;
> + return 0;
> +}
> +
> +int __weak pcibios_sriov_disable(struct pci_dev *pdev)
> +{
> + return 0;
> }
>
> static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> {
> int rc;
> - int i, j;
> + int i;
> int nres;
> u16 offset, stride, initial;
> struct resource *res;
> @@ -245,7 +250,6 @@ static int sriov_enable(struct pci_dev *dev, int
> nr_virtfn)
> struct pci_sriov *iov = dev->sriov;
> int bars = 0;
> int bus;
> - int retval;
>
> if (!nr_virtfn)
> return 0;
> @@ -322,10 +326,11 @@ static int sriov_enable(struct pci_dev *dev, int
> nr_virtfn)
> if (nr_virtfn < initial)
> initial = nr_virtfn;
>
> - if ((retval = pcibios_sriov_enable(dev, initial))) {
> + rc = pcibios_sriov_enable(dev, initial);
> + if (rc) {
> dev_err(&dev->dev, "failure %d from pcibios_sriov_enable()\n",
> - retval);
> - return retval;
> + rc);
> + goto err_pcibios;
> }
>
> for (i = 0; i < initial; i++) {
> @@ -340,25 +345,23 @@ static int sriov_enable(struct pci_dev *dev, int
> nr_virtfn)
> return 0;
>
> failed:
> - for (j = 0; j < i; j++)
> - virtfn_remove(dev, j, 0);
> + while (i--)
> + virtfn_remove(dev, i, 0);
>
> + pcibios_sriov_disable(dev);
> +err_pcibios:
> iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
> pci_cfg_access_lock(dev);
> pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
> - pci_iov_set_numvfs(dev, 0);
> ssleep(1);
> pci_cfg_access_unlock(dev);
>
> if (iov->link != dev->devfn)
> sysfs_remove_link(&dev->dev.kobj, "dep_link");
>
> - return rc;
> -}
> + pci_iov_set_numvfs(dev, 0);
Do you have a spec pointer for the 1 sec delay before clearing NumVFs?
Does we need to clear NumVFs while holding the cfg access lock?
> -int __weak pcibios_sriov_disable(struct pci_dev *pdev)
> -{
> - return 0;
> + return rc;
> }
>
> static void sriov_disable(struct pci_dev *dev)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
