Re: [PATCH v2 5/6] cxl/mem: Rename cxl_dvsec_decode_init() to cxl_hdm_decode_init()

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: <linux-cxl@vger.kernel.org>, <ben.widawsky@intel.com>,
	<ira.weiny@intel.com>, <vishal.l.verma@intel.com>,
	<alison.schofield@intel.com>
Subject: Re: [PATCH v2 5/6] cxl/mem: Rename cxl_dvsec_decode_init() to cxl_hdm_decode_init()
Date: Fri, 25 Mar 2022 11:50:56 +0000	[thread overview]
Message-ID: <20220325115056.000058b2@huawei.com> (raw)
In-Reply-To: <164730736435.3806189.2537160791687837469.stgit@dwillia2-desk3.amr.corp.intel.com>

On Mon, 14 Mar 2022 18:22:44 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> cxl_dvsec_decode_init() is tasked with checking whether legacy DVSEC
> range based decode is in effect, or whether HDM can be enabled / already
> is enabled. As such it either succeeds or fails and that result is the
> return value. The @do_hdm_init variable is misleading in the case where
> HDM operation is already found to be active, so just call it @retval.

The text above is confusing for me.  Which part is justifying the
function rename and which part is for the retval?

Otherwise LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/mem.c            |   12 ++++++------
>  tools/testing/cxl/mock_mem.c |    2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 50704deb2ff0..3baae1332760 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -68,7 +68,7 @@ static int create_endpoint(struct cxl_memdev *cxlmd,
>  }
>  
>  /**
> - * cxl_dvsec_decode_init() - Setup HDM decoding for the endpoint
> + * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
>   * @cxlds: Device state
>   *
>   * Additionally, enables global HDM decoding. Warning: don't call this outside
> @@ -79,12 +79,12 @@ static int create_endpoint(struct cxl_memdev *cxlmd,
>   * decoders, or if it can not be determined if DVSEC Ranges are in use.
>   * Otherwise, returns true.
>   */
> -__mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
> +__mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
>  {
>  	struct cxl_endpoint_dvsec_info *info = &cxlds->info;
>  	struct cxl_register_map map;
>  	struct cxl_component_reg_map *cmap = &map.component_map;
> -	bool global_enable, do_hdm_init = false;
> +	bool global_enable, retval = false;
>  	void __iomem *crb;
>  	u32 global_ctrl;
>  
> @@ -113,7 +113,7 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
>  		goto out;
>  	}
>  
> -	do_hdm_init = true;
> +	retval = true;
>  
>  	/*
>  	 * Permanently (for this boot at least) opt the device into HDM
> @@ -129,7 +129,7 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
>  
>  out:
>  	iounmap(crb);
> -	return do_hdm_init;
> +	return retval;
>  }
>  
>  static int cxl_mem_probe(struct device *dev)
> @@ -160,7 +160,7 @@ static int cxl_mem_probe(struct device *dev)
>  	 * If DVSEC ranges are being used instead of HDM decoder registers there
>  	 * is no use in trying to manage those.
>  	 */
> -	if (!cxl_dvsec_decode_init(cxlds)) {
> +	if (!cxl_hdm_decode_init(cxlds)) {
>  		dev_err(dev,
>  			"Legacy range registers configuration prevents HDM operation.\n");
>  		return -EBUSY;
> diff --git a/tools/testing/cxl/mock_mem.c b/tools/testing/cxl/mock_mem.c
> index d1dec5845139..69946f678cfa 100644
> --- a/tools/testing/cxl/mock_mem.c
> +++ b/tools/testing/cxl/mock_mem.c
> @@ -4,7 +4,7 @@
>  #include <linux/types.h>
>  
>  struct cxl_dev_state;
> -bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
> +bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
>  {
>  	return true;
>  }
> 