by Xu Yilun

[permalink] [raw]

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

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);

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.
2. factor out fdata from pdata, add fdata helpers.
3. massive pdata->fdata replacement.
4. delete all unused pdata helpers.

Please check if it is possible.

Thanks,
Yilun

2024-04-23 17:08:58

by Xu Yilun

[permalink] [raw]

Subject: Re: [RFC PATCH v2 3/9] fpga: dfl: migrate AFU MMIO region management driver to dfl_feature_dev_data

On Tue, Apr 09, 2024 at 11:56:19PM +0000, Colberg, Peter wrote:
> On Tue, 2024-04-09 at 19:39 -0400, Peter Colberg wrote:
> > This change separates out most of the symbol name changes required by this
> > patch series for the file: drivers/fpga/dfl-afu-region.c. This is done to
> > split a single monolithic change into multiple, smaller patches at the
> > request of the maintainer.
> >
> > Signed-off-by: Peter Colberg <[email protected]>
> > ---
> > v2:
> > - Split monolithic patch into series at request of maintainer
> > ---
> > drivers/fpga/dfl-afu-region.c | 51 ++++++++++++++++++-----------------
> > 1 file changed, 26 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl-afu-region.c b/drivers/fpga/dfl-afu-region.c
> > index 2e7b41629406..b11a5b21e666 100644
> > --- a/drivers/fpga/dfl-afu-region.c
> > +++ b/drivers/fpga/dfl-afu-region.c
> > @@ -12,11 +12,11 @@
> >
> > /**
> > * afu_mmio_region_init - init function for afu mmio region support
> > - * @pdata: afu platform device's pdata.
> > + * @fdata: afu feature dev data
> > */
> > -void afu_mmio_region_init(struct dfl_feature_platform_data *pdata)
> > +void afu_mmio_region_init(struct dfl_feature_dev_data *fdata)
> > {
> > - struct dfl_afu *afu = dfl_fpga_pdata_get_private(pdata);
> > + struct dfl_afu *afu = dfl_fpga_fdata_get_private(fdata);
> >
> > INIT_LIST_HEAD(&afu->regions);
> > }
> > @@ -39,7 +39,7 @@ static struct dfl_afu_mmio_region *get_region_by_index(struct dfl_afu *afu,
> > /**
> > * afu_mmio_region_add - add a mmio region to given feature dev.
> > *
> > - * @pdata: afu platform device's pdata.
> > + * @fdata: afu feature dev data
> > * @region_index: region index.
> > * @region_size: region size.
> > * @phys: region's physical address of this region.
> > @@ -47,14 +47,15 @@ static struct dfl_afu_mmio_region *get_region_by_index(struct dfl_afu *afu,
> > *
> > * Return: 0 on success, negative error code otherwise.
> > */
> > -int afu_mmio_region_add(struct dfl_feature_platform_data *pdata,
> > +int afu_mmio_region_add(struct dfl_feature_dev_data *fdata,
> > u32 region_index, u64 region_size, u64 phys, u32 flags)
> > {
> > + struct device *dev = &fdata->dev->dev;
> > struct dfl_afu_mmio_region *region;
> > struct dfl_afu *afu;
> > int ret = 0;
> >
> > - region = devm_kzalloc(&pdata->dev->dev, sizeof(*region), GFP_KERNEL);
> > + region = devm_kzalloc(dev, sizeof(*region), GFP_KERNEL);
> > if (!region)
> > return -ENOMEM;
> >
> > @@ -63,13 +64,13 @@ int afu_mmio_region_add(struct dfl_feature_platform_data *pdata,
> > region->phys = phys;
> > region->flags = flags;
> >
> > - mutex_lock(&pdata->lock);
> > + mutex_lock(&fdata->lock);
> >
> > - afu = dfl_fpga_pdata_get_private(pdata);
> > + afu = dfl_fpga_fdata_get_private(fdata);
> >
> > /* check if @index already exists */
> > if (get_region_by_index(afu, region_index)) {
> > - mutex_unlock(&pdata->lock);
> > + mutex_unlock(&fdata->lock);
> > ret = -EEXIST;
> > goto exit;
> > }
> > @@ -80,37 +81,37 @@ int afu_mmio_region_add(struct dfl_feature_platform_data *pdata,
> >
> > afu->region_cur_offset += region_size;
> > afu->num_regions++;
> > - mutex_unlock(&pdata->lock);
> > + mutex_unlock(&fdata->lock);
> >
> > return 0;
> >
> > exit:
> > - devm_kfree(&pdata->dev->dev, region);
> > + devm_kfree(dev, region);
>
> An internal reviewer commented that calling devm_kfree() in almost all
> cases shows a misunderstanding of object lifetime and may unveil bugs.
> They suggested to either drop the explicit devm_kfree(), or move from
> devm_*() to plain allocation.
>
> I could not find specific documentation on the recommended use cases
> for devm_kfree() to immediately free a resource on error, but the
> description of devres groups advises that explicit freeing using
> devres_release_group() is usually useful in midlayer drivers where
> interface functions should not have side effects [1].
>
> Which implementation would you prefer and why? Dropping devm_kfree(),
> moving to plain allocation, or leaving everything as is?

Using devm_*() usually means the lifecycle of the allocated object
should be the same as the device. Otherwise use the plain allocation.
Please check which case fits for you.

Thanks,
Yilun

>
> [1] https://docs.kernel.org/driver-api/driver-model/devres.html#devres-group
>
> Thanks,
> Peter
>
> > return ret;
> > }
> >
> > /**
> > * afu_mmio_region_destroy - destroy all mmio regions under given feature dev.
> > - * @pdata: afu platform device's pdata.
> > + * @fdata: afu feature dev data
> > */
> > -void afu_mmio_region_destroy(struct dfl_feature_platform_data *pdata)
> > +void afu_mmio_region_destroy(struct dfl_feature_dev_data *fdata)
> > {
> > - struct dfl_afu *afu = dfl_fpga_pdata_get_private(pdata);
> > + struct dfl_afu *afu = dfl_fpga_fdata_get_private(fdata);
> > struct dfl_afu_mmio_region *tmp, *region;
> >
> > list_for_each_entry_safe(region, tmp, &afu->regions, node)
> > - devm_kfree(&pdata->dev->dev, region);
> > + devm_kfree(&fdata->dev->dev, region);
> > }
> >
> > /**
> > * afu_mmio_region_get_by_index - find an afu region by index.
> > - * @pdata: afu platform device's pdata.
> > + * @fdata: afu feature dev data
> > * @region_index: region index.
> > * @pregion: ptr to region for result.
> > *
> > * Return: 0 on success, negative error code otherwise.
> > */
> > -int afu_mmio_region_get_by_index(struct dfl_feature_platform_data *pdata,
> > +int afu_mmio_region_get_by_index(struct dfl_feature_dev_data *fdata,
> > u32 region_index,
> > struct dfl_afu_mmio_region *pregion)
> > {
> > @@ -118,8 +119,8 @@ int afu_mmio_region_get_by_index(struct dfl_feature_platform_data *pdata,
> > struct dfl_afu *afu;
> > int ret = 0;
> >
> > - mutex_lock(&pdata->lock);
> > - afu = dfl_fpga_pdata_get_private(pdata);
> > + mutex_lock(&fdata->lock);
> > + afu = dfl_fpga_fdata_get_private(fdata);
> > region = get_region_by_index(afu, region_index);
> > if (!region) {
> > ret = -EINVAL;
> > @@ -127,14 +128,14 @@ int afu_mmio_region_get_by_index(struct dfl_feature_platform_data *pdata,
> > }
> > *pregion = *region;
> > exit:
> > - mutex_unlock(&pdata->lock);
> > + mutex_unlock(&fdata->lock);
> > return ret;
> > }
> >
> > /**
> > * afu_mmio_region_get_by_offset - find an afu mmio region by offset and size
> > *
> > - * @pdata: afu platform device's pdata.
> > + * @fdata: afu feature dev data
> > * @offset: region offset from start of the device fd.
> > * @size: region size.
> > * @pregion: ptr to region for result.
> > @@ -144,7 +145,7 @@ int afu_mmio_region_get_by_index(struct dfl_feature_platform_data *pdata,
> > *
> > * Return: 0 on success, negative error code otherwise.
> > */
> > -int afu_mmio_region_get_by_offset(struct dfl_feature_platform_data *pdata,
> > +int afu_mmio_region_get_by_offset(struct dfl_feature_dev_data *fdata,
> > u64 offset, u64 size,
> > struct dfl_afu_mmio_region *pregion)
> > {
> > @@ -152,8 +153,8 @@ int afu_mmio_region_get_by_offset(struct dfl_feature_platform_data *pdata,
> > struct dfl_afu *afu;
> > int ret = 0;
> >
> > - mutex_lock(&pdata->lock);
> > - afu = dfl_fpga_pdata_get_private(pdata);
> > + mutex_lock(&fdata->lock);
> > + afu = dfl_fpga_fdata_get_private(fdata);
> > for_each_region(region, afu)
> > if (region->offset <= offset &&
> > region->offset + region->size >= offset + size) {
> > @@ -162,6 +163,6 @@ int afu_mmio_region_get_by_offset(struct dfl_feature_platform_data *pdata,
> > }
> > ret = -EINVAL;
> > exit:
> > - mutex_unlock(&pdata->lock);
> > + mutex_unlock(&fdata->lock);
> > return ret;
> > }
>