Message-ID: <21e1306333d397858a906bb17bc3a6908b18be22.camel@intel.com>
Date: Thu, 19 Sep 2024 21:49:42 +0000
From: "Colberg, Peter" <peter.colberg@...el.com>
To: "yilun.xu@...ux.intel.com" <yilun.xu@...ux.intel.com>
CC: "Xu, Yilun" <yilun.xu@...el.com>, "linux-fpga@...r.kernel.org"
	<linux-fpga@...r.kernel.org>, "mdf@...nel.org" <mdf@...nel.org>, "Wu, Hao"
	<hao.wu@...el.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "russ.weight@...ux.dev"
	<russ.weight@...ux.dev>, "Pagani, Marco" <marpagan@...hat.com>,
	"trix@...hat.com" <trix@...hat.com>, "russell.h.weight@...el.com"
	<russell.h.weight@...el.com>, "matthew.gerlach@...ux.intel.com"
	<matthew.gerlach@...ux.intel.com>
Subject: Re: [RFC PATCH v2 9/9] fpga: dfl: fix kernel warning on port
 release/assign for SRIOV

On Fri, 2024-06-14 at 10:44 +0800, Xu Yilun wrote:
> On Wed, Jun 12, 2024 at 10:16:29PM +0000, Colberg, Peter wrote:
> > On Tue, 2024-04-23 at 23:36 +0800, Xu Yilun wrote:
> > > On Tue, Apr 09, 2024 at 07:39:42PM -0400, Peter Colberg wrote:
> > > > From: Xu Yilun <yilun.xu@...el.com>
> > > > 
> > > > DFL ports are registered as platform devices in PF mode. The port device
> > > > should be removed from the host when the user wants to configure the
> > > > port as a VF and pass through to a virtual machine. The FME device
> > > > ioctls DFL_FPGA_FME_PORT_RELEASE/ASSIGN are designed for this purpose.
> > > > 
> > > > In the previous implementation, the port platform device is not completely
> > > > destroyed on port release: it is removed from the system by
> > > > platform_device_del(), but the platform device instance is retained.
> > > > When the port assign ioctl is called, the platform device is added back by
> > > > platform_device_add(), which conflicts with this comment of device_add():
> > > > "Do not call this routine more than once for any device structure", and
> > > > will cause a kernel warning at runtime.
> > > > 
> > > > This patch tries to completely unregister the port platform device on
> > > > release and registers a new one on assign. But the main work is to remove
> > > > the dependency on struct dfl_feature_platform_data for many internal DFL
> > > > APIs. This structure holds many DFL enumeration infos for feature devices.
> > > > Many DFL APIs are expected to work with these info even when the port
> > > > platform device is unregistered. But with the change the platform_data will
> > > > be freed in this case. So this patch introduces a new structure
> > > > dfl_feature_dev_data for these APIs, which acts similarly to the previous
> > > > dfl_feature_platform_data. The dfl_feature_platform_data then only needs a
> > > > pointer to dfl_feature_dev_data to make the feature device driver work.
> > > > 
> > > > Signed-off-by: Xu Yilun <yilun.xu@...el.com>
> > > > Signed-off-by: Russ Weight <russell.h.weight@...el.com>
> > > > Signed-off-by: Peter Colberg <peter.colberg@...el.com>
> > > > ---
> > > > v2:
> > > > - Split monolithic patch into series at request of maintainer
> > > > - Substitute binfo->type for removed function feature_dev_id_type() in
> > > >   parse_feature_irqs().
> > > > - Return ERR_PTR(-ENOMEM) on !feature->params in
> > > >   binfo_create_feature_dev_data().
> > > > - Reorder cdev as first member of struct dfl_feature_platform_data
> > > >   such that container_of() to obtain pdata evaluates to a no-op.
> > > > - Align kernel-doc function name for __dfl_fpga_cdev_find_port_data().
> > > > ---
> > > >  drivers/fpga/dfl-afu-main.c |   9 +-
> > > >  drivers/fpga/dfl-fme-br.c   |  24 +-
> > > >  drivers/fpga/dfl-fme-main.c |   6 +-
> > > >  drivers/fpga/dfl.c          | 430 +++++++++++++++++-------------------
> > > >  drivers/fpga/dfl.h          |  86 +++++---
> > > >  5 files changed, 281 insertions(+), 274 deletions(-)
> > > > 
> > > > diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
> > > > index 42928cc7e42b..ead03b7aea70 100644
> > > > --- a/drivers/fpga/dfl-afu-main.c
> > > > +++ b/drivers/fpga/dfl-afu-main.c
> > > > @@ -143,9 +143,8 @@ static int port_reset(struct platform_device *pdev)
> > > >  	return ret;
> > > >  }
> > > >  
> > > > -static int port_get_id(struct platform_device *pdev)
> > > > +static int port_get_id(struct dfl_feature_dev_data *fdata)
> > > >  {
> > > > -	struct dfl_feature_dev_data *fdata = to_dfl_feature_dev_data(&pdev->dev);
> > > >  	void __iomem *base;
> > > >  
> > > >  	base = dfl_get_feature_ioaddr_by_id(fdata, PORT_FEATURE_ID_HEADER);
> > > > @@ -156,7 +155,8 @@ static int port_get_id(struct platform_device *pdev)
> > > >  static ssize_t
> > > >  id_show(struct device *dev, struct device_attribute *attr, char *buf)
> > > >  {
> > > > -	int id = port_get_id(to_platform_device(dev));
> > > > +	struct dfl_feature_dev_data *fdata = to_dfl_feature_dev_data(dev);
> > > > +	int id = port_get_id(fdata);
> > > 
> > 
> > Thank you for the comprehensive review.
> > 
> > > My quick idea is we go with these steps:
> > > 1. refactor struct dfl_feature_platform_data then replace all dev/pdev
> > >    arguments with pdata when necessary.
> > 
> > Could you outline how far the refactoring should go? The main changes
> > are introduced with the destruction of the platform device on port
> 
> Yes, exactly. And the goal is to make the change in a standalone patch
> so that everyone can find it, rather than bury in other massive
> replacements.
> 
> > release. If the refactoring retains the platform device but adds all
> > the new members to pdata, I find that this patch would introduce non-
> > trivial intermediate code that is then deleted in a subsequent patch.
> 
> That would not be a problem, as long as they clearly get explained, and
> in one patch series. Sometimes we need intermediate code to ensure a
> patch for one change, which makes people easy to read.
> 
> > 
> > > 2. factor out fdata from pdata, add fdata helpers.
> > > 3. massive pdata->fdata replacement.
> > > 4. delete all unused pdata helpers.
> > 
> > The (roughly) reverse order seems to produce the smallest patch set:
> 
> I don't think 'smallest' is the major concern, but it's fine if you
> firstly addressed other concerns. I cannot actually tell if it is
> better until the code is seen. But to emphasize on, the core change is
> splited out, the massive replacement patches are just replacements so
> they can be easily overviewed and skipped.

Thank you for the guidance. I have restructured the series to move the
massive replacements into two patches, "fpga: dfl: pass feature
platform data instead of device as argument" and "fpga: dfl: refactor
functions to take/return feature device data". The other patches
address auxiliary, functional and non-functional concerns, such that
the final patch "fpga: dfl: fix kernel warning on port release/assign
for SRIOV" is now as small as feasible and focuses on the core change.

Thanks,
Peter

> 
> Thanks,
> Yilun 
> 
> > 
> > 1. Replace function argument `struct device *dev` with `struct
> > dfl_feature_platform_data *pdata` as needed.
> > 2. #define dfl_feature_dev_data dfl_feature_platform_data and massive
> > pdata -> fdata replacement.
> > 3. Remove #define dfl_feature_dev_data, factor out dfl_feature_dev_data
> > from dfl_feature_platform_data, and destroy platform device on release.
> > 
> > Thanks,
> > Peter

Powered by blists - more mailing lists