Message-ID: <97093c23ae13a1fdcc52f85e5658a39e8a5ce716.camel@intel.com>
Date: Wed, 12 Jun 2024 22:16:29 +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 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
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.

> 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:

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