Re: [PATCH 2/3] cxl/region: Clarify when a cxld->commit() callback is mandatory

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

On Fri, 16 Dec 2022 17:33:38 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Both cxl_switch_decoders() and cxl_endpoint_decoders() are considered by
> cxl_region_decode_commit(). Flag cases where cxl_switch_decoders with
> multiple targets, or cxl_endpoint_decoders do not have a commit callback
> set. The switch case is unlikely to happen since switches are only
> enumerated by the CXL core, but the endpoint case may support decoders
> defined by drivers outside of drivers/cxl, like accerator drivers.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Seems reasonable though (just for consistency with earlier discussions)
I don't like assumption that nr->targets is the right thing to decide on.
It's fine to have decoders in single target case and if we do
they should be committed / not registered as pass through decoders etc.

Hmm. I've never tested the single downstream CXL switch port case.
Similar to HB in that case HDM decoders are optional. I've no idea
what we do about that corner currently.  One to add to my tests ;)


Anyhow, this is fine
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>




> ---
>  drivers/cxl/core/region.c |   19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c11a6ab5e48d..60828d01972a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -156,6 +156,22 @@ static int cxl_region_decode_reset(struct cxl_region *cxlr, int count)
>  	return 0;
>  }
>  
> +static int commit_decoder(struct cxl_decoder *cxld)
> +{
> +	struct cxl_switch_decoder *cxlsd = NULL;
> +
> +	if (cxld->commit)
> +		return cxld->commit(cxld);
> +
> +	if (is_switch_decoder(&cxld->dev))
> +		cxlsd = to_cxl_switch_decoder(&cxld->dev);
> +
> +	if (dev_WARN_ONCE(&cxld->dev, !cxlsd || cxlsd->nr_targets > 1,
> +			  "->commit() is required\n"))
> +		return -ENXIO;
> +	return 0;
> +}
> +
>  static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
> @@ -174,8 +190,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  		     iter = to_cxl_port(iter->dev.parent)) {
>  			cxl_rr = cxl_rr_load(iter, cxlr);
>  			cxld = cxl_rr->decoder;
> -			if (cxld->commit)
> -				rc = cxld->commit(cxld);
> +			rc = commit_decoder(cxld);
>  			if (rc)
>  				break;
>  		}
>