From mboxrd@z Thu Jan  1 00:00:00 1970
From: Alexander Duyck <alexander.duyck@gmail.com>
Subject: Re: [pci PATCH v6 1/5] pci: Add pci_sriov_configure_simple for PFs
 that don't manage VF resources
Date: Wed, 14 Mar 2018 08:28:18 -0700
Message-ID: <CAKgT0UdmU7Rc+eOz2FrtmSET2MqAnErGPQza2fmDChmU6__8OQ@mail.gmail.com>
References: <20180313212508.3553.65326.stgit@localhost.localdomain>
 <20180313212754.3553.72176.stgit@localhost.localdomain> <20180314085421.GA28796@lst.de>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Cc: Bjorn Helgaas <bhelgaas@google.com>,
        "Duyck, Alexander H" <alexander.h.duyck@intel.com>,
        linux-pci@vger.kernel.org, virtio-dev@lists.oasis-open.org,
        kvm@vger.kernel.org, Netdev <netdev@vger.kernel.org>,
        "Daly, Dan" <dan.daly@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-nvme@lists.infradead.org,
        Keith Busch <keith.busch@intel.com>, netanel@amazon.com,
        Don Dutile <ddutile@redhat.com>,
        Maximilian Heyne <mheyne@amazon.de>,
        "Wang, Liang-min" <liang-min.wang@intel.com>,
        "Rustad, Mark D" <mark.d.rustad@intel.com>,
        David Woodhouse <dwmw2@infradead.org>, dwmw@amazon.co.uk
To: Christoph Hellwig <hch@lst.de>
Return-path: <linux-kernel-owner@vger.kernel.org>
In-Reply-To: <20180314085421.GA28796@lst.de>
Sender: linux-kernel-owner@vger.kernel.org
List-Id: netdev.vger.kernel.org

On Wed, Mar 14, 2018 at 1:54 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Tue, Mar 13, 2018 at 02:28:49PM -0700, Alexander Duyck wrote:
>> 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
>> 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 <hch@lst.de>

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