Received-SPF: pass (google.com: domain of linux-kernel+bounces-154758-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 23 Apr 2024 17:01:48 +0800
From: Xu Yilun <yilun.xu@linux.intel.com>
To: Peter Colberg <peter.colberg@intel.com>
Cc: Wu Hao <hao.wu@intel.com>, Tom Rix <trix@redhat.com>,
	Moritz Fischer <mdf@kernel.org>, Xu Yilun <yilun.xu@intel.com>,
	linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org,
	Russ Weight <russ.weight@linux.dev>,
	Marco Pagani <marpagan@redhat.com>,
	Matthew Gerlach <matthew.gerlach@linux.intel.com>
Subject: Re: [RFC PATCH v2 2/9] fpga: dfl: migrate AFU DMA region management
 driver to dfl_feature_dev_data
Message-ID: <Zid4/GH1hL5YRboH@yilunxu-OptiPlex-7050>
References: <20240409233942.828440-1-peter.colberg@intel.com>
 <20240409233942.828440-3-peter.colberg@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240409233942.828440-3-peter.colberg@intel.com>

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.

> 
> 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...

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