Re: [PATCH v2 12/20] cxl/port: Split endpoint and switch port probe

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 10 Feb 2023 01:06:33 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan points out that the shared code between the switch and endpoint
> case is small. Before adding another is_cxl_endpoint() conditional,
> just split the two cases.
> 
> Rather than duplicate the "Couldn't enumerate decoders" error message
> take the opportunity to improve the error messages in
> devm_cxl_enumerate_decoders().
> 
> Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Link: http://lore.kernel.org/r/20230208170724.000067ec@Huawei.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
LGTM.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/hdm.c |   11 ++++++--
>  drivers/cxl/port.c     |   69 +++++++++++++++++++++++++++---------------------
>  2 files changed, 47 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index dcc16d7cb8f3..a0891c3464f1 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -826,7 +826,8 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
>  			cxled = cxl_endpoint_decoder_alloc(port);
>  			if (IS_ERR(cxled)) {
>  				dev_warn(&port->dev,
> -					 "Failed to allocate the decoder\n");
> +					 "Failed to allocate decoder%d.%d\n",
> +					 port->id, i);
>  				return PTR_ERR(cxled);
>  			}
>  			cxld = &cxled->cxld;
> @@ -836,7 +837,8 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
>  			cxlsd = cxl_switch_decoder_alloc(port, target_count);
>  			if (IS_ERR(cxlsd)) {
>  				dev_warn(&port->dev,
> -					 "Failed to allocate the decoder\n");
> +					 "Failed to allocate decoder%d.%d\n",
> +					 port->id, i);
>  				return PTR_ERR(cxlsd);
>  			}
>  			cxld = &cxlsd->cxld;
> @@ -844,13 +846,16 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
>  
>  		rc = init_hdm_decoder(port, cxld, target_map, hdm, i, &dpa_base);
>  		if (rc) {
> +			dev_warn(&port->dev,
> +				 "Failed to initialize decoder%d.%d\n",
> +				 port->id, i);
>  			put_device(&cxld->dev);
>  			return rc;
>  		}
>  		rc = add_hdm_decoder(port, cxld, target_map);
>  		if (rc) {
>  			dev_warn(&port->dev,
> -				 "Failed to add decoder to port\n");
> +				 "Failed to add decoder%d.%d\n", port->id, i);
>  			return rc;
>  		}
>  	}
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 5453771bf330..a8d46a67b45e 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -30,55 +30,64 @@ static void schedule_detach(void *cxlmd)
>  	schedule_cxl_memdev_detach(cxlmd);
>  }
>  
> -static int cxl_port_probe(struct device *dev)
> +static int cxl_switch_port_probe(struct cxl_port *port)
>  {
> -	struct cxl_port *port = to_cxl_port(dev);
>  	struct cxl_hdm *cxlhdm;
>  	int rc;
>  
> +	rc = devm_cxl_port_enumerate_dports(port);
> +	if (rc < 0)
> +		return rc;
>  
> -	if (!is_cxl_endpoint(port)) {
> -		rc = devm_cxl_port_enumerate_dports(port);
> -		if (rc < 0)
> -			return rc;
> -		if (rc == 1)
> -			return devm_cxl_add_passthrough_decoder(port);
> -	}
> +	if (rc == 1)
> +		return devm_cxl_add_passthrough_decoder(port);
>  
>  	cxlhdm = devm_cxl_setup_hdm(port);
>  	if (IS_ERR(cxlhdm))
>  		return PTR_ERR(cxlhdm);
>  
> -	if (is_cxl_endpoint(port)) {
> -		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);
> -		struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	return devm_cxl_enumerate_decoders(cxlhdm);
> +}
>  
> -		/* Cache the data early to ensure is_visible() works */
> -		read_cdat_data(port);
> +static int cxl_endpoint_port_probe(struct cxl_port *port)
> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_hdm *cxlhdm;
> +	int rc;
> +
> +	cxlhdm = devm_cxl_setup_hdm(port);
> +	if (IS_ERR(cxlhdm))
> +		return PTR_ERR(cxlhdm);
>  
> -		get_device(&cxlmd->dev);
> -		rc = devm_add_action_or_reset(dev, schedule_detach, cxlmd);
> -		if (rc)
> -			return rc;
> +	/* Cache the data early to ensure is_visible() works */
> +	read_cdat_data(port);
>  
> -		rc = cxl_hdm_decode_init(cxlds, cxlhdm);
> -		if (rc)
> -			return rc;
> +	get_device(&cxlmd->dev);
> +	rc = devm_add_action_or_reset(&port->dev, schedule_detach, cxlmd);
> +	if (rc)
> +		return rc;
>  
> -		rc = cxl_await_media_ready(cxlds);
> -		if (rc) {
> -			dev_err(dev, "Media not active (%d)\n", rc);
> -			return rc;
> -		}
> -	}
> +	rc = cxl_hdm_decode_init(cxlds, cxlhdm);
> +	if (rc)
> +		return rc;
>  
> -	rc = devm_cxl_enumerate_decoders(cxlhdm);
> +	rc = cxl_await_media_ready(cxlds);
>  	if (rc) {
> -		dev_err(dev, "Couldn't enumerate decoders (%d)\n", rc);
> +		dev_err(&port->dev, "Media not active (%d)\n", rc);
>  		return rc;
>  	}
>  
> -	return 0;
> +	return devm_cxl_enumerate_decoders(cxlhdm);
> +}
> +
> +static int cxl_port_probe(struct device *dev)
> +{
> +	struct cxl_port *port = to_cxl_port(dev);
> +
> +	if (is_cxl_endpoint(port))
> +		return cxl_endpoint_port_probe(port);
> +	return cxl_switch_port_probe(port);
>  }
>  
>  static ssize_t CDAT_read(struct file *filp, struct kobject *kobj,
>