[PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws()

[PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws()

[Dan Williams]

As a follow on to the recent rework of __cxl_parse_cfmws() to always
return errors [1], use cleanup.h helpers to remove goto and other cleanups
now that logging is moved to the cxl_parse_cfmws() wrapper.

This ends up adding more code than it deletes, but __cxl_parse_cfmws()
itself does get smaller. The takeaway from the cond_no_free_ptr()
discussion [2] was to not add new macros to handle the cases where
no_free_ptr() is awkward, instead rework the code to have helpers and
clearer delineation of responsibility.

Now one might say that  __free(del_cxl_resource) is excessive given it
is immediately registered with add_or_reset_cxl_resource(). The
rationale for keeping it is that it forces use of "no_free_ptr()" on the
argument passed to add_or_reset_cxl_resource(). That in turn makes it
clear that @res is NULL for the rest of the function which is part of
the point of the cleanup helpers, to turn subtle use after free errors
[3] into loud NULL pointer de-references.

Link: http://lore.kernel.org/r/170820177238.631006.1012639681618409284.stgit@dwillia2-xfh.jf.intel.com [1]
Link: http://lore.kernel.org/r/CAHk-=whBVhnh=KSeBBRet=E7qJAwnPR_aj5em187Q3FiD+LXnA@mail.gmail.com [2]
Link: http://lore.kernel.org/r/20230714093146.2253438-1-leitao@debian.org [3]
Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Closes: http://lore.kernel.org/r/20240219124041.00002bda@Huawei.com
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v1:
- drop the introduction of cond_no_free_ptr() in favor of reorganizing
  the code to clarify the handoff of "free on error" responsibility.

 drivers/cxl/acpi.c |   99 ++++++++++++++++++++++++++++++----------------------
 drivers/cxl/cxl.h  |    5 +++
 2 files changed, 62 insertions(+), 42 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 1a3e6aafbdcc..1a47207932e2 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -316,28 +316,64 @@ static const struct cxl_root_ops acpi_root_ops = {
 	.qos_class = cxl_acpi_qos_class,
 };
 
+static void del_cxl_resource(struct resource *res)
+{
+	if (!res)
+		return;
+	kfree(res->name);
+	kfree(res);
+}
+
+static struct resource *alloc_cxl_resource(resource_size_t base,
+					   resource_size_t n, const char *fmt,
+					   ...)
+{
+	va_list ap;
+
+	struct resource *res __free(kfree) = kzalloc(sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return NULL;
+
+	res->start = base;
+	res->end = base + n - 1;
+	res->flags = IORESOURCE_MEM;
+
+	va_start(ap, fmt);
+	res->name = kvasprintf(GFP_KERNEL, fmt, ap);
+	va_end(ap);
+
+	if (!res->name)
+		return NULL;
+	return no_free_ptr(res);
+}
+
+static int add_or_reset_cxl_resource(struct resource *parent, struct resource *res)
+{
+	int rc = insert_resource(parent, res);
+
+	if (rc)
+		del_cxl_resource(res);
+	return rc;
+}
+
+DEFINE_FREE(put_cxlrd, struct cxl_root_decoder *,
+	    if (!IS_ERR_OR_NULL(_T)) put_device(&_T->cxlsd.cxld.dev))
+DEFINE_FREE(del_cxl_resource, struct resource *, if (_T) del_cxl_resource(_T))
 static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
 			     struct cxl_cfmws_context *ctx)
 {
 	int target_map[CXL_DECODER_MAX_INTERLEAVE];
 	struct cxl_port *root_port = ctx->root_port;
-	struct resource *cxl_res = ctx->cxl_res;
 	struct cxl_cxims_context cxims_ctx;
-	struct cxl_root_decoder *cxlrd;
 	struct device *dev = ctx->dev;
 	cxl_calc_hb_fn cxl_calc_hb;
 	struct cxl_decoder *cxld;
 	unsigned int ways, i, ig;
-	struct resource *res;
 	int rc;
 
 	rc = cxl_acpi_cfmws_verify(dev, cfmws);
-	if (rc) {
-		dev_err(dev, "CFMWS range %#llx-%#llx not registered\n",
-			cfmws->base_hpa,
-			cfmws->base_hpa + cfmws->window_size - 1);
+	if (rc)
 		return rc;
-	}
 
 	rc = eiw_to_ways(cfmws->interleave_ways, &ways);
 	if (rc)
@@ -348,29 +384,24 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
 	for (i = 0; i < ways; i++)
 		target_map[i] = cfmws->interleave_targets[i];
 
-	res = kzalloc(sizeof(*res), GFP_KERNEL);
+	struct resource *res __free(del_cxl_resource) =
+		alloc_cxl_resource(cfmws->base_hpa, cfmws->window_size,
+				   "CXL Window %d", ctx->id++);
 	if (!res)
 		return -ENOMEM;
 
-	res->name = kasprintf(GFP_KERNEL, "CXL Window %d", ctx->id++);
-	if (!res->name)
-		goto err_name;
-
-	res->start = cfmws->base_hpa;
-	res->end = cfmws->base_hpa + cfmws->window_size - 1;
-	res->flags = IORESOURCE_MEM;
-
 	/* add to the local resource tracking to establish a sort order */
-	rc = insert_resource(cxl_res, res);
+	rc = add_or_reset_cxl_resource(ctx->cxl_res, no_free_ptr(res));
 	if (rc)
-		goto err_insert;
+		return rc;
 
 	if (cfmws->interleave_arithmetic == ACPI_CEDT_CFMWS_ARITHMETIC_MODULO)
 		cxl_calc_hb = cxl_hb_modulo;
 	else
 		cxl_calc_hb = cxl_hb_xor;
 
-	cxlrd = cxl_root_decoder_alloc(root_port, ways, cxl_calc_hb);
+	struct cxl_root_decoder *cxlrd __free(put_cxlrd) =
+		cxl_root_decoder_alloc(root_port, ways, cxl_calc_hb);
 	if (IS_ERR(cxlrd))
 		return PTR_ERR(cxlrd);
 
@@ -378,8 +409,8 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
 	cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions);
 	cxld->target_type = CXL_DECODER_HOSTONLYMEM;
 	cxld->hpa_range = (struct range) {
-		.start = res->start,
-		.end = res->end,
+		.start = cfmws->base_hpa,
+		.end = cfmws->base_hpa + cfmws->window_size - 1,
 	};
 	cxld->interleave_ways = ways;
 	/*
@@ -399,11 +430,10 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
 			rc = acpi_table_parse_cedt(ACPI_CEDT_TYPE_CXIMS,
 						   cxl_parse_cxims, &cxims_ctx);
 			if (rc < 0)
-				goto err_xormap;
+				return rc;
 			if (!cxlrd->platform_data) {
 				dev_err(dev, "No CXIMS for HBIG %u\n", ig);
-				rc = -EINVAL;
-				goto err_xormap;
+				return -EINVAL;
 			}
 		}
 	}
@@ -411,18 +441,9 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
 	cxlrd->qos_class = cfmws->qtg_id;
 
 	rc = cxl_decoder_add(cxld, target_map);
-err_xormap:
 	if (rc)
-		put_device(&cxld->dev);
-	else
-		rc = cxl_decoder_autoremove(dev, cxld);
-	return rc;
-
-err_insert:
-	kfree(res->name);
-err_name:
-	kfree(res);
-	return -ENOMEM;
+		return rc;
+	return cxl_root_decoder_autoremove(dev, no_free_ptr(cxlrd));
 }
 
 static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
@@ -692,12 +713,6 @@ static void cxl_acpi_lock_reset_class(void *dev)
 	device_lock_reset_class(dev);
 }
 
-static void del_cxl_resource(struct resource *res)
-{
-	kfree(res->name);
-	kfree(res);
-}
-
 static void cxl_set_public_resource(struct resource *priv, struct resource *pub)
 {
 	priv->desc = (unsigned long) pub;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 003feebab79b..8bc044a4a965 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -776,6 +776,11 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map);
 struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port);
 int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map);
 int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld);
+static inline int cxl_root_decoder_autoremove(struct device *host,
+					      struct cxl_root_decoder *cxlrd)
+{
+	return cxl_decoder_autoremove(host, &cxlrd->cxlsd.cxld);
+}
 int cxl_endpoint_autoremove(struct cxl_memdev *cxlmd, struct cxl_port *endpoint);
 
 /**

Re: [PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws()

[Verma, Vishal L]

On Wed, 2024-02-28 at 12:30 -0800, Dan Williams wrote:
> As a follow on to the recent rework of __cxl_parse_cfmws() to always
> return errors [1], use cleanup.h helpers to remove goto and other cleanups
> now that logging is moved to the cxl_parse_cfmws() wrapper.
> 
> This ends up adding more code than it deletes, but __cxl_parse_cfmws()
> itself does get smaller. The takeaway from the cond_no_free_ptr()
> discussion [2] was to not add new macros to handle the cases where
> no_free_ptr() is awkward, instead rework the code to have helpers and
> clearer delineation of responsibility.
> 
> Now one might say that  __free(del_cxl_resource) is excessive given it
> is immediately registered with add_or_reset_cxl_resource(). The
> rationale for keeping it is that it forces use of "no_free_ptr()" on the
> argument passed to add_or_reset_cxl_resource(). That in turn makes it
> clear that @res is NULL for the rest of the function which is part of
> the point of the cleanup helpers, to turn subtle use after free errors
> [3] into loud NULL pointer de-references.
> 
> Link: http://lore.kernel.org/r/170820177238.631006.1012639681618409284.stgit@dwillia2-xfh.jf.intel.com [1]
> Link: http://lore.kernel.org/r/CAHk-=whBVhnh=KSeBBRet=E7qJAwnPR_aj5em187Q3FiD+LXnA@mail.gmail.com [2]
> Link: http://lore.kernel.org/r/20230714093146.2253438-1-leitao@debian.org [3]
> Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Closes: http://lore.kernel.org/r/20240219124041.00002bda@Huawei.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

The helpers make sense, and overall looks good to me.

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> ---
> Changes since v1:
> - drop the introduction of cond_no_free_ptr() in favor of reorganizing
>   the code to clarify the handoff of "free on error" responsibility.
> 
>  drivers/cxl/acpi.c |   99 ++++++++++++++++++++++++++++++----------------------
>  drivers/cxl/cxl.h  |    5 +++
>  2 files changed, 62 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 1a3e6aafbdcc..1a47207932e2 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -316,28 +316,64 @@ static const struct cxl_root_ops acpi_root_ops = {
>  	.qos_class = cxl_acpi_qos_class,
>  };
>  
> +static void del_cxl_resource(struct resource *res)
> +{
> +	if (!res)
> +		return;
> +	kfree(res->name);
> +	kfree(res);
> +}
> +
> +static struct resource *alloc_cxl_resource(resource_size_t base,
> +					   resource_size_t n, const char *fmt,
> +					   ...)
> +{
> +	va_list ap;
> +
> +	struct resource *res __free(kfree) = kzalloc(sizeof(*res), GFP_KERNEL);
> +	if (!res)
> +		return NULL;
> +
> +	res->start = base;
> +	res->end = base + n - 1;
> +	res->flags = IORESOURCE_MEM;
> +
> +	va_start(ap, fmt);
> +	res->name = kvasprintf(GFP_KERNEL, fmt, ap);
> +	va_end(ap);
> +
> +	if (!res->name)
> +		return NULL;
> +	return no_free_ptr(res);
> +}
> +
> +static int add_or_reset_cxl_resource(struct resource *parent, struct resource *res)
> +{
> +	int rc = insert_resource(parent, res);
> +
> +	if (rc)
> +		del_cxl_resource(res);
> +	return rc;
> +}
> +
> +DEFINE_FREE(put_cxlrd, struct cxl_root_decoder *,
> +	    if (!IS_ERR_OR_NULL(_T)) put_device(&_T->cxlsd.cxld.dev))
> +DEFINE_FREE(del_cxl_resource, struct resource *, if (_T) del_cxl_resource(_T))
>  static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
>  			     struct cxl_cfmws_context *ctx)
>  {
>  	int target_map[CXL_DECODER_MAX_INTERLEAVE];
>  	struct cxl_port *root_port = ctx->root_port;
> -	struct resource *cxl_res = ctx->cxl_res;
>  	struct cxl_cxims_context cxims_ctx;
> -	struct cxl_root_decoder *cxlrd;
>  	struct device *dev = ctx->dev;
>  	cxl_calc_hb_fn cxl_calc_hb;
>  	struct cxl_decoder *cxld;
>  	unsigned int ways, i, ig;
> -	struct resource *res;
>  	int rc;
>  
>  	rc = cxl_acpi_cfmws_verify(dev, cfmws);
> -	if (rc) {
> -		dev_err(dev, "CFMWS range %#llx-%#llx not registered\n",
> -			cfmws->base_hpa,
> -			cfmws->base_hpa + cfmws->window_size - 1);
> +	if (rc)
>  		return rc;
> -	}
>  
>  	rc = eiw_to_ways(cfmws->interleave_ways, &ways);
>  	if (rc)
> @@ -348,29 +384,24 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
>  	for (i = 0; i < ways; i++)
>  		target_map[i] = cfmws->interleave_targets[i];
>  
> -	res = kzalloc(sizeof(*res), GFP_KERNEL);
> +	struct resource *res __free(del_cxl_resource) =
> +		alloc_cxl_resource(cfmws->base_hpa, cfmws->window_size,
> +				   "CXL Window %d", ctx->id++);
>  	if (!res)
>  		return -ENOMEM;
>  
> -	res->name = kasprintf(GFP_KERNEL, "CXL Window %d", ctx->id++);
> -	if (!res->name)
> -		goto err_name;
> -
> -	res->start = cfmws->base_hpa;
> -	res->end = cfmws->base_hpa + cfmws->window_size - 1;
> -	res->flags = IORESOURCE_MEM;
> -
>  	/* add to the local resource tracking to establish a sort order */
> -	rc = insert_resource(cxl_res, res);
> +	rc = add_or_reset_cxl_resource(ctx->cxl_res, no_free_ptr(res));
>  	if (rc)
> -		goto err_insert;
> +		return rc;
>  
>  	if (cfmws->interleave_arithmetic == ACPI_CEDT_CFMWS_ARITHMETIC_MODULO)
>  		cxl_calc_hb = cxl_hb_modulo;
>  	else
>  		cxl_calc_hb = cxl_hb_xor;
>  
> -	cxlrd = cxl_root_decoder_alloc(root_port, ways, cxl_calc_hb);
> +	struct cxl_root_decoder *cxlrd __free(put_cxlrd) =
> +		cxl_root_decoder_alloc(root_port, ways, cxl_calc_hb);
>  	if (IS_ERR(cxlrd))
>  		return PTR_ERR(cxlrd);
>  
> @@ -378,8 +409,8 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
>  	cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions);
>  	cxld->target_type = CXL_DECODER_HOSTONLYMEM;
>  	cxld->hpa_range = (struct range) {
> -		.start = res->start,
> -		.end = res->end,
> +		.start = cfmws->base_hpa,
> +		.end = cfmws->base_hpa + cfmws->window_size - 1,
>  	};
>  	cxld->interleave_ways = ways;
>  	/*
> @@ -399,11 +430,10 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
>  			rc = acpi_table_parse_cedt(ACPI_CEDT_TYPE_CXIMS,
>  						   cxl_parse_cxims, &cxims_ctx);
>  			if (rc < 0)
> -				goto err_xormap;
> +				return rc;
>  			if (!cxlrd->platform_data) {
>  				dev_err(dev, "No CXIMS for HBIG %u\n", ig);
> -				rc = -EINVAL;
> -				goto err_xormap;
> +				return -EINVAL;
>  			}
>  		}
>  	}
> @@ -411,18 +441,9 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
>  	cxlrd->qos_class = cfmws->qtg_id;
>  
>  	rc = cxl_decoder_add(cxld, target_map);
> -err_xormap:
>  	if (rc)
> -		put_device(&cxld->dev);
> -	else
> -		rc = cxl_decoder_autoremove(dev, cxld);
> -	return rc;
> -
> -err_insert:
> -	kfree(res->name);
> -err_name:
> -	kfree(res);
> -	return -ENOMEM;
> +		return rc;
> +	return cxl_root_decoder_autoremove(dev, no_free_ptr(cxlrd));
>  }
>  
>  static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
> @@ -692,12 +713,6 @@ static void cxl_acpi_lock_reset_class(void *dev)
>  	device_lock_reset_class(dev);
>  }
>  
> -static void del_cxl_resource(struct resource *res)
> -{
> -	kfree(res->name);
> -	kfree(res);
> -}
> -
>  static void cxl_set_public_resource(struct resource *priv, struct resource *pub)
>  {
>  	priv->desc = (unsigned long) pub;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 003feebab79b..8bc044a4a965 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -776,6 +776,11 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map);
>  struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port);
>  int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map);
>  int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld);
> +static inline int cxl_root_decoder_autoremove(struct device *host,
> +					      struct cxl_root_decoder *cxlrd)
> +{
> +	return cxl_decoder_autoremove(host, &cxlrd->cxlsd.cxld);
> +}
>  int cxl_endpoint_autoremove(struct cxl_memdev *cxlmd, struct cxl_port *endpoint);
>  
>  /**
> 
> 

Re: [PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws()

[Alison Schofield]

On Wed, Feb 28, 2024 at 12:30:56PM -0800, Dan Williams wrote:
> As a follow on to the recent rework of __cxl_parse_cfmws() to always
> return errors [1], use cleanup.h helpers to remove goto and other cleanups
> now that logging is moved to the cxl_parse_cfmws() wrapper.

one question about helper below...


>
snip

> +static struct resource *alloc_cxl_resource(resource_size_t base,
> +					   resource_size_t n, const char *fmt,
> +					   ...)
> +{
> +	va_list ap;
> +
> +	struct resource *res __free(kfree) = kzalloc(sizeof(*res), GFP_KERNEL);
> +	if (!res)
> +		return NULL;
> +
> +	res->start = base;
> +	res->end = base + n - 1;
> +	res->flags = IORESOURCE_MEM;
> +
> +	va_start(ap, fmt);
> +	res->name = kvasprintf(GFP_KERNEL, fmt, ap);
> +	va_end(ap);

Why is alloc_cxl_resource() defined w variable params and using
kvasprintf(), rather than tailored to the one caller and used
kasprintf()?

Perhaps you have bigger plans for this helper?

Other than that,

Reviewed-by: Alison Schofield <alison.schofield@gmail.com>

> +

snip

Re: [PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws()

[Alison Schofield]

On Fri, Mar 01, 2024 at 10:29:36AM -0800, Alison Schofield wrote:
> On Wed, Feb 28, 2024 at 12:30:56PM -0800, Dan Williams wrote:
> > As a follow on to the recent rework of __cxl_parse_cfmws() to always
> > return errors [1], use cleanup.h helpers to remove goto and other cleanups
> > now that logging is moved to the cxl_parse_cfmws() wrapper.
> 
> one question about helper below...
> 
> 
> >
> snip
> 
> > +static struct resource *alloc_cxl_resource(resource_size_t base,
> > +					   resource_size_t n, const char *fmt,
> > +					   ...)
> > +{
> > +	va_list ap;
> > +
> > +	struct resource *res __free(kfree) = kzalloc(sizeof(*res), GFP_KERNEL);
> > +	if (!res)
> > +		return NULL;
> > +
> > +	res->start = base;
> > +	res->end = base + n - 1;
> > +	res->flags = IORESOURCE_MEM;
> > +
> > +	va_start(ap, fmt);
> > +	res->name = kvasprintf(GFP_KERNEL, fmt, ap);
> > +	va_end(ap);
> 
> Why is alloc_cxl_resource() defined w variable params and using
> kvasprintf(), rather than tailored to the one caller and used
> kasprintf()?
> 
> Perhaps you have bigger plans for this helper?
> 
> Other than that,
> 
> Reviewed-by: Alison Schofield <alison.schofield@gmail.com>

oopsy...
Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> 
> > +
> 
> snip
> 
> 

Re: [PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws()

[Dan Williams]

Alison Schofield wrote:
> On Wed, Feb 28, 2024 at 12:30:56PM -0800, Dan Williams wrote:
> > As a follow on to the recent rework of __cxl_parse_cfmws() to always
> > return errors [1], use cleanup.h helpers to remove goto and other cleanups
> > now that logging is moved to the cxl_parse_cfmws() wrapper.
> 
> one question about helper below...
> 
> 
> >
> snip
> 
> > +static struct resource *alloc_cxl_resource(resource_size_t base,
> > +					   resource_size_t n, const char *fmt,
> > +					   ...)
> > +{
> > +	va_list ap;
> > +
> > +	struct resource *res __free(kfree) = kzalloc(sizeof(*res), GFP_KERNEL);
> > +	if (!res)
> > +		return NULL;
> > +
> > +	res->start = base;
> > +	res->end = base + n - 1;
> > +	res->flags = IORESOURCE_MEM;
> > +
> > +	va_start(ap, fmt);
> > +	res->name = kvasprintf(GFP_KERNEL, fmt, ap);
> > +	va_end(ap);
> 
> Why is alloc_cxl_resource() defined w variable params and using
> kvasprintf(), rather than tailored to the one caller and used
> kasprintf()?
> 
> Perhaps you have bigger plans for this helper?

Good point, thanks! I think this function can just take the window id for now.

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 1a47207932e2..db38ead0353a 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -325,25 +325,20 @@ static void del_cxl_resource(struct resource *res)
 }
 
 static struct resource *alloc_cxl_resource(resource_size_t base,
-                                          resource_size_t n, const char *fmt,
-                                          ...)
+                                          resource_size_t n, int id)
 {
-       va_list ap;
-
        struct resource *res __free(kfree) = kzalloc(sizeof(*res), GFP_KERNEL);
+
        if (!res)
                return NULL;
 
        res->start = base;
        res->end = base + n - 1;
        res->flags = IORESOURCE_MEM;
-
-       va_start(ap, fmt);
-       res->name = kvasprintf(GFP_KERNEL, fmt, ap);
-       va_end(ap);
-
+       res->name = kasprintf(GFP_KERNEL, "CXL Window %d", id);
        if (!res->name)
                return NULL;
+
        return no_free_ptr(res);
 }
 
@@ -384,9 +379,8 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
        for (i = 0; i < ways; i++)
                target_map[i] = cfmws->interleave_targets[i];
 
-       struct resource *res __free(del_cxl_resource) =
-               alloc_cxl_resource(cfmws->base_hpa, cfmws->window_size,
-                                  "CXL Window %d", ctx->id++);
+       struct resource *res __free(del_cxl_resource) = alloc_cxl_resource(
+               cfmws->base_hpa, cfmws->window_size, ctx->id++);
        if (!res)
                return -ENOMEM;
 

[PATCH v3] cxl/acpi: Cleanup __cxl_parse_cfmws()

[Dan Williams]

As a follow on to the recent rework of __cxl_parse_cfmws() to always
return errors [1], use cleanup.h helpers to remove goto and other cleanups
now that logging is moved to the cxl_parse_cfmws() wrapper.

This ends up adding more code than it deletes, but __cxl_parse_cfmws()
itself does get smaller. The takeaway from the cond_no_free_ptr()
discussion [2] was to not add new macros to handle the cases where
no_free_ptr() is awkward, instead rework the code to have helpers and
clearer delineation of responsibility.

Now one might say that  __free(del_cxl_resource) is excessive given it
is immediately registered with add_or_reset_cxl_resource(). The
rationale for keeping it is that it forces use of "no_free_ptr()" on the
argument passed to add_or_reset_cxl_resource(). That in turn makes it
clear that @res is NULL for the rest of the function which is part of
the point of the cleanup helpers, to turn subtle use after free errors
[3] into loud NULL pointer de-references.

Link: http://lore.kernel.org/r/170820177238.631006.1012639681618409284.stgit@dwillia2-xfh.jf.intel.com [1]
Link: http://lore.kernel.org/r/CAHk-=whBVhnh=KSeBBRet=E7qJAwnPR_aj5em187Q3FiD+LXnA@mail.gmail.com [2]
Link: http://lore.kernel.org/r/20230714093146.2253438-1-leitao@debian.org [3]
Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Closes: http://lore.kernel.org/r/20240219124041.00002bda@Huawei.com
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v2:
- Drop kvasprintf() usage in alloc_cxl_resource (Alison)

 drivers/cxl/acpi.c |   93 +++++++++++++++++++++++++++++-----------------------
 drivers/cxl/cxl.h  |    5 +++
 2 files changed, 56 insertions(+), 42 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index af5cb818f84d..32091379a97b 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -316,28 +316,59 @@ static const struct cxl_root_ops acpi_root_ops = {
 	.qos_class = cxl_acpi_qos_class,
 };
 
+static void del_cxl_resource(struct resource *res)
+{
+	if (!res)
+		return;
+	kfree(res->name);
+	kfree(res);
+}
+
+static struct resource *alloc_cxl_resource(resource_size_t base,
+					   resource_size_t n, int id)
+{
+	struct resource *res __free(kfree) = kzalloc(sizeof(*res), GFP_KERNEL);
+
+	if (!res)
+		return NULL;
+
+	res->start = base;
+	res->end = base + n - 1;
+	res->flags = IORESOURCE_MEM;
+	res->name = kasprintf(GFP_KERNEL, "CXL Window %d", id);
+	if (!res->name)
+		return NULL;
+
+	return no_free_ptr(res);
+}
+
+static int add_or_reset_cxl_resource(struct resource *parent, struct resource *res)
+{
+	int rc = insert_resource(parent, res);
+
+	if (rc)
+		del_cxl_resource(res);
+	return rc;
+}
+
+DEFINE_FREE(put_cxlrd, struct cxl_root_decoder *,
+	    if (!IS_ERR_OR_NULL(_T)) put_device(&_T->cxlsd.cxld.dev))
+DEFINE_FREE(del_cxl_resource, struct resource *, if (_T) del_cxl_resource(_T))
 static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
 			     struct cxl_cfmws_context *ctx)
 {
 	int target_map[CXL_DECODER_MAX_INTERLEAVE];
 	struct cxl_port *root_port = ctx->root_port;
-	struct resource *cxl_res = ctx->cxl_res;
 	struct cxl_cxims_context cxims_ctx;
-	struct cxl_root_decoder *cxlrd;
 	struct device *dev = ctx->dev;
 	cxl_calc_hb_fn cxl_calc_hb;
 	struct cxl_decoder *cxld;
 	unsigned int ways, i, ig;
-	struct resource *res;
 	int rc;
 
 	rc = cxl_acpi_cfmws_verify(dev, cfmws);
-	if (rc) {
-		dev_err(dev, "CFMWS range %#llx-%#llx not registered\n",
-			cfmws->base_hpa,
-			cfmws->base_hpa + cfmws->window_size - 1);
+	if (rc)
 		return rc;
-	}
 
 	rc = eiw_to_ways(cfmws->interleave_ways, &ways);
 	if (rc)
@@ -348,29 +379,23 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
 	for (i = 0; i < ways; i++)
 		target_map[i] = cfmws->interleave_targets[i];
 
-	res = kzalloc(sizeof(*res), GFP_KERNEL);
+	struct resource *res __free(del_cxl_resource) = alloc_cxl_resource(
+		cfmws->base_hpa, cfmws->window_size, ctx->id++);
 	if (!res)
 		return -ENOMEM;
 
-	res->name = kasprintf(GFP_KERNEL, "CXL Window %d", ctx->id++);
-	if (!res->name)
-		goto err_name;
-
-	res->start = cfmws->base_hpa;
-	res->end = cfmws->base_hpa + cfmws->window_size - 1;
-	res->flags = IORESOURCE_MEM;
-
 	/* add to the local resource tracking to establish a sort order */
-	rc = insert_resource(cxl_res, res);
+	rc = add_or_reset_cxl_resource(ctx->cxl_res, no_free_ptr(res));
 	if (rc)
-		goto err_insert;
+		return rc;
 
 	if (cfmws->interleave_arithmetic == ACPI_CEDT_CFMWS_ARITHMETIC_MODULO)
 		cxl_calc_hb = cxl_hb_modulo;
 	else
 		cxl_calc_hb = cxl_hb_xor;
 
-	cxlrd = cxl_root_decoder_alloc(root_port, ways, cxl_calc_hb);
+	struct cxl_root_decoder *cxlrd __free(put_cxlrd) =
+		cxl_root_decoder_alloc(root_port, ways, cxl_calc_hb);
 	if (IS_ERR(cxlrd))
 		return PTR_ERR(cxlrd);
 
@@ -378,8 +403,8 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
 	cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions);
 	cxld->target_type = CXL_DECODER_HOSTONLYMEM;
 	cxld->hpa_range = (struct range) {
-		.start = res->start,
-		.end = res->end,
+		.start = cfmws->base_hpa,
+		.end = cfmws->base_hpa + cfmws->window_size - 1,
 	};
 	cxld->interleave_ways = ways;
 	/*
@@ -399,11 +424,10 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
 			rc = acpi_table_parse_cedt(ACPI_CEDT_TYPE_CXIMS,
 						   cxl_parse_cxims, &cxims_ctx);
 			if (rc < 0)
-				goto err_xormap;
+				return rc;
 			if (!cxlrd->platform_data) {
 				dev_err(dev, "No CXIMS for HBIG %u\n", ig);
-				rc = -EINVAL;
-				goto err_xormap;
+				return -EINVAL;
 			}
 		}
 	}
@@ -411,18 +435,9 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
 	cxlrd->qos_class = cfmws->qtg_id;
 
 	rc = cxl_decoder_add(cxld, target_map);
-err_xormap:
 	if (rc)
-		put_device(&cxld->dev);
-	else
-		rc = cxl_decoder_autoremove(dev, cxld);
-	return rc;
-
-err_insert:
-	kfree(res->name);
-err_name:
-	kfree(res);
-	return -ENOMEM;
+		return rc;
+	return cxl_root_decoder_autoremove(dev, no_free_ptr(cxlrd));
 }
 
 static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
@@ -694,12 +709,6 @@ static void cxl_acpi_lock_reset_class(void *dev)
 	device_lock_reset_class(dev);
 }
 
-static void del_cxl_resource(struct resource *res)
-{
-	kfree(res->name);
-	kfree(res);
-}
-
 static void cxl_set_public_resource(struct resource *priv, struct resource *pub)
 {
 	priv->desc = (unsigned long) pub;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 534e25e2f0a4..71ac7a5ada80 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -781,6 +781,11 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map);
 struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port);
 int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map);
 int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld);
+static inline int cxl_root_decoder_autoremove(struct device *host,
+					      struct cxl_root_decoder *cxlrd)
+{
+	return cxl_decoder_autoremove(host, &cxlrd->cxlsd.cxld);
+}
 int cxl_endpoint_autoremove(struct cxl_memdev *cxlmd, struct cxl_port *endpoint);
 
 /**

Re: [PATCH v3] cxl/acpi: Cleanup __cxl_parse_cfmws()

[Alison Schofield]

On Fri, Apr 05, 2024 at 03:05:50PM -0700, Dan Williams wrote:
> As a follow on to the recent rework of __cxl_parse_cfmws() to always
> return errors [1], use cleanup.h helpers to remove goto and other cleanups
> now that logging is moved to the cxl_parse_cfmws() wrapper.
> 
> This ends up adding more code than it deletes, but __cxl_parse_cfmws()
> itself does get smaller. The takeaway from the cond_no_free_ptr()
> discussion [2] was to not add new macros to handle the cases where
> no_free_ptr() is awkward, instead rework the code to have helpers and
> clearer delineation of responsibility.
> 
> Now one might say that  __free(del_cxl_resource) is excessive given it
> is immediately registered with add_or_reset_cxl_resource(). The
> rationale for keeping it is that it forces use of "no_free_ptr()" on the
> argument passed to add_or_reset_cxl_resource(). That in turn makes it
> clear that @res is NULL for the rest of the function which is part of
> the point of the cleanup helpers, to turn subtle use after free errors
> [3] into loud NULL pointer de-references.
> 
> Link: http://lore.kernel.org/r/170820177238.631006.1012639681618409284.stgit@dwillia2-xfh.jf.intel.com [1]
> Link: http://lore.kernel.org/r/CAHk-=whBVhnh=KSeBBRet=E7qJAwnPR_aj5em187Q3FiD+LXnA@mail.gmail.com [2]
> Link: http://lore.kernel.org/r/20230714093146.2253438-1-leitao@debian.org [3]
> Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Closes: http://lore.kernel.org/r/20240219124041.00002bda@Huawei.com
> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>


Doubling down ;)

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> ---
> Changes since v2:
> - Drop kvasprintf() usage in alloc_cxl_resource (Alison)
> 
>  drivers/cxl/acpi.c |   93 +++++++++++++++++++++++++++++-----------------------
>  drivers/cxl/cxl.h  |    5 +++
>  2 files changed, 56 insertions(+), 42 deletions(-)

snip

Re: [PATCH v3] cxl/acpi: Cleanup __cxl_parse_cfmws()

[Jonathan Cameron]

On Fri, 05 Apr 2024 15:05:50 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> As a follow on to the recent rework of __cxl_parse_cfmws() to always
> return errors [1], use cleanup.h helpers to remove goto and other cleanups
> now that logging is moved to the cxl_parse_cfmws() wrapper.
> 
> This ends up adding more code than it deletes, but __cxl_parse_cfmws()
> itself does get smaller. The takeaway from the cond_no_free_ptr()
> discussion [2] was to not add new macros to handle the cases where
> no_free_ptr() is awkward, instead rework the code to have helpers and
> clearer delineation of responsibility.
> 
> Now one might say that  __free(del_cxl_resource) is excessive given it
> is immediately registered with add_or_reset_cxl_resource(). The
> rationale for keeping it is that it forces use of "no_free_ptr()" on the
> argument passed to add_or_reset_cxl_resource(). That in turn makes it
> clear that @res is NULL for the rest of the function which is part of
> the point of the cleanup helpers, to turn subtle use after free errors
> [3] into loud NULL pointer de-references.
> 
> Link: http://lore.kernel.org/r/170820177238.631006.1012639681618409284.stgit@dwillia2-xfh.jf.intel.com [1]
> Link: http://lore.kernel.org/r/CAHk-=whBVhnh=KSeBBRet=E7qJAwnPR_aj5em187Q3FiD+LXnA@mail.gmail.com [2]
> Link: http://lore.kernel.org/r/20230714093146.2253438-1-leitao@debian.org [3]
> Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Closes: http://lore.kernel.org/r/20240219124041.00002bda@Huawei.com
> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

I guess I may have missed the boat on reviewing this one, but if not
a few late comments inline.  

Jonathan


> ---
> Changes since v2:
> - Drop kvasprintf() usage in alloc_cxl_resource (Alison)
> 
>  drivers/cxl/acpi.c |   93 +++++++++++++++++++++++++++++-----------------------
>  drivers/cxl/cxl.h  |    5 +++
>  2 files changed, 56 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index af5cb818f84d..32091379a97b 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c

> +
> +static struct resource *alloc_cxl_resource(resource_size_t base,
> +					   resource_size_t n, int id)
> +{
> +	struct resource *res __free(kfree) = kzalloc(sizeof(*res), GFP_KERNEL);
> +
> +	if (!res)
> +		return NULL;
> +
> +	res->start = base;
> +	res->end = base + n - 1;
> +	res->flags = IORESOURCE_MEM;
> +	res->name = kasprintf(GFP_KERNEL, "CXL Window %d", id);

If you are going to call a function something as generic
as 'alloc_cxl_resource' I wouldn't expect to see such a specific string.

alloc_cxl_fmw_resource() would be fine with this.

> +	if (!res->name)
> +		return NULL;
> +
> +	return no_free_ptr(res);
> +}

> +
> +DEFINE_FREE(put_cxlrd, struct cxl_root_decoder *,
> +	    if (!IS_ERR_OR_NULL(_T)) put_device(&_T->cxlsd.cxld.dev))
> +DEFINE_FREE(del_cxl_resource, struct resource *, if (_T) del_cxl_resource(_T))
>  static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
>  			     struct cxl_cfmws_context *ctx)
>  {
>  	int target_map[CXL_DECODER_MAX_INTERLEAVE];
>  	struct cxl_port *root_port = ctx->root_port;
> -	struct resource *cxl_res = ctx->cxl_res;
>  	struct cxl_cxims_context cxims_ctx;
> -	struct cxl_root_decoder *cxlrd;
>  	struct device *dev = ctx->dev;
>  	cxl_calc_hb_fn cxl_calc_hb;
>  	struct cxl_decoder *cxld;
>  	unsigned int ways, i, ig;
> -	struct resource *res;
>  	int rc;
>  
>  	rc = cxl_acpi_cfmws_verify(dev, cfmws);
> -	if (rc) {
> -		dev_err(dev, "CFMWS range %#llx-%#llx not registered\n",
> -			cfmws->base_hpa,
> -			cfmws->base_hpa + cfmws->window_size - 1);

Not sure how this is related to rest of the change.

> +	if (rc)
>  		return rc;
> -	}
>  
>  	rc = eiw_to_ways(cfmws->interleave_ways, &ways);
>  	if (rc)

...

Re: [PATCH v3] cxl/acpi: Cleanup __cxl_parse_cfmws()

[Dan Williams]

Jonathan Cameron wrote:
> On Fri, 05 Apr 2024 15:05:50 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > As a follow on to the recent rework of __cxl_parse_cfmws() to always
> > return errors [1], use cleanup.h helpers to remove goto and other cleanups
> > now that logging is moved to the cxl_parse_cfmws() wrapper.
> > 
> > This ends up adding more code than it deletes, but __cxl_parse_cfmws()
> > itself does get smaller. The takeaway from the cond_no_free_ptr()
> > discussion [2] was to not add new macros to handle the cases where
> > no_free_ptr() is awkward, instead rework the code to have helpers and
> > clearer delineation of responsibility.
> > 
> > Now one might say that  __free(del_cxl_resource) is excessive given it
> > is immediately registered with add_or_reset_cxl_resource(). The
> > rationale for keeping it is that it forces use of "no_free_ptr()" on the
> > argument passed to add_or_reset_cxl_resource(). That in turn makes it
> > clear that @res is NULL for the rest of the function which is part of
> > the point of the cleanup helpers, to turn subtle use after free errors
> > [3] into loud NULL pointer de-references.
> > 
> > Link: http://lore.kernel.org/r/170820177238.631006.1012639681618409284.stgit@dwillia2-xfh.jf.intel.com [1]
> > Link: http://lore.kernel.org/r/CAHk-=whBVhnh=KSeBBRet=E7qJAwnPR_aj5em187Q3FiD+LXnA@mail.gmail.com [2]
> > Link: http://lore.kernel.org/r/20230714093146.2253438-1-leitao@debian.org [3]
> > Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Closes: http://lore.kernel.org/r/20240219124041.00002bda@Huawei.com
> > Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> I guess I may have missed the boat on reviewing this one, but if not
> a few late comments inline.  
> 
> Jonathan
> 
> 
> > ---
> > Changes since v2:
> > - Drop kvasprintf() usage in alloc_cxl_resource (Alison)
> > 
> >  drivers/cxl/acpi.c |   93 +++++++++++++++++++++++++++++-----------------------
> >  drivers/cxl/cxl.h  |    5 +++
> >  2 files changed, 56 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index af5cb818f84d..32091379a97b 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> 
> > +
> > +static struct resource *alloc_cxl_resource(resource_size_t base,
> > +					   resource_size_t n, int id)
> > +{
> > +	struct resource *res __free(kfree) = kzalloc(sizeof(*res), GFP_KERNEL);
> > +
> > +	if (!res)
> > +		return NULL;
> > +
> > +	res->start = base;
> > +	res->end = base + n - 1;
> > +	res->flags = IORESOURCE_MEM;
> > +	res->name = kasprintf(GFP_KERNEL, "CXL Window %d", id);
> 
> If you are going to call a function something as generic
> as 'alloc_cxl_resource' I wouldn't expect to see such a specific string.
> 
> alloc_cxl_fmw_resource() would be fine with this.

This name was chosen for symmetry with the existing "del_cxl_resource()",
and it really does feed a generic "CXL resource" concept since the rest of
the kernel does not care about the ACPI'ism of "CFMWS". The kernel
considers these resources with IORES_DESC_CXL specified.

> 
> > +	if (!res->name)
> > +		return NULL;
> > +
> > +	return no_free_ptr(res);
> > +}
> 
> > +
> > +DEFINE_FREE(put_cxlrd, struct cxl_root_decoder *,
> > +	    if (!IS_ERR_OR_NULL(_T)) put_device(&_T->cxlsd.cxld.dev))
> > +DEFINE_FREE(del_cxl_resource, struct resource *, if (_T) del_cxl_resource(_T))
> >  static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
> >  			     struct cxl_cfmws_context *ctx)
> >  {
> >  	int target_map[CXL_DECODER_MAX_INTERLEAVE];
> >  	struct cxl_port *root_port = ctx->root_port;
> > -	struct resource *cxl_res = ctx->cxl_res;
> >  	struct cxl_cxims_context cxims_ctx;
> > -	struct cxl_root_decoder *cxlrd;
> >  	struct device *dev = ctx->dev;
> >  	cxl_calc_hb_fn cxl_calc_hb;
> >  	struct cxl_decoder *cxld;
> >  	unsigned int ways, i, ig;
> > -	struct resource *res;
> >  	int rc;
> >  
> >  	rc = cxl_acpi_cfmws_verify(dev, cfmws);
> > -	if (rc) {
> > -		dev_err(dev, "CFMWS range %#llx-%#llx not registered\n",
> > -			cfmws->base_hpa,
> > -			cfmws->base_hpa + cfmws->window_size - 1);
> 
> Not sure how this is related to rest of the change.

Recall that in 5c6224bfabbf ("cxl/acpi: Fix load failures due to single
window creation failure") was a fix that also unified some of the print
messages in cxl_parse_cfmws(). This follow-on cleanup finalizes that with
conversion to scoped-based resource management and removal of redundant
error messages.

Otherwise:

__cxl_parse_cfmws()
    dev_err(dev, "CFMWS range %#llx-%#llx not registered\n",
            cfmws->base_hpa,
            cfmws->base_hpa + cfmws->window_size - 1);

...duplicates:

cxl_parse_cfmws()
    dev_err(dev,
            "Failed to add decode range: [%#llx - %#llx] (%d)\n",
            cfmws->base_hpa,
            cfmws->base_hpa + cfmws->window_size - 1, rc);

Re: [PATCH v3] cxl/acpi: Cleanup __cxl_parse_cfmws()

[Jonathan Cameron]

On Tue, 30 Apr 2024 15:46:08 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Fri, 05 Apr 2024 15:05:50 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >   
> > > As a follow on to the recent rework of __cxl_parse_cfmws() to always
> > > return errors [1], use cleanup.h helpers to remove goto and other cleanups
> > > now that logging is moved to the cxl_parse_cfmws() wrapper.
> > > 
> > > This ends up adding more code than it deletes, but __cxl_parse_cfmws()
> > > itself does get smaller. The takeaway from the cond_no_free_ptr()
> > > discussion [2] was to not add new macros to handle the cases where
> > > no_free_ptr() is awkward, instead rework the code to have helpers and
> > > clearer delineation of responsibility.
> > > 
> > > Now one might say that  __free(del_cxl_resource) is excessive given it
> > > is immediately registered with add_or_reset_cxl_resource(). The
> > > rationale for keeping it is that it forces use of "no_free_ptr()" on the
> > > argument passed to add_or_reset_cxl_resource(). That in turn makes it
> > > clear that @res is NULL for the rest of the function which is part of
> > > the point of the cleanup helpers, to turn subtle use after free errors
> > > [3] into loud NULL pointer de-references.
> > > 
> > > Link: http://lore.kernel.org/r/170820177238.631006.1012639681618409284.stgit@dwillia2-xfh.jf.intel.com [1]
> > > Link: http://lore.kernel.org/r/CAHk-=whBVhnh=KSeBBRet=E7qJAwnPR_aj5em187Q3FiD+LXnA@mail.gmail.com [2]
> > > Link: http://lore.kernel.org/r/20230714093146.2253438-1-leitao@debian.org [3]
> > > Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > > Closes: http://lore.kernel.org/r/20240219124041.00002bda@Huawei.com
> > > Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> > > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> > 
> > I guess I may have missed the boat on reviewing this one, but if not
> > a few late comments inline.  
> > 
> > Jonathan
> > 
> >   
> > > ---
> > > Changes since v2:
> > > - Drop kvasprintf() usage in alloc_cxl_resource (Alison)
> > > 
> > >  drivers/cxl/acpi.c |   93 +++++++++++++++++++++++++++++-----------------------
> > >  drivers/cxl/cxl.h  |    5 +++
> > >  2 files changed, 56 insertions(+), 42 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > index af5cb818f84d..32091379a97b 100644
> > > --- a/drivers/cxl/acpi.c
> > > +++ b/drivers/cxl/acpi.c  
> >   
> > > +
> > > +static struct resource *alloc_cxl_resource(resource_size_t base,
> > > +					   resource_size_t n, int id)
> > > +{
> > > +	struct resource *res __free(kfree) = kzalloc(sizeof(*res), GFP_KERNEL);
> > > +
> > > +	if (!res)
> > > +		return NULL;
> > > +
> > > +	res->start = base;
> > > +	res->end = base + n - 1;
> > > +	res->flags = IORESOURCE_MEM;
> > > +	res->name = kasprintf(GFP_KERNEL, "CXL Window %d", id);  
> > 
> > If you are going to call a function something as generic
> > as 'alloc_cxl_resource' I wouldn't expect to see such a specific string.
> > 
> > alloc_cxl_fmw_resource() would be fine with this.  
> 
> This name was chosen for symmetry with the existing "del_cxl_resource()",
> and it really does feed a generic "CXL resource" concept since the rest of
> the kernel does not care about the ACPI'ism of "CFMWS". The kernel
> considers these resources with IORES_DESC_CXL specified.

hmm. Ok. Smells funny, but fair enough.

> 
> >   
> > > +	if (!res->name)
> > > +		return NULL;
> > > +
> > > +	return no_free_ptr(res);
> > > +}  
> >   
> > > +
> > > +DEFINE_FREE(put_cxlrd, struct cxl_root_decoder *,
> > > +	    if (!IS_ERR_OR_NULL(_T)) put_device(&_T->cxlsd.cxld.dev))
> > > +DEFINE_FREE(del_cxl_resource, struct resource *, if (_T) del_cxl_resource(_T))
> > >  static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
> > >  			     struct cxl_cfmws_context *ctx)
> > >  {
> > >  	int target_map[CXL_DECODER_MAX_INTERLEAVE];
> > >  	struct cxl_port *root_port = ctx->root_port;
> > > -	struct resource *cxl_res = ctx->cxl_res;
> > >  	struct cxl_cxims_context cxims_ctx;
> > > -	struct cxl_root_decoder *cxlrd;
> > >  	struct device *dev = ctx->dev;
> > >  	cxl_calc_hb_fn cxl_calc_hb;
> > >  	struct cxl_decoder *cxld;
> > >  	unsigned int ways, i, ig;
> > > -	struct resource *res;
> > >  	int rc;
> > >  
> > >  	rc = cxl_acpi_cfmws_verify(dev, cfmws);
> > > -	if (rc) {
> > > -		dev_err(dev, "CFMWS range %#llx-%#llx not registered\n",
> > > -			cfmws->base_hpa,
> > > -			cfmws->base_hpa + cfmws->window_size - 1);  
> > 
> > Not sure how this is related to rest of the change.  
> 
> Recall that in 5c6224bfabbf ("cxl/acpi: Fix load failures due to single
> window creation failure") was a fix that also unified some of the print
> messages in cxl_parse_cfmws(). This follow-on cleanup finalizes that with
> conversion to scoped-based resource management and removal of redundant
> error messages.
> 
> Otherwise:
> 
> __cxl_parse_cfmws()
>     dev_err(dev, "CFMWS range %#llx-%#llx not registered\n",
>             cfmws->base_hpa,
>             cfmws->base_hpa + cfmws->window_size - 1);
> 
> ...duplicates:
> 
> cxl_parse_cfmws()
>     dev_err(dev,
>             "Failed to add decode range: [%#llx - %#llx] (%d)\n",
>             cfmws->base_hpa,
>             cfmws->base_hpa + cfmws->window_size - 1, rc);
Ah. I guess this is one of those 'other cleanups' in the commit
message.

Fair enough,

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

Jonathan