Received-SPF: pass (google.com: domain of linux-kernel+bounces-155370-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 23 Apr 2024 22:22:59 +0800
From: Xu Yilun <yilun.xu@linux.intel.com>
To: "Colberg, Peter" <peter.colberg@intel.com>
Cc: "Wu, Hao" <hao.wu@intel.com>, "mdf@kernel.org" <mdf@kernel.org>,
	"linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>,
	"Xu, Yilun" <yilun.xu@intel.com>, "Rix, Tom" <trix@redhat.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"russ.weight@linux.dev" <russ.weight@linux.dev>,
	"Pagani, Marco" <marpagan@redhat.com>,
	"matthew.gerlach@linux.intel.com" <matthew.gerlach@linux.intel.com>
Subject: Re: [RFC PATCH v2 3/9] fpga: dfl: migrate AFU MMIO region management
 driver to dfl_feature_dev_data
Message-ID: <ZifEQ42hMPazjiWd@yilunxu-OptiPlex-7050>
References: <20240409233942.828440-1-peter.colberg@intel.com>
 <20240409233942.828440-4-peter.colberg@intel.com>
 <1aa50550adfb3917e94dd4f553ee88005133cd15.camel@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1aa50550adfb3917e94dd4f553ee88005133cd15.camel@intel.com>

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