cxl/acpi: Cleanup __cxl_parse_cfmws()

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
Link: http://lore.kernel.org/r/CAHk-=whBVhnh=KSeBBRet=E7qJAwnPR_aj5em187Q3FiD+LXnA@mail.gmail.com
Link: http://lore.kernel.org/r/20230714093146.2253438-1-leitao@debian.org
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>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Link: https://lore.kernel.org/r/171235474028.2718248.14109646123143505522.stgit@dwillia2-xfh.jf.intel.com
Signed-off-by: Dave Jiang <dave.jiang@intel.com>

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index cb8c155a2c9b3dbdcbf00f198c5783b9559f8a89..571069863c62946c8341a1c98810fd0325c69e95 100644 (file)
--- 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,
@@ -683,12 +698,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 2a09db5f72eed7eb2874ee049dd406a9662bfc59..80f58b96dc1c1a67bfa28a4659c98b09041df2b5 100644 (file)
--- 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);
 
 /**