[PATCH V4 0/4] coresight: trbe: Enable ACPI based devices

[PATCH V4 0/4] coresight: trbe: Enable ACPI based devices

[Anshuman Khandual]

This series enables detection of ACPI based TRBE devices via a stand alone
purpose built representative platform device. But as a pre-requisite this
changes coresight_platform_data structure assignment for the TRBE device.

This series is based on v6.5-rc4 kernel, is also dependent on the following
EDK2 changes posted earlier by Sami.

https://edk2.groups.io/g/devel/message/107239
https://edk2.groups.io/g/devel/message/107241

Changes in V4:

- Added in-code comment for arm_trbe_device_probe()
- Reverted back using IS_ENABLED() for SPE PMU platform device
- Replaced #ifdef with IS_ENABLED() for TRBE platform device
- Protected arm_trbe_acpi_match with ACPI_PTR() - preventing a build failure
  when CONFIG_ACPI is not enabled
- Added __maybe_unused for arm_acpi_register_pmu_device() and dropped config
  checks with IS_ENABLED()

Changes in V3:

https://lore.kernel.org/all/20230803055652.1322801-1-anshuman.khandual@arm.com/

- Changed ARMV8_TRBE_PDEV_NAME from "arm-trbe-acpi" into "arm,trbe"
- Dropped local variable 'matched'
- Replaced 'matched' with 'valid gsi' as being already matched once
- Moved find_acpi_cpu_topology_hetero_id() outside conditional check

Changes in V2:

https://lore.kernel.org/all/20230801094052.750416-1-anshuman.khandual@arm.com/

- Refactored arm_spe_acpi_register_device() in a separate patch
- Renamed trbe_acpi_resources as trbe_resources
- Renamed trbe_acpi_dev as trbe_dev

Changes in V1:

https://lore.kernel.org/all/20230728112733.359620-1-anshuman.khandual@arm.com/

Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: James Clark <james.clark@arm.com>
Cc: coresight@lists.linaro.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Anshuman Khandual (4):
  arm_pmu: acpi: Refactor arm_spe_acpi_register_device()
  arm_pmu: acpi: Add a representative platform device for TRBE
  coresight: trbe: Add a representative coresight_platform_data for TRBE
  coresight: trbe: Enable ACPI based TRBE devices

 arch/arm64/include/asm/acpi.h                |   3 +
 drivers/hwtracing/coresight/coresight-trbe.c |  26 +++-
 drivers/hwtracing/coresight/coresight-trbe.h |   2 +
 drivers/perf/arm_pmu_acpi.c                  | 138 +++++++++++++------
 include/linux/perf/arm_pmu.h                 |   1 +
 5 files changed, 128 insertions(+), 42 deletions(-)

-- 
2.25.1


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

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

[Anshuman Khandual]

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;
+}
+
 #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)
-- 
2.25.1


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

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

[Suzuki K Poulose]

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

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

[Will Deacon]

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

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

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

[Suzuki K Poulose]

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


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

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

[Anshuman Khandual]

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 ?

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

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

[Will Deacon]

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

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

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

[Anshuman Khandual]

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

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

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

[Anshuman Khandual]

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) {

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

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

[Will Deacon]

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

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

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

[Anshuman Khandual]

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.

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

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

[Will Deacon]

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

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

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

[Anshuman Khandual]

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

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

[PATCH V4 2/4] arm_pmu: acpi: Add a representative platform device for TRBE

[Anshuman Khandual]

ACPI TRBE does not have a HID for identification which could create and add
a platform device into the platform bus. Also without a platform device, it
cannot be probed and bound to a platform driver.

This creates a dummy platform device for TRBE after ascertaining that ACPI
provides required interrupts uniformly across all cpus on the system. This
device gets created inside drivers/perf/arm_pmu_acpi.c to accommodate TRBE
being built as a module.

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
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/acpi.h |  3 +++
 drivers/perf/arm_pmu_acpi.c   | 35 +++++++++++++++++++++++++++++++++++
 include/linux/perf/arm_pmu.h  |  1 +
 3 files changed, 39 insertions(+)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index bd68e1b7f29f..4d537d56eb84 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -42,6 +42,9 @@
 #define ACPI_MADT_GICC_SPE  (offsetof(struct acpi_madt_generic_interrupt, \
 	spe_interrupt) + sizeof(u16))
 
+#define ACPI_MADT_GICC_TRBE  (offsetof(struct acpi_madt_generic_interrupt, \
+	trbe_interrupt) + sizeof(u16))
+
 /* Basic configuration for ACPI */
 #ifdef	CONFIG_ACPI
 pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
index 72454bef2a70..845683ca7c64 100644
--- a/drivers/perf/arm_pmu_acpi.c
+++ b/drivers/perf/arm_pmu_acpi.c
@@ -164,6 +164,40 @@ static inline void arm_spe_acpi_register_device(void)
 }
 #endif /* CONFIG_ARM_SPE_PMU */
 
