Re: [PATCH V4 1/4] arm_pmu: acpi: Refactor arm_spe_acpi_register_device()

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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)


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel