linux-kernel - Re: [PATCH 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <87wp9d5ekz.fsf@concordia.ellerman.id.au>
Date:   Fri, 19 May 2017 20:24:44 +1000
From:   Michael Ellerman <mpe@...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,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Joel Stanley <joel@...abs.au.ibm.com>,
        Paul Mackerras <paulus@...ba.org>
Subject: Re: [PATCH 4/4] arch/powerpc/44x/fsp2: wdt tcr update instead of whole rewrite

Hi Ivan,

Ivan Mikhaylov <ivan@...ibm.com> writes:
> 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
> write to the TCR register results in an inadvertent clearing of the
> watchdog configuration bits, causing the 476 to reset.
> Panic happens in case of error as silently reboot without any outputs
> on serial.

That sounds reasonable.

> 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".

> +	 * write to the TCR register results in an inadvertent clearing of the
> +	 * watchdog configuration bits, causing the 440 to reset.
> +	 */
> +	tcr = mfspr(SPRN_TCR);
> +	tcr &= TCR_WP_MASK; /* clear all bits except for TCR[WP] */
> +	tcr |= TCR_DIE; /* enable decrementer */
> +	mtspr(SPRN_TCR, tcr);
> +#else
>  	/* Enable decrementer interrupt */
>  	mtspr(SPRN_TCR, TCR_DIE);
> +#endif
> +
>  #endif /* defined(CONFIG_BOOKE) || defined(CONFIG_40x) */

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]

You can just do something like:

  #if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
  	unsigned int tcr;

  #ifdef CONFIG_FSP2
  	tcr = mfspr(SPRN_TCR);
  	tcr &= TCR_WP_MASK; /* clear all bits except for TCR[WP] */
  #else
  	tcr = 0;
  #endif
  	tcr |= TCR_DIE; /* enable decrementer */
  	mtspr(SPRN_TCR, TCR_DIE);
  #endif /* defined(CONFIG_BOOKE) || defined(CONFIG_40x) */


Or you could possibly just always leave TCR[WP], is there any case where
it would be correct to clear that?

cheers