Re: [PATCH] cxl/acpi: Warn on unsupported platform config detection

[Dan Williams <dan.j.williams@intel.com>]

Fabio M. De Francesco wrote:
> Each Host Bridge instance has a corresponding CXL Host Bridge Structure
> (CHBS) ACPI table that identifies its capabilities. CHBS tables can be
> two types: RCRB and CHBCR.
> 
> If a Host Bridge is attached to a device that is operating in Restricted
> CXL Device Mode (RCD), BIOS publishes an RCRB with the base address of
> registers that describe its capabilities.
> 
> However, the new (CXL 2.0+) Component registers (e.g., Extended Security
> Capability), can only be accessed by means of a base address published
> with a CHBCR.
> 
> An algorithm to locate a CHBCR associated with an RCRB would be too
> invasive to land without some concrete motivation.
> 
> Therefore, just print a message to inform of unsupported config.
> 
> Count how many different CHBS "Version" types are detected by
> cxl_get_chbs_iter(). Then make cxl_get_chbs() print a warning if that sum
> is greater than 1.
> 
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> ---
>  drivers/cxl/acpi.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 571069863c62..9e226a65a5ea 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -482,6 +482,7 @@ struct cxl_chbs_context {
>  	unsigned long long uid;
>  	resource_size_t base;
>  	u32 cxl_version;
> +	int count;
>  };
>  
>  static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg,
> @@ -490,10 +491,17 @@ static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg,
>  	struct cxl_chbs_context *ctx = arg;
>  	struct acpi_cedt_chbs *chbs;
>  
> -	if (ctx->base != CXL_RESOURCE_NONE)
> +	chbs = (struct acpi_cedt_chbs *) header;
> +
> +	if (chbs->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11 &&
> +	    chbs->length != CXL_RCRB_SIZE)
>  		return 0;
>  
> -	chbs = (struct acpi_cedt_chbs *) header;
> +	if (ctx->cxl_version != chbs->cxl_version)
> +		ctx->count++;
> +
> +	if (ctx->base != CXL_RESOURCE_NONE)
> +		return 0;
>  
>  	if (ctx->uid != chbs->uid)
>  		return 0;
> @@ -502,10 +510,6 @@ static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg,
>  	if (!chbs->base)
>  		return 0;
>  
> -	if (chbs->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11 &&
> -	    chbs->length != CXL_RCRB_SIZE)
> -		return 0;
> -
>  	ctx->base = chbs->base;
>  
>  	return 0;
> @@ -533,6 +537,10 @@ static int cxl_get_chbs(struct device *dev, struct acpi_device *hb,
>  
>  	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs_iter, ctx);
>  
> +	if (ctx->count > 1)
> +		/* Disclaim eRCD support given some component register may only be found via CHBCR */
> +		dev_info(dev, "Unsupported platform config, mixed Virtual Host and Restricted CXL Host hierarchy.");

I believe this is already queued, but my personal preference is that
multiline statements include brackets, or move the comment above the "if
()", so either:

	/* Disclaim eRCD support given some component register may only be found via CHBCR */
	if (ctx->count > 1)
		dev_info(dev, "Unsupported platform config, mixed Virtual Host and Restricted CXL Host hierarchy.");

...or:

	if (ctx->count > 1) {
		/* Disclaim eRCD support given some component register may only be found via CHBCR */
		dev_info(dev, "Unsupported platform config, mixed Virtual Host and Restricted CXL Host hierarchy.");
	}

...but don't spin the patch just for that fixup.