| Message ID | 20230808082247.383405-2-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: trbe: Enable ACPI based devices | expand |
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)
On Tue, Aug 08, 2023 at 09:48:16AM +0100, Suzuki K Poulose wrote: > 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. How does this return a positive value? > > + 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 ? If there are no irqs, why doesn't this return 0? arm_acpi_register_pmu_device() should only fail if either: - The static resources passed in are broken - The tables are not homogeneous - We fail to register the interrupt so something is amiss. > 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. I disagree. If the ACPI tables are borked, we should print a message saying so. Will
On 08/08/2023 14:16, Will Deacon wrote: > On Tue, Aug 08, 2023 at 09:48:16AM +0100, Suzuki K Poulose wrote: >> 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. > > How does this return a positive value? Right now, there aren't. My point is this function returns a "return value" of another function. And the caller of this function doesn't really follow the "check" it needs. e.g.: ret = foo(); if (ret < 0) error; return ret; And the caller only checks for if (ret) error; This seems fragile. > >>> + 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 ? > > If there are no irqs, why doesn't this return 0? Apologies, I missed that. > arm_acpi_register_pmu_device() should only fail if either: > > - The static resources passed in are broken > - The tables are not homogeneous > - We fail to register the interrupt > > so something is amiss. Agreed. We don't need duplicate messages about an error ? i.e., one in arm_acpi_register_pmu_device() and another one in the caller ? (Of course adding any missing error msgs). > >> 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. > > I disagree. If the ACPI tables are borked, we should print a message saying > so. Ok, fair point. Suzuki > > Will
On 8/9/23 18:24, Suzuki K Poulose wrote: > On 08/08/2023 14:16, Will Deacon wrote: >> On Tue, Aug 08, 2023 at 09:48:16AM +0100, Suzuki K Poulose wrote: >>> 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. >> >> How does this return a positive value? > > Right now, there aren't. My point is this function returns a "return value" of another function. And the caller of this function doesn't > really follow the "check" it needs. e.g.: > > ret = foo(); > if (ret < 0) > error; > return ret; > > > > And the caller only checks for > > if (ret) > error; > > This seems fragile. > >> >>>> + 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 ? >> >> If there are no irqs, why doesn't this return 0? > > Apologies, I missed that. > >> arm_acpi_register_pmu_device() should only fail if either: >> >> - The static resources passed in are broken >> - The tables are not homogeneous >> - We fail to register the interrupt >> >> so something is amiss. > > Agreed. We don't need duplicate messages about an error ? > i.e., one in arm_acpi_register_pmu_device() and another > one in the caller ? (Of course adding any missing error msgs). > > >> >>> 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. >> >> I disagree. If the ACPI tables are borked, we should print a message saying >> so. > > Ok, fair point. I am confused, what's the conclusion, should we just leave this unchanged ?
On 8/8/23 13:52, 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; > + > + 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; > + } > + } As discussed on the previous version i.e V3 thread, will move the 'this_gsi' check after parse_gsi(), inside if (!gsi) conditional block. This will treat subsequent cpu parse_gsi()'s failure as a mismatch thus triggering the pr_warn() message. diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c index 845683ca7c64..6eae772d6298 100644 --- a/drivers/perf/arm_pmu_acpi.c +++ b/drivers/perf/arm_pmu_acpi.c @@ -98,11 +98,11 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 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) { + if (!this_gsi) + return 0; + hetid = this_hetid; gsi = this_gsi; } else if (hetid != this_hetid || gsi != this_gsi) {
On Fri, Aug 11, 2023 at 02:13:42PM +0530, Anshuman Khandual wrote: > On 8/8/23 13:52, 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; > > + > > + 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; > > + } > > + } > > As discussed on the previous version i.e V3 thread, will move the > 'this_gsi' check after parse_gsi(), inside if (!gsi) conditional > block. This will treat subsequent cpu parse_gsi()'s failure as a > mismatch thus triggering the pr_warn() message. > > diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c > index 845683ca7c64..6eae772d6298 100644 > --- a/drivers/perf/arm_pmu_acpi.c > +++ b/drivers/perf/arm_pmu_acpi.c > @@ -98,11 +98,11 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 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) { > + if (!this_gsi) > + return 0; Why do you need this hunk? Will
On Wed, Aug 09, 2023 at 01:54:55PM +0100, Suzuki K Poulose wrote: > On 08/08/2023 14:16, Will Deacon wrote: > > On Tue, Aug 08, 2023 at 09:48:16AM +0100, Suzuki K Poulose wrote: > > > 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 > > > > + 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. > > > > How does this return a positive value? > > Right now, there aren't. My point is this function returns a "return value" > of another function. And the caller of this function doesn't > really follow the "check" it needs. e.g.: > > ret = foo(); > if (ret < 0) > error; > return ret; > > > > And the caller only checks for > > if (ret) > error; > > This seems fragile. Yeah, the '< 0' check is weird. I'd be inclined to drop that entirely from the helper function tbh... > > > > + 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 ? > > > > If there are no irqs, why doesn't this return 0? > > Apologies, I missed that. > > > arm_acpi_register_pmu_device() should only fail if either: > > > > - The static resources passed in are broken > > - The tables are not homogeneous > > - We fail to register the interrupt > > > > so something is amiss. > > Agreed. We don't need duplicate messages about an error ? > i.e., one in arm_acpi_register_pmu_device() and another > one in the caller ? (Of course adding any missing error msgs). ... and then just print the registration failure message in the caller. Will
On 8/11/23 15:42, Will Deacon wrote: > On Fri, Aug 11, 2023 at 02:13:42PM +0530, Anshuman Khandual wrote: >> On 8/8/23 13:52, 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; >>> + >>> + 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; >>> + } >>> + } >> >> As discussed on the previous version i.e V3 thread, will move the >> 'this_gsi' check after parse_gsi(), inside if (!gsi) conditional >> block. This will treat subsequent cpu parse_gsi()'s failure as a >> mismatch thus triggering the pr_warn() message. >> >> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c >> index 845683ca7c64..6eae772d6298 100644 >> --- a/drivers/perf/arm_pmu_acpi.c >> +++ b/drivers/perf/arm_pmu_acpi.c >> @@ -98,11 +98,11 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 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) { >> + if (!this_gsi) >> + return 0; > > Why do you need this hunk? Otherwise '0' gsi on all cpus would just clear the above homogeneity test, and end up in acpi_register_gsi() making it fail, but with the following warning before returning with -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; } Is this behaviour better than returning 0 after detecting '0' gsi in the first cpu to avoid the above mentioned scenario ? Although 0 gsi followed by non-zero ones will still end up warning about a mismatch.
On Fri, Aug 11, 2023 at 03:55:43PM +0530, Anshuman Khandual wrote: > > > On 8/11/23 15:42, Will Deacon wrote: > > On Fri, Aug 11, 2023 at 02:13:42PM +0530, Anshuman Khandual wrote: > >> On 8/8/23 13:52, 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; > >>> + > >>> + 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; > >>> + } > >>> + } > >> > >> As discussed on the previous version i.e V3 thread, will move the > >> 'this_gsi' check after parse_gsi(), inside if (!gsi) conditional > >> block. This will treat subsequent cpu parse_gsi()'s failure as a > >> mismatch thus triggering the pr_warn() message. > >> > >> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c > >> index 845683ca7c64..6eae772d6298 100644 > >> --- a/drivers/perf/arm_pmu_acpi.c > >> +++ b/drivers/perf/arm_pmu_acpi.c > >> @@ -98,11 +98,11 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 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) { > >> + if (!this_gsi) > >> + return 0; > > > > Why do you need this hunk? > > Otherwise '0' gsi on all cpus would just clear the above homogeneity > test, and end up in acpi_register_gsi() making it fail, but with the > following warning before returning with -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; > } Ah gotcha, thanks. > Is this behaviour better than returning 0 after detecting '0' gsi in > the first cpu to avoid the above mentioned scenario ? Although 0 gsi > followed by non-zero ones will still end up warning about a mismatch. Can we move the check _after_ the loop, then? That way, we still detect mismatches but we'll quietly return 0 if nobody has an interrupt. Will
On 8/11/23 16:30, Will Deacon wrote: > On Fri, Aug 11, 2023 at 03:55:43PM +0530, Anshuman Khandual wrote: >> >> >> On 8/11/23 15:42, Will Deacon wrote: >>> On Fri, Aug 11, 2023 at 02:13:42PM +0530, Anshuman Khandual wrote: >>>> On 8/8/23 13:52, 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; >>>>> + >>>>> + 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; >>>>> + } >>>>> + } >>>> >>>> As discussed on the previous version i.e V3 thread, will move the >>>> 'this_gsi' check after parse_gsi(), inside if (!gsi) conditional >>>> block. This will treat subsequent cpu parse_gsi()'s failure as a >>>> mismatch thus triggering the pr_warn() message. >>>> >>>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c >>>> index 845683ca7c64..6eae772d6298 100644 >>>> --- a/drivers/perf/arm_pmu_acpi.c >>>> +++ b/drivers/perf/arm_pmu_acpi.c >>>> @@ -98,11 +98,11 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 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) { >>>> + if (!this_gsi) >>>> + return 0; >>> >>> Why do you need this hunk? >> >> Otherwise '0' gsi on all cpus would just clear the above homogeneity >> test, and end up in acpi_register_gsi() making it fail, but with the >> following warning before returning with -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; >> } > > Ah gotcha, thanks. > >> Is this behaviour better than returning 0 after detecting '0' gsi in >> the first cpu to avoid the above mentioned scenario ? Although 0 gsi >> followed by non-zero ones will still end up warning about a mismatch. > > Can we move the check _after_ the loop, then? That way, we still detect > mismatches but we'll quietly return 0 if nobody has an interrupt. Sure, will fold in the following changes instead. diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c index 845683ca7c64..d7beb035345a 100644 --- a/drivers/perf/arm_pmu_acpi.c +++ b/drivers/perf/arm_pmu_acpi.c @@ -98,9 +98,6 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 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; @@ -111,6 +108,15 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len, } } + /* + * This is a special case where no cpu on + * the system has the interrupt and which + * could not have been detected via above + * homogeneous mismatch test. + */ + if (!this_gsi) + return 0; + 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);
On 8/11/23 15:49, Will Deacon wrote: > On Wed, Aug 09, 2023 at 01:54:55PM +0100, Suzuki K Poulose wrote: >> On 08/08/2023 14:16, Will Deacon wrote: >>> On Tue, Aug 08, 2023 at 09:48:16AM +0100, Suzuki K Poulose wrote: >>>> 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 >>>>> + 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. >>> >>> How does this return a positive value? >> >> Right now, there aren't. My point is this function returns a "return value" >> of another function. And the caller of this function doesn't >> really follow the "check" it needs. e.g.: >> >> ret = foo(); >> if (ret < 0) >> error; >> return ret; >> >> >> >> And the caller only checks for >> >> if (ret) >> error; >> >> This seems fragile. > > Yeah, the '< 0' check is weird. I'd be inclined to drop that entirely > from the helper function tbh... static int __maybe_unused arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len, u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *)) { ..... ..... 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; } We would still need to call acpi_unregister_gsi() in the helper itself in case previous platform_device_register() did not work correctly. AFAICS, acpi_unregister_gsi() cannot be moved to the caller. We could change the error check as 'if (ret)' which is the case in many other places in the tree. Also drop the above generic pr_warn() error message. static int __maybe_unused arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len, u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *)) { ..... ..... ret = platform_device_register(pdev); if (ret) acpi_unregister_gsi(gsi); return ret; } > >>>>> + 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 ? >>> >>> If there are no irqs, why doesn't this return 0? >> >> Apologies, I missed that. >> >>> arm_acpi_register_pmu_device() should only fail if either: >>> >>> - The static resources passed in are broken >>> - The tables are not homogeneous >>> - We fail to register the interrupt >>> >>> so something is amiss. >> >> Agreed. We don't need duplicate messages about an error ? >> i.e., one in arm_acpi_register_pmu_device() and another >> one in the caller ? (Of course adding any missing error msgs). > > ... and then just print the registration failure message in the caller. But we already have such messages in respective callers. static void arm_spe_acpi_register_device(void) { 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"); } static void arm_trbe_acpi_register_device(void) { int ret = arm_acpi_register_pmu_device(&trbe_dev, ACPI_MADT_GICC_TRBE, arm_trbe_parse_gsi); if (ret) pr_warn("ACPI: TRBE: Unable to register device\n"); }
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; +} + #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"); - acpi_unregister_gsi(gsi); - } } #else static inline void arm_spe_acpi_register_device(void)