Dan Williams wrote:
[..]
> > This is definitely not nice to read. We are randomly setting an
> > apparently unrelated pointer to NULL. At very least the __free
> > should operating on cxld then we can use
So, how about this... I don't hate it:
-- 8< --
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 1a3e6aafbdcc..94ff38960f99 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -316,6 +316,8 @@ static const struct cxl_root_ops acpi_root_ops = {
.qos_class = cxl_acpi_qos_class,
};
+DEFINE_FREE(put_cxlrd, struct cxl_root_decoder *,
+ if (!IS_ERR_OR_NULL(_T)) put_device(&_T->cxlsd.cxld.dev))
static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
struct cxl_cfmws_context *ctx)
{
@@ -328,16 +330,11 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
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 +345,31 @@ 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(kfree) = kzalloc(sizeof(*res), GFP_KERNEL);
if (!res)
return -ENOMEM;
- res->name = kasprintf(GFP_KERNEL, "CXL Window %d", ctx->id++);
- if (!res->name)
- goto err_name;
+ const char *name __free(kfree) =
+ kasprintf(GFP_KERNEL, "CXL Window %d", ctx->id++);
+ if (!name)
+ return -ENOMEM;
+ res->name = 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);
- if (rc)
- goto err_insert;
+ cond_no_free_ptr(rc == 0, return rc, res, name);
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_decocder *cxlrd __free(put_cxlrd) =
+ cxl_root_decoder_alloc(root_port, ways, cxl_calc_hb);
if (IS_ERR(cxlrd))
return PTR_ERR(cxlrd);
@@ -378,8 +377,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 +398,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 +409,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,
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);
/**
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..9bf242e22191 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -77,6 +77,29 @@ const volatile void * __must_check_fn(const volatile void *val)
#define return_ptr(p) return no_free_ptr(p)
+#define __cond_no_free_ptrs(p) ({__auto_type __always_unused __ptr = no_free_ptr(p);})
+#define __cond_no_free_ptrs1(p, ...) __cond_no_free_ptrs(p)
+#define __cond_no_free_ptrs2(p, ...) \
+ __cond_no_free_ptrs(p), __cond_no_free_ptrs1(__VA_ARGS__)
+#define __cond_no_free_ptrs3(p, ...) \
+ __cond_no_free_ptrs(p), __cond_no_free_ptrs2(__VA_ARGS__)
+
+/*
+ * When an object is built up by an amalgamation of multiple
+ * allocations each of those need to be cleaned up on error, but once
+ * the object is registered all of those cleanups can be cancelled.
+ * cond_no_free_ptr() arranges to call no_free_ptr() on all its
+ * arguments (up to 3) if @condition is true and runs @_fail otherwise
+ * (typically to return and trigger auto-cleanup).
+ */
+#define cond_no_free_ptr(condition, _fail, ...) \
+ if (condition) { \
+ CONCATENATE(__cond_no_free_ptrs, COUNT_ARGS(__VA_ARGS__)) \
+ (__VA_ARGS__); \
+ BUILD_BUG_ON(COUNT_ARGS(__VA_ARGS__) > 3); \
+ } else { \
+ _fail; \
+ }
/*
* DEFINE_CLASS(name, type, exit, init, init_args...):
