RE: [PATCH v1 4/7] fpga: dfl: fix VF creation when ports have no local BAR space

["Zhang, Tianfei" <tianfei.zhang@intel.com>]

> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Tuesday, February 15, 2022 11:51 PM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>; Wu, Hao <hao.wu@intel.com>;
> mdf@kernel.org; Xu, Yilun <yilun.xu@intel.com>; linux-fpga@vger.kernel.org;
> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: corbet@lwn.net; Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Subject: Re: [PATCH v1 4/7] fpga: dfl: fix VF creation when ports have no local
> BAR space
> 
> 
> On 2/14/22 3:26 AM, Tianfei zhang wrote:
> > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >
> > When a port is not connected to the same PCIe endpoint as the FME, the
> > port does not need to be released before being virtualized.  Fix VF
> > creation code to handle this new use
> Similar, how does this fit in with iofs, this looks like it would not be valid for the
> existing cards

IOFS introducing multiple methods for PR and AFU access.
1. Legacy Model.
2. Micro-Personas in AFU.
3. Multiple VFs per PR slot.

For 1 and 2 model, there are 1:1 mapping between Port device and PR slot (or entire AFU). In virtualization,
it should release the Port device firstly and then assign to VM. In this models, the DFL driver will track  that
the number of port devices has released (cdev->released_port_num in dfl_fpga_cdev_config_ports_vf() function)
are equal with the numbers of SRIOV or not. But in model 3, it has multiple VFs per PR slot, and assign the VF to VM 
without release the port device, so the tracking mechanism of cdev->released_port_num is not workable on new
model. This patch want to handle this new model during VF creation.

> > case.
> >
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > ---
> >   drivers/fpga/dfl.c | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index
> > 26f8cf890700..cfc539a656f0 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -1705,15 +1705,22 @@
> EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_ports_pf);
> >   int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vfs)
> >   {
> >   	struct dfl_feature_platform_data *pdata;
> > -	int ret = 0;
> > +	int ret = 0, port_count = 0;
> >
> >   	mutex_lock(&cdev->lock);
> > +
> > +	list_for_each_entry(pdata, &cdev->port_dev_list, node) {
> > +		if (pdata->dev)
> 
> This looks wrong,
> 
> pdata->dev is dereferenced below, if there is a case were (!pdata->dev)
> here there would be crash later.
> 
> > +			continue;
> > +		port_count++;
> 
> how does this work when only some of the ports are handled in the new way ?

This code want to handle the " Multiple VFs per PR slot" model as I mentioned above.
In new model, the port_count want to count that how many port devices have released.
This code looks not good readability, I try to re-write it.

> 
> Tom
> 
> > +	}
> > +
> >   	/*
> >   	 * can't turn multiple ports into 1 VF device, only 1 port for 1 VF
> >   	 * device, so if released port number doesn't match VF device number,
> >   	 * then reject the request with -EINVAL error code.
> >   	 */
> > -	if (cdev->released_port_num != num_vfs) {
> > +	if (port_count && cdev->released_port_num != num_vfs) {
> >   		ret = -EINVAL;
> >   		goto done;
> >   	}