Jonathan Cameron wrote:
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()
Can keep that as a fallback, but if Peter / Linus agree to the syntax of cond_guard(), which follows from scoped_cond_guard(), I would ask that they consider cond_no_free_ptr() as well. Single statement termination of variables paired with the single statement that took on ownership has an appealing symmetry to me.
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
Thanks for taking a look. I'll run cond_no_free_ptr() by more folks and if it continues to get a cold reception I'll drop it, or maybe a third way develops...