Re: [PATCH] cxl/acpi: Fix load failures due to single window creation failure

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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...):