On Sat, 24 Feb 2024 11:30:27 -0800 Dan Williams dan.j.williams@intel.com wrote:
Dan Williams wrote:
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:
...and the version that actually compiles, fixed up cxl_root_decoder declaration and dropped the BUILD_BUG_ON() since it will naturally fail to compile if more than the supported number of variables is passed to cond_no_free_ptr():
-- 8< -- diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index 1a3e6aafbdcc..5c1dc4adf80d 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) { @@ -323,21 +325,15 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
/* 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);
I'm not convinced this is that much clearer than rc = insert_resource(cxl_res, res); if (rc) return rc; no_check_no_free_ptrs(res); no_check_no_free_ptrs(name);
with better naming and with that being defined in similar way to your __cond_no_free_ptrs()
Just keeping them in the same code block is probably enough to indicate that these are there because of success of insert_resource() + no need to handle bigger and bigger sets of params in the future.
Rest looks good to me
Jonathan
...
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h index c2d09bc4f976..e156fed88f51 100644 --- a/include/linux/cleanup.h +++ b/include/linux/cleanup.h @@ -77,6 +77,28 @@ 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);})
Nasty ;)
+#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 there are occasions
- where 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__); \- } else { \
_fail; \- }
/*
- DEFINE_CLASS(name, type, exit, init, init_args...):