linux-kernel - Re: [PATCH] clocksource/drivers/arm_global_timer: Always use {readl|writel}

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 13 Nov 2015 10:28:01 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	linux-arm-kernel@...ts.infradead.org
Cc:	Jisheng Zhang <jszhang@...vell.com>, srinivas.kandagatla@...il.com,
	maxime.coquelin@...com, patrice.chotard@...com,
	daniel.lezcano@...aro.org, tglx@...utronix.de, kernel@...inux.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clocksource/drivers/arm_global_timer: Always use {readl|writel}_relaxed

On Friday 13 November 2015 16:40:25 Jisheng Zhang wrote:
> On Fri, 13 Nov 2015 16:34:38 +0800
> Jisheng Zhang <jszhang@...vell.com> wrote:
> 
> > This driver use both readl/writel and their relaxed version, this patch
> > tries to unify the io accesses.
> 
> I'm sorry. This is the version I'd like to send for review and merge. Can you
> please kindly have a review?

I would prefer to use write_relaxed() as sparingly as we can, it is too
hard to verify each case to ensure that we don't have to watch out
for ordering or locking issues.

> > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
> > index a2cb6fa..84a5a5d 100644
> > --- a/drivers/clocksource/arm_global_timer.c
> > +++ b/drivers/clocksource/arm_global_timer.c
> > @@ -99,27 +99,27 @@ static void gt_compare_set(unsigned long delta, int periodic)
> >  
> >  	counter += delta;
> >  	ctrl = GT_CONTROL_TIMER_ENABLE;
> > -	writel(ctrl, gt_base + GT_CONTROL);
> > -	writel(lower_32_bits(counter), gt_base + GT_COMP0);
> > -	writel(upper_32_bits(counter), gt_base + GT_COMP1);
> > +	writel_relaxed(ctrl, gt_base + GT_CONTROL);
> > +	writel_relaxed(lower_32_bits(counter), gt_base + GT_COMP0);
> > +	writel_relaxed(upper_32_bits(counter), gt_base + GT_COMP1);
> >  
> >  	if (periodic) {
> > -		writel(delta, gt_base + GT_AUTO_INC);
> > +		writel_relaxed(delta, gt_base + GT_AUTO_INC);
> >  		ctrl |= GT_CONTROL_AUTO_INC;
> >  	}
> >  
> >  	ctrl |= GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE;
> > -	writel(ctrl, gt_base + GT_CONTROL);
> > +	writel_relaxed(ctrl, gt_base + GT_CONTROL);
> >  }

This seems fine. Do you have any performance numbers to show how much
we save per call on a platform you care about, and how often it is
called for a typical workload?

I see that _gt_counter_read() already uses readl_relaxed(), and it
seems to be a much bigger win there, as we read the clock more
often than we write the comparator, so the person who did that
probably thought that this one wasn't important enough. Can you
add an explanation in the changelog why you think that was a
mistake?

Unifying the accessors across a driver is not enough of a reason
I think.

> >  static int gt_clockevent_shutdown(struct clock_event_device *evt)
> >  {
> >  	unsigned long ctrl;
> >  
> > -	ctrl = readl(gt_base + GT_CONTROL);
> > +	ctrl = readl_relaxed(gt_base + GT_CONTROL);
> >  	ctrl &= ~(GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE |
> >  		  GT_CONTROL_AUTO_INC);
> > -	writel(ctrl, gt_base + GT_CONTROL);
> > +	writel_relaxed(ctrl, gt_base + GT_CONTROL);
> >  	return 0;
> >  }

This is certainly not performance critical, better leave it using
the standard accessors.

> > @@ -212,11 +212,11 @@ static u64 notrace gt_sched_clock_read(void)
> >  
> >  static void __init gt_clocksource_init(void)
> >  {
> > -	writel(0, gt_base + GT_CONTROL);
> > -	writel(0, gt_base + GT_COUNTER0);
> > -	writel(0, gt_base + GT_COUNTER1);
> > +	writel_relaxed(0, gt_base + GT_CONTROL);
> > +	writel_relaxed(0, gt_base + GT_COUNTER0);
> > +	writel_relaxed(0, gt_base + GT_COUNTER1);
> >  	/* enables timer on all the cores */
> > -	writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
> > +	writel_relaxed(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
> >  
> >  #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
> >  	sched_clock_register(gt_sched_clock_read, 64, gt_clk_rate);
> 
> 

Same here.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/