Re: [PATCH v6 2/5] cxl: Convert find_cxl_root() to return a 'struct cxl_root *'

[Robert Richter <rrichter@amd.com>]

On 08.01.24 12:43:50, Dan Williams wrote:
> Robert Richter wrote:
> > On 05.01.24 15:07:40, Dave Jiang wrote:

> > > @@ -349,15 +348,19 @@ static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
> > >  {
> > >  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> > >  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> > > -	struct cxl_port *root_port __free(put_device) = NULL;
> > >  	LIST_HEAD(__discard);
> > >  	struct list_head *discard __free(dpa_perf) = &__discard;
> > > +	struct cxl_port *root_port;
> > >  	int rc;
> > >  
> > > -	root_port = find_cxl_root(cxlmd->endpoint);
> > > -	if (!root_port)
> > > +	struct cxl_root *cxl_root __free(put_cxl_root) =
> > > +		find_cxl_root(cxlmd->endpoint);
> > 
> > That's the drawback that lines get very long. Not sure if that was
> > discussed before, maybe just assign NULL in the definition and then
> > have the first assignment right after?
> 
> That style recommendation was something we were discussing here:
> 
> http://lore.kernel.org/r/20240104223725.GA1829769@bhelgaas
> 
> ...where the outcome I took away was "try to keep the get/put in the
> same statement, especially if the function has multiple usages of
> __free()".

I see the point here. An alternative would be to have one __free per
block, but additional blocks would introduce indentation. Maybe worth
to consider anyway as that addresses the style issue too.

I am fine that my other comments are addressed with top patches too.

Thanks,

-Robert