On 2022-02-02 10:13, Lorenzo Pieralisi wrote:
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.
Sure, I did actually start down that route, but then thought maybe the
existing style gave an excuse to be clever :)
I'll respin it into the style of arm_smmu_v3_count_resources() for v2.
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.
Even if the resource allocation was factored out into every
implementation, there's still some unavoidable overlap between knowing
which resources you want to allocate beforehand and knowing which
resources you need to initialise afterwards. Honestly I think the
current shape of things is the best compromise already.
}
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).
In this case, the price of not repeating "node->revision > 0" would be
"res_cnt == 3 || (res_cnt == 2 && !pmcg->overflow_gsiv)" - definitely
not an improvement to my eye.
Cheers,
Robin.
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
