* [PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws()
@ 2024-02-28 20:30 Dan Williams
2024-03-01 0:57 ` Verma, Vishal L
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Dan Williams @ 2024-02-28 20:30 UTC (permalink / raw)
To: linux-cxl; +Cc: Jonathan Cameron
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);
/**
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws()
2024-02-28 20:30 [PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws() Dan Williams
@ 2024-03-01 0:57 ` Verma, Vishal L
2024-03-01 18:29 ` Alison Schofield
2024-04-05 22:05 ` [PATCH v3] " Dan Williams
2 siblings, 0 replies; 10+ messages in thread
From: Verma, Vishal L @ 2024-03-01 0:57 UTC (permalink / raw)
To: Williams, Dan J, linux-cxl; +Cc: Jonathan.Cameron
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);
>
> /**
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws()
2024-02-28 20:30 [PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws() Dan Williams
2024-03-01 0:57 ` Verma, Vishal L
@ 2024-03-01 18:29 ` Alison Schofield
2024-03-01 19:19 ` Alison Schofield
2024-03-01 19:34 ` Dan Williams
2024-04-05 22:05 ` [PATCH v3] " Dan Williams
2 siblings, 2 replies; 10+ messages in thread
From: Alison Schofield @ 2024-03-01 18:29 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-cxl, Jonathan Cameron
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws()
2024-03-01 18:29 ` Alison Schofield
@ 2024-03-01 19:19 ` Alison Schofield
2024-03-01 19:34 ` Dan Williams
1 sibling, 0 replies; 10+ messages in thread
From: Alison Schofield @ 2024-03-01 19:19 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-cxl, Jonathan Cameron
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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws()
2024-03-01 18:29 ` Alison Schofield
2024-03-01 19:19 ` Alison Schofield
@ 2024-03-01 19:34 ` Dan Williams
1 sibling, 0 replies; 10+ messages in thread
From: Dan Williams @ 2024-03-01 19:34 UTC (permalink / raw)
To: Alison Schofield, Dan Williams; +Cc: linux-cxl, Jonathan Cameron
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;
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3] cxl/acpi: Cleanup __cxl_parse_cfmws()
2024-02-28 20:30 [PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws() Dan Williams
2024-03-01 0:57 ` Verma, Vishal L
2024-03-01 18:29 ` Alison Schofield
@ 2024-04-05 22:05 ` Dan Williams
2024-04-05 23:56 ` Alison Schofield
2024-04-22 15:59 ` Jonathan Cameron
2 siblings, 2 replies; 10+ messages in thread
From: Dan Williams @ 2024-04-05 22:05 UTC (permalink / raw)
To: dave.jiang; +Cc: Jonathan Cameron, Alison Schofield, Vishal Verma, linux-cxl
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);
/**
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] cxl/acpi: Cleanup __cxl_parse_cfmws()
2024-04-05 22:05 ` [PATCH v3] " Dan Williams
@ 2024-04-05 23:56 ` Alison Schofield
2024-04-22 15:59 ` Jonathan Cameron
1 sibling, 0 replies; 10+ messages in thread
From: Alison Schofield @ 2024-04-05 23:56 UTC (permalink / raw)
To: Dan Williams; +Cc: dave.jiang, Jonathan Cameron, Vishal Verma, linux-cxl
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] cxl/acpi: Cleanup __cxl_parse_cfmws()
2024-04-05 22:05 ` [PATCH v3] " Dan Williams
2024-04-05 23:56 ` Alison Schofield
@ 2024-04-22 15:59 ` Jonathan Cameron
2024-04-30 22:46 ` Dan Williams
1 sibling, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2024-04-22 15:59 UTC (permalink / raw)
To: Dan Williams; +Cc: dave.jiang, Alison Schofield, Vishal Verma, linux-cxl
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)
...
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] cxl/acpi: Cleanup __cxl_parse_cfmws()
2024-04-22 15:59 ` Jonathan Cameron
@ 2024-04-30 22:46 ` Dan Williams
2024-05-01 12:21 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Dan Williams @ 2024-04-30 22:46 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: dave.jiang, Alison Schofield, Vishal Verma, linux-cxl
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);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] cxl/acpi: Cleanup __cxl_parse_cfmws()
2024-04-30 22:46 ` Dan Williams
@ 2024-05-01 12:21 ` Jonathan Cameron
0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2024-05-01 12:21 UTC (permalink / raw)
To: Dan Williams; +Cc: dave.jiang, Alison Schofield, Vishal Verma, linux-cxl
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-05-01 12:22 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-28 20:30 [PATCH v2] cxl/acpi: Cleanup __cxl_parse_cfmws() Dan Williams
2024-03-01 0:57 ` Verma, Vishal L
2024-03-01 18:29 ` Alison Schofield
2024-03-01 19:19 ` Alison Schofield
2024-03-01 19:34 ` Dan Williams
2024-04-05 22:05 ` [PATCH v3] " Dan Williams
2024-04-05 23:56 ` Alison Schofield
2024-04-22 15:59 ` Jonathan Cameron
2024-04-30 22:46 ` Dan Williams
2024-05-01 12:21 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).