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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux