linux-kernel - Re: [4/4] arch/powerpc/44x/fsp2: wdt tcr update instead of whole rewrite

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

Message-Id: <3wcSXH5BdSz9s81@ozlabs.org>
Date:   Tue, 30 May 2017 19:11:27 +1000 (AEST)
From:   Michael Ellerman <patch-notifications@...erman.id.au>
To:     Ivan Mikhaylov <ivan@...ibm.com>,
        Alistair Popple <alistair@...ple.id.au>,
        Matt Porter <mporter@...nel.crashing.org>
Cc:     linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        Paul Mackerras <paulus@...ba.org>,
        Joel Stanley <joel@....id.au>
Subject: Re: [4/4] arch/powerpc/44x/fsp2: wdt tcr update instead of whole rewrite

On Fri, 2017-05-19 at 15:47:05 UTC, Ivan Mikhaylov wrote:
> Hi Michael,
> 
> >> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> >> index 2b33cfa..f75e512 100644
> >> --- a/arch/powerpc/kernel/time.c
> >> +++ b/arch/powerpc/kernel/time.c
> >> @@ -738,12 +738,28 @@ static int __init get_freq(char *name, int cells, unsigned long *val)
> >>  
> >>  static void start_cpu_decrementer(void)
> >>  {
> >> +	unsigned int tcr;
> >>  #if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> >>  	/* Clear any pending timer interrupts */
> >>  	mtspr(SPRN_TSR, TSR_ENW | TSR_WIS | TSR_DIS | TSR_FIS);
> >>  
> >> +#ifdef CONFIG_FSP2
> >> +	/*
> >> +	 * Prevent a kernel panic caused by unintentionally clearing TCR
> >> +	 * watchdog bits.  At this point in the kernel boot, the watchdog has
> >> +	 * already been enabled by u-boot.  The original code's attempt to
> >
> > Don't refer to "the original code", as it doesn't exist anymore now that
> > we've patched it. Just say something like ".. so we must not clear the
> > watchdog configuration bits".
> Ok, got it.
> 
> >That breaks the build for other platforms:
> >
> > arch/powerpc/kernel/time.c: In function ‘start_cpu_decrementer’:
> > arch/powerpc/kernel/time.c:741:15: error: unused variable ‘tcr’ [-Werror=unused-variable]
> >
> Oops, didn't notice, my fault. 
> 
> >Or you could possibly just always leave TCR[WP], is there any case where
> >it would be correct to clear that?
> >
> >cheers
> 
> >From my point of view it's possible. I've checked docu and on idea
> it should be possible cause WP is only affecting watchdog ping time.
> Which in case of '00' is very small, around ~5 ms. 
> Ben also in next message said about get rid of ifdef for FSP2.
> 
> And now patch looks like this, What do you think Michael, Ben?
> 
>  arch/powerpc/kernel/time.c |   15 +++++++++++++--
>  1 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index bc2e08d..2411c49 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -718,11 +718,22 @@ static int __init get_freq(char *name, int cells, unsigned long *val)
>  static void start_cpu_decrementer(void)
>  {
>  #if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> +	unsigned int tcr;
>  	/* Clear any pending timer interrupts */
>  	mtspr(SPRN_TSR, TSR_ENW | TSR_WIS | TSR_DIS | TSR_FIS);
>  
> -	/* Enable decrementer interrupt */
> -	mtspr(SPRN_TCR, TCR_DIE);
> +	tcr = mfspr(SPRN_TCR);
> +	/*
> +	 * At this point in the kernel boot, the watchdog has already
> +	 * been enabled by u-boot. If we set it this to '00' it may
> +	 * trigger watchdog earlier than needed which will cause
> +	 * inattentional kernel panic. In this case we leaving TCR[WP]
> +	 * bit setting from uboot/bootstrap.
> +	 */
> +	tcr &= TCR_WP_MASK; /* clear all bits except for TCR[WP] */
> +	tcr |= TCR_DIE; /* enable decrementer */
> +	mtspr(SPRN_TCR, tcr);
> +
>  #endif /* defined(CONFIG_BOOKE) || defined(CONFIG_40x) */
>  }
>  

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/6e2f03e292ef46eed2b31b0a344a91

cheers