Re: [RFC PATCH v2 2/9] fpga: dfl: migrate AFU DMA region management driver to dfl_feature_dev_data

["Colberg, Peter" <peter.colberg@intel.com>]

On Tue, 2024-04-23 at 17:01 +0800, Xu Yilun wrote:
> On Tue, Apr 09, 2024 at 07:39:35PM -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-dma-region.c. 
> 
> > This is done
> > to split a single monolithic change into multiple, smaller patches at the
> > request of the maintainer.
> 
> This sentence provides no useful info.

I have removed this sentence and described the restructuring into
logical, self-contained patches in the changelog of the v3 series.

> 
> > 
> > Signed-off-by: Peter Colberg <peter.colberg@intel.com>
> > ---
> > v2:
> > - Split monolithic patch into series at request of maintainer
> > - Reorder local variables in afu_dma_unpin_pages() to reverse Christmas
> >   tree order.
> > ---
> >  drivers/fpga/dfl-afu-dma-region.c | 119 +++++++++++++++---------------
> >  1 file changed, 61 insertions(+), 58 deletions(-)
> > 
> > diff --git a/drivers/fpga/dfl-afu-dma-region.c b/drivers/fpga/dfl-afu-dma-region.c
> > index 02b60fde0430..fb45e51b12af 100644
> > --- a/drivers/fpga/dfl-afu-dma-region.c
> > +++ b/drivers/fpga/dfl-afu-dma-region.c
> > @@ -16,26 +16,26 @@
> >  
> >  #include "dfl-afu.h"
> >  
> > -void afu_dma_region_init(struct dfl_feature_platform_data *pdata)
> > +void afu_dma_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);
> >  
> >  	afu->dma_regions = RB_ROOT;
> >  }
> >  
> >  /**
> >   * afu_dma_pin_pages - pin pages of given dma memory region
> > - * @pdata: feature device platform data
> > + * @fdata: feature dev data
> >   * @region: dma memory region to be pinned
> >   *
> >   * Pin all the pages of given dfl_afu_dma_region.
> >   * Return 0 for success or negative error code.
> >   */
> > -static int afu_dma_pin_pages(struct dfl_feature_platform_data *pdata,
> > +static int afu_dma_pin_pages(struct dfl_feature_dev_data *fdata,
> >  			     struct dfl_afu_dma_region *region)
> >  {
> >  	int npages = region->length >> PAGE_SHIFT;
> > -	struct device *dev = &pdata->dev->dev;
> > +	struct device *dev = &fdata->dev->dev;
> >  	int ret, pinned;
> >  
> >  	ret = account_locked_vm(current->mm, npages, true);
> > @@ -73,17 +73,17 @@ static int afu_dma_pin_pages(struct dfl_feature_platform_data *pdata,
> >  
> >  /**
> >   * afu_dma_unpin_pages - unpin pages of given dma memory region
> > - * @pdata: feature device platform data
> > + * @fdata: feature dev data
> >   * @region: dma memory region to be unpinned
> >   *
> >   * Unpin all the pages of given dfl_afu_dma_region.
> >   * Return 0 for success or negative error code.
> >   */
> > -static void afu_dma_unpin_pages(struct dfl_feature_platform_data *pdata,
> > +static void afu_dma_unpin_pages(struct dfl_feature_dev_data *fdata,
> >  				struct dfl_afu_dma_region *region)
> >  {
> >  	long npages = region->length >> PAGE_SHIFT;
> > -	struct device *dev = &pdata->dev->dev;
> > +	struct device *dev = &fdata->dev->dev;
> >  
> >  	unpin_user_pages(region->pages, npages);
> >  	kfree(region->pages);
> > @@ -133,20 +133,21 @@ static bool dma_region_check_iova(struct dfl_afu_dma_region *region,
> >  
> >  /**
> >   * afu_dma_region_add - add given dma region to rbtree
> > - * @pdata: feature device platform data
> > + * @fdata: feature dev data
> >   * @region: dma region to be added
> >   *
> >   * Return 0 for success, -EEXIST if dma region has already been added.
> >   *
> > - * Needs to be called with pdata->lock heold.
> > + * Needs to be called with fdata->lock held.
> >   */
> > -static int afu_dma_region_add(struct dfl_feature_platform_data *pdata,
> > +static int afu_dma_region_add(struct dfl_feature_dev_data *fdata,
> >  			      struct dfl_afu_dma_region *region)
> >  {
> > -	struct dfl_afu *afu = dfl_fpga_pdata_get_private(pdata);
> > +	struct dfl_afu *afu = dfl_fpga_fdata_get_private(fdata);
> > +	struct device *dev = &fdata->dev->dev;
> 
> Don't introduce any other unnecessary changes in the big
> symbol replacement patch.  People could read over all the same
> replacements quickly, even if they are massive. But if there are
> some other changes in between...

I have moved the addition of local `dev` pointers into a separate patch
"fpga: dfl: afu: define local pointer to feature device".

> 
> >  	struct rb_node **new, *parent = NULL;
> >  
> > -	dev_dbg(&pdata->dev->dev, "add region (iova = %llx)\n",
> > +	dev_dbg(dev, "add region (iova = %llx)\n",
> >  		(unsigned long long)region->iova);

Thanks,
Peter