Re: [PATCH] ACPI/IORT: Check node revision for PMCG resources

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Mon, Jan 31, 2022 at 03:03:24PM +0000, Robin Murphy wrote:
> The original version of the IORT PMCG definition had an oversight
> wherein there was no way to describe the second register page for an
> implementation using the recommended RELOC_CTRS feature. Although the
> spec was fixed, and the final patches merged to ACPICA and Linux written
> against the new version, it seems that some old firmware based on the
> original revision has survived and turned up in the wild.
> 
> Add a check for the original PMCG definition, and avoid filling in the
> second memory resource with nonsense if so. Otherwise it is likely that
> something horrible will happen when the PMCG driver attempts to probe.
> 
> Reported-by: Michael Petlan <mpetlan@redhat.com>
> Fixes: 24e516049360 ("ACPI/IORT: Add support for PMCG")
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/acpi/arm64/iort.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 3b23fb775ac4..aaa1f0411a5a 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -1344,16 +1344,17 @@ static int __init arm_smmu_v3_pmcg_count_resources(struct acpi_iort_node *node)
>  	pmcg = (struct acpi_iort_pmcg *)node->node_data;
>  
>  	/*
> -	 * There are always 2 memory resources.
> -	 * If the overflow_gsiv is present then add that for a total of 3.
> +	 * There should normally be 2 memory resources, but apparently the
> +	 * oversight from IORT rev. C managed to escape into the wild.
>  	 */
> -	return pmcg->overflow_gsiv ? 3 : 2;
> +	return 1 + (node->revision > 0) + (pmcg->overflow_gsiv != 0);

It is compact but (nit) I'd rather use a construct like:

if (node->revision > 0)
	res_cnt++;

with a comment explaining it so that we can remember why the node
revision implies an additional resource.

Actually - I noticed that the logic in .dev_count_resources() and
dev_init_resources() is somewhat duplicated - maybe we can add a
resource_count param to dev_init_resources() but I am not sure
it will improve things much.

>  }
>  
>  static void __init arm_smmu_v3_pmcg_init_resources(struct resource *res,
>  						   struct acpi_iort_node *node)
>  {
>  	struct acpi_iort_pmcg *pmcg;
> +	int n = 1;
>  
>  	/* Retrieve PMCG specific data */
>  	pmcg = (struct acpi_iort_pmcg *)node->node_data;
> @@ -1361,13 +1362,15 @@ static void __init arm_smmu_v3_pmcg_init_resources(struct resource *res,
>  	res[0].start = pmcg->page0_base_address;
>  	res[0].end = pmcg->page0_base_address + SZ_4K - 1;
>  	res[0].flags = IORESOURCE_MEM;
> -	res[1].start = pmcg->page1_base_address;
> -	res[1].end = pmcg->page1_base_address + SZ_4K - 1;
> -	res[1].flags = IORESOURCE_MEM;
> +	if (node->revision > 0) {
> +		res[n].start = pmcg->page1_base_address;
> +		res[n].end = pmcg->page1_base_address + SZ_4K - 1;
> +		res[n++].flags = IORESOURCE_MEM;
> +	}

See above. If we knew the number of resource we could avoid repeating
node->revision > 0 check but I don't think it would improve things
anyway (ie we know how many resources we are allocating but we still
need to check why a resource has to be added - eg node->revision > 0).

Thanks,
Lorenzo

>  	if (pmcg->overflow_gsiv)
>  		acpi_iort_register_irq(pmcg->overflow_gsiv, "overflow",
> -				       ACPI_EDGE_SENSITIVE, &res[2]);
> +				       ACPI_EDGE_SENSITIVE, &res[n]);
>  }
>  
>  static struct acpi_platform_list pmcg_plat_info[] __initdata = {
> -- 
> 2.28.0.dirty
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel