[PATCH] perf/arm-cmn: don't claim resource during ioremap() for CMN700 with ACPI

[PATCH] perf/arm-cmn: don't claim resource during ioremap() for CMN700 with ACPI

[Yin Fengwei]

Currently, arm-cmn PMU driver assumes ACPI claim resource
for CMN600 + ACPI. But with CMN700 + ACPI, the device probe
failed because of resource claim failes when ioremap() is
called:
[   10.837300] arm-cmn ARMHC700:00: error -EBUSY: can't request region for resource [mem 0x40000000-0x4fffffff]
[   10.847310] arm-cmn ARMHC700:00: probe with driver arm-cmn failed with error -16
[   10.854726] arm-cmn ARMHC700:02: error -EBUSY: can't request region for resource [mem 0x40040000000-0x4004fffffff]
[   10.865085] arm-cmn ARMHC700:02: probe with driver arm-cmn failed with error -16

Let CMN700 + ACPI do same as CMN600 + ACPI to allow CMN700
work in ACPI env.

Signed-off-by: Yin Fengwei <fengwei_yin@linux.alibaba.com>
---
I am also wondering whether we could just drop the CMN600 part id
check here if ACPI companion device claimed the resource?

 drivers/perf/arm-cmn.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index ef959e66db7c..8b5322a2aa6e 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -2559,7 +2559,8 @@ static int arm_cmn_probe(struct platform_device *pdev)
 	cmn->part = (unsigned long)device_get_match_data(cmn->dev);
 	platform_set_drvdata(pdev, cmn);
 
