[V3,1/4] arm_pmu: acpi: Refactor arm_spe_acpi_register_device()
Commit Message
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 | 107 ++++++++++++++++++++++--------------
1 file changed, 67 insertions(+), 40 deletions(-)
Comments
On 8/3/23 11:26, Anshuman Khandual wrote:
> + /*
> + * 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;
Hello Will,
Moved parse_gsi() return code checking to its original place just to
make it similar in semantics to existing 'gicc->header.length check'.
If 'gsi' is valid i.e atleast a single cpu has been probed, return
-ENXIO indicating mismatch, otherwise just return 0.
- Anshuman
On Thu, Aug 03, 2023 at 11:43:27AM +0530, Anshuman Khandual wrote:
>
>
> On 8/3/23 11:26, Anshuman Khandual wrote:
> > + /*
> > + * 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;
>
> Hello Will,
>
> Moved parse_gsi() return code checking to its original place just to
> make it similar in semantics to existing 'gicc->header.length check'.
> If 'gsi' is valid i.e atleast a single cpu has been probed, return
> -ENXIO indicating mismatch, otherwise just return 0.
Wouldn't that still be the case without the check in this hunk? We'd run
into the homogeneous check and return -ENXIO from there, no?
Will
On 8/4/23 22:09, Will Deacon wrote:
> On Thu, Aug 03, 2023 at 11:43:27AM +0530, Anshuman Khandual wrote:
>>
>>
>> On 8/3/23 11:26, Anshuman Khandual wrote:
>>> + /*
>>> + * 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;
>>
>> Hello Will,
>>
>> Moved parse_gsi() return code checking to its original place just to
>> make it similar in semantics to existing 'gicc->header.length check'.
>> If 'gsi' is valid i.e atleast a single cpu has been probed, return
>> -ENXIO indicating mismatch, otherwise just return 0.
>
> Wouldn't that still be the case without the check in this hunk? We'd run
> into the homogeneous check and return -ENXIO from there, no?
Although the return code will be the same i.e -ENXIO, but not for the same reason.
this_gsi = parse_gsi(gicc);
if (!this_gsi)
return gsi ? -ENXIO : 0;
This returns 0 when IRQ could not be parsed for the first cpu, but returns -ENXIO
for subsequent cpus. Although return code -ENXIO here still indicates IRQ parsing
to have failed.
} else if (hetid != this_hetid || gsi != this_gsi) {
pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
return -ENXIO;
}
This returns -ENXIO when there is a IRQ mismatch. But if the above check is not
there, -ENXIO return code here could not be classified into IRQ parse problem or
mismatch without looking into the IRQ value.
On Mon, Aug 07, 2023 at 11:03:40AM +0530, Anshuman Khandual wrote:
> On 8/4/23 22:09, Will Deacon wrote:
> > On Thu, Aug 03, 2023 at 11:43:27AM +0530, Anshuman Khandual wrote:
> >> On 8/3/23 11:26, Anshuman Khandual wrote:
> >>> + /*
> >>> + * 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;
> >>
> >> Moved parse_gsi() return code checking to its original place just to
> >> make it similar in semantics to existing 'gicc->header.length check'.
> >> If 'gsi' is valid i.e atleast a single cpu has been probed, return
> >> -ENXIO indicating mismatch, otherwise just return 0.
> >
> > Wouldn't that still be the case without the check in this hunk? We'd run
> > into the homogeneous check and return -ENXIO from there, no?
> Although the return code will be the same i.e -ENXIO, but not for the same reason.
>
> this_gsi = parse_gsi(gicc);
> if (!this_gsi)
> return gsi ? -ENXIO : 0;
>
> This returns 0 when IRQ could not be parsed for the first cpu, but returns -ENXIO
> for subsequent cpus. Although return code -ENXIO here still indicates IRQ parsing
> to have failed.
>
> } else if (hetid != this_hetid || gsi != this_gsi) {
> pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
> return -ENXIO;
> }
>
> This returns -ENXIO when there is a IRQ mismatch. But if the above check is not
> there, -ENXIO return code here could not be classified into IRQ parse problem or
> mismatch without looking into the IRQ value.
Sorry, but I don't understand your point here. If any of this fails, there's
going to be some debugging needed to look at the ACPI tables; the only
difference with my suggestion is that you'll get a message indicating that
the devices aren't homogeneous, which I think is helpful.
Will
On 8/8/23 18:51, Will Deacon wrote:
> On Mon, Aug 07, 2023 at 11:03:40AM +0530, Anshuman Khandual wrote:
>> On 8/4/23 22:09, Will Deacon wrote:
>>> On Thu, Aug 03, 2023 at 11:43:27AM +0530, Anshuman Khandual wrote:
>>>> On 8/3/23 11:26, Anshuman Khandual wrote:
>>>>> + /*
>>>>> + * 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;
>>>>
>>>> Moved parse_gsi() return code checking to its original place just to
>>>> make it similar in semantics to existing 'gicc->header.length check'.
>>>> If 'gsi' is valid i.e atleast a single cpu has been probed, return
>>>> -ENXIO indicating mismatch, otherwise just return 0.
>>>
>>> Wouldn't that still be the case without the check in this hunk? We'd run
>>> into the homogeneous check and return -ENXIO from there, no?
>> Although the return code will be the same i.e -ENXIO, but not for the same reason.
>>
>> this_gsi = parse_gsi(gicc);
>> if (!this_gsi)
>> return gsi ? -ENXIO : 0;
>>
>> This returns 0 when IRQ could not be parsed for the first cpu, but returns -ENXIO
>> for subsequent cpus. Although return code -ENXIO here still indicates IRQ parsing
>> to have failed.
>>
>> } else if (hetid != this_hetid || gsi != this_gsi) {
>> pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
>> return -ENXIO;
>> }
>>
>> This returns -ENXIO when there is a IRQ mismatch. But if the above check is not
>> there, -ENXIO return code here could not be classified into IRQ parse problem or
>> mismatch without looking into the IRQ value.
>
> Sorry, but I don't understand your point here. If any of this fails, there's
> going to be some debugging needed to look at the ACPI tables; the only
> difference with my suggestion is that you'll get a message indicating that
> the devices aren't homogeneous, which I think is helpful.
I dont have strong opinion either way. Hence will move 'this_gsi' check inside the
!gsi conditional check like you had suggested earlier.
@@ -70,6 +70,65 @@ 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, 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;
+}
+#endif
+
+#ifdef CONFIG_ARM_SPE_PMU
static struct resource spe_resources[] = {
{
/* irq */
@@ -84,6 +143,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 +155,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)