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 <peter.colberg@intel.com>
---
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);
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;
}
--
2.44.0On 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 <peter.colberg@intel.com>
> ---
> 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?
[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;
> }
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 <peter.colberg@intel.com>
> > ---
> > 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;
> > }
>
On Tue, 2024-04-23 at 22:22 +0800, Xu Yilun wrote:
> 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 <peter.colberg@intel.com>
> > > ---
> > > 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.
devm_kzalloc() as used currently is appropriate since the allocated
object should have the same lifetime as the device.
Thanks,
Peter
>
> 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;
> > > }
> >
© 2016 - 2026 Red Hat, Inc.