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@xxxxxxxxxx>
> Fixes: 24e516049360 ("ACPI/IORT: Add support for PMCG")
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> ---
> 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
>
