linux-kernel - Re: [PATCH 1/1] clocksource: sun4i: Save and restore timer registers before and after sleeping

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>] [day] [month] [year] [list]

Message-ID: <20201019093146.purztwtytlozva3t@gilmour.lan>
Date:   Mon, 19 Oct 2020 11:31:46 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     wuyan <wuyan@...winnertech.com>
Cc:     daniel.lezcano@...aro.org, tglx@...utronix.de, wens@...e.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        frank@...winnertech.com
Subject: Re: [PATCH 1/1] clocksource: sun4i: Save and restore timer registers
 before and after sleeping

Hi!

On Sat, Oct 10, 2020 at 06:46:03PM +0800, wuyan wrote:
> Signed-off-by: wuyan <wuyan@...winnertech.com>

A commit log would be welcome here. Also, the last time you contributed
you used the name Martin Wu in your Signed-off-by, it would be nice to
be consistent there.

> Change-Id: I7edbc00fd0968d0301757f5a75dbd6f53d6a7cd7

This should be removed

> ---
>  drivers/clocksource/timer-sun4i.c | 45 +++++++++++++++++++++++++++++--
>  1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-sun4i.c b/drivers/clocksource/timer-sun4i.c
> index 0ba8155b8287..49fb6b90ec15 100644
> --- a/drivers/clocksource/timer-sun4i.c
> +++ b/drivers/clocksource/timer-sun4i.c
> @@ -29,6 +29,7 @@
>  #define TIMER_IRQ_EN_REG	0x00
>  #define TIMER_IRQ_EN(val)		BIT(val)
>  #define TIMER_IRQ_ST_REG	0x04
> +#define TIMER_IRQ_CLEAR(val)		BIT(val)
>  #define TIMER_CTL_REG(val)	(0x10 * val + 0x10)
>  #define TIMER_CTL_ENABLE		BIT(0)
>  #define TIMER_CTL_RELOAD		BIT(1)
> @@ -41,6 +42,19 @@
>  
>  #define TIMER_SYNC_TICKS	3
>  
> +/* Registers which needs to be saved and restored before and after sleeping */
> +static u32 regs_offset[] = {

It would be better to have a prefix (like sun4i_timer to be consistent)
there so that we know it's less confusing and we know it's not some
generic thing.

> +	TIMER_IRQ_EN_REG,
> +	TIMER_IRQ_ST_REG,

Why do you need to save the interrupt status register?

> +	TIMER_CTL_REG(0),
> +	TIMER_INTVAL_REG(0),
> +	TIMER_CNTVAL_REG(0),
> +	TIMER_CTL_REG(1),
> +	TIMER_INTVAL_REG(1),
> +	TIMER_CNTVAL_REG(1),
> +};
> +static u32 regs_backup[ARRAY_SIZE(regs_offset)];

We should store this one in the timer_of struct so that we don't have
any issue if there's two timers at some point.

>  /*
>   * When we disable a timer, we need to wait at least for 2 cycles of
>   * the timer source clock. We will use for that the clocksource timer
> @@ -82,10 +96,37 @@ static void sun4i_clkevt_time_start(void __iomem *base, u8 timer,
>  	       base + TIMER_CTL_REG(timer));
>  }
>  
> +static inline void save_regs(void __iomem *base)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(regs_offset); i++)
> +		regs_backup[i] = readl(base + regs_offset[i]);
> +}
> +
> +static inline void restore_regs(void __iomem *base)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(regs_offset); i++)
> +		writel(regs_backup[i], base + regs_offset[i]);
> +}
> +

Same thing here, using the prefix would be nice for those two functions
name.

>  static int sun4i_clkevt_shutdown(struct clock_event_device *evt)
>  {
>  	struct timer_of *to = to_timer_of(evt);
>  
> +	save_regs(timer_of_base(to));
> +	sun4i_clkevt_time_stop(timer_of_base(to), 0);
> +
> +	return 0;
> +}
> +
> +static int sun4i_tick_resume(struct clock_event_device *evt)
> +{
> +	struct timer_of *to = to_timer_of(evt);
> +
> +	restore_regs(timer_of_base(to));
>  	sun4i_clkevt_time_stop(timer_of_base(to), 0);
>  
>  	return 0;
> @@ -126,7 +167,7 @@ static int sun4i_clkevt_next_event(unsigned long evt,
>  
>  static void sun4i_timer_clear_interrupt(void __iomem *base)
>  {
> -	writel(TIMER_IRQ_EN(0), base + TIMER_IRQ_ST_REG);
> +	writel(TIMER_IRQ_CLEAR(0), base + TIMER_IRQ_ST_REG);
>  }

This is mostly a cosmetic change right? Either way, it should be in a
separate patch.

Thanks!
Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)