On 08/08/2023 09:22, 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 | 105 ++++++++++++++++++++++-------------- 1 file changed, 65 insertions(+), 40 deletions(-)
diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c index 90815ad762eb..72454bef2a70 100644 --- a/drivers/perf/arm_pmu_acpi.c +++ b/drivers/perf/arm_pmu_acpi.c @@ -69,6 +69,63 @@ static void arm_pmu_acpi_unregister_irq(int cpu) acpi_unregister_gsi(gsi); } +static int __maybe_unused +arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
+{
- int cpu, this_hetid, hetid, irq, ret;
- u16 this_gsi, 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;
gicc = acpi_cpu_get_madt_gicc(cpu);
if (gicc->header.length < len)
return gsi ? -ENXIO : 0;
this_gsi = parse_gsi(gicc);
if (!this_gsi)
return gsi ? -ENXIO : 0;
this_hetid = find_acpi_cpu_topology_hetero_id(cpu);
if (!gsi) {
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;
A postivie return value here could confuse the caller. Also, with my comment below, we don't really need to return something from here.
+}
- #if IS_ENABLED(CONFIG_ARM_SPE_PMU) static struct resource spe_resources[] = { {
@@ -84,6 +141,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 +153,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");
With this change, a system without SPE interrupt description always generates the above message. Is this intended ? Could we not drop the above message as all the other possible error scenarios are reported. We could simply make the above helper void, see my comment above.
Suzuki
acpi_unregister_gsi(gsi);
- } } #else static inline void arm_spe_acpi_register_device(void)