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"); }