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

From: Robin Murphy
Date: Wed Feb 19 2025 - 08:16:26 EST


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.