Re: [PATCH v10 04/15] cxl/hdm: Use stored Component Register mappings to map HDM decoder capability

[Dan Williams]

Robert Richter wrote:
> Dan,
> 
> On 14.09.23 17:15:44, Dan Williams wrote:
> > Dan Williams wrote:
> > > Terry Bowman wrote:
> > > > From: Robert Richter <rrichter@xxxxxxx>
> > > > 
> > > > Now, that the Component Register mappings are stored, use them to
> > > > enable and map the HDM decoder capabilities. The Component Registers
> > > > do not need to be probed again for this, remove probing code.
> > > > 
> > > > The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
> > > > Endpoint's component register mappings are located in the cxlds and
> > > > else in the port's structure. Provide a helper function
> > > > cxl_port_get_comp_map() to locate the mappings depending on the
> > > > component's type.
> > > > 
> > > > Signed-off-by: Robert Richter <rrichter@xxxxxxx>
> > > > Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
> > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > > > Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > > > ---
> > > >  drivers/cxl/core/hdm.c | 65 +++++++++++++++++++++++-------------------
> > > >  1 file changed, 35 insertions(+), 30 deletions(-)
> > > > 
> > > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > > index 17c8ba8c75e0..892a1fb5e4c6 100644
> > > > --- a/drivers/cxl/core/hdm.c
> > > > +++ b/drivers/cxl/core/hdm.c
> > > > @@ -81,27 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
> > > >  		cxlhdm->interleave_mask |= GENMASK(14, 12);
> > > >  }
> > > >  
> > > > -static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
> > > > -				struct cxl_component_regs *regs)
> > > > -{
> > > > -	struct cxl_register_map map = {
> > > > -		.dev = &port->dev,
> > > > -		.resource = port->component_reg_phys,
> > > > -		.base = crb,
> > > > -		.max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
> > > > -	};
> > > > -
> > > > -	cxl_probe_component_regs(&port->dev, crb, &map.component_map);
> > > > -	if (!map.component_map.hdm_decoder.valid) {
> > > > -		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> > > > -		/* unique error code to indicate no HDM decoder capability */
> > > > -		return -ENODEV;
> > > > -	}
> > > > -
> > > > -	return cxl_map_component_regs(&map, &port->dev, regs,
> > > > -				      BIT(CXL_CM_CAP_CAP_ID_HDM));
> > > > -}
> > > > -
> > > >  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> > > >  {
> > > >  	struct cxl_hdm *cxlhdm;
> > > > @@ -146,6 +125,22 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> > > >  	return true;
> > > >  }
> > > >  
> > > > +static struct cxl_register_map *cxl_port_get_comp_map(struct cxl_port *port)
> > > > +{
> > > > +	/*
> > > > +	 * HDM capability applies to Endpoints, USPs and VH Host
> > > > +	 * Bridges. The Endpoint's component register mappings are
> > > > +	 * located in the cxlds.
> > > > +	 */
> > > > +	if (is_cxl_endpoint(port)) {
> > > > +		struct cxl_memdev *memdev = to_cxl_memdev(port->uport_dev);
> > > > +
> > > > +		return &memdev->cxlds->comp_map;
> > > > +	}
> > > > +
> > > > +	return &port->comp_map;
> > > > +}
> > > 
> > > This was the function I was hoping would disappear in the new version.
> > > cxl_pci and cxl_port care about different register blocks and have
> > > different mapping lifetimes. I think that justifies having the
> > > endpoint->comp_map be valid for everything that the cxl_port driver
> > > cares about even though it duplicates some informatiom from
> > > cxlds->comp_map.
> > 
> > In the interest of time I went ahead and reflowed this patch to the
> > below and it is passing my tests. I also noticed some other @dev vs
> > @host confusion in some of the previous register conversion so perhaps I
> > should just send out v11 with this all rolled together...
> 
> just a quick update here.
> 
> We were going to send v11 a couple of days ago but I found an issue
> during testing, see below. If you don't mind I will send it out next
> week with a fix for that included.

Sounds good, I will wait for that update. I had found a couple more
issues as well, like come some confusion on the @host for cxl_dport
register mapping. I will send just that out so you can incorporate.

There was also the feedback on moving register mapping calls out of the
__devm_cxl_add_dport() and into the driver that makes use of them.
Similar for root port interrupt enabling.

> 
> > 
> > -- >8 --
> > Subject: cxl/hdm: Use stored Component Register mappings to map HDM decoder capability
> > 
> > From: Robert Richter <rrichter@xxxxxxx>
> > 
> > Now, that the Component Register mappings are stored, use them to
> > enable and map the HDM decoder capabilities. The Component Registers
> > do not need to be probed again for this, remove probing code.
> > 
> > The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
> > Endpoint's component register mappings are located in the cxlds and
> > else in the port's structure. Duplicate the cxlds->comp_map in
> > port->comp_map for endpoint ports.
> > 
> > Signed-off-by: Robert Richter <rrichter@xxxxxxx>
> > Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
> > Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > [rework to drop cxl_port_get_comp_map()]
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > ---
> >  drivers/cxl/core/hdm.c  |   48 +++++++++++++++++++----------------------------
> >  drivers/cxl/core/port.c |   29 ++++++++++++++++++++++------
> >  drivers/cxl/mem.c       |    5 ++---
> >  3 files changed, 43 insertions(+), 39 deletions(-)
> 
> Patch look good to me.
> 
> I have a similar implementation, but did that with a
> cxl_port_clone_regs() function in cxl_endpoint_port_probe(). I can
> take this version instead.
> 
> During testing I found an issue freeing IO resources with devm for RCH
> mode. The endpoint is not removed if the cxl_mem driver is
> unbound. Then, the resources of the endpoint that also holds the IO
> mappings are not freed. A subsequent IO map fails when rebinding the
> driver again. It looks like cxl_mem_find_port() is broken for RCDs
> preventing the port from being autoremoved. I am working on a fix for
> this and will test the whole series again.

Oh, I see that bug, yes, good find. The cxl-topology.sh test is only
checking for endpoint port removal on parent port removal, but RCDs do
not have the same port interaction when the cxl_mem driver detaches.

I expect you found that delete_enpoint() is the problem in that it
assumes the parent_port has a driver, which for RCH it does not because
that's the CXL root port.

In any event, glad to see this moving forward, and glad to let you
drive the next version.