by Colberg, Peter

[permalink] [raw]

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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Russ Weight <[email protected]>
> > Signed-off-by: Peter Colberg <[email protected]>
> > ---
> > 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

2024-06-14 02:52:36

by Xu Yilun

[permalink] [raw]

Subject: Re: [RFC PATCH v2 9/9] fpga: dfl: fix kernel warning on port release/assign for SRIOV

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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Russ Weight <[email protected]>
> > > Signed-off-by: Peter Colberg <[email protected]>
> > > ---
> > > 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.

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