Messages in this thread | | | Date | Tue, 24 Aug 2021 17:44:51 +0100 | From | Mark Rutland <> | Subject | Re: [PATCH 05/13] clocksource/arm_arch_timer: Fix MMIO base address vs callback ordering issue |
| |
On Mon, Aug 09, 2021 at 04:26:43PM +0100, Marc Zyngier wrote: > The MMIO timer base address gets published after we have registered > the callbacks and the interrupt handler, which is... a bit dangerous. > > Fix this by moving the base address publication to the point where > we register the timer, and expose a pointer to the timer structure > itself rather than a naked value. > > Signed-off-by: Marc Zyngier <maz@kernel.org>
I don't have agood setup to test this with, but this looks good to me, and builds cleanly for arm/arm64, so:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> --- > drivers/clocksource/arm_arch_timer.c | 27 +++++++++++++-------------- > 1 file changed, 13 insertions(+), 14 deletions(-) > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index 160464f75017..ca7761d8459a 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -54,13 +54,13 @@ > > static unsigned arch_timers_present __initdata; > > -static void __iomem *arch_counter_base __ro_after_init; > - > struct arch_timer { > void __iomem *base; > struct clock_event_device evt; > }; > > +static struct arch_timer *arch_timer_mem __ro_after_init; > + > #define to_arch_timer(e) container_of(e, struct arch_timer, evt) > > static u32 arch_timer_rate __ro_after_init; > @@ -975,9 +975,9 @@ static u64 arch_counter_get_cntvct_mem(void) > u32 vct_lo, vct_hi, tmp_hi; > > do { > - vct_hi = readl_relaxed(arch_counter_base + CNTVCT_HI); > - vct_lo = readl_relaxed(arch_counter_base + CNTVCT_LO); > - tmp_hi = readl_relaxed(arch_counter_base + CNTVCT_HI); > + vct_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI); > + vct_lo = readl_relaxed(arch_timer_mem->base + CNTVCT_LO); > + tmp_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI); > } while (vct_hi != tmp_hi); > > return ((u64) vct_hi << 32) | vct_lo; > @@ -1168,25 +1168,25 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq) > { > int ret; > irq_handler_t func; > - struct arch_timer *t; > > - t = kzalloc(sizeof(*t), GFP_KERNEL); > - if (!t) > + arch_timer_mem = kzalloc(sizeof(*arch_timer_mem), GFP_KERNEL); > + if (!arch_timer_mem) > return -ENOMEM; > > - t->base = base; > - t->evt.irq = irq; > - __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &t->evt); > + arch_timer_mem->base = base; > + arch_timer_mem->evt.irq = irq; > + __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &arch_timer_mem->evt); > > if (arch_timer_mem_use_virtual) > func = arch_timer_handler_virt_mem; > else > func = arch_timer_handler_phys_mem; > > - ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &t->evt); > + ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &arch_timer_mem->evt); > if (ret) { > pr_err("Failed to request mem timer irq\n"); > - kfree(t); > + kfree(arch_timer_mem); > + arch_timer_mem = NULL; > } > > return ret; > @@ -1444,7 +1444,6 @@ arch_timer_mem_frame_register(struct arch_timer_mem_frame *frame) > return ret; > } > > - arch_counter_base = base; > arch_timers_present |= ARCH_TIMER_TYPE_MEM; > > return 0; > -- > 2.30.2 >
| |