| Message ID | eb104cfdf44ef9bf7b4f271555e1496d2bf781bf.1500510157.git.mirq-linux@rere.qmqm.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 20, 2017 at 02:29:23AM +0200, Michał Mirosław wrote: > Share l2x0_base between cache-l2x0 and pmu drivers. They are duplicates. This is not quite true, and this change introduces bugs. The l2x0 pmu driver only sets its copy of the base for an l2x0 variant it recognises: > -void __init l2x0_pmu_register(void __iomem *base, u32 part) > +void __init l2x0_pmu_register(u32 part) > { > /* > * Determine whether we support the PMU, and choose the name for sysfs. > @@ -503,8 +502,7 @@ void __init l2x0_pmu_register(void __iomem *base, u32 part) > * supported by this driver. > * > * We must defer registering the PMU until the perf subsystem is up and > - * running, so just stash the name and base, and leave that to another > - * initcall. > + * running, so just stash the name, and leave that to another initcall. > */ > switch (part & L2X0_CACHE_ID_PART_MASK) { > case L2X0_CACHE_ID_PART_L220: > @@ -516,8 +514,6 @@ void __init l2x0_pmu_register(void __iomem *base, u32 part) > default: > return; > } > - > - l2x0_base = base; ... note that we may return *before* setting l2x0_base. This (deliberately) prevents us from erroneously registering a PMU for l2x0 variants for which we don't know how to drive any IMPLEMENTATION DEFINED PMU functionality. This cahnge breaks that, and will result in l2x0_pmu_init() trying to register a PMU with a NULL name. So NAK to this change. Thanks, Mark.
On Thu, Oct 05, 2017 at 05:48:32PM +0100, Mark Rutland wrote: > On Thu, Jul 20, 2017 at 02:29:23AM +0200, Michał Mirosław wrote: > > Share l2x0_base between cache-l2x0 and pmu drivers. They are duplicates. > > This is not quite true, and this change introduces bugs. > > The l2x0 pmu driver only sets its copy of the base for an l2x0 variant > it recognises: > > > -void __init l2x0_pmu_register(void __iomem *base, u32 part) > > +void __init l2x0_pmu_register(u32 part) > > { > > /* > > * Determine whether we support the PMU, and choose the name for sysfs. > > @@ -503,8 +502,7 @@ void __init l2x0_pmu_register(void __iomem *base, u32 part) > > * supported by this driver. > > * > > * We must defer registering the PMU until the perf subsystem is up and > > - * running, so just stash the name and base, and leave that to another > > - * initcall. > > + * running, so just stash the name, and leave that to another initcall. > > */ > > switch (part & L2X0_CACHE_ID_PART_MASK) { > > case L2X0_CACHE_ID_PART_L220: > > @@ -516,8 +514,6 @@ void __init l2x0_pmu_register(void __iomem *base, u32 part) > > default: > > return; > > } > > - > > - l2x0_base = base; > > ... note that we may return *before* setting l2x0_base. > > This (deliberately) prevents us from erroneously registering a PMU for > l2x0 variants for which we don't know how to drive any IMPLEMENTATION > DEFINED PMU functionality. > > This cahnge breaks that, and will result in l2x0_pmu_init() trying to > register a PMU with a NULL name. > > So NAK to this change. Also I don't want the virtual base address of the L2 cache exposed to the world, that's an invitation for people to do what they did years ago, and throw code left right and centre into the tree that accesses the L2 cache directly from all sorts of places - the result was very much a mess that took a big patch series to sort out.
diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h index 736292b42fca..492de655e4f3 100644 --- a/arch/arm/include/asm/hardware/cache-l2x0.h +++ b/arch/arm/include/asm/hardware/cache-l2x0.h @@ -167,11 +167,11 @@ static inline int l2x0_of_init(u32 aux_val, u32 aux_mask) #endif #ifdef CONFIG_CACHE_L2X0_PMU -void l2x0_pmu_register(void __iomem *base, u32 part); +void l2x0_pmu_register(u32 part); void l2x0_pmu_suspend(void); void l2x0_pmu_resume(void); #else -static inline void l2x0_pmu_register(void __iomem *base, u32 part) {} +static inline void l2x0_pmu_register(u32 part) {} static inline void l2x0_pmu_suspend(void) {} static inline void l2x0_pmu_resume(void) {} #endif @@ -193,6 +193,7 @@ struct l2x0_regs { unsigned long aux2_ctrl; }; +extern void __iomem *l2x0_base; extern struct l2x0_regs l2x0_saved_regs; #endif /* __ASSEMBLY__ */ diff --git a/arch/arm/mm/cache-l2x0-pmu.c b/arch/arm/mm/cache-l2x0-pmu.c index 0a1e2280141f..1f36801d337f 100644 --- a/arch/arm/mm/cache-l2x0-pmu.c +++ b/arch/arm/mm/cache-l2x0-pmu.c @@ -29,7 +29,6 @@ #define PMU_NR_COUNTERS 2 -static void __iomem *l2x0_base; static struct pmu *l2x0_pmu; static cpumask_t pmu_cpu; @@ -491,7 +490,7 @@ void l2x0_pmu_resume(void) l2x0_pmu_enable(l2x0_pmu); } -void __init l2x0_pmu_register(void __iomem *base, u32 part) +void __init l2x0_pmu_register(u32 part) { /* * Determine whether we support the PMU, and choose the name for sysfs. @@ -503,8 +502,7 @@ void __init l2x0_pmu_register(void __iomem *base, u32 part) * supported by this driver. * * We must defer registering the PMU until the perf subsystem is up and - * running, so just stash the name and base, and leave that to another - * initcall. + * running, so just stash the name, and leave that to another initcall. */ switch (part & L2X0_CACHE_ID_PART_MASK) { case L2X0_CACHE_ID_PART_L220: @@ -516,8 +514,6 @@ void __init l2x0_pmu_register(void __iomem *base, u32 part) default: return; } - - l2x0_base = base; } static __init int l2x0_pmu_init(void) diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c index ea1e70ff4568..bbfbc18399f9 100644 --- a/arch/arm/mm/cache-l2x0.c +++ b/arch/arm/mm/cache-l2x0.c @@ -48,13 +48,13 @@ struct l2c_init_data { #define CACHE_LINE_SIZE 32 -static void __iomem *l2x0_base; static const struct l2c_init_data *l2x0_data; static DEFINE_RAW_SPINLOCK(l2x0_lock); static u32 l2x0_way_mask; /* Bitmask of active ways */ static u32 l2x0_size; static unsigned long sync_reg_offset = L2X0_CACHE_SYNC; +void __iomem *l2x0_base; struct l2x0_regs l2x0_saved_regs; static bool l2x0_bresp_disable; @@ -900,7 +900,7 @@ static int __init __l2c_init(const struct l2c_init_data *data, pr_info("%s: CACHE_ID 0x%08x, AUX_CTRL 0x%08x\n", data->type, cache_id, aux); - l2x0_pmu_register(l2x0_base, cache_id); + l2x0_pmu_register(cache_id); return 0; }
Share l2x0_base between cache-l2x0 and pmu drivers. They are duplicates. Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- arch/arm/include/asm/hardware/cache-l2x0.h | 5 +++-- arch/arm/mm/cache-l2x0-pmu.c | 8 ++------ arch/arm/mm/cache-l2x0.c | 4 ++-- 3 files changed, 7 insertions(+), 10 deletions(-)