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)