| Message ID | 20240409233942.828440-4-peter.colberg@intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | fpga: dfl: fix kernel warning on port release/assign for SRIOV | expand |
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? [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; > > } >
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; }
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(-)