+#if IS_ENABLED(CONFIG_CORESIGHT_TRBE)
+static struct resource trbe_resources[] = {
+	{
+		/* irq */
+		.flags          = IORESOURCE_IRQ,
+	}
+};
+
+static struct platform_device trbe_dev = {
+	.name = ARMV8_TRBE_PDEV_NAME,
+	.id = -1,
+	.resource = trbe_resources,
+	.num_resources = ARRAY_SIZE(trbe_resources)
+};
+
+static u16 arm_trbe_parse_gsi(struct acpi_madt_generic_interrupt *gicc)
+{
+	return gicc->trbe_interrupt;
+}
+
+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");
+}
+#else
+static inline void arm_trbe_acpi_register_device(void)
+{
+
+}
+#endif /* CONFIG_CORESIGHT_TRBE */
+
 static int arm_pmu_acpi_parse_irqs(void)
 {
 	int irq, cpu, irq_cpu, err;
@@ -399,6 +433,7 @@ static int arm_pmu_acpi_init(void)
 		return 0;
 
 	arm_spe_acpi_register_device();
+	arm_trbe_acpi_register_device();
 
 	return 0;
 }
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index a0801f68762b..143fbc10ecfe 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -187,5 +187,6 @@ void armpmu_free_irq(int irq, int cpu);
 #endif /* CONFIG_ARM_PMU */
 
 #define ARMV8_SPE_PDEV_NAME "arm,spe-v1"
+#define ARMV8_TRBE_PDEV_NAME "arm,trbe"
 
 #endif /* __ARM_PMU_H__ */
-- 
2.25.1


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

[PATCH V4 3/4] coresight: trbe: Add a representative coresight_platform_data for TRBE

[Anshuman Khandual]

TRBE coresight devices do not need regular connections information, as the
paths get built between all percpu source and their respective percpu sink
devices. Please refer 'commit 2cd87a7b293d ("coresight: core: Add support
for dedicated percpu sinks")' which added support for percpu sink devices.

coresight_register() expect device connections via the platform_data. TRBE
devices do not have any graph connections and thus is empty. With upcoming
ACPI support for TRBE, we do not get a real acpi_device and thus
coresight_get_platform_dat() will end up in failures. Hence this allocates
a zeroed coresight_platform_data structure and assigns that back into the
device.

Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: coresight@lists.linaro.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 drivers/hwtracing/coresight/coresight-trbe.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 7720619909d6..9455d62ca620 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -1494,9 +1494,20 @@ static int arm_trbe_device_probe(struct platform_device *pdev)
 	if (!drvdata)
 		return -ENOMEM;
 
-	pdata = coresight_get_platform_data(dev);
-	if (IS_ERR(pdata))
-		return PTR_ERR(pdata);
+	/*
+	 * TRBE coresight devices do not need regular connections
+	 * information, as the paths get built between all percpu
+	 * source and their respective percpu sink devices. Though
+	 * coresight_register() expect device connections via the
+	 * platform_data, which TRBE devices do not have. As they
+	 * are not real ACPI devices, coresight_get_platform_data()
+	 * ends up failing. Instead let's allocate a dummy zeroed
+	 * coresight_platform_data structure and assign that back
+	 * into the device for that purpose.
+	 */
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
 
 	dev_set_drvdata(dev, drvdata);
 	dev->platform_data = pdata;
-- 
2.25.1


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

[PATCH V4 4/4] coresight: trbe: Enable ACPI based TRBE devices

[Anshuman Khandual]

This detects and enables ACPI based TRBE devices via the dummy platform
device created earlier for this purpose.

Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: coresight@lists.linaro.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 drivers/hwtracing/coresight/coresight-trbe.c | 9 +++++++++
 drivers/hwtracing/coresight/coresight-trbe.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 9455d62ca620..9c59e2652b20 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -1548,7 +1548,16 @@ static const struct of_device_id arm_trbe_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, arm_trbe_of_match);
 
+#ifdef CONFIG_ACPI
+static const struct platform_device_id arm_trbe_acpi_match[] = {
+	{ ARMV8_TRBE_PDEV_NAME, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, arm_trbe_acpi_match);
+#endif
+
 static struct platform_driver arm_trbe_driver = {
+	.id_table = ACPI_PTR(arm_trbe_acpi_match),
 	.driver	= {
 		.name = DRVNAME,
 		.of_match_table = of_match_ptr(arm_trbe_of_match),
diff --git a/drivers/hwtracing/coresight/coresight-trbe.h b/drivers/hwtracing/coresight/coresight-trbe.h
index 77cbb5c63878..fce1735d5c58 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.h
+++ b/drivers/hwtracing/coresight/coresight-trbe.h
@@ -7,11 +7,13 @@
  *
  * Author: Anshuman Khandual <anshuman.khandual@arm.com>
  */
+#include <linux/acpi.h>
 #include <linux/coresight.h>
 #include <linux/device.h>
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
+#include <linux/perf/arm_pmu.h>
 #include <linux/platform_device.h>
 #include <linux/smp.h>
 
-- 
2.25.1


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