diff mbox

[pci,v5,1/4] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources

Message ID 20180312172031.3487.20651.stgit@localhost.localdomain (mailing list archive)
State New, archived
Headers show

Commit Message

Alexander Duyck March 12, 2018, 5:21 p.m. UTC 
From: Alexander Duyck <alexander.h.duyck@intel.com>

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 <alexander.h.duyck@intel.com>
---

v5: New patch replacing pci_sriov_configure_unmanaged with
      pci_sriov_configure_simple
    Dropped bits related to autoprobe changes

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

Comments

Keith Busch March 12, 2018, 5:40 p.m. UTC | #1 
On Mon, Mar 12, 2018 at 10:21:29AM -0700, Alexander Duyck wrote:
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 024a1beda008..9cab9d0d51dc 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

I recommend stubbing 'pci_sriov_configure_simple' or defining it to
NULL in the '#else' section here so you don't need to repeat the "#ifdef
CONFIG_PCI_IOV" in each driver wishing to use this function. Otherwise
looks fine to me.
Alexander Duyck March 12, 2018, 6:09 p.m. UTC | #2 
On Mon, Mar 12, 2018 at 10:40 AM, Keith Busch <keith.busch@intel.com> wrote:
> On Mon, Mar 12, 2018 at 10:21:29AM -0700, Alexander Duyck wrote:
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 024a1beda008..9cab9d0d51dc 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
>
> I recommend stubbing 'pci_sriov_configure_simple' or defining it to
> NULL in the '#else' section here so you don't need to repeat the "#ifdef
> CONFIG_PCI_IOV" in each driver wishing to use this function. Otherwise
> looks fine to me.

My concern with defining it as NULL is that somebody may end up
calling it in the future directly and that may end up causing issues.
One thought I have been debating is moving it to a different file. I
am just not sure where the best place to put something like this would
be. I could move this function to drivers/pci/pci.c if everyone is
okay with it and then I could just strip the contents out by wrapping
them in a #ifdef instead.
Keith Busch March 12, 2018, 6:23 p.m. UTC | #3 
On Mon, Mar 12, 2018 at 11:09:34AM -0700, Alexander Duyck wrote:
> On Mon, Mar 12, 2018 at 10:40 AM, Keith Busch <keith.busch@intel.com> wrote:
> > On Mon, Mar 12, 2018 at 10:21:29AM -0700, Alexander Duyck wrote:
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index 024a1beda008..9cab9d0d51dc 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
> >
> > I recommend stubbing 'pci_sriov_configure_simple' or defining it to
> > NULL in the '#else' section here so you don't need to repeat the "#ifdef
> > CONFIG_PCI_IOV" in each driver wishing to use this function. Otherwise
> > looks fine to me.
> 
> My concern with defining it as NULL is that somebody may end up
> calling it in the future directly and that may end up causing issues.
> One thought I have been debating is moving it to a different file. I
> am just not sure where the best place to put something like this would
> be. I could move this function to drivers/pci/pci.c if everyone is
> okay with it and then I could just strip the contents out by wrapping
> them in a #ifdef instead.

Okay, instead of NULL, a stub implementation in the header file may
suffice when CONFIG_PCI_IOV is not defined:

static inline int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn)
{
	return -ENOSYS;
}

See pci_iov_virtfn_bus, pci_iov_virtfn_devfn, pci_iov_add_virtfn, or
pci_enable_sriov for other examples.
Alexander Duyck March 12, 2018, 8:17 p.m. UTC | #4 
On Mon, Mar 12, 2018 at 11:23 AM, Keith Busch <keith.busch@intel.com> wrote:
> On Mon, Mar 12, 2018 at 11:09:34AM -0700, Alexander Duyck wrote:
>> On Mon, Mar 12, 2018 at 10:40 AM, Keith Busch <keith.busch@intel.com> wrote:
>> > On Mon, Mar 12, 2018 at 10:21:29AM -0700, Alexander Duyck wrote:
>> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> >> index 024a1beda008..9cab9d0d51dc 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
>> >
>> > I recommend stubbing 'pci_sriov_configure_simple' or defining it to
>> > NULL in the '#else' section here so you don't need to repeat the "#ifdef
>> > CONFIG_PCI_IOV" in each driver wishing to use this function. Otherwise
>> > looks fine to me.
>>
>> My concern with defining it as NULL is that somebody may end up
>> calling it in the future directly and that may end up causing issues.
>> One thought I have been debating is moving it to a different file. I
>> am just not sure where the best place to put something like this would
>> be. I could move this function to drivers/pci/pci.c if everyone is
>> okay with it and then I could just strip the contents out by wrapping
>> them in a #ifdef instead.
>
> Okay, instead of NULL, a stub implementation in the header file may
> suffice when CONFIG_PCI_IOV is not defined:
>
> static inline int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn)
> {
>         return -ENOSYS;
> }
>
> See pci_iov_virtfn_bus, pci_iov_virtfn_devfn, pci_iov_add_virtfn, or
> pci_enable_sriov for other examples.

No, I am aware of those. The problem is they aren't accessed as
function pointers. As such converting them to static inline functions
is easy. As I am sure you are aware an "inline" function doesn't
normally generate a function pointer.

Actually my original idea has been complicated further by the fact
that I realized my code is accessing functions that are static in the
iov.c file. I'll need to think about how to come up with a better
solution for this.
Christoph Hellwig March 13, 2018, 7:44 a.m. UTC | #5 
On Mon, Mar 12, 2018 at 01:17:00PM -0700, Alexander Duyck wrote:
> No, I am aware of those. The problem is they aren't accessed as
> function pointers. As such converting them to static inline functions
> is easy. As I am sure you are aware an "inline" function doesn't
> normally generate a function pointer.

I think Keith's original idea of defining them to NULL is right.  That
takes care of all the current trivial assign to struct cases.

If someone wants to call these functions they'll still need the ifdef
around the call as those won't otherwise compile, but they probably
want the ifdef around the whole caller anyway.
diff mbox

Patch

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..9cab9d0d51dc 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