Re: [PATCH v4 1/2] perf/smmuv3: Don't reserve the PMCG register spaces
On 2021/2/1 20:54, Will Deacon wrote: > On Sat, Jan 30, 2021 at 03:14:13PM +0800, Zhen Lei wrote: >> According to the SMMUv3 specification: >> Each PMCG counter group is represented by one 4KB page (Page 0) with one >> optional additional 4KB page (Page 1), both of which are at IMPLEMENTATION >> DEFINED base addresses. >> >> This means that the PMCG register spaces may be within the 64KB pages of >> the SMMUv3 register space. When both the SMMU and PMCG drivers reserve >> their own resources, a resource conflict occurs. >> >> To avoid this conflict, don't reserve the PMCG regions. >> >> Suggested-by: Robin Murphy >> Signed-off-by: Zhen Lei >> --- >> drivers/perf/arm_smmuv3_pmu.c | 25 +++-- >> 1 file changed, 19 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c >> index 74474bb322c3f26..5e894f957c7b935 100644 >> --- a/drivers/perf/arm_smmuv3_pmu.c >> +++ b/drivers/perf/arm_smmuv3_pmu.c >> @@ -793,17 +793,30 @@ static int smmu_pmu_probe(struct platform_device *pdev) >> .capabilities = PERF_PMU_CAP_NO_EXCLUDE, >> }; >> >> -smmu_pmu->reg_base = devm_platform_get_and_ioremap_resource(pdev, 0, >> _0); >> -if (IS_ERR(smmu_pmu->reg_base)) >> -return PTR_ERR(smmu_pmu->reg_base); >> +/* >> + * The register spaces of the PMCG may be in the register space of >> + * other devices. For example, SMMU. Therefore, the PMCG resources are >> + * not reserved to avoid resource conflicts with other drivers. >> + */ >> +res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> +if (!res_0) >> +return ERR_PTR(-EINVAL); > > I tried to apply this, but you've got your return type in a muddle: I'm dizzy. I don't know how this bug patch came out. I just pinched my leg, like I'm still in the real world. The "ERR_PTR()" of the four ERR_PTR(xxx) should be removed. Can you help me? Or I send a new one. > > @@ @@ > +drivers/perf/arm_smmuv3_pmu.c: In function ‘smmu_pmu_probe’: > +drivers/perf/arm_smmuv3_pmu.c:803:10: warning: returning ‘void *’ from a > function with return type ‘int’ makes integer from pointer without a cast > [-Wint-conversion] > + 803 | return ERR_PTR(-EINVAL); > + | ^~~~ > +drivers/perf/arm_smmuv3_pmu.c:803:31: warning: incorrect type in return > expression (different base types) [sparse] > +drivers/perf/arm_smmuv3_pmu.c:803:31:expected int [sparse] > +drivers/perf/arm_smmuv3_pmu.c:803:31:got void * [sparse] > +drivers/perf/arm_smmuv3_pmu.c:806:10: warning: returning ‘void *’ from a > function with return type ‘int’ makes integer from pointer without a cast > [-Wint-conversion] > + 806 | return ERR_PTR(-ENOMEM); > + | ^~~~ > +drivers/perf/arm_smmuv3_pmu.c:806:31: warning: incorrect type in return > expression (different base types) [sparse] > +drivers/perf/arm_smmuv3_pmu.c:806:31:expected int [sparse] > +drivers/perf/arm_smmuv3_pmu.c:806:31:got void * [sparse] > +drivers/perf/arm_smmuv3_pmu.c:816:11: warning: returning ‘void *’ from a > function with return type ‘int’ makes integer from pointer without a cast > [-Wint-conversion] > + 816 |return ERR_PTR(-EINVAL); > + | ^~~~ > +drivers/perf/arm_smmuv3_pmu.c:816:39: warning: incorrect type in return > expression (different base types) [sparse] > +drivers/perf/arm_smmuv3_pmu.c:816:39:expected int [sparse] > +drivers/perf/arm_smmuv3_pmu.c:816:39:got void * [sparse] > +drivers/perf/arm_smmuv3_pmu.c:819:11: warning: returning ‘void *’ from a > function with return type ‘int’ makes integer from pointer without a cast > [-Wint-conversion] > + 819 |return ERR_PTR(-ENOMEM); > + | ^~~~ > +drivers/perf/arm_smmuv3_pmu.c:819:39: warning: incorrect type in return > expression (different base types) [sparse] > +drivers/perf/arm_smmuv3_pmu.c:819:39:expected int [sparse] > +drivers/perf/arm_smmuv3_pmu.c:819:39:got void * [sparse] > > Will > > . >
Re: [PATCH v4 1/2] perf/smmuv3: Don't reserve the PMCG register spaces
On Sat, Jan 30, 2021 at 03:14:13PM +0800, Zhen Lei wrote: > According to the SMMUv3 specification: > Each PMCG counter group is represented by one 4KB page (Page 0) with one > optional additional 4KB page (Page 1), both of which are at IMPLEMENTATION > DEFINED base addresses. > > This means that the PMCG register spaces may be within the 64KB pages of > the SMMUv3 register space. When both the SMMU and PMCG drivers reserve > their own resources, a resource conflict occurs. > > To avoid this conflict, don't reserve the PMCG regions. > > Suggested-by: Robin Murphy > Signed-off-by: Zhen Lei > --- > drivers/perf/arm_smmuv3_pmu.c | 25 +++-- > 1 file changed, 19 insertions(+), 6 deletions(-) > > diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c > index 74474bb322c3f26..5e894f957c7b935 100644 > --- a/drivers/perf/arm_smmuv3_pmu.c > +++ b/drivers/perf/arm_smmuv3_pmu.c > @@ -793,17 +793,30 @@ static int smmu_pmu_probe(struct platform_device *pdev) > .capabilities = PERF_PMU_CAP_NO_EXCLUDE, > }; > > - smmu_pmu->reg_base = devm_platform_get_and_ioremap_resource(pdev, 0, > _0); > - if (IS_ERR(smmu_pmu->reg_base)) > - return PTR_ERR(smmu_pmu->reg_base); > + /* > + * The register spaces of the PMCG may be in the register space of > + * other devices. For example, SMMU. Therefore, the PMCG resources are > + * not reserved to avoid resource conflicts with other drivers. > + */ > + res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res_0) > + return ERR_PTR(-EINVAL); I tried to apply this, but you've got your return type in a muddle: @@ @@ +drivers/perf/arm_smmuv3_pmu.c: In function ‘smmu_pmu_probe’: +drivers/perf/arm_smmuv3_pmu.c:803:10: warning: returning ‘void *’ from a function with return type ‘int’ makes integer from pointer without a cast [-Wint-conversion] + 803 | return ERR_PTR(-EINVAL); + | ^~~~ +drivers/perf/arm_smmuv3_pmu.c:803:31: warning: incorrect type in return expression (different base types) [sparse] +drivers/perf/arm_smmuv3_pmu.c:803:31:expected int [sparse] +drivers/perf/arm_smmuv3_pmu.c:803:31:got void * [sparse] +drivers/perf/arm_smmuv3_pmu.c:806:10: warning: returning ‘void *’ from a function with return type ‘int’ makes integer from pointer without a cast [-Wint-conversion] + 806 | return ERR_PTR(-ENOMEM); + | ^~~~ +drivers/perf/arm_smmuv3_pmu.c:806:31: warning: incorrect type in return expression (different base types) [sparse] +drivers/perf/arm_smmuv3_pmu.c:806:31:expected int [sparse] +drivers/perf/arm_smmuv3_pmu.c:806:31:got void * [sparse] +drivers/perf/arm_smmuv3_pmu.c:816:11: warning: returning ‘void *’ from a function with return type ‘int’ makes integer from pointer without a cast [-Wint-conversion] + 816 |return ERR_PTR(-EINVAL); + | ^~~~ +drivers/perf/arm_smmuv3_pmu.c:816:39: warning: incorrect type in return expression (different base types) [sparse] +drivers/perf/arm_smmuv3_pmu.c:816:39:expected int [sparse] +drivers/perf/arm_smmuv3_pmu.c:816:39:got void * [sparse] +drivers/perf/arm_smmuv3_pmu.c:819:11: warning: returning ‘void *’ from a function with return type ‘int’ makes integer from pointer without a cast [-Wint-conversion] + 819 |return ERR_PTR(-ENOMEM); + | ^~~~ +drivers/perf/arm_smmuv3_pmu.c:819:39: warning: incorrect type in return expression (different base types) [sparse] +drivers/perf/arm_smmuv3_pmu.c:819:39:expected int [sparse] +drivers/perf/arm_smmuv3_pmu.c:819:39:got void * [sparse] Will
Re: [PATCH v4 1/2] perf/smmuv3: Don't reserve the PMCG register spaces
On 2021-01-30 07:14, Zhen Lei wrote:
According to the SMMUv3 specification:
Each PMCG counter group is represented by one 4KB page (Page 0) with one
optional additional 4KB page (Page 1), both of which are at IMPLEMENTATION
DEFINED base addresses.
This means that the PMCG register spaces may be within the 64KB pages of
the SMMUv3 register space. When both the SMMU and PMCG drivers reserve
their own resources, a resource conflict occurs.
To avoid this conflict, don't reserve the PMCG regions.
I said my review on v3 stood either way, but for the avoidance of doubt,
Reviewed-by: Robin Murphy
I hadn't considered that a comment is a very good idea, in case the
cleanup-script crew find this in future and try to "simplify" it :)
Thanks,
Robin.
Suggested-by: Robin Murphy
Signed-off-by: Zhen Lei
---
drivers/perf/arm_smmuv3_pmu.c | 25 +++--
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index 74474bb322c3f26..5e894f957c7b935 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -793,17 +793,30 @@ static int smmu_pmu_probe(struct platform_device *pdev)
.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
};
- smmu_pmu->reg_base = devm_platform_get_and_ioremap_resource(pdev, 0, _0);
- if (IS_ERR(smmu_pmu->reg_base))
- return PTR_ERR(smmu_pmu->reg_base);
+ /*
+* The register spaces of the PMCG may be in the register space of
+* other devices. For example, SMMU. Therefore, the PMCG resources are
+* not reserved to avoid resource conflicts with other drivers.
+*/
+ res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res_0)
+ return ERR_PTR(-EINVAL);
+ smmu_pmu->reg_base = devm_ioremap(dev, res_0->start,
resource_size(res_0));
+ if (!smmu_pmu->reg_base)
+ return ERR_PTR(-ENOMEM);
cfgr = readl_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CFGR);
/* Determine if page 1 is present */
if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) {
- smmu_pmu->reloc_base = devm_platform_ioremap_resource(pdev, 1);
- if (IS_ERR(smmu_pmu->reloc_base))
- return PTR_ERR(smmu_pmu->reloc_base);
+ struct resource *res_1;
+
+ res_1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!res_1)
+ return ERR_PTR(-EINVAL);
+ smmu_pmu->reloc_base = devm_ioremap(dev, res_1->start,
resource_size(res_1));
+ if (!smmu_pmu->reloc_base)
+ return ERR_PTR(-ENOMEM);
} else {
smmu_pmu->reloc_base = smmu_pmu->reg_base;
}
Re: [PATCH v4 1/2] perf/smmuv3: Don't reserve the PMCG register spaces
Hi, Robin:
Can you review this patch again?
On 2021/1/30 15:14, Zhen Lei wrote:
> According to the SMMUv3 specification:
> Each PMCG counter group is represented by one 4KB page (Page 0) with one
> optional additional 4KB page (Page 1), both of which are at IMPLEMENTATION
> DEFINED base addresses.
>
> This means that the PMCG register spaces may be within the 64KB pages of
> the SMMUv3 register space. When both the SMMU and PMCG drivers reserve
> their own resources, a resource conflict occurs.
>
> To avoid this conflict, don't reserve the PMCG regions.
>
> Suggested-by: Robin Murphy
> Signed-off-by: Zhen Lei
> ---
> drivers/perf/arm_smmuv3_pmu.c | 25 +++--
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
> index 74474bb322c3f26..5e894f957c7b935 100644
> --- a/drivers/perf/arm_smmuv3_pmu.c
> +++ b/drivers/perf/arm_smmuv3_pmu.c
> @@ -793,17 +793,30 @@ static int smmu_pmu_probe(struct platform_device *pdev)
> .capabilities = PERF_PMU_CAP_NO_EXCLUDE,
> };
>
> - smmu_pmu->reg_base = devm_platform_get_and_ioremap_resource(pdev, 0,
> _0);
> - if (IS_ERR(smmu_pmu->reg_base))
> - return PTR_ERR(smmu_pmu->reg_base);
> + /*
> + * The register spaces of the PMCG may be in the register space of
> + * other devices. For example, SMMU. Therefore, the PMCG resources are
> + * not reserved to avoid resource conflicts with other drivers.
> + */
> + res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res_0)
> + return ERR_PTR(-EINVAL);
> + smmu_pmu->reg_base = devm_ioremap(dev, res_0->start,
> resource_size(res_0));
> + if (!smmu_pmu->reg_base)
> + return ERR_PTR(-ENOMEM);
>
> cfgr = readl_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CFGR);
>
> /* Determine if page 1 is present */
> if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) {
> - smmu_pmu->reloc_base = devm_platform_ioremap_resource(pdev, 1);
> - if (IS_ERR(smmu_pmu->reloc_base))
> - return PTR_ERR(smmu_pmu->reloc_base);
> + struct resource *res_1;
> +
> + res_1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res_1)
> + return ERR_PTR(-EINVAL);
> + smmu_pmu->reloc_base = devm_ioremap(dev, res_1->start,
> resource_size(res_1));
> + if (!smmu_pmu->reloc_base)
> + return ERR_PTR(-ENOMEM);
> } else {
> smmu_pmu->reloc_base = smmu_pmu->reg_base;
> }
>
[PATCH v4 1/2] perf/smmuv3: Don't reserve the PMCG register spaces
According to the SMMUv3 specification:
Each PMCG counter group is represented by one 4KB page (Page 0) with one
optional additional 4KB page (Page 1), both of which are at IMPLEMENTATION
DEFINED base addresses.
This means that the PMCG register spaces may be within the 64KB pages of
the SMMUv3 register space. When both the SMMU and PMCG drivers reserve
their own resources, a resource conflict occurs.
To avoid this conflict, don't reserve the PMCG regions.
Suggested-by: Robin Murphy
Signed-off-by: Zhen Lei
---
drivers/perf/arm_smmuv3_pmu.c | 25 +++--
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index 74474bb322c3f26..5e894f957c7b935 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -793,17 +793,30 @@ static int smmu_pmu_probe(struct platform_device *pdev)
.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
};
- smmu_pmu->reg_base = devm_platform_get_and_ioremap_resource(pdev, 0,
_0);
- if (IS_ERR(smmu_pmu->reg_base))
- return PTR_ERR(smmu_pmu->reg_base);
+ /*
+* The register spaces of the PMCG may be in the register space of
+* other devices. For example, SMMU. Therefore, the PMCG resources are
+* not reserved to avoid resource conflicts with other drivers.
+*/
+ res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res_0)
+ return ERR_PTR(-EINVAL);
+ smmu_pmu->reg_base = devm_ioremap(dev, res_0->start,
resource_size(res_0));
+ if (!smmu_pmu->reg_base)
+ return ERR_PTR(-ENOMEM);
cfgr = readl_relaxed(smmu_pmu->reg_base + SMMU_PMCG_CFGR);
/* Determine if page 1 is present */
if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) {
- smmu_pmu->reloc_base = devm_platform_ioremap_resource(pdev, 1);
- if (IS_ERR(smmu_pmu->reloc_base))
- return PTR_ERR(smmu_pmu->reloc_base);
+ struct resource *res_1;
+
+ res_1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!res_1)
+ return ERR_PTR(-EINVAL);
+ smmu_pmu->reloc_base = devm_ioremap(dev, res_1->start,
resource_size(res_1));
+ if (!smmu_pmu->reloc_base)
+ return ERR_PTR(-ENOMEM);
} else {
smmu_pmu->reloc_base = smmu_pmu->reg_base;
}
--
1.8.3
