Re: [pci PATCH v6 1/5] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources

2018-03-14 Thread Alexander Duyck 
On Wed, Mar 14, 2018 at 1:54 AM, Christoph Hellwig  wrote:
> On Tue, Mar 13, 2018 at 02:28:49PM -0700, Alexander Duyck wrote:
>> From: Alexander Duyck 
>>
>> This patch adds a common configuration function called
>> pci_sriov_configure_simple that will allow for managing VFs on devices
>> where the PF is not capable of managing VF resources.
>>
>> Signed-off-by: Alexander Duyck 
>> ---
>>
>> v5: New patch replacing pci_sriov_configure_unmanaged with
>>   pci_sriov_configure_simple
>> Dropped bits related to autoprobe changes
>> v6: Defined pci_sriov_configure_simple as NULL if IOV is disabled
>>
>>  drivers/pci/iov.c   |   32 
>>  include/linux/pci.h |3 +++
>>  2 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index 677924ae0350..bd7021491fdb 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -807,3 +807,35 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
>>   return dev->sriov->total_VFs;
>>  }
>>  EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
>> +
>> +/**
>> + * pci_sriov_configure_simple - helper to configure unmanaged SR-IOV
>> + * @dev: the PCI device
>> + * @nr_virtfn: number of virtual functions to enable, 0 to disable
>> + *
>> + * Used to provide generic enable/disable SR-IOV option for devices
>> + * that do not manage the VFs generated by their driver
>> + */
>> +int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn)
>> +{
>> + int err = -EINVAL;
>
> This assignment seems like it is never used..

It applies in the case where we are setting a number of VFs when VFs
are already allocated. It was needed since the last statement ended in
an if.

>> +
>> + might_sleep();
>> +
>> + if (!dev->is_physfn)
>> + return -ENODEV;
>> +
>> + if (pci_vfs_assigned(dev)) {
>> + pci_warn(dev,
>> +  "Cannot modify SR-IOV while VFs are assigned\n");
>> + err = -EPERM;
>
> Why not:
>
> if (pci_vfs_assigned(dev)) {
> pci_warn(dev,
>  "Cannot modify SR-IOV while VFs are assigned\n");
> return -EPERM;
> }

Symmetry. I had this as one of 3 blocks and just did it this way to be
consistent with the next two statements. I could probably just return
from each if block if that is preferred.

> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig 

For v7 I can probably drop the err parameter and just return the
necessary error values directly.

Re: [pci PATCH v6 1/5] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources

2018-03-14 Thread Christoph Hellwig 
On Tue, Mar 13, 2018 at 02:28:49PM -0700, Alexander Duyck wrote:
> From: Alexander Duyck 
> 
> This patch adds a common configuration function called
> pci_sriov_configure_simple that will allow for managing VFs on devices
> where the PF is not capable of managing VF resources.
> 
> Signed-off-by: Alexander Duyck 
> ---
> 
> v5: New patch replacing pci_sriov_configure_unmanaged with
>   pci_sriov_configure_simple
> Dropped bits related to autoprobe changes
> v6: Defined pci_sriov_configure_simple as NULL if IOV is disabled
> 
>  drivers/pci/iov.c   |   32 
>  include/linux/pci.h |3 +++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 677924ae0350..bd7021491fdb 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -807,3 +807,35 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
>   return dev->sriov->total_VFs;
>  }
>  EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
> +
> +/**
> + * pci_sriov_configure_simple - helper to configure unmanaged SR-IOV
> + * @dev: the PCI device
> + * @nr_virtfn: number of virtual functions to enable, 0 to disable
> + *
> + * Used to provide generic enable/disable SR-IOV option for devices
> + * that do not manage the VFs generated by their driver
> + */
> +int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn)
> +{
> + int err = -EINVAL;

This assignment seems like it is never used..

> +
> + might_sleep();
> +
> + if (!dev->is_physfn)
> + return -ENODEV;
> +
> + if (pci_vfs_assigned(dev)) {
> + pci_warn(dev,
> +  "Cannot modify SR-IOV while VFs are assigned\n");
> + err = -EPERM;

Why not:

if (pci_vfs_assigned(dev)) {
pci_warn(dev,
 "Cannot modify SR-IOV while VFs are assigned\n");
return -EPERM;
}

Otherwise looks good:

Reviewed-by: Christoph Hellwig

[pci PATCH v6 1/5] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources

2018-03-13 Thread Alexander Duyck 
From: Alexander Duyck 

This patch adds a common configuration function called
pci_sriov_configure_simple that will allow for managing VFs on devices
where the PF is not capable of managing VF resources.

Signed-off-by: Alexander Duyck 
---

v5: New patch replacing pci_sriov_configure_unmanaged with
  pci_sriov_configure_simple
Dropped bits related to autoprobe changes
v6: Defined pci_sriov_configure_simple as NULL if IOV is disabled

 drivers/pci/iov.c   |   32 
 include/linux/pci.h |3 +++
 2 files changed, 35 insertions(+)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 677924ae0350..bd7021491fdb 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -807,3 +807,35 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
return dev->sriov->total_VFs;
 }
 EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
+
+/**
+ * pci_sriov_configure_simple - helper to configure unmanaged SR-IOV
+ * @dev: the PCI device
+ * @nr_virtfn: number of virtual functions to enable, 0 to disable
+ *
+ * Used to provide generic enable/disable SR-IOV option for devices
+ * that do not manage the VFs generated by their driver
+ */
+int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn)
+{
+   int err = -EINVAL;
+
+   might_sleep();
+
+   if (!dev->is_physfn)
+   return -ENODEV;
+
+   if (pci_vfs_assigned(dev)) {
+   pci_warn(dev,
+"Cannot modify SR-IOV while VFs are assigned\n");
+   err = -EPERM;
+   } else if (!nr_virtfn) {
+   sriov_disable(dev);
+   err = 0;
+   } else if (!dev->sriov->num_VFs) {
+   err = sriov_enable(dev, nr_virtfn);
+   }
+
+   return err ? err : nr_virtfn;
+}
+EXPORT_SYMBOL_GPL(pci_sriov_configure_simple);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 024a1beda008..f3099e940cda 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1953,6 +1953,7 @@ static inline void pci_mmcfg_late_init(void) { }
 int pci_vfs_assigned(struct pci_dev *dev);
 int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
 int pci_sriov_get_totalvfs(struct pci_dev *dev);
+int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
 resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
 void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
 #else
@@ -1980,6 +1981,8 @@ static inline int pci_sriov_set_totalvfs(struct pci_dev 
*dev, u16 numvfs)
 { return 0; }
 static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
 { return 0; }
+/* since this expected to be used as a function pointer just define as NULL */
+#define pci_sriov_configure_simple NULL
 static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int 
resno)
 { return 0; }
 static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { 
}