Jonathan Cameron wrote:
> 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()
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...
