Message-ID: <ZQTSRk6gOg41ybG1@rric.localdomain>
Date:   Fri, 15 Sep 2023 23:53:10 +0200
From:   Robert Richter <rrichter@....com>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     Terry Bowman <terry.bowman@....com>, alison.schofield@...el.com,
        vishal.l.verma@...el.com, ira.weiny@...el.com, bwidawsk@...nel.org,
        dave.jiang@...el.com, Jonathan.Cameron@...wei.com,
        linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org,
        bhelgaas@...gle.com
Subject: Re: [PATCH v10 04/15] cxl/hdm: Use stored Component Register
 mappings to map HDM decoder capability

Dan,

On 14.09.23 17:15:44, Dan Williams wrote:
> Dan Williams wrote:
> > Terry Bowman wrote:
> > > From: Robert Richter <rrichter@....com>
> > > 
> > > 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@....com>
> > > Signed-off-by: Terry Bowman <terry.bowman@....com>
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > > Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> > > ---
> > >  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.

> 
> -- >8 --
> Subject: cxl/hdm: Use stored Component Register mappings to map HDM decoder capability
> 
> From: Robert Richter <rrichter@....com>
> 
> 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@....com>
> Signed-off-by: Terry Bowman <terry.bowman@....com>
> Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> [rework to drop cxl_port_get_comp_map()]
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> ---
>  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.

Thanks,

-Robert

> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 11d9971f3e8c..ced7801516d2 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -81,26 +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 = {
> -		.host = &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, regs, BIT(CXL_CM_CAP_CAP_ID_HDM));
> -}
> -
>  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>  {
>  	struct cxl_hdm *cxlhdm;
> @@ -155,7 +135,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>  {
>  	struct device *dev = &port->dev;
>  	struct cxl_hdm *cxlhdm;
> -	void __iomem *crb;
> +	struct cxl_register_map *comp_map;
>  	int rc;
>  
>  	cxlhdm = devm_kzalloc(dev, sizeof(*cxlhdm), GFP_KERNEL);
> @@ -164,19 +144,29 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>  	cxlhdm->port = port;
>  	dev_set_drvdata(dev, cxlhdm);
>  
> -	crb = ioremap(port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
> -	if (!crb && info && info->mem_enabled) {
> -		cxlhdm->decoder_count = info->ranges;
> -		return cxlhdm;
> -	} else if (!crb) {
> +	comp_map = &port->comp_map;
> +	if (comp_map->resource == CXL_RESOURCE_NONE) {
> +		if (info && info->mem_enabled) {
> +			cxlhdm->decoder_count = info->ranges;
> +			return cxlhdm;
> +		}
> +		WARN_ON(1);
>  		dev_err(dev, "No component registers mapped\n");
>  		return ERR_PTR(-ENXIO);
>  	}
>  
> -	rc = map_hdm_decoder_regs(port, crb, &cxlhdm->regs);
> -	iounmap(crb);
> -	if (rc)
> +	if (!comp_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 ERR_PTR(-ENODEV);
> +	}
> +
> +	rc = cxl_map_component_regs(comp_map, &cxlhdm->regs,
> +				    BIT(CXL_CM_CAP_CAP_ID_HDM));
> +	if (rc) {
> +		dev_dbg(dev, "Failed to map HDM capability.\n");
>  		return ERR_PTR(rc);
> +	}
>  
>  	parse_hdm_decoder_caps(cxlhdm);
>  	if (cxlhdm->decoder_count == 0) {
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 3732925162e4..64fcb5b22372 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -741,16 +741,31 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
>  		return port;
>  
>  	dev = &port->dev;
> -	if (is_cxl_memdev(uport_dev))
> +	if (is_cxl_memdev(uport_dev)) {
> +		struct cxl_memdev *cxlmd = to_cxl_memdev(uport_dev);
> +		struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
>  		rc = dev_set_name(dev, "endpoint%d", port->id);
> -	else if (parent_dport)
> +		if (rc)
> +			goto err;
> +
> +		/*
> +		 * The endpoint driver already enumerated the component and RAS
> +		 * registers. Reuse that enumeration while prepping them to be
> +		 * mapped by the cxl_port driver.
> +		 */
> +		port->comp_map = cxlds->comp_map;
> +		port->comp_map.host = &port->dev;
> +	} else if (parent_dport) {
>  		rc = dev_set_name(dev, "port%d", port->id);
> -	else
> -		rc = dev_set_name(dev, "root%d", port->id);
> -	if (rc)
> -		goto err;
> +		if (rc)
> +			goto err;
>  
> -	rc = cxl_port_setup_regs(port, component_reg_phys);
> +		rc = cxl_port_setup_regs(port, component_reg_phys);
> +		if (rc)
> +			goto err;
> +	} else
> +		rc = dev_set_name(dev, "root%d", port->id);
>  	if (rc)
>  		goto err;
>  
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 317c7548e4e9..04107058739b 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -49,7 +49,6 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>  				 struct cxl_dport *parent_dport)
>  {
>  	struct cxl_port *parent_port = parent_dport->port;
> -	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_port *endpoint, *iter, *down;
>  	int rc;
>  
> @@ -65,8 +64,8 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>  		ep->next = down;
>  	}
>  
> -	endpoint = devm_cxl_add_port(host, &cxlmd->dev,
> -				     cxlds->component_reg_phys,
> +	/* Note: endpoint port component registers are derived from @cxlds */
> +	endpoint = devm_cxl_add_port(host, &cxlmd->dev, CXL_RESOURCE_NONE,
>  				     parent_dport);
>  	if (IS_ERR(endpoint))
>  		return PTR_ERR(endpoint);

Powered by blists - more mailing lists