Re: [PATCH v1 10/29] cxl/region: Add function to find a port's switch decoder by range

[Robert Richter]

On Fri, Jan 17, 2025 at 03:31:34PM -0600, Ben Cheatham wrote:
> On 1/7/25 8:09 AM, Robert Richter wrote:
> > Factor out code to find the switch decoder of a port for a specific
> > address range. Reuse the code to search a root decoder, create the
> > function cxl_port_find_switch_decoder() and rework
> > match_root_decoder_by_range() to be usable for switch decoders too.
> > 
> > Signed-off-by: Robert Richter <rrichter@xxxxxxx>
> > ---
> >  drivers/cxl/core/region.c | 43 +++++++++++++++++++++++----------------
> >  1 file changed, 25 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 5750ed2796a8..48add814924b 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -3189,19 +3189,35 @@ static int devm_cxl_add_dax_region(struct cxl_region *cxlr)
> >  	return rc;
> >  }
> >  
> > -static int match_root_decoder_by_range(struct device *dev, void *data)
> > +static int match_decoder_by_range(struct device *dev, void *data)
> >  {
> >  	struct range *r1, *r2 = data;
> > -	struct cxl_root_decoder *cxlrd;
> > +	struct cxl_decoder *cxld;
> >  
> > -	if (!is_root_decoder(dev))
> > +	if (!is_switch_decoder(dev))
> >  		return 0;
> >  
> > -	cxlrd = to_cxl_root_decoder(dev);
> > -	r1 = &cxlrd->cxlsd.cxld.hpa_range;
> > +	cxld = to_cxl_decoder(dev);
> > +	r1 = &cxld->hpa_range;
> >  	return range_contains(r1, r2);
> >  }
> >  
> > +static struct cxl_decoder *
> > +cxl_port_find_switch_decoder(struct cxl_port *port, struct range *hpa)
> > +{
> > +	/*
> > +	 * device_find_child() creates a reference to the root
> > +	 * decoder. Since the root decoder exists as long as the root
> > +	 * port exists and the endpoint already holds a reference to
> > +	 * the root port, this additional reference is not needed.
> > +	 * Free it here.
> > +	 */
> 
> Is this comment still true? I haven't read the rest of the series yet, but there's
> nothing enforcing that this function is called on a root port. If it's meant to
> only be used for root ports then it should probably be named that way.
> 
> Also, if it is meant to be used for a general switch decoder, can we always free
> the reference? If so then all that needs to happen is a comment update, otherwise
> you'll need to keep the reference and put a comment somewhere that the function
> needs a matching put_device().

In general, the assumption is true and all ports in the hierarchy
exist as long as the endpoint exists. The reference can be freed. I
have updated the comment:

/*
 * device_find_child() increments the reference count of the
 * the switch decoder's parent port to protect the reference
 * to its child. The port is already a parent of the endpoint
 * decoder's port, at least indirectly in the port hierarchy.
 * Thus, the endpoint already holds a reference for the parent
 * port of the switch decoder. Free the unnecessary reference
 * here.
 */

Thanks for catching this.

-Robert