Sanity checking all the GICC tables for same interrupt number, and ensuring
a homogeneous ACPI based machine, could be used for other platform devices
as well. Hence this refactors arm_spe_acpi_register_device() into a common
helper arm_acpi_register_pmu_device().
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Co-developed-by: Will Deacon <will@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
drivers/perf/arm_pmu_acpi.c | 110 +++++++++++++++++++++++-------------
1 file changed, 70 insertions(+), 40 deletions(-)
diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
index 90815ad762eb..d9d5a7bbb92f 100644
--- a/drivers/perf/arm_pmu_acpi.c
+++ b/drivers/perf/arm_pmu_acpi.c
@@ -70,6 +70,68 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
}
#if IS_ENABLED(CONFIG_ARM_SPE_PMU)
+static int
+arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
+ u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
+{
+ int cpu, hetid, irq, ret;
+ bool matched = false;
+ u16 gsi = 0;
+
+ /*
+ * Ensure that platform device must have IORESOURCE_IRQ
+ * resource to hold gsi interrupt.
+ */
+ if (pdev->num_resources != 1)
+ return -ENXIO;
+
+ if (pdev->resource[0].flags != IORESOURCE_IRQ)
+ return -ENXIO;
+
+ /*
+ * Sanity check all the GICC tables for the same interrupt
+ * number. For now, only support homogeneous ACPI machines.
+ */
+ for_each_possible_cpu(cpu) {
+ struct acpi_madt_generic_interrupt *gicc;
+ u16 this_gsi;
+
+ gicc = acpi_cpu_get_madt_gicc(cpu);
+ if (gicc->header.length < len)
+ return matched ? -ENXIO : 0;
+
+ this_gsi = parse_gsi(gicc);
+ if (!this_gsi)
+ return matched ? -ENXIO : 0;
+
+ if (!matched) {
+ hetid = find_acpi_cpu_topology_hetero_id(cpu);
+ gsi = this_gsi;
+ matched = true;
+ } else if (hetid != find_acpi_cpu_topology_hetero_id(cpu) ||
+ gsi != this_gsi) {
+ pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
+ return -ENXIO;
+ }
+ }
+
+ irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
+ if (irq < 0) {
+ pr_warn("ACPI: %s Unable to register interrupt: %d\n", pdev->name, gsi);
+ return -ENXIO;
+ }
+
+ pdev->resource[0].start = irq;
+ ret = platform_device_register(pdev);
+ if (ret < 0) {
+ pr_warn("ACPI: %s: Unable to register device\n", pdev->name);
+ acpi_unregister_gsi(gsi);
+ }
+ return ret;
+}
+#endif
+
+#ifdef CONFIG_ARM_SPE_PMU
static struct resource spe_resources[] = {
{
/* irq */
@@ -84,6 +146,11 @@ static struct platform_device spe_dev = {
.num_resources = ARRAY_SIZE(spe_resources)
};
+static u16 arm_spe_parse_gsi(struct acpi_madt_generic_interrupt *gicc)
+{
+ return gicc->spe_interrupt;
+}
+
/*
* For lack of a better place, hook the normal PMU MADT walk
* and create a SPE device if we detect a recent MADT with
@@ -91,47 +158,10 @@ static struct platform_device spe_dev = {
*/
static void arm_spe_acpi_register_device(void)
{
- int cpu, hetid, irq, ret;
- bool first = true;
- u16 gsi = 0;
-
- /*
- * Sanity check all the GICC tables for the same interrupt number.
- * For now, we only support homogeneous ACPI/SPE machines.
- */
- for_each_possible_cpu(cpu) {
- struct acpi_madt_generic_interrupt *gicc;
-
- gicc = acpi_cpu_get_madt_gicc(cpu);
- if (gicc->header.length < ACPI_MADT_GICC_SPE)
- return;
-
- if (first) {
- gsi = gicc->spe_interrupt;
- if (!gsi)
- return;
- hetid = find_acpi_cpu_topology_hetero_id(cpu);
- first = false;
- } else if ((gsi != gicc->spe_interrupt) ||
- (hetid != find_acpi_cpu_topology_hetero_id(cpu))) {
- pr_warn("ACPI: SPE must be homogeneous\n");
- return;
- }
- }
-
- irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE,
- ACPI_ACTIVE_HIGH);
- if (irq < 0) {
- pr_warn("ACPI: SPE Unable to register interrupt: %d\n", gsi);
- return;
- }
-
- spe_resources[0].start = irq;
- ret = platform_device_register(&spe_dev);
- if (ret < 0) {
+ int ret = arm_acpi_register_pmu_device(&spe_dev, ACPI_MADT_GICC_SPE,
+ arm_spe_parse_gsi);
+ if (ret)
pr_warn("ACPI: SPE: Unable to register device\n");
- acpi_unregister_gsi(gsi);
- }
}
#else
static inline void arm_spe_acpi_register_device(void)
--
2.25.1On Tue, Aug 01, 2023 at 03:10:49PM +0530, Anshuman Khandual wrote:
> Sanity checking all the GICC tables for same interrupt number, and ensuring
> a homogeneous ACPI based machine, could be used for other platform devices
> as well. Hence this refactors arm_spe_acpi_register_device() into a common
> helper arm_acpi_register_pmu_device().
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Co-developed-by: Will Deacon <will@kernel.org>
> Signed-off-by: Will Deacon <will@kernel.org>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> drivers/perf/arm_pmu_acpi.c | 110 +++++++++++++++++++++++-------------
> 1 file changed, 70 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
> index 90815ad762eb..d9d5a7bbb92f 100644
> --- a/drivers/perf/arm_pmu_acpi.c
> +++ b/drivers/perf/arm_pmu_acpi.c
> @@ -70,6 +70,68 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
> }
>
> #if IS_ENABLED(CONFIG_ARM_SPE_PMU)
> +static int
> +arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
> + u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
> +{
> + int cpu, hetid, irq, ret;
> + bool matched = false;
> + u16 gsi = 0;
> +
> + /*
> + * Ensure that platform device must have IORESOURCE_IRQ
> + * resource to hold gsi interrupt.
> + */
> + if (pdev->num_resources != 1)
> + return -ENXIO;
> +
> + if (pdev->resource[0].flags != IORESOURCE_IRQ)
> + return -ENXIO;
> +
> + /*
> + * Sanity check all the GICC tables for the same interrupt
> + * number. For now, only support homogeneous ACPI machines.
> + */
> + for_each_possible_cpu(cpu) {
> + struct acpi_madt_generic_interrupt *gicc;
> + u16 this_gsi;
> +
> + gicc = acpi_cpu_get_madt_gicc(cpu);
> + if (gicc->header.length < len)
> + return matched ? -ENXIO : 0;
> +
> + this_gsi = parse_gsi(gicc);
> + if (!this_gsi)
> + return matched ? -ENXIO : 0;
I think you can push this check into the conditional below...
> +
> + if (!matched) {
> + hetid = find_acpi_cpu_topology_hetero_id(cpu);
... i.e. add a:
if (!this_gsi)
return -ENXIO;
here.
> + gsi = this_gsi;
> + matched = true;
And then, come to think of it, can we get rid of 'matched' altogether?
Since a gsi of 0 is treated as invalid, we could just check that instead,
no? So this becomes:
gicc = acpi_cpu_get_madt_gicc(cpu);
if (gicc->header.length < len)
return gsi ? -ENXIO : 0;
this_hetid = find_acpi_cpu_topology_hetero_id(cpu);
this_gsi = parse_gsi(gicc);
if (!gsi) {
if (!this_gsi)
return -ENXIO;
gsi = this_gsi;
hetid = this_hetid;
} else if (hetid != this_hetid || gsi != this_gsi) {
pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
return -ENXIO;
}
What do you reckon?
Will
On 8/1/23 20:19, Will Deacon wrote:
> On Tue, Aug 01, 2023 at 03:10:49PM +0530, Anshuman Khandual wrote:
>> Sanity checking all the GICC tables for same interrupt number, and ensuring
>> a homogeneous ACPI based machine, could be used for other platform devices
>> as well. Hence this refactors arm_spe_acpi_register_device() into a common
>> helper arm_acpi_register_pmu_device().
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Co-developed-by: Will Deacon <will@kernel.org>
>> Signed-off-by: Will Deacon <will@kernel.org>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> drivers/perf/arm_pmu_acpi.c | 110 +++++++++++++++++++++++-------------
>> 1 file changed, 70 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
>> index 90815ad762eb..d9d5a7bbb92f 100644
>> --- a/drivers/perf/arm_pmu_acpi.c
>> +++ b/drivers/perf/arm_pmu_acpi.c
>> @@ -70,6 +70,68 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
>> }
>>
>> #if IS_ENABLED(CONFIG_ARM_SPE_PMU)
>> +static int
>> +arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
>> + u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
>> +{
>> + int cpu, hetid, irq, ret;
>> + bool matched = false;
>> + u16 gsi = 0;
>> +
>> + /*
>> + * Ensure that platform device must have IORESOURCE_IRQ
>> + * resource to hold gsi interrupt.
>> + */
>> + if (pdev->num_resources != 1)
>> + return -ENXIO;
>> +
>> + if (pdev->resource[0].flags != IORESOURCE_IRQ)
>> + return -ENXIO;
>> +
>> + /*
>> + * Sanity check all the GICC tables for the same interrupt
>> + * number. For now, only support homogeneous ACPI machines.
>> + */
>> + for_each_possible_cpu(cpu) {
>> + struct acpi_madt_generic_interrupt *gicc;
>> + u16 this_gsi;
>> +
>> + gicc = acpi_cpu_get_madt_gicc(cpu);
>> + if (gicc->header.length < len)
>> + return matched ? -ENXIO : 0;
>> +
>> + this_gsi = parse_gsi(gicc);
>> + if (!this_gsi)
>> + return matched ? -ENXIO : 0;
>
> I think you can push this check into the conditional below...
>
>> +
>> + if (!matched) {
>> + hetid = find_acpi_cpu_topology_hetero_id(cpu);
>
> ... i.e. add a:
>
> if (!this_gsi)
> return -ENXIO;
>
> here.
>
>> + gsi = this_gsi;
>> + matched = true;
>
> And then, come to think of it, can we get rid of 'matched' altogether?
> Since a gsi of 0 is treated as invalid, we could just check that instead,
> no? So this becomes:
>
>
> gicc = acpi_cpu_get_madt_gicc(cpu);
> if (gicc->header.length < len)
> return gsi ? -ENXIO : 0;
>
> this_hetid = find_acpi_cpu_topology_hetero_id(cpu);
> this_gsi = parse_gsi(gicc);
> if (!gsi) {
> if (!this_gsi)
> return -ENXIO;
Unlike before, this returns -ENXIO on the very first instance itself without
a match. Previously this returned 0 in such cases. But I guess that is okay.
> gsi = this_gsi;
> hetid = this_hetid;
> } else if (hetid != this_hetid || gsi != this_gsi) {
> pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
> return -ENXIO;
> }
>
>
> What do you reckon?
This makes sense, and the modified function will look something like
arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
{
int cpu, irq, ret;
u16 gsi = 0;
/*
* Ensure that platform device must have IORESOURCE_IRQ
* resource to hold gsi interrupt.
*/
if (pdev->num_resources != 1)
return -ENXIO;
if (pdev->resource[0].flags != IORESOURCE_IRQ)
return -ENXIO;
/*
* Sanity check all the GICC tables for the same interrupt
* number. For now, only support homogeneous ACPI machines.
*/
for_each_possible_cpu(cpu) {
struct acpi_madt_generic_interrupt *gicc;
int this_hetid, hetid;
u16 this_gsi;
gicc = acpi_cpu_get_madt_gicc(cpu);
if (gicc->header.length < len)
return gsi ? -ENXIO : 0;
this_hetid = find_acpi_cpu_topology_hetero_id(cpu);
this_gsi = parse_gsi(gicc);
if (!gsi) {
if (!this_gsi)
return -ENXIO;
hetid = this_hetid;
gsi = this_gsi;
} else if (hetid != this_hetid || gsi != this_gsi) {
pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
return -ENXIO;
}
}
irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
if (irq < 0) {
pr_warn("ACPI: %s Unable to register interrupt: %d\n", pdev->name, gsi);
return -ENXIO;
}
pdev->resource[0].start = irq;
ret = platform_device_register(pdev);
if (ret < 0) {
pr_warn("ACPI: %s: Unable to register device\n", pdev->name);
acpi_unregister_gsi(gsi);
}
return ret;
}
© 2016 - 2026 Red Hat, Inc.