On Sat, 24 Feb 2024 11:30:27 -0800
Dan Williams <dan.j.williams@xxxxxxxxx> 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...):
