| Message ID | 20210127113258.1421-2-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf/smmuv3: Don't reserve the PMCG register spaces | expand |
On 2021-01-27 11:32, 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'm still not a fan of this get_and_ioremap notion in general, especially when the "helper" function ends up over twice the size of all the code it replaces[1], but for the actual functional change here, Reviewed-by: Robin Murphy <robin.murphy@arm.com> > Suggested-by: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > drivers/perf/arm_smmuv3_pmu.c | 27 +++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c > index 74474bb322c3f26..e5e505a0804fe53 100644 > --- a/drivers/perf/arm_smmuv3_pmu.c > +++ b/drivers/perf/arm_smmuv3_pmu.c > @@ -761,6 +761,29 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu) > dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options); > } > > +static void __iomem * > +smmu_pmu_get_and_ioremap_resource(struct platform_device *pdev, > + unsigned int index, > + struct resource **res) > +{ > + void __iomem *base; > + struct resource *r; > + > + r = platform_get_resource(pdev, IORESOURCE_MEM, index); > + if (!r) { > + dev_err(&pdev->dev, "invalid resource\n"); > + return ERR_PTR(-EINVAL); > + } > + if (res) > + *res = r; > + > + base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); > + if (!base) > + return ERR_PTR(-ENOMEM); > + > + return base; > +} > + > static int smmu_pmu_probe(struct platform_device *pdev) > { > struct smmu_pmu *smmu_pmu; > @@ -793,7 +816,7 @@ 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, &res_0); > + smmu_pmu->reg_base = smmu_pmu_get_and_ioremap_resource(pdev, 0, &res_0); > if (IS_ERR(smmu_pmu->reg_base)) > return PTR_ERR(smmu_pmu->reg_base); > > @@ -801,7 +824,7 @@ static int smmu_pmu_probe(struct platform_device *pdev) > > /* Determine if page 1 is present */ > if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) { > - smmu_pmu->reloc_base = devm_platform_ioremap_resource(pdev, 1); > + smmu_pmu->reloc_base = smmu_pmu_get_and_ioremap_resource(pdev, 1, NULL); > if (IS_ERR(smmu_pmu->reloc_base)) > return PTR_ERR(smmu_pmu->reloc_base); > } else { > [1] ----->8----- drivers/perf/arm_smmuv3_pmu.c | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-) diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c index e5e505a0804f..c9adbc7b55a1 100644 --- a/drivers/perf/arm_smmuv3_pmu.c +++ b/drivers/perf/arm_smmuv3_pmu.c @@ -761,33 +761,10 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu) dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options); } -static void __iomem * -smmu_pmu_get_and_ioremap_resource(struct platform_device *pdev, - unsigned int index, - struct resource **res) -{ - void __iomem *base; - struct resource *r; - - r = platform_get_resource(pdev, IORESOURCE_MEM, index); - if (!r) { - dev_err(&pdev->dev, "invalid resource\n"); - return ERR_PTR(-EINVAL); - } - if (res) - *res = r; - - base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); - if (!base) - return ERR_PTR(-ENOMEM); - - return base; -} - static int smmu_pmu_probe(struct platform_device *pdev) { struct smmu_pmu *smmu_pmu; - struct resource *res_0; + struct resource *res_0, *res_1; u32 cfgr, reg_size; u64 ceid_64[2]; int irq, err; @@ -816,7 +793,10 @@ static int smmu_pmu_probe(struct platform_device *pdev) .capabilities = PERF_PMU_CAP_NO_EXCLUDE, }; - smmu_pmu->reg_base = smmu_pmu_get_and_ioremap_resource(pdev, 0, &res_0); + 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 (IS_ERR(smmu_pmu->reg_base)) return PTR_ERR(smmu_pmu->reg_base); @@ -824,7 +804,10 @@ static int smmu_pmu_probe(struct platform_device *pdev) /* Determine if page 1 is present */ if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) { - smmu_pmu->reloc_base = smmu_pmu_get_and_ioremap_resource(pdev, 1, NULL); + res_1 = platform_get_resource(pdev, IORESOURCE_MEM, 1); + if (!res_1) + return -EINVAL; + smmu_pmu->reloc_base = devm_ioremap(dev, res_1->start, resource_size(res_1)); if (IS_ERR(smmu_pmu->reloc_base)) return PTR_ERR(smmu_pmu->reloc_base); } else {
On 2021/1/29 23:06, Robin Murphy wrote: > On 2021-01-27 11:32, 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'm still not a fan of this get_and_ioremap notion in general, especially when the "helper" function ends up over twice the size of all the code it replaces[1], but for the actual functional change here, OK，I'll change it to [1] and add some comments. > > Reviewed-by: Robin Murphy <robin.murphy@arm.com> > >> Suggested-by: Robin Murphy <robin.murphy@arm.com> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> --- >> drivers/perf/arm_smmuv3_pmu.c | 27 +++++++++++++++++++++++++-- >> 1 file changed, 25 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c >> index 74474bb322c3f26..e5e505a0804fe53 100644 >> --- a/drivers/perf/arm_smmuv3_pmu.c >> +++ b/drivers/perf/arm_smmuv3_pmu.c >> @@ -761,6 +761,29 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu) >> dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options); >> } >> +static void __iomem * >> +smmu_pmu_get_and_ioremap_resource(struct platform_device *pdev, >> + unsigned int index, >> + struct resource **res) >> +{ >> + void __iomem *base; >> + struct resource *r; >> + >> + r = platform_get_resource(pdev, IORESOURCE_MEM, index); >> + if (!r) { >> + dev_err(&pdev->dev, "invalid resource\n"); >> + return ERR_PTR(-EINVAL); >> + } >> + if (res) >> + *res = r; >> + >> + base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); >> + if (!base) >> + return ERR_PTR(-ENOMEM); >> + >> + return base; >> +} >> + >> static int smmu_pmu_probe(struct platform_device *pdev) >> { >> struct smmu_pmu *smmu_pmu; >> @@ -793,7 +816,7 @@ 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, &res_0); >> + smmu_pmu->reg_base = smmu_pmu_get_and_ioremap_resource(pdev, 0, &res_0); >> if (IS_ERR(smmu_pmu->reg_base)) >> return PTR_ERR(smmu_pmu->reg_base); >> @@ -801,7 +824,7 @@ static int smmu_pmu_probe(struct platform_device *pdev) >> /* Determine if page 1 is present */ >> if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) { >> - smmu_pmu->reloc_base = devm_platform_ioremap_resource(pdev, 1); >> + smmu_pmu->reloc_base = smmu_pmu_get_and_ioremap_resource(pdev, 1, NULL); >> if (IS_ERR(smmu_pmu->reloc_base)) >> return PTR_ERR(smmu_pmu->reloc_base); >> } else { >> > > [1] > ----->8----- > drivers/perf/arm_smmuv3_pmu.c | 35 +++++++++-------------------------- > 1 file changed, 9 insertions(+), 26 deletions(-) > > diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c > index e5e505a0804f..c9adbc7b55a1 100644 > --- a/drivers/perf/arm_smmuv3_pmu.c > +++ b/drivers/perf/arm_smmuv3_pmu.c > @@ -761,33 +761,10 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu) > dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options); > } > > -static void __iomem * > -smmu_pmu_get_and_ioremap_resource(struct platform_device *pdev, > - unsigned int index, > - struct resource **res) > -{ > - void __iomem *base; > - struct resource *r; > - > - r = platform_get_resource(pdev, IORESOURCE_MEM, index); > - if (!r) { > - dev_err(&pdev->dev, "invalid resource\n"); > - return ERR_PTR(-EINVAL); > - } > - if (res) > - *res = r; > - > - base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); > - if (!base) > - return ERR_PTR(-ENOMEM); > - > - return base; > -} > - > static int smmu_pmu_probe(struct platform_device *pdev) > { > struct smmu_pmu *smmu_pmu; > - struct resource *res_0; > + struct resource *res_0, *res_1; > u32 cfgr, reg_size; > u64 ceid_64[2]; > int irq, err; > @@ -816,7 +793,10 @@ static int smmu_pmu_probe(struct platform_device *pdev) > .capabilities = PERF_PMU_CAP_NO_EXCLUDE, > }; > > - smmu_pmu->reg_base = smmu_pmu_get_and_ioremap_resource(pdev, 0, &res_0); > + 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 (IS_ERR(smmu_pmu->reg_base)) > return PTR_ERR(smmu_pmu->reg_base); > > @@ -824,7 +804,10 @@ static int smmu_pmu_probe(struct platform_device *pdev) > > /* Determine if page 1 is present */ > if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) { > - smmu_pmu->reloc_base = smmu_pmu_get_and_ioremap_resource(pdev, 1, NULL); > + res_1 = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (!res_1) > + return -EINVAL; > + smmu_pmu->reloc_base = devm_ioremap(dev, res_1->start, resource_size(res_1)); > if (IS_ERR(smmu_pmu->reloc_base)) > return PTR_ERR(smmu_pmu->reloc_base); > } else { > > . >
diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c index 74474bb322c3f26..e5e505a0804fe53 100644 --- a/drivers/perf/arm_smmuv3_pmu.c +++ b/drivers/perf/arm_smmuv3_pmu.c @@ -761,6 +761,29 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu) dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options); } +static void __iomem * +smmu_pmu_get_and_ioremap_resource(struct platform_device *pdev, + unsigned int index, + struct resource **res) +{ + void __iomem *base; + struct resource *r; + + r = platform_get_resource(pdev, IORESOURCE_MEM, index); + if (!r) { + dev_err(&pdev->dev, "invalid resource\n"); + return ERR_PTR(-EINVAL); + } + if (res) + *res = r; + + base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); + if (!base) + return ERR_PTR(-ENOMEM); + + return base; +} + static int smmu_pmu_probe(struct platform_device *pdev) { struct smmu_pmu *smmu_pmu; @@ -793,7 +816,7 @@ 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, &res_0); + smmu_pmu->reg_base = smmu_pmu_get_and_ioremap_resource(pdev, 0, &res_0); if (IS_ERR(smmu_pmu->reg_base)) return PTR_ERR(smmu_pmu->reg_base); @@ -801,7 +824,7 @@ static int smmu_pmu_probe(struct platform_device *pdev) /* Determine if page 1 is present */ if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) { - smmu_pmu->reloc_base = devm_platform_ioremap_resource(pdev, 1); + smmu_pmu->reloc_base = smmu_pmu_get_and_ioremap_resource(pdev, 1, NULL); if (IS_ERR(smmu_pmu->reloc_base)) return PTR_ERR(smmu_pmu->reloc_base); } else {
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 <robin.murphy@arm.com> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- drivers/perf/arm_smmuv3_pmu.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)