Re: [PATCH v14 19/34] cxl/port: Fix devm resource leaks around with dport management

[Jonathan Cameron]

> 	rc = check_no_existing_dport(port, dport_dev);
> 	if (rc)
> 		return ERR_PTR(rc);
> 
> then can rename new_dport to dport for this patch.
> Or don't bother hiding it and just use dport variable for both. 
> 
> 
> >  	int rc;
> >  
> > @@ -1615,29 +1649,47 @@ static struct cxl_dport *cxl_port_add_dport(struct cxl_port *port,
> >  		return ERR_PTR(-EBUSY);
> >  	}
> >  
> > -	struct cxl_dport *new_dport __free(del_cxl_dport) =
> > -		devm_cxl_add_dport_by_dev(port, dport_dev);
> > -	if (IS_ERR(new_dport))
> > -		return new_dport;
> > -
> > -	cxl_switch_parse_cdat(new_dport);
> > +	/*
> > +	 * With the first dport arrival it is now safe to start looking at
> > +	 * component registers. Be careful to not strand resources if dport
> > +	 * creation ultimately fails.
> > +	 */
> > +	struct cxl_port *port_group __free(cxl_port_release_group) =
> > +		cxl_port_open_group(port);  
> 
> So this is relies on everything being registered against port->dev, whereas
> for root ports this then gets handed off to port->uport_dev.
> That's a bit obscure - hence request for some patch description text
> on who owns what resources + when.
> 
> > +	if (IS_ERR(port_group))
> > +		return ERR_CAST(port_group);
> >  
> > -	if (ida_is_empty(&port->decoder_ida)) {
> > +	if (port->nr_dports == 0) {
> >  		rc = devm_cxl_switch_port_decoders_setup(port);
> >  		if (rc)
> >  			return ERR_PTR(rc);
I'm not totally sure I have an appropriate base for messing with this but
with just this patch on top of cxl/for-7.0/cxl-init I'm getting problems
with the reorder here as the devm_cxl_switch_port fails to map HDM decoders.
On a simple single switch couple of devices test.

I'm probably suffering Friday afternoon syndrome (and naughty testing just
a middle patch in a series)

My 'guess' is that it's because we no longer add the port first.
I couldn't spot any other patch messing with related logic earlier in Terry's series
(and wanted to keep what I was messing with for trying alternative fixes to a minimum)

Any thoughts on what I'm missing?

Jonathan


> > -		dev_dbg(&port->dev, "first dport%d:%s added with decoders\n",
> > -			new_dport->port_id, dev_name(dport_dev));
> > -		return no_free_ptr(new_dport);
> > +		/*
> > +		 * Note, when nr_dports returns to zero the port is unregistered
> > +		 * and triggers cleanup. I.e. no need for open-coded release
> > +		 * action on dport removal. See cxl_detach_ep() for that logic.
> > +		 */
> >  	}
> >  
> > +	new_dport = cxl_add_dport_by_dev(port, dport_dev);
> > +	if (IS_ERR(new_dport))
> > +		return new_dport;
> > +
> > +	rc = cxl_dport_autoremove(new_dport);
> > +	if (rc)
> > +		return ERR_PTR(rc);
> > +
> > +	cxl_switch_parse_cdat(new_dport);
> > +
> > +	/* group tracking no longer needed, dport successfully added */
> > +	cxl_port_remove_group(no_free_ptr(port_group));