-	if (cmn->part == PART_CMN600 && has_acpi_companion(cmn->dev)) {
+	if (((cmn->part == PART_CMN600) || (cmn->part == PART_CMN700)) &&
+			has_acpi_companion(cmn->dev)) {
 		rootnode = arm_cmn600_acpi_probe(pdev, cmn);
 	} else {
 		rootnode = 0;
@@ -2649,7 +2650,7 @@ MODULE_DEVICE_TABLE(of, arm_cmn_of_match);
 static const struct acpi_device_id arm_cmn_acpi_match[] = {
 	{ "ARMHC600", PART_CMN600 },
 	{ "ARMHC650" },
-	{ "ARMHC700" },
+	{ "ARMHC700", PART_CMN700 },
 	{}
 };
 MODULE_DEVICE_TABLE(acpi, arm_cmn_acpi_match);
-- 
2.43.5

Re: [PATCH] perf/arm-cmn: don't claim resource during ioremap() for CMN700 with ACPI

[YinFengwei]

On Tue, Feb 18, 2025 at 09:21:11AM +0800, Yin Fengwei wrote:
> Currently, arm-cmn PMU driver assumes ACPI claim resource
> for CMN600 + ACPI. But with CMN700 + ACPI, the device probe
> failed because of resource claim failes when ioremap() is
> called:
> [   10.837300] arm-cmn ARMHC700:00: error -EBUSY: can't request region for resource [mem 0x40000000-0x4fffffff]
> [   10.847310] arm-cmn ARMHC700:00: probe with driver arm-cmn failed with error -16
> [   10.854726] arm-cmn ARMHC700:02: error -EBUSY: can't request region for resource [mem 0x40040000000-0x4004fffffff]
> [   10.865085] arm-cmn ARMHC700:02: probe with driver arm-cmn failed with error -16
> 
> Let CMN700 + ACPI do same as CMN600 + ACPI to allow CMN700
> work in ACPI env.
> 
> Signed-off-by: Yin Fengwei <fengwei_yin@linux.alibaba.com>
> ---
> I am also wondering whether we could just drop the CMN600 part id
> check here if ACPI companion device claimed the resource?
> 
Sorry. Just saw the link
https://lore.kernel.org/all/1676535470-120560-1-git-send-email-renyu.zj@linux.alibaba.com/
after I hit send button. May continue the discussion there. Thanks.


Regards
Yin, Fengwei

Re: [PATCH] perf/arm-cmn: don't claim resource during ioremap() for CMN700 with ACPI

[Robin Murphy]

On 2025-02-18 1:21 am, Yin Fengwei wrote:
> Currently, arm-cmn PMU driver assumes ACPI claim resource
> for CMN600 + ACPI. But with CMN700 + ACPI, the device probe
> failed because of resource claim failes when ioremap() is
> called:
> [   10.837300] arm-cmn ARMHC700:00: error -EBUSY: can't request region for resource [mem 0x40000000-0x4fffffff]
> [   10.847310] arm-cmn ARMHC700:00: probe with driver arm-cmn failed with error -16
> [   10.854726] arm-cmn ARMHC700:02: error -EBUSY: can't request region for resource [mem 0x40040000000-0x4004fffffff]
> [   10.865085] arm-cmn ARMHC700:02: probe with driver arm-cmn failed with error -16
> 
> Let CMN700 + ACPI do same as CMN600 + ACPI to allow CMN700
> work in ACPI env.

No, the CMN-600 routine is a special case for CMN-600 having two nested 
memory resources of its own. CMN-700 and everything else only have one 
memory resource, so that is not appropriate. What else is claiming the 
region to cause a conflict?

Thanks,
Robin.

> Signed-off-by: Yin Fengwei <fengwei_yin@linux.alibaba.com>
> ---
> I am also wondering whether we could just drop the CMN600 part id
> check here if ACPI companion device claimed the resource?
> 
>   drivers/perf/arm-cmn.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index ef959e66db7c..8b5322a2aa6e 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -2559,7 +2559,8 @@ static int arm_cmn_probe(struct platform_device *pdev)
>   	cmn->part = (unsigned long)device_get_match_data(cmn->dev);
>   	platform_set_drvdata(pdev, cmn);
>   
> -	if (cmn->part == PART_CMN600 && has_acpi_companion(cmn->dev)) {
> +	if (((cmn->part == PART_CMN600) || (cmn->part == PART_CMN700)) &&
> +			has_acpi_companion(cmn->dev)) {
>   		rootnode = arm_cmn600_acpi_probe(pdev, cmn);
>   	} else {
>   		rootnode = 0;
> @@ -2649,7 +2650,7 @@ MODULE_DEVICE_TABLE(of, arm_cmn_of_match);
>   static const struct acpi_device_id arm_cmn_acpi_match[] = {
>   	{ "ARMHC600", PART_CMN600 },
>   	{ "ARMHC650" },
> -	{ "ARMHC700" },
> +	{ "ARMHC700", PART_CMN700 },
>   	{}
>   };
>   MODULE_DEVICE_TABLE(acpi, arm_cmn_acpi_match);

Re: [PATCH] perf/arm-cmn: don't claim resource during ioremap() for CMN700 with ACPI

[YinFengwei]

On Tue, Feb 18, 2025 at 10:31:42AM +0800, Robin Murphy wrote:
> On 2025-02-18 1:21 am, Yin Fengwei wrote:
> > Currently, arm-cmn PMU driver assumes ACPI claim resource
> > for CMN600 + ACPI. But with CMN700 + ACPI, the device probe
> > failed because of resource claim failes when ioremap() is
> > called:
> > [   10.837300] arm-cmn ARMHC700:00: error -EBUSY: can't request region for resource [mem 0x40000000-0x4fffffff]
> > [   10.847310] arm-cmn ARMHC700:00: probe with driver arm-cmn failed with error -16
> > [   10.854726] arm-cmn ARMHC700:02: error -EBUSY: can't request region for resource [mem 0x40040000000-0x4004fffffff]
> > [   10.865085] arm-cmn ARMHC700:02: probe with driver arm-cmn failed with error -16
> > 
> > Let CMN700 + ACPI do same as CMN600 + ACPI to allow CMN700
> > work in ACPI env.
> 
> No, the CMN-600 routine is a special case for CMN-600 having two nested
> memory resources of its own. CMN-700 and everything else only have one
> memory resource, so that is not appropriate. What else is claiming the
> region to cause a conflict?
You are right. I just saw the discussion after I sent this patch. :(.
I proposed new fix in that thread which align with the spec.

Could you please take a look? Thanks.


Regards
Yin, Fengwei

> 
> Thanks,
> Robin.
> 
> > Signed-off-by: Yin Fengwei <fengwei_yin@linux.alibaba.com>
> > ---
> > I am also wondering whether we could just drop the CMN600 part id
> > check here if ACPI companion device claimed the resource?
> > 
> >   drivers/perf/arm-cmn.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> > index ef959e66db7c..8b5322a2aa6e 100644
> > --- a/drivers/perf/arm-cmn.c
> > +++ b/drivers/perf/arm-cmn.c
> > @@ -2559,7 +2559,8 @@ static int arm_cmn_probe(struct platform_device *pdev)
> >   	cmn->part = (unsigned long)device_get_match_data(cmn->dev);
> >   	platform_set_drvdata(pdev, cmn);
> > -	if (cmn->part == PART_CMN600 && has_acpi_companion(cmn->dev)) {
> > +	if (((cmn->part == PART_CMN600) || (cmn->part == PART_CMN700)) &&
> > +			has_acpi_companion(cmn->dev)) {
> >   		rootnode = arm_cmn600_acpi_probe(pdev, cmn);
> >   	} else {
> >   		rootnode = 0;
> > @@ -2649,7 +2650,7 @@ MODULE_DEVICE_TABLE(of, arm_cmn_of_match);
> >   static const struct acpi_device_id arm_cmn_acpi_match[] = {
> >   	{ "ARMHC600", PART_CMN600 },
> >   	{ "ARMHC650" },
> > -	{ "ARMHC700" },
> > +	{ "ARMHC700", PART_CMN700 },
> >   	{}
> >   };
> >   MODULE_DEVICE_TABLE(acpi, arm_cmn_acpi_match);

Re: [PATCH] perf/arm-cmn: don't claim resource during ioremap() for CMN700 with ACPI

[YinFengwei]

On Tue, Feb 18, 2025 at 10:31:42AM +0800, Robin Murphy wrote:
> On 2025-02-18 1:21 am, Yin Fengwei wrote:
> > Currently, arm-cmn PMU driver assumes ACPI claim resource
> > for CMN600 + ACPI. But with CMN700 + ACPI, the device probe
> > failed because of resource claim failes when ioremap() is
> > called:
> > [   10.837300] arm-cmn ARMHC700:00: error -EBUSY: can't request region for resource [mem 0x40000000-0x4fffffff]
> > [   10.847310] arm-cmn ARMHC700:00: probe with driver arm-cmn failed with error -16
> > [   10.854726] arm-cmn ARMHC700:02: error -EBUSY: can't request region for resource [mem 0x40040000000-0x4004fffffff]
> > [   10.865085] arm-cmn ARMHC700:02: probe with driver arm-cmn failed with error -16
> > 
> > Let CMN700 + ACPI do same as CMN600 + ACPI to allow CMN700
> > work in ACPI env.
> 
> No, the CMN-600 routine is a special case for CMN-600 having two nested
> memory resources of its own. CMN-700 and everything else only have one
> memory resource, so that is not appropriate. What else is claiming the
> region to cause a conflict?
Sorry. Forgot the link for the new proposed fix:
https://lore.kernel.org/all/Z7QYlUP6nfBNMXsv@U-V2QX163P-2032.local/

My understanding is that there are two problems here:
1. ACPI claim the memory range and that's why we see this -EBUSY error
   with correct code path for CMN700 + ACPI table.

2. It's not correct to apply CMN600 probe method to CMN700 because
   CMN600 has two nested memory resouces while CMN700 should only have
   one memory resource. And you don't want to introduce trick to handle
   incorect ACPI DSDT.

Regards
Yin, Fengwei

> 
> Thanks,
> Robin.

Re: [PATCH] perf/arm-cmn: don't claim resource during ioremap() for CMN700 with ACPI

[Robin Murphy]

On 2025-02-18 10:58 am, YinFengwei wrote:
> On Tue, Feb 18, 2025 at 10:31:42AM +0800, Robin Murphy wrote:
>> On 2025-02-18 1:21 am, Yin Fengwei wrote:
>>> Currently, arm-cmn PMU driver assumes ACPI claim resource
>>> for CMN600 + ACPI. But with CMN700 + ACPI, the device probe
>>> failed because of resource claim failes when ioremap() is
>>> called:
>>> [   10.837300] arm-cmn ARMHC700:00: error -EBUSY: can't request region for resource [mem 0x40000000-0x4fffffff]
>>> [   10.847310] arm-cmn ARMHC700:00: probe with driver arm-cmn failed with error -16
>>> [   10.854726] arm-cmn ARMHC700:02: error -EBUSY: can't request region for resource [mem 0x40040000000-0x4004fffffff]
>>> [   10.865085] arm-cmn ARMHC700:02: probe with driver arm-cmn failed with error -16
>>>
>>> Let CMN700 + ACPI do same as CMN600 + ACPI to allow CMN700
>>> work in ACPI env.
>>
>> No, the CMN-600 routine is a special case for CMN-600 having two nested
>> memory resources of its own. CMN-700 and everything else only have one
>> memory resource, so that is not appropriate. What else is claiming the
>> region to cause a conflict?
> Sorry. Forgot the link for the new proposed fix:
> https://lore.kernel.org/all/Z7QYlUP6nfBNMXsv@U-V2QX163P-2032.local/

Yes, I saw that. It's a broken diff that won't even compile, with no 
explanation of what it's supposed to be trying to achieve or why. I'm 
not sure what you're asking me to comment on.

> My understanding is that there are two problems here:
> 1. ACPI claim the memory range and that's why we see this -EBUSY error
>     with correct code path for CMN700 + ACPI table.

No, it's fine to claim the exact *same* range that the ACPI companion 
owns; the identical requests just nest inside each other. I don't have a 
CMN-700 to hand but here's a selection of other drivers doing just that 
from /proc/iomem on my system:

12600000-12600fff : ARMH0011:00
   12600000-12600fff : ARMH0011:00 ARMH0011:00
12610000-12610fff : ARMH0011:01
   12610000-12610fff : ARMH0011:01 ARMH0011:01
126b0000-126b0fff : APMC0D0F:00
   126b0000-126b0fff : APMC0D0F:00 APMC0D0F:00
126f0000-126f0fff : APMC0D81:00
   126f0000-126f0fff : APMC0D81:00 APMC0D81:00

And I know people are using the CMN-700 PMU on other ACPI systems 
without issue, so there's nothing wrong with the binding or the driver 
in general.

The resource conflict only arises when a request overlaps an existing 
region inexactly. Either your firmware is describing the CMN 
incorrectly, or some other driver is claiming conflicting iomem regions 
for some reason.

Thanks,
Robin.

> 2. It's not correct to apply CMN600 probe method to CMN700 because
>     CMN600 has two nested memory resouces while CMN700 should only have
>     one memory resource. And you don't want to introduce trick to handle
>     incorect ACPI DSDT.
> 
> Regards
> Yin, Fengwei
> 
>>
>> Thanks,
>> Robin.

Re: [PATCH] perf/arm-cmn: don't claim resource during ioremap() for CMN700 with ACPI

[YinFengwei]

Add Jing Zhang as we will continue discussion in this thread.

On Tue, Feb 18, 2025 at 12:31:10PM +0800, Robin Murphy wrote:
> On 2025-02-18 10:58 am, YinFengwei wrote:
> > On Tue, Feb 18, 2025 at 10:31:42AM +0800, Robin Murphy wrote:
> > > On 2025-02-18 1:21 am, Yin Fengwei wrote:
> > > > Currently, arm-cmn PMU driver assumes ACPI claim resource
> > > > for CMN600 + ACPI. But with CMN700 + ACPI, the device probe
> > > > failed because of resource claim failes when ioremap() is
> > > > called:
> > > > [   10.837300] arm-cmn ARMHC700:00: error -EBUSY: can't request region for resource [mem 0x40000000-0x4fffffff]
> > > > [   10.847310] arm-cmn ARMHC700:00: probe with driver arm-cmn failed with error -16
> > > > [   10.854726] arm-cmn ARMHC700:02: error -EBUSY: can't request region for resource [mem 0x40040000000-0x4004fffffff]
> > > > [   10.865085] arm-cmn ARMHC700:02: probe with driver arm-cmn failed with error -16
> > > > 
> > > > Let CMN700 + ACPI do same as CMN600 + ACPI to allow CMN700
> > > > work in ACPI env.
> > > 
> > > No, the CMN-600 routine is a special case for CMN-600 having two nested
> > > memory resources of its own. CMN-700 and everything else only have one
> > > memory resource, so that is not appropriate. What else is claiming the
> > > region to cause a conflict?
> > Sorry. Forgot the link for the new proposed fix:
> > https://lore.kernel.org/all/Z7QYlUP6nfBNMXsv@U-V2QX163P-2032.local/
> 
> Yes, I saw that. It's a broken diff that won't even compile, with no
> explanation of what it's supposed to be trying to achieve or why. I'm not
> sure what you're asking me to comment on.
My bad. I will attatch the full patch at the end of this mail.

> 
> > My understanding is that there are two problems here:
> > 1. ACPI claim the memory range and that's why we see this -EBUSY error
> >     with correct code path for CMN700 + ACPI table.
> 
> No, it's fine to claim the exact *same* range that the ACPI companion owns;
> the identical requests just nest inside each other. I don't have a CMN-700
> to hand but here's a selection of other drivers doing just that from
> /proc/iomem on my system:
> 
> 12600000-12600fff : ARMH0011:00
>   12600000-12600fff : ARMH0011:00 ARMH0011:00
> 12610000-12610fff : ARMH0011:01
>   12610000-12610fff : ARMH0011:01 ARMH0011:01
> 126b0000-126b0fff : APMC0D0F:00
>   126b0000-126b0fff : APMC0D0F:00 APMC0D0F:00
> 126f0000-126f0fff : APMC0D81:00
>   126f0000-126f0fff : APMC0D81:00 APMC0D81:00
I believe this works only for parents/children resource node. Otherwise,
there will be conflict.

> 
> And I know people are using the CMN-700 PMU on other ACPI systems without
> issue, so there's nothing wrong with the binding or the driver in general.
> 
> The resource conflict only arises when a request overlaps an existing region
> inexactly. Either your firmware is describing the CMN incorrectly, or some
> other driver is claiming conflicting iomem regions for some reason.
No. It's not ACPI table problem. The problem is mentioned in comments of
function arm_cmn600_acpi_probe():
        /*
         * Note that devm_ioremap_resource() is dumb and won't let the platform
         * device claim cfg when the ACPI companion device has already claimed
         * root within it. But since they *are* already both claimed in the
         * appropriate name, we don't really need to do it again here anyway.
         */

So I suppose for ACPI env, we should use devm_ioremap() as cmn600 does.
And make CMN700 handling follow spec exactly.
> 
> Thanks,
> Robin.

Regards
Yin, Fengwei

Full patch here (base commit is: 2408a807bfc3f738850ef5ad5e3fd59d66168996):

From a542d42626ec1c92cbe546b3012b1a913df35360 Mon Sep 17 00:00:00 2001
From: Yin Fengwei <fengwei_yin@linux.alibaba.com>
Date: Mon, 17 Feb 2025 15:48:24 +0800
Subject: [PATCH 1/2] perf/arm-cmn: don't claim resource during ioremap() for
 CMN700 with ACPI

Currently, arm-cmn PMU driver assumes ACPI claim resource
for CMN600 + ACPI. But with CMN700 + ACPI, the device probe
failed because of resource claim failes when function
devm_platform_ioremap_resource(pdev, 0) is called:

[   10.837300] arm-cmn ARMHC700:00: error -EBUSY: can't request region for resource [mem 0x40000000-0x4fffffff]
[   10.847310] arm-cmn ARMHC700:00: probe with driver arm-cmn failed with error -16
[   10.854726] arm-cmn ARMHC700:02: error -EBUSY: can't request region for resource [mem 0x40040000000-0x4004fffffff]
[   10.865085] arm-cmn ARMHC700:02: probe with driver arm-cmn failed with error -16

Let all ACPI cases call devm_ioremap(cmn->dev, cfg) just
like current CMN600 does.

Signed-off-by: Yin Fengwei <fengwei_yin@linux.alibaba.com>
---
 drivers/perf/arm-cmn.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index ef959e66db7c..5993a46c5560 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -2510,20 +2510,26 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 	return 0;
 }
 
-static int arm_cmn600_acpi_probe(struct platform_device *pdev, struct arm_cmn *cmn)
+static int arm_acpi_probe(struct platform_device *pdev, struct arm_cmn *cmn)
 {
-	struct resource *cfg, *root;
+	struct resource *cfg, *root = NULL;
 
 	cfg = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!cfg)
 		return -EINVAL;
 
-	root = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	if (!root)
-		return -EINVAL;
+	if (cmn->part == PART_CMN600) {
+		/* Per "ACPI for Arm Components" Table 16, CMN600 has
+		 * nested root node memory range.
+		 */
+		root = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (!root)
+			return -EINVAL;
+
+		if (!resource_contains(cfg, root))
+			swap(cfg, root);
+	}
 
-	if (!resource_contains(cfg, root))
-		swap(cfg, root);
 	/*
 	 * Note that devm_ioremap_resource() is dumb and won't let the platform
 	 * device claim cfg when the ACPI companion device has already claimed
@@ -2534,7 +2540,7 @@ static int arm_cmn600_acpi_probe(struct platform_device *pdev, struct arm_cmn *c
 	if (!cmn->base)
 		return -ENOMEM;
 
-	return root->start - cfg->start;
+	return root ? (root->start - cfg->start) : 0;
 }
 
 static int arm_cmn600_of_probe(struct device_node *np)
@@ -2559,9 +2565,9 @@ static int arm_cmn_probe(struct platform_device *pdev)
 	cmn->part = (unsigned long)device_get_match_data(cmn->dev);
 	platform_set_drvdata(pdev, cmn);
 
-	if (cmn->part == PART_CMN600 && has_acpi_companion(cmn->dev)) {
-		rootnode = arm_cmn600_acpi_probe(pdev, cmn);
-	} else {
+	if (has_acpi_companion(cmn->dev))
+		rootnode = arm_acpi_probe(pdev, cmn);
+	else {
 		rootnode = 0;
 		cmn->base = devm_platform_ioremap_resource(pdev, 0);
 		if (IS_ERR(cmn->base))
-- 
2.43.5

Re: [PATCH] perf/arm-cmn: don't claim resource during ioremap() for CMN700 with ACPI

[Robin Murphy]

On 2025-02-19 1:50 am, YinFengwei wrote:
> Add Jing Zhang as we will continue discussion in this thread.
> 
> On Tue, Feb 18, 2025 at 12:31:10PM +0800, Robin Murphy wrote:
>> On 2025-02-18 10:58 am, YinFengwei wrote:
>>> On Tue, Feb 18, 2025 at 10:31:42AM +0800, Robin Murphy wrote:
>>>> On 2025-02-18 1:21 am, Yin Fengwei wrote:
>>>>> Currently, arm-cmn PMU driver assumes ACPI claim resource
>>>>> for CMN600 + ACPI. But with CMN700 + ACPI, the device probe
>>>>> failed because of resource claim failes when ioremap() is
>>>>> called:
>>>>> [   10.837300] arm-cmn ARMHC700:00: error -EBUSY: can't request region for resource [mem 0x40000000-0x4fffffff]
>>>>> [   10.847310] arm-cmn ARMHC700:00: probe with driver arm-cmn failed with error -16
>>>>> [   10.854726] arm-cmn ARMHC700:02: error -EBUSY: can't request region for resource [mem 0x40040000000-0x4004fffffff]
>>>>> [   10.865085] arm-cmn ARMHC700:02: probe with driver arm-cmn failed with error -16
>>>>>
>>>>> Let CMN700 + ACPI do same as CMN600 + ACPI to allow CMN700
>>>>> work in ACPI env.
>>>>
>>>> No, the CMN-600 routine is a special case for CMN-600 having two nested
>>>> memory resources of its own. CMN-700 and everything else only have one
>>>> memory resource, so that is not appropriate. What else is claiming the
>>>> region to cause a conflict?
>>> Sorry. Forgot the link for the new proposed fix:
>>> https://lore.kernel.org/all/Z7QYlUP6nfBNMXsv@U-V2QX163P-2032.local/
>>
>> Yes, I saw that. It's a broken diff that won't even compile, with no
>> explanation of what it's supposed to be trying to achieve or why. I'm not
>> sure what you're asking me to comment on.
> My bad. I will attatch the full patch at the end of this mail.
> 
>>
>>> My understanding is that there are two problems here:
>>> 1. ACPI claim the memory range and that's why we see this -EBUSY error
>>>      with correct code path for CMN700 + ACPI table.
>>
>> No, it's fine to claim the exact *same* range that the ACPI companion owns;
>> the identical requests just nest inside each other. I don't have a CMN-700
>> to hand but here's a selection of other drivers doing just that from
>> /proc/iomem on my system:
>>
>> 12600000-12600fff : ARMH0011:00
>>    12600000-12600fff : ARMH0011:00 ARMH0011:00
>> 12610000-12610fff : ARMH0011:01
>>    12610000-12610fff : ARMH0011:01 ARMH0011:01
>> 126b0000-126b0fff : APMC0D0F:00
>>    126b0000-126b0fff : APMC0D0F:00 APMC0D0F:00
>> 126f0000-126f0fff : APMC0D81:00
>>    126f0000-126f0fff : APMC0D81:00 APMC0D81:00
> I believe this works only for parents/children resource node. Otherwise,
> there will be conflict.

I don't understand what you mean by that. The point here is that these
are simple devices with a single memory resource (just like CMN-700),
where in each case, a driver using devm_{platform_}ioremap_resource()
(just like arm-cmn) has happily claimed (2nd line) the same resource
already defined by the ACPI layer (1st line). Admittedly it's a little
unclear since they both use the same name, but still.

>>
>> And I know people are using the CMN-700 PMU on other ACPI systems without
>> issue, so there's nothing wrong with the binding or the driver in general.
>>
>> The resource conflict only arises when a request overlaps an existing region
>> inexactly. Either your firmware is describing the CMN incorrectly, or some
>> other driver is claiming conflicting iomem regions for some reason.
> No. It's not ACPI table problem. The problem is mentioned in comments of
> function arm_cmn600_acpi_probe():
>          /*
>           * Note that devm_ioremap_resource() is dumb and won't let the platform
>           * device claim cfg when the ACPI companion device has already claimed
>           * root within it. But since they *are* already both claimed in the
>           * appropriate name, we don't really need to do it again here anyway.
>           */

Sigh... No, this is unique to CMN-600, because only the CMN-600 ACPI
binding depends on nested resources, such that the resource tree
starts off looking like this:

50000000-5fffffff : ARMHC600:00
   50d00000-50d03fff : ARMHC600:00

If we wanted to, we can still quite happily claim the root node
resource:

--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -2410,6 +2410,8 @@ static int arm_cmn600_acpi_probe(struct platform_device *pdev, struct arm_cmn *c

         if (!resource_contains(cfg, root))
                 swap(cfg, root);
+
+       devm_request_mem_region(cmn->dev, root->start, resource_size(root), "arm-cmn!");
         /*
          * Note that devm_ioremap_resource() is dumb and won't let the platform
          * device claim cfg when the ACPI companion device has already claimed


...which then nests like so:

50000000-5fffffff : ARMHC600:00
   50d00000-50d03fff : ARMHC600:00
     50d00000-50d03fff : arm-cmn!

but what we cannot do is claim the whole 50000000-5fffffff region again
because that cannot nest within the existing 50d00000-50d03fff region.

> So I suppose for ACPI env, we should use devm_ioremap() as cmn600 does.
> And make CMN700 handling follow spec exactly.

As I said, the driver already supports the CMN-700 APCI binding
perfectly well. If your CMN is described correctly then you need to
answer my question of what *other* driver is claiming conflicting
resources and why (and if so, also why that should be specific to ACPI).

Thanks,
Robin.

Re: [PATCH] perf/arm-cmn: don't claim resource during ioremap() for CMN700 with ACPI

[YinFengwei]

On Wed, Feb 19, 2025 at 01:16:13PM +0800, Robin Murphy wrote:
> On 2025-02-19 1:50 am, YinFengwei wrote:
> > Add Jing Zhang as we will continue discussion in this thread.
> > 
> > On Tue, Feb 18, 2025 at 12:31:10PM +0800, Robin Murphy wrote:
> > > On 2025-02-18 10:58 am, YinFengwei wrote:
> > > > On Tue, Feb 18, 2025 at 10:31:42AM +0800, Robin Murphy wrote:
> > > > > On 2025-02-18 1:21 am, Yin Fengwei wrote:
> > > > > > Currently, arm-cmn PMU driver assumes ACPI claim resource
> > > > > > for CMN600 + ACPI. But with CMN700 + ACPI, the device probe
> > > > > > failed because of resource claim failes when ioremap() is
> > > > > > called:
> > > > > > [   10.837300] arm-cmn ARMHC700:00: error -EBUSY: can't request region for resource [mem 0x40000000-0x4fffffff]
> > > > > > [   10.847310] arm-cmn ARMHC700:00: probe with driver arm-cmn failed with error -16
> > > > > > [   10.854726] arm-cmn ARMHC700:02: error -EBUSY: can't request region for resource [mem 0x40040000000-0x4004fffffff]
> > > > > > [   10.865085] arm-cmn ARMHC700:02: probe with driver arm-cmn failed with error -16
> > > > > > 
> > > > > > Let CMN700 + ACPI do same as CMN600 + ACPI to allow CMN700
> > > > > > work in ACPI env.
> > > > > 
> > > > > No, the CMN-600 routine is a special case for CMN-600 having two nested
> > > > > memory resources of its own. CMN-700 and everything else only have one
> > > > > memory resource, so that is not appropriate. What else is claiming the
> > > > > region to cause a conflict?
> > > > Sorry. Forgot the link for the new proposed fix:
> > > > https://lore.kernel.org/all/Z7QYlUP6nfBNMXsv@U-V2QX163P-2032.local/
> > > 
> > > Yes, I saw that. It's a broken diff that won't even compile, with no
> > > explanation of what it's supposed to be trying to achieve or why. I'm not
> > > sure what you're asking me to comment on.
> > My bad. I will attatch the full patch at the end of this mail.
> > 
> > > 
> > > > My understanding is that there are two problems here:
> > > > 1. ACPI claim the memory range and that's why we see this -EBUSY error
> > > >      with correct code path for CMN700 + ACPI table.
> > > 
> > > No, it's fine to claim the exact *same* range that the ACPI companion owns;
> > > the identical requests just nest inside each other. I don't have a CMN-700
> > > to hand but here's a selection of other drivers doing just that from
> > > /proc/iomem on my system:
> > > 
> > > 12600000-12600fff : ARMH0011:00
> > >    12600000-12600fff : ARMH0011:00 ARMH0011:00
> > > 12610000-12610fff : ARMH0011:01
> > >    12610000-12610fff : ARMH0011:01 ARMH0011:01
> > > 126b0000-126b0fff : APMC0D0F:00
> > >    126b0000-126b0fff : APMC0D0F:00 APMC0D0F:00
> > > 126f0000-126f0fff : APMC0D81:00
> > >    126f0000-126f0fff : APMC0D81:00 APMC0D81:00
> > I believe this works only for parents/children resource node. Otherwise,
> > there will be conflict.
> 
> I don't understand what you mean by that. The point here is that these
> are simple devices with a single memory resource (just like CMN-700),
> where in each case, a driver using devm_{platform_}ioremap_resource()
> (just like arm-cmn) has happily claimed (2nd line) the same resource
> already defined by the ACPI layer (1st line). Admittedly it's a little
> unclear since they both use the same name, but still.
> 
> > > 
> > > And I know people are using the CMN-700 PMU on other ACPI systems without
> > > issue, so there's nothing wrong with the binding or the driver in general.
> > > 
> > > The resource conflict only arises when a request overlaps an existing region
> > > inexactly. Either your firmware is describing the CMN incorrectly, or some
> > > other driver is claiming conflicting iomem regions for some reason.
> > No. It's not ACPI table problem. The problem is mentioned in comments of
> > function arm_cmn600_acpi_probe():
> >          /*
> >           * Note that devm_ioremap_resource() is dumb and won't let the platform
> >           * device claim cfg when the ACPI companion device has already claimed
> >           * root within it. But since they *are* already both claimed in the
> >           * appropriate name, we don't really need to do it again here anyway.
> >           */
> 
> Sigh... No, this is unique to CMN-600, because only the CMN-600 ACPI
> binding depends on nested resources, such that the resource tree
> starts off looking like this:
> 
> 50000000-5fffffff : ARMHC600:00
>   50d00000-50d03fff : ARMHC600:00
> 
> If we wanted to, we can still quite happily claim the root node
> resource:
> 
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -2410,6 +2410,8 @@ static int arm_cmn600_acpi_probe(struct platform_device *pdev, struct arm_cmn *c
> 
>         if (!resource_contains(cfg, root))
>                 swap(cfg, root);
> +
> +       devm_request_mem_region(cmn->dev, root->start, resource_size(root), "arm-cmn!");
>         /*
>          * Note that devm_ioremap_resource() is dumb and won't let the platform
>          * device claim cfg when the ACPI companion device has already claimed
> 
> 
> ...which then nests like so:
> 
> 50000000-5fffffff : ARMHC600:00
>   50d00000-50d03fff : ARMHC600:00
>     50d00000-50d03fff : arm-cmn!
Yes. You are correct. This demo explains thing clearly to me. Thanks.

Regards
Yin, Fengwei

> 
> but what we cannot do is claim the whole 50000000-5fffffff region again
> because that cannot nest within the existing 50d00000-50d03fff region.
> 
> > So I suppose for ACPI env, we should use devm_ioremap() as cmn600 does.
> > And make CMN700 handling follow spec exactly.
> 
> As I said, the driver already supports the CMN-700 APCI binding
> perfectly well. If your CMN is described correctly then you need to
> answer my question of what *other* driver is claiming conflicting
> resources and why (and if so, also why that should be specific to ACPI).
> 
> Thanks,
> Robin.