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 V3:
- 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/[email protected]/
- 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/[email protected]/
Cc: Sami Mujawar <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: James Clark <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
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 | 15 +-
drivers/hwtracing/coresight/coresight-trbe.h | 1 +
drivers/perf/arm_pmu_acpi.c | 142 +++++++++++++------
include/linux/perf/arm_pmu.h | 1 +
5 files changed, 119 insertions(+), 43 deletions(-)
--
2.25.1
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 <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
drivers/hwtracing/coresight/coresight-trbe.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 7720619909d6..e1d9d06e7725 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -1494,9 +1494,9 @@ 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);
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
dev_set_drvdata(dev, drvdata);
dev->platform_data = pdata;
--
2.25.1
This detects and enables ACPI based TRBE devices via the dummy platform
device created earlier for this purpose.
Cc: Suzuki K Poulose <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
drivers/hwtracing/coresight/coresight-trbe.c | 9 +++++++++
drivers/hwtracing/coresight/coresight-trbe.h | 1 +
2 files changed, 10 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index e1d9d06e7725..f884883e9018 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -1537,7 +1537,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 = 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..94e67009848a 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.h
+++ b/drivers/hwtracing/coresight/coresight-trbe.h
@@ -12,6 +12,7 @@
#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
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 <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: [email protected]
Cc: [email protected]
Co-developed-by: Will Deacon <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
Signed-off-by: Anshuman Khandual <[email protected]>
---
drivers/perf/arm_pmu_acpi.c | 107 ++++++++++++++++++++++--------------
1 file changed, 67 insertions(+), 40 deletions(-)
diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
index 90815ad762eb..235c14766a36 100644
--- a/drivers/perf/arm_pmu_acpi.c
+++ b/drivers/perf/arm_pmu_acpi.c
@@ -70,6 +70,65 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
}
#if IS_ENABLED(CONFIG_ARM_SPE_PMU)
+static int
+arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
+ u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
+{
+ int cpu, this_hetid, hetid, irq, ret;
+ u16 this_gsi, gsi = 0;
+
+ /*
+ * Ensure that platform device must have IORESOURCE_IRQ
+ * resource to hold gsi interrupt.
+ */
+ if (pdev->num_resources != 1)
+ return -ENXIO;
+
+ if (pdev->resource[0].flags != IORESOURCE_IRQ)
+ return -ENXIO;
+
+ /*
+ * Sanity check all the GICC tables for the same interrupt
+ * number. For now, only support homogeneous ACPI machines.
+ */
+ for_each_possible_cpu(cpu) {
+ struct acpi_madt_generic_interrupt *gicc;
+
+ gicc = acpi_cpu_get_madt_gicc(cpu);
+ if (gicc->header.length < len)
+ return gsi ? -ENXIO : 0;
+
+ this_gsi = parse_gsi(gicc);
+ if (!this_gsi)
+ return gsi ? -ENXIO : 0;
+
+ this_hetid = find_acpi_cpu_topology_hetero_id(cpu);
+ if (!gsi) {
+ hetid = this_hetid;
+ gsi = this_gsi;
+ } else if (hetid != this_hetid || gsi != this_gsi) {
+ pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
+ return -ENXIO;
+ }
+ }
+
+ irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
+ if (irq < 0) {
+ pr_warn("ACPI: %s Unable to register interrupt: %d\n", pdev->name, gsi);
+ return -ENXIO;
+ }
+
+ pdev->resource[0].start = irq;
+ ret = platform_device_register(pdev);
+ if (ret < 0) {
+ pr_warn("ACPI: %s: Unable to register device\n", pdev->name);
+ acpi_unregister_gsi(gsi);
+ }
+ return ret;
+}
+#endif
+
+#ifdef CONFIG_ARM_SPE_PMU
static struct resource spe_resources[] = {
{
/* irq */
@@ -84,6 +143,11 @@ static struct platform_device spe_dev = {
.num_resources = ARRAY_SIZE(spe_resources)
};
+static u16 arm_spe_parse_gsi(struct acpi_madt_generic_interrupt *gicc)
+{
+ return gicc->spe_interrupt;
+}
+
/*
* For lack of a better place, hook the normal PMU MADT walk
* and create a SPE device if we detect a recent MADT with
@@ -91,47 +155,10 @@ static struct platform_device spe_dev = {
*/
static void arm_spe_acpi_register_device(void)
{
- int cpu, hetid, irq, ret;
- bool first = true;
- u16 gsi = 0;
-
- /*
- * Sanity check all the GICC tables for the same interrupt number.
- * For now, we only support homogeneous ACPI/SPE machines.
- */
- for_each_possible_cpu(cpu) {
- struct acpi_madt_generic_interrupt *gicc;
-
- gicc = acpi_cpu_get_madt_gicc(cpu);
- if (gicc->header.length < ACPI_MADT_GICC_SPE)
- return;
-
- if (first) {
- gsi = gicc->spe_interrupt;
- if (!gsi)
- return;
- hetid = find_acpi_cpu_topology_hetero_id(cpu);
- first = false;
- } else if ((gsi != gicc->spe_interrupt) ||
- (hetid != find_acpi_cpu_topology_hetero_id(cpu))) {
- pr_warn("ACPI: SPE must be homogeneous\n");
- return;
- }
- }
-
- irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE,
- ACPI_ACTIVE_HIGH);
- if (irq < 0) {
- pr_warn("ACPI: SPE Unable to register interrupt: %d\n", gsi);
- return;
- }
-
- spe_resources[0].start = irq;
- ret = platform_device_register(&spe_dev);
- if (ret < 0) {
+ int ret = arm_acpi_register_pmu_device(&spe_dev, ACPI_MADT_GICC_SPE,
+ arm_spe_parse_gsi);
+ if (ret)
pr_warn("ACPI: SPE: Unable to register device\n");
- acpi_unregister_gsi(gsi);
- }
}
#else
static inline void arm_spe_acpi_register_device(void)
--
2.25.1
On 8/3/23 11:26, Anshuman Khandual wrote:
> + /*
> + * Sanity check all the GICC tables for the same interrupt
> + * number. For now, only support homogeneous ACPI machines.
> + */
> + for_each_possible_cpu(cpu) {
> + struct acpi_madt_generic_interrupt *gicc;
> +
> + gicc = acpi_cpu_get_madt_gicc(cpu);
> + if (gicc->header.length < len)
> + return gsi ? -ENXIO : 0;
> +
> + this_gsi = parse_gsi(gicc);
> + if (!this_gsi)
> + return gsi ? -ENXIO : 0;
Hello Will,
Moved parse_gsi() return code checking to its original place just to
make it similar in semantics to existing 'gicc->header.length check'.
If 'gsi' is valid i.e atleast a single cpu has been probed, return
-ENXIO indicating mismatch, otherwise just return 0.
- Anshuman
On 03/08/2023 06:56, Anshuman Khandual wrote:
> 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 <[email protected]>
> Cc: Mike Leach <[email protected]>
> Cc: Leo Yan <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> drivers/hwtracing/coresight/coresight-trbe.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 7720619909d6..e1d9d06e7725 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -1494,9 +1494,9 @@ 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);
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
Please could you add a comment in here, on why we use a dummy platform
data ? It is good to have documented it in the code too.
Suzuki
>
> dev_set_drvdata(dev, drvdata);
> dev->platform_data = pdata;
On 8/3/23 19:25, Suzuki K Poulose wrote:
> On 03/08/2023 06:56, Anshuman Khandual wrote:
>> 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 <[email protected]>
>> Cc: Mike Leach <[email protected]>
>> Cc: Leo Yan <[email protected]>
>> Cc: Alexander Shishkin <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Anshuman Khandual <[email protected]>
>> ---
>> drivers/hwtracing/coresight/coresight-trbe.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>> index 7720619909d6..e1d9d06e7725 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -1494,9 +1494,9 @@ 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);
>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata)
>> + return -ENOMEM;
>
> Please could you add a comment in here, on why we use a dummy platform
> data ? It is good to have documented it in the code too.
Sure, will add the following in-code documentation.
+ /*
+ * 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_dat()
+ * ends up failing. Instead let's allocate a dummy zeroed
+ * coresight_platform_data structure and assign that back
+ * into the device for that purpose.
+ */
On 04/08/2023 10:18, Anshuman Khandual wrote:
>
>
> On 8/3/23 19:25, Suzuki K Poulose wrote:
>> On 03/08/2023 06:56, Anshuman Khandual wrote:
>>> 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 <[email protected]>
>>> Cc: Mike Leach <[email protected]>
>>> Cc: Leo Yan <[email protected]>
>>> Cc: Alexander Shishkin <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Signed-off-by: Anshuman Khandual <[email protected]>
>>> ---
>>> drivers/hwtracing/coresight/coresight-trbe.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>>> index 7720619909d6..e1d9d06e7725 100644
>>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>>> @@ -1494,9 +1494,9 @@ 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);
>>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>> + if (!pdata)
>>> + return -ENOMEM;
>>
>> Please could you add a comment in here, on why we use a dummy platform
>> data ? It is good to have documented it in the code too.
>
> Sure, will add the following in-code documentation.
>
> + /*
> + * 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_dat()
minor nit: s/coresight_get_platform_dat/coresight_get_platform_data/
here and above in the description.
Otherwise, looks good.
Suzuki
On Thu, Aug 03, 2023 at 11:43:27AM +0530, Anshuman Khandual wrote:
>
>
> On 8/3/23 11:26, Anshuman Khandual wrote:
> > + /*
> > + * Sanity check all the GICC tables for the same interrupt
> > + * number. For now, only support homogeneous ACPI machines.
> > + */
> > + for_each_possible_cpu(cpu) {
> > + struct acpi_madt_generic_interrupt *gicc;
> > +
> > + gicc = acpi_cpu_get_madt_gicc(cpu);
> > + if (gicc->header.length < len)
> > + return gsi ? -ENXIO : 0;
> > +
> > + this_gsi = parse_gsi(gicc);
> > + if (!this_gsi)
> > + return gsi ? -ENXIO : 0;
>
> Hello Will,
>
> Moved parse_gsi() return code checking to its original place just to
> make it similar in semantics to existing 'gicc->header.length check'.
> If 'gsi' is valid i.e atleast a single cpu has been probed, return
> -ENXIO indicating mismatch, otherwise just return 0.
Wouldn't that still be the case without the check in this hunk? We'd run
into the homogeneous check and return -ENXIO from there, no?
Will
On 8/3/23 11:26, Anshuman Khandual wrote:
> This detects and enables ACPI based TRBE devices via the dummy platform
> device created earlier for this purpose.
>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Mike Leach <[email protected]>
> Cc: Leo Yan <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> drivers/hwtracing/coresight/coresight-trbe.c | 9 +++++++++
> drivers/hwtracing/coresight/coresight-trbe.h | 1 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index e1d9d06e7725..f884883e9018 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -1537,7 +1537,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 = arm_trbe_acpi_match,
The build problem [1] reported on the first version of the series still exists
here i.e arm_trbe_acpi_match is hidden without CONFIG_ACPI. I had assumed that
CONFIG_CORESIGHT always enables CONFIG_ACPI, which is not the case. Following
random config (with CONFIG_ACPI=n and CONFIG_CORESIGHT_TRBE=y) easily triggers
the build problem.
https://download.01.org/0day-ci/archive/20230805/[email protected]/config
make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm64 -s -j 128
drivers/hwtracing/coresight/coresight-trbe.c:1563:23: error: implicit declaration of function ‘ACPI_PTR’ [-Werror=implicit-function-declaration]
1563 | .acpi_match_table = ACPI_PTR(arm_trbe_acpi_match),
| ^~~~~~~~
drivers/hwtracing/coresight/coresight-trbe.c:1563:32: error: ‘arm_trbe_acpi_match’ undeclared here (not in a function); did you mean ‘arm_trbe_of_match’?
1563 | .acpi_match_table = ACPI_PTR(arm_trbe_acpi_match),
| ^~~~~~~~~~~~~~~~~~~
| arm_trbe_of_match
Following config wrap around fixes the problem.
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -1557,7 +1557,9 @@ MODULE_DEVICE_TABLE(platform, arm_trbe_acpi_match);
#endif
static struct platform_driver arm_trbe_driver = {
+#ifdef CONFIG_ACPI
.id_table = arm_trbe_acpi_match,
+#endif
.driver = {
.name = DRVNAME,
.of_match_table = of_match_ptr(arm_trbe_of_match),
Please not that unlike other coresight drivers, TRBE is not using 'acpi_device_id'
based "acpi_match_table = ACPI_PTR" construct. But regardless, ACPI_PTR() seems to
be an alternate (probably better) solution as well.
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -1557,7 +1557,7 @@ MODULE_DEVICE_TABLE(platform, arm_trbe_acpi_match);
#endif
static struct platform_driver arm_trbe_driver = {
- .id_table = arm_trbe_acpi_match,
+ .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 94e67009848a..fce1735d5c58 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.h
+++ b/drivers/hwtracing/coresight/coresight-trbe.h
@@ -7,6 +7,7 @@
*
* Author: Anshuman Khandual <[email protected]>
*/
+#include <linux/acpi.h>
#include <linux/coresight.h>
#include <linux/device.h>
#include <linux/irq.h>
[1] https://lore.kernel.org/all/[email protected]/
> .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..94e67009848a 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.h
> +++ b/drivers/hwtracing/coresight/coresight-trbe.h
> @@ -12,6 +12,7 @@
> #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>
>
On 8/4/23 22:09, Will Deacon wrote:
> On Thu, Aug 03, 2023 at 11:43:27AM +0530, Anshuman Khandual wrote:
>>
>>
>> On 8/3/23 11:26, Anshuman Khandual wrote:
>>> + /*
>>> + * Sanity check all the GICC tables for the same interrupt
>>> + * number. For now, only support homogeneous ACPI machines.
>>> + */
>>> + for_each_possible_cpu(cpu) {
>>> + struct acpi_madt_generic_interrupt *gicc;
>>> +
>>> + gicc = acpi_cpu_get_madt_gicc(cpu);
>>> + if (gicc->header.length < len)
>>> + return gsi ? -ENXIO : 0;
>>> +
>>> + this_gsi = parse_gsi(gicc);
>>> + if (!this_gsi)
>>> + return gsi ? -ENXIO : 0;
>>
>> Hello Will,
>>
>> Moved parse_gsi() return code checking to its original place just to
>> make it similar in semantics to existing 'gicc->header.length check'.
>> If 'gsi' is valid i.e atleast a single cpu has been probed, return
>> -ENXIO indicating mismatch, otherwise just return 0.
>
> Wouldn't that still be the case without the check in this hunk? We'd run
> into the homogeneous check and return -ENXIO from there, no?
Although the return code will be the same i.e -ENXIO, but not for the same reason.
this_gsi = parse_gsi(gicc);
if (!this_gsi)
return gsi ? -ENXIO : 0;
This returns 0 when IRQ could not be parsed for the first cpu, but returns -ENXIO
for subsequent cpus. Although return code -ENXIO here still indicates IRQ parsing
to have failed.
} else if (hetid != this_hetid || gsi != this_gsi) {
pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
return -ENXIO;
}
This returns -ENXIO when there is a IRQ mismatch. But if the above check is not
there, -ENXIO return code here could not be classified into IRQ parse problem or
mismatch without looking into the IRQ value.
On 07/08/2023 05:43, Anshuman Khandual wrote:
>
>
> On 8/3/23 11:26, Anshuman Khandual wrote:
>> This detects and enables ACPI based TRBE devices via the dummy platform
>> device created earlier for this purpose.
>>
>> Cc: Suzuki K Poulose <[email protected]>
>> Cc: Mike Leach <[email protected]>
>> Cc: Leo Yan <[email protected]>
>> Cc: Alexander Shishkin <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Anshuman Khandual <[email protected]>
>> ---
>> drivers/hwtracing/coresight/coresight-trbe.c | 9 +++++++++
>> drivers/hwtracing/coresight/coresight-trbe.h | 1 +
>> 2 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>> index e1d9d06e7725..f884883e9018 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -1537,7 +1537,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 = arm_trbe_acpi_match,
>
> The build problem [1] reported on the first version of the series still exists
> here i.e arm_trbe_acpi_match is hidden without CONFIG_ACPI. I had assumed that
> CONFIG_CORESIGHT always enables CONFIG_ACPI, which is not the case. Following
> random config (with CONFIG_ACPI=n and CONFIG_CORESIGHT_TRBE=y) easily triggers
> the build problem.
>
> https://download.01.org/0day-ci/archive/20230805/[email protected]/config
>
> make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm64 -s -j 128
> drivers/hwtracing/coresight/coresight-trbe.c:1563:23: error: implicit declaration of function ‘ACPI_PTR’ [-Werror=implicit-function-declaration]
> 1563 | .acpi_match_table = ACPI_PTR(arm_trbe_acpi_match),
> | ^~~~~~~~
> drivers/hwtracing/coresight/coresight-trbe.c:1563:32: error: ‘arm_trbe_acpi_match’ undeclared here (not in a function); did you mean ‘arm_trbe_of_match’?
> 1563 | .acpi_match_table = ACPI_PTR(arm_trbe_acpi_match),
> | ^~~~~~~~~~~~~~~~~~~
> | arm_trbe_of_match
>
> Following config wrap around fixes the problem.
>
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -1557,7 +1557,9 @@ MODULE_DEVICE_TABLE(platform, arm_trbe_acpi_match);
> #endif
>
> static struct platform_driver arm_trbe_driver = {
> +#ifdef CONFIG_ACPI
> .id_table = arm_trbe_acpi_match,
> +#endif
> .driver = {
> .name = DRVNAME,
> .of_match_table = of_match_ptr(arm_trbe_of_match),
>
> Please not that unlike other coresight drivers, TRBE is not using 'acpi_device_id'
> based "acpi_match_table = ACPI_PTR" construct. But regardless, ACPI_PTR() seems to
> be an alternate (probably better) solution as well.
>
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -1557,7 +1557,7 @@ MODULE_DEVICE_TABLE(platform, arm_trbe_acpi_match);
> #endif
>
> static struct platform_driver arm_trbe_driver = {
> - .id_table = arm_trbe_acpi_match,
> + .id_table = ACPI_PTR(arm_trbe_acpi_match),
This is preferred.
> .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 94e67009848a..fce1735d5c58 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.h
> +++ b/drivers/hwtracing/coresight/coresight-trbe.h
> @@ -7,6 +7,7 @@
> *
> * Author: Anshuman Khandual <[email protected]>
> */
> +#include <linux/acpi.h>
Shouldn't this be added in trbe.c ? Does trbe.h depend on any ACPI headers ?
Suzuki
On 8/7/23 17:07, Suzuki K Poulose wrote:
> On 07/08/2023 05:43, Anshuman Khandual wrote:
>>
>>
>> On 8/3/23 11:26, Anshuman Khandual wrote:
>>> This detects and enables ACPI based TRBE devices via the dummy platform
>>> device created earlier for this purpose.
>>>
>>> Cc: Suzuki K Poulose <[email protected]>
>>> Cc: Mike Leach <[email protected]>
>>> Cc: Leo Yan <[email protected]>
>>> Cc: Alexander Shishkin <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Signed-off-by: Anshuman Khandual <[email protected]>
>>> ---
>>> drivers/hwtracing/coresight/coresight-trbe.c | 9 +++++++++
>>> drivers/hwtracing/coresight/coresight-trbe.h | 1 +
>>> 2 files changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>>> index e1d9d06e7725..f884883e9018 100644
>>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>>> @@ -1537,7 +1537,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 = arm_trbe_acpi_match,
>>
>> The build problem [1] reported on the first version of the series still exists
>> here i.e arm_trbe_acpi_match is hidden without CONFIG_ACPI. I had assumed that
>> CONFIG_CORESIGHT always enables CONFIG_ACPI, which is not the case. Following
>> random config (with CONFIG_ACPI=n and CONFIG_CORESIGHT_TRBE=y) easily triggers
>> the build problem.
>>
>> https://download.01.org/0day-ci/archive/20230805/[email protected]/config
>>
>> make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm64 -s -j 128
>> drivers/hwtracing/coresight/coresight-trbe.c:1563:23: error: implicit declaration of function ‘ACPI_PTR’ [-Werror=implicit-function-declaration]
>> 1563 | .acpi_match_table = ACPI_PTR(arm_trbe_acpi_match),
>> | ^~~~~~~~
>> drivers/hwtracing/coresight/coresight-trbe.c:1563:32: error: ‘arm_trbe_acpi_match’ undeclared here (not in a function); did you mean ‘arm_trbe_of_match’?
>> 1563 | .acpi_match_table = ACPI_PTR(arm_trbe_acpi_match),
>> | ^~~~~~~~~~~~~~~~~~~
>> | arm_trbe_of_match
>>
>> Following config wrap around fixes the problem.
>>
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -1557,7 +1557,9 @@ MODULE_DEVICE_TABLE(platform, arm_trbe_acpi_match);
>> #endif
>> static struct platform_driver arm_trbe_driver = {
>> +#ifdef CONFIG_ACPI
>> .id_table = arm_trbe_acpi_match,
>> +#endif
>> .driver = {
>> .name = DRVNAME,
>> .of_match_table = of_match_ptr(arm_trbe_of_match),
>>
>> Please not that unlike other coresight drivers, TRBE is not using 'acpi_device_id'
>> based "acpi_match_table = ACPI_PTR" construct. But regardless, ACPI_PTR() seems to
>> be an alternate (probably better) solution as well.
>>
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -1557,7 +1557,7 @@ MODULE_DEVICE_TABLE(platform, arm_trbe_acpi_match);
>> #endif
>> static struct platform_driver arm_trbe_driver = {
>> - .id_table = arm_trbe_acpi_match,
>> + .id_table = ACPI_PTR(arm_trbe_acpi_match),
>
> This is preferred.
>
>> .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 94e67009848a..fce1735d5c58 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.h
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.h
>> @@ -7,6 +7,7 @@
>> *
>> * Author: Anshuman Khandual <[email protected]>
>> */
>> +#include <linux/acpi.h>
>
> Shouldn't this be added in trbe.c ? Does trbe.h depend on any ACPI headers ?
The convention we have followed till now is all include/linux/ headers required
in TRBE driver is included via trbe.h not directly in trbe.c, just followed the
same principle this time around for acpi.h and perf/arm_pmu.h as well.
On Mon, Aug 07, 2023 at 11:03:40AM +0530, Anshuman Khandual wrote:
> On 8/4/23 22:09, Will Deacon wrote:
> > On Thu, Aug 03, 2023 at 11:43:27AM +0530, Anshuman Khandual wrote:
> >> On 8/3/23 11:26, Anshuman Khandual wrote:
> >>> + /*
> >>> + * Sanity check all the GICC tables for the same interrupt
> >>> + * number. For now, only support homogeneous ACPI machines.
> >>> + */
> >>> + for_each_possible_cpu(cpu) {
> >>> + struct acpi_madt_generic_interrupt *gicc;
> >>> +
> >>> + gicc = acpi_cpu_get_madt_gicc(cpu);
> >>> + if (gicc->header.length < len)
> >>> + return gsi ? -ENXIO : 0;
> >>> +
> >>> + this_gsi = parse_gsi(gicc);
> >>> + if (!this_gsi)
> >>> + return gsi ? -ENXIO : 0;
> >>
> >> Moved parse_gsi() return code checking to its original place just to
> >> make it similar in semantics to existing 'gicc->header.length check'.
> >> If 'gsi' is valid i.e atleast a single cpu has been probed, return
> >> -ENXIO indicating mismatch, otherwise just return 0.
> >
> > Wouldn't that still be the case without the check in this hunk? We'd run
> > into the homogeneous check and return -ENXIO from there, no?
> Although the return code will be the same i.e -ENXIO, but not for the same reason.
>
> this_gsi = parse_gsi(gicc);
> if (!this_gsi)
> return gsi ? -ENXIO : 0;
>
> This returns 0 when IRQ could not be parsed for the first cpu, but returns -ENXIO
> for subsequent cpus. Although return code -ENXIO here still indicates IRQ parsing
> to have failed.
>
> } else if (hetid != this_hetid || gsi != this_gsi) {
> pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
> return -ENXIO;
> }
>
> This returns -ENXIO when there is a IRQ mismatch. But if the above check is not
> there, -ENXIO return code here could not be classified into IRQ parse problem or
> mismatch without looking into the IRQ value.
Sorry, but I don't understand your point here. If any of this fails, there's
going to be some debugging needed to look at the ACPI tables; the only
difference with my suggestion is that you'll get a message indicating that
the devices aren't homogeneous, which I think is helpful.
Will
On 8/8/23 18:51, Will Deacon wrote:
> On Mon, Aug 07, 2023 at 11:03:40AM +0530, Anshuman Khandual wrote:
>> On 8/4/23 22:09, Will Deacon wrote:
>>> On Thu, Aug 03, 2023 at 11:43:27AM +0530, Anshuman Khandual wrote:
>>>> On 8/3/23 11:26, Anshuman Khandual wrote:
>>>>> + /*
>>>>> + * Sanity check all the GICC tables for the same interrupt
>>>>> + * number. For now, only support homogeneous ACPI machines.
>>>>> + */
>>>>> + for_each_possible_cpu(cpu) {
>>>>> + struct acpi_madt_generic_interrupt *gicc;
>>>>> +
>>>>> + gicc = acpi_cpu_get_madt_gicc(cpu);
>>>>> + if (gicc->header.length < len)
>>>>> + return gsi ? -ENXIO : 0;
>>>>> +
>>>>> + this_gsi = parse_gsi(gicc);
>>>>> + if (!this_gsi)
>>>>> + return gsi ? -ENXIO : 0;
>>>>
>>>> Moved parse_gsi() return code checking to its original place just to
>>>> make it similar in semantics to existing 'gicc->header.length check'.
>>>> If 'gsi' is valid i.e atleast a single cpu has been probed, return
>>>> -ENXIO indicating mismatch, otherwise just return 0.
>>>
>>> Wouldn't that still be the case without the check in this hunk? We'd run
>>> into the homogeneous check and return -ENXIO from there, no?
>> Although the return code will be the same i.e -ENXIO, but not for the same reason.
>>
>> this_gsi = parse_gsi(gicc);
>> if (!this_gsi)
>> return gsi ? -ENXIO : 0;
>>
>> This returns 0 when IRQ could not be parsed for the first cpu, but returns -ENXIO
>> for subsequent cpus. Although return code -ENXIO here still indicates IRQ parsing
>> to have failed.
>>
>> } else if (hetid != this_hetid || gsi != this_gsi) {
>> pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
>> return -ENXIO;
>> }
>>
>> This returns -ENXIO when there is a IRQ mismatch. But if the above check is not
>> there, -ENXIO return code here could not be classified into IRQ parse problem or
>> mismatch without looking into the IRQ value.
>
> Sorry, but I don't understand your point here. If any of this fails, there's
> going to be some debugging needed to look at the ACPI tables; the only
> difference with my suggestion is that you'll get a message indicating that
> the devices aren't homogeneous, which I think is helpful.
I dont have strong opinion either way. Hence will move 'this_gsi' check inside the
!gsi conditional check like you had suggested earlier.
Hi Anshuman,
kernel test robot noticed the following build errors:
[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on arm/for-next soc/for-next linus/master arm/fixes v6.5-rc5 next-20230809]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Anshuman-Khandual/arm_pmu-acpi-Refactor-arm_spe_acpi_register_device/20230803-135907
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link: https://lore.kernel.org/r/20230803055652.1322801-5-anshuman.khandual%40arm.com
patch subject: [PATCH V3 4/4] coresight: trbe: Enable ACPI based TRBE devices
config: arm64-randconfig-r071-20230813 (https://download.01.org/0day-ci/archive/20230814/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230814/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> drivers/hwtracing/coresight/coresight-trbe.c:1549:21: error: 'arm_trbe_acpi_match' undeclared here (not in a function); did you mean 'arm_trbe_of_match'?
1549 | .id_table = arm_trbe_acpi_match,
| ^~~~~~~~~~~~~~~~~~~
| arm_trbe_of_match
vim +1549 drivers/hwtracing/coresight/coresight-trbe.c
1547
1548 static struct platform_driver arm_trbe_driver = {
> 1549 .id_table = arm_trbe_acpi_match,
1550 .driver = {
1551 .name = DRVNAME,
1552 .of_match_table = of_match_ptr(arm_trbe_of_match),
1553 .suppress_bind_attrs = true,
1554 },
1555 .probe = arm_trbe_device_probe,
1556 .remove = arm_trbe_device_remove,
1557 };
1558
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki