| Message ID | 2080993.BUNZpO9uLL@wuerfel (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Simon Horman |
| Headers | show |
On Tue, May 19, 2015 at 11:55:12PM +0200, Arnd Bergmann wrote: > On Tuesday 19 May 2015 23:44:58 Heiko Stuebner wrote: > > > > Michael Niewoehner tested this on a rk3188 (Cortex-A9) and wrote in [0] > > > Tested-by: Michael Niewoehner <mniewoeh@stud.hs-offenburg.de> > > > > > > Tested on Radxa Rock Pro with RK3188. > > > The kernel panics on reboot I had before and also a kernel BUG when running > > > "memtester 1900M" went away and the rock seems to run stable now. > > > > We should probably create a separate fix for that and add it to the stable > kernels then. I would suggest something like the untested patch below, > which takes advantage of the fact that we already have separate assignments > for the start_secondary function pointer for A9 and A17. Your patch is also continuing the pattern with this code... > diff --git a/arch/arm/mach-rockchip/headsmp.S b/arch/arm/mach-rockchip/headsmp.S > index 46c22dedf632..ae0077e8fe98 100644 > --- a/arch/arm/mach-rockchip/headsmp.S > +++ b/arch/arm/mach-rockchip/headsmp.S > @@ -16,9 +16,6 @@ > #include <linux/init.h> > > ENTRY(rockchip_secondary_startup) > - mrc p15, 0, r0, c0, c0, 0 @ read main ID register > - ldr r1, =0x00000c09 @ Cortex-A9 primary part number > - teq r0, r1 > beq v7_invalidate_l1 > b secondary_startup If you look carefully at this, you'll notice that it's utter crap. (Sorry, but it is.) It has two problems: 1. It'll never match a Cortex-A9 CPU. Cortex-A9 has a MIDR value of 0x412fc09a, not 0x00000c09. The bit position of the part number field isn't even right. 2. If it does match, then we branch to "v7_invalidate_l1" without setting the link register: we'll never return back here (we'll return to whatever random value the link register contains) and so we'll never make it to secondary_startup. *Thankfully*, because of (1), this branch will never be taken - this is it's saving grace. Your patch introduces a /third/ form of crapiness: 3. If the PSR happens to have Z=1, the "beq" instruction will be taken, thereby crashing the system because of (2). The /simplest/ change which would fix this problem is to just change proc-v7.S. The remainder is effectively a cleanup removing redundant code.
On Tuesday 19 May 2015 23:07:21 Russell King - ARM Linux wrote: > If you look carefully at this, you'll notice that it's utter crap. > (Sorry, but it is.) It has two problems: > > 1. It'll never match a Cortex-A9 CPU. Cortex-A9 has a MIDR value of > 0x412fc09a, not 0x00000c09. The bit position of the part number > field isn't even right. > > 2. If it does match, then we branch to "v7_invalidate_l1" without setting > the link register: we'll never return back here (we'll return to whatever > random value the link register contains) and so we'll never make it to > secondary_startup. *Thankfully*, because of (1), this branch will > never be taken - this is it's saving grace. Yes, I've understood both before. > Your patch introduces a /third/ form of crapiness: > > 3. If the PSR happens to have Z=1, the "beq" instruction will be taken, > thereby crashing the system because of (2). Right, this was the result of sloppiness on my side when fat-fingering a patch for illustration. > The /simplest/ change which would fix this problem is to just change > proc-v7.S. The remainder is effectively a cleanup removing redundant > code. Fair enough. I wasn't sure if we're confident enough about that change already to put it into stable backports. If the risk is low enough, that's fine. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 20, 2015 at 12:18:43AM +0200, Arnd Bergmann wrote: > On Tuesday 19 May 2015 23:07:21 Russell King - ARM Linux wrote: > > The /simplest/ change which would fix this problem is to just change > > proc-v7.S. The remainder is effectively a cleanup removing redundant > > code. > > Fair enough. I wasn't sure if we're confident enough about that > change already to put it into stable backports. If the risk is low > enough, that's fine. When the kernel is entered from the decompressor, the last steps just before calling the kernel is to clean and invalidate the caches, then turn them off, and then jump to the kernel. So, there _should_ be no dirty cache lines which we rely upon at this point - if there was dirty data in the cache, then it hasn't been flushed to memory, and we're potentially going to crash because some part of the decompressed kernel image hasn't been flushed out to memory. So I'll put a confidence value of 99.999% on this. The case which might bite people is if they bypass the decompressor. Even then, leaving bits of the kernel image in the caches is really dodgy.
diff --git a/arch/arm/mach-rockchip/headsmp.S b/arch/arm/mach-rockchip/headsmp.S index 46c22dedf632..ae0077e8fe98 100644 --- a/arch/arm/mach-rockchip/headsmp.S +++ b/arch/arm/mach-rockchip/headsmp.S @@ -16,9 +16,6 @@ #include <linux/init.h> ENTRY(rockchip_secondary_startup) - mrc p15, 0, r0, c0, c0, 0 @ read main ID register - ldr r1, =0x00000c09 @ Cortex-A9 primary part number - teq r0, r1 beq v7_invalidate_l1 b secondary_startup ENDPROC(rockchip_secondary_startup) diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c index 5b4ca3c3c879..5f46724cca2f 100644 --- a/arch/arm/mach-rockchip/platsmp.c +++ b/arch/arm/mach-rockchip/platsmp.c @@ -149,8 +149,7 @@ static int __cpuinit rockchip_boot_secondary(unsigned int cpu, * sram_base_addr + 8: start address for pc * */ udelay(10); - writel(virt_to_phys(rockchip_secondary_startup), - sram_base_addr + 8); + writel(virt_to_phys(secondary_startup), sram_base_addr + 8); writel(0xDEADBEAF, sram_base_addr + 4); dsb_sev(); }