| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <gfgtxwe3fmh4unleypyrp2qxchrqcz7wqzyoy7om2zjqev6ggf@gnnqnmusecc3>
Date: Wed, 19 Feb 2025 09:50:15 +0800
From: YinFengwei <fengwei_yin@...ux.alibaba.com>
To: Robin Murphy <robin.murphy@....com>
Cc: will@...nel.org, mark.rutland@....com,
linux-arm-kernel@...ts.infradead.org, linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
jie.li.linux@...ux.alibaba.com, renyu.zj@...ux.alibaba.com
Subject: Re: [PATCH] perf/arm-cmn: don't claim resource during ioremap() for
CMN700 with ACPI
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@...ux.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@...ux.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
Powered by blists - more mailing lists