Re: [PATCH v3 03/12] fpga: dfl: pci: enable SRIOV support.

[Wu Hao <hao.wu@intel.com>]

On Wed, Jul 24, 2019 at 11:37:44AM +0200, Greg KH wrote:
> On Tue, Jul 23, 2019 at 12:51:26PM +0800, Wu Hao wrote:
> > This patch enables the standard sriov support. It allows user to
> > enable SRIOV (and VFs), then user could pass through accelerators
> > (VFs) into virtual machine or use VFs directly in host.
> > 
> > Signed-off-by: Zhang Yi Z <yi.z.zhang@intel.com>
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > Acked-by: Alan Tull <atull@kernel.org>
> > Acked-by: Moritz Fischer <mdf@kernel.org>
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> > v2: remove DRV/MODULE_VERSION modifications.
> > ---
> >  drivers/fpga/dfl-pci.c | 39 +++++++++++++++++++++++++++++++++++++++
> >  drivers/fpga/dfl.c     | 41 +++++++++++++++++++++++++++++++++++++++++
> >  drivers/fpga/dfl.h     |  1 +
> >  3 files changed, 81 insertions(+)
> > 
> > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> > index 66b5720..0e65d81 100644
> > --- a/drivers/fpga/dfl-pci.c
> > +++ b/drivers/fpga/dfl-pci.c
> > @@ -223,8 +223,46 @@ int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
> >  	return ret;
> >  }
> >  
> > +static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs)
> > +{
> > +	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> > +	struct dfl_fpga_cdev *cdev = drvdata->cdev;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&cdev->lock);
> > +
> > +	if (!num_vfs) {
> > +		/*
> > +		 * disable SRIOV and then put released ports back to default
> > +		 * PF access mode.
> > +		 */
> > +		pci_disable_sriov(pcidev);
> > +
> > +		__dfl_fpga_cdev_config_port_vf(cdev, false);
> > +
> > +	} else if (cdev->released_port_num == num_vfs) {
> > +		/*
> > +		 * only enable SRIOV if cdev has matched released ports, put
> > +		 * released ports into VF access mode firstly.
> > +		 */
> > +		__dfl_fpga_cdev_config_port_vf(cdev, true);
> > +
> > +		ret = pci_enable_sriov(pcidev, num_vfs);
> > +		if (ret)
> > +			__dfl_fpga_cdev_config_port_vf(cdev, false);
> > +	} else {
> > +		ret = -EINVAL;
> > +	}
> > +
> > +	mutex_unlock(&cdev->lock);
> > +	return ret;
> > +}
> > +
> >  static void cci_pci_remove(struct pci_dev *pcidev)
> >  {
> > +	if (dev_is_pf(&pcidev->dev))
> > +		cci_pci_sriov_configure(pcidev, 0);
> > +
> >  	cci_remove_feature_devs(pcidev);
> >  	pci_disable_pcie_error_reporting(pcidev);
> >  }
> > @@ -234,6 +272,7 @@ static void cci_pci_remove(struct pci_dev *pcidev)
> >  	.id_table = cci_pcie_id_tbl,
> >  	.probe = cci_pci_probe,
> >  	.remove = cci_pci_remove,
> > +	.sriov_configure = cci_pci_sriov_configure,
> >  };
> >  
> >  module_pci_driver(cci_pci_driver);
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index e04ed45..c3a8e1d 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -1112,6 +1112,47 @@ int dfl_fpga_cdev_config_port(struct dfl_fpga_cdev *cdev, int port_id,
> >  }
> >  EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_port);
> >  
> > +static void config_port_vf(struct device *fme_dev, int port_id, bool is_vf)
> > +{
> > +	void __iomem *base;
> > +	u64 v;
> > +
> > +	base = dfl_get_feature_ioaddr_by_id(fme_dev, FME_FEATURE_ID_HEADER);
> > +
> > +	v = readq(base + FME_HDR_PORT_OFST(port_id));
> > +
> > +	v &= ~FME_PORT_OFST_ACC_CTRL;
> > +	v |= FIELD_PREP(FME_PORT_OFST_ACC_CTRL,
> > +			is_vf ? FME_PORT_OFST_ACC_VF : FME_PORT_OFST_ACC_PF);
> > +
> > +	writeq(v, base + FME_HDR_PORT_OFST(port_id));
> > +}
> > +
> > +/**
> > + * __dfl_fpga_cdev_config_port_vf - configure port to VF access mode
> > + *
> > + * @cdev: parent container device.
> > + * @if_vf: true for VF access mode, and false for PF access mode
> > + *
> > + * Return: 0 on success, negative error code otherwise.
> > + *
> > + * This function is needed in sriov configuration routine. It could be used to
> > + * configures the released ports access mode to VF or PF.
> > + * The caller needs to hold lock for protection.
> > + */
> > +void __dfl_fpga_cdev_config_port_vf(struct dfl_fpga_cdev *cdev, bool is_vf)
> > +{
> > +	struct dfl_feature_platform_data *pdata;
> > +
> > +	list_for_each_entry(pdata, &cdev->port_dev_list, node) {
> > +		if (device_is_registered(&pdata->dev->dev))
> > +			continue;
> > +
> > +		config_port_vf(cdev->fme_dev, pdata->id, is_vf);
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(__dfl_fpga_cdev_config_port_vf);
> 
> Why are you exporting a function with a leading __?
> 
> You are expecting someone else, in who knows what code, to do locking
> correctly?  If so, and the caller always has to have a local lock, then
> it's not a big deal, just drop the '__', otherwise if you have to have a
> specific lock for a specific device, then you have a really complex and
> probably broken api here :(

Yes, I just want to remind the user of this API, caller needs to hold the
lock to protect the list. I fully agree, it does make sense to make the
APIs easy to use. I will try to improve this, maybe move the lock inside
this function, then API user doesn't need to know the details of locking.

Thanks a lot for the comments, it really helps.

Hao

> 
> thanks,
> 
> greg k-h