Message ID | 20131018205405.GA28835@codethink.co.uk |
---|---|
State | New |
Headers | show |
Hi Ben, A few nits here... Firstly, I'd suggest using [GIT PULL] instead of [PULL REQ] in the subject. Many people filter on the first, so you might get missed with the latter. On Fri, Oct 18, 2013 at 09:54:05PM +0100, Ben Dooks wrote: > This series has been well tested and it would be great to get this > merged now. So here is the pull request: > > The following changes since commit 8b5ede69d24db939f52b47effff2f6fe1e83e08b: > > powerpc/irq: Don't switch to irq stack from softirq stack (2013-10-07 14:19:39 -0700) Secondly, I'd suggest basing against a specific tag in mainline if possible, rather than a random commit. I think -rc3 matches rmk's current devel-stable branch. > are available in the git repository at: > > ssh://git@git.baserock.org/delta/linux baserock/bjdooks/312-rc4/be/core-v2 This is an ssh URL, so it can't be pulled without some form of authentication! Use git:// instead for pull requests. It's also good form to put your pull requests on separate branches from what you have been using for development, to make it clear that it is stable. Another method is to use a signed tag (although this seems more common with arm-soc than others). Do you think you could send another pull request please? Cheers, Will
On 19/10/13 18:09, Will Deacon wrote: > Hi Ben, > > A few nits here... > > Firstly, I'd suggest using [GIT PULL] instead of [PULL REQ] in the subject. > Many people filter on the first, so you might get missed with the latter. > > On Fri, Oct 18, 2013 at 09:54:05PM +0100, Ben Dooks wrote: >> This series has been well tested and it would be great to get this >> merged now. So here is the pull request: >> >> The following changes since commit 8b5ede69d24db939f52b47effff2f6fe1e83e08b: >> >> powerpc/irq: Don't switch to irq stack from softirq stack (2013-10-07 14:19:39 -0700) > > Secondly, I'd suggest basing against a specific tag in mainline if possible, > rather than a random commit. I think -rc3 matches rmk's current > devel-stable branch. > >> are available in the git repository at: >> >> ssh://git@git.baserock.org/delta/linux baserock/bjdooks/312-rc4/be/core-v2 > > This is an ssh URL, so it can't be pulled without some form of > authentication! Use git:// instead for pull requests. > > It's also good form to put your pull requests on separate branches from what > you have been using for development, to make it clear that it is stable. > Another method is to use a signed tag (although this seems more common with > arm-soc than others). > > Do you think you could send another pull request please? Ok, sorted.
Hi Russell, On Mon, Oct 28, 2013 at 12:47:36AM +0000, Russell King - ARM Linux wrote: > On Sat, Oct 19, 2013 at 08:51:35PM +0100, Ben Dooks wrote: > > On 19/10/13 18:09, Will Deacon wrote: > >> Do you think you could send another pull request please? > > > > Ok, sorted. > > Pulled, but there was a conflict. Please check this resolution (it's > copy'n'pasted). I'll probably be in linux-next tomorrow in any case, > but any mistake here can be fixed. This doesn't look quite right to me, but unfortunately I'm going be spending most (all?) of today trying to catch a flight out of the UK. Hopefully Dave or Ben can investigate further, but comments below. > diff --cc arch/arm/kernel/head.S > index 54547947a4e9,a047acfa6b6d..000000000000 > --- a/arch/arm/kernel/head.S > +++ b/arch/arm/kernel/head.S > @@@ -602,28 -586,26 +606,39 @@@ __fixup_a_pv_table > b 2f > 1: add r7, r3 > ldrh ip, [r7, #2] > + ARM_BE8(rev16 ip, ip) > - and ip, 0x8f00 > - orr ip, r6 @ mask in offset bits 31-24 > + tst ip, #0x4000 > + and ip, #0x8f00 > + orrne ip, r6 @ mask in offset bits 31-24 > + orreq ip, r0 @ mask in offset bits 7-0 > + ARM_BE8(rev16 ip, ip) > strh ip, [r7, #2] > + ldrheq ip, [r7] > + biceq ip, #0x20 > + orreq ip, ip, r0, lsr #16 > + strheq ip, [r7] There are new halfword accesses here without any conditional revs. > 2: cmp r4, r5 > ldrcc r7, [r4], #4 @ use branch for delay slot > bcc 1b > bx lr > #else > + moveq r0, #0x400000 @ set bit 22, mov to mvn instruction > b 2f > 1: ldr ip, [r7, r3] > + #ifdef CONFIG_CPU_ENDIAN_BE8 > + @ in BE8, we load data in BE, but instructions still in LE > + bic ip, ip, #0xff000000 > - orr ip, ip, r6, lsl#24 > ++ tst ip, #0x000f0000 @ check the rotation field Since that orr with shift has been removed, I think the masks for the BE case are now incorrect... > ++ orrne ip, ip, r6, lsl #24 @ mask in offset bits 31-24 > ++ biceq ip, ip, #0x00004000 @ clear bit 22 > ++ orreq ip, ip, r0, lsl #24 @ mask in offset bits 7-0 > + #else > bic ip, ip, #0x000000ff > - orr ip, ip, r6 @ mask in offset bits 31-24 > + tst ip, #0xf00 @ check the rotation field > + orrne ip, ip, r6 @ mask in offset bits 31-24 > + biceq ip, ip, #0x400000 @ clear bit 22 ...which seems to be confirmed by the updated LE code (everything is off by a byte). Somebody should probably sit down with the conflicting patch and port the BE changes over. I think the relevant patch is "ARM: mm: Correct virt_to_phys patching for 64 bit physical addresses". In fact, looking at *that* patch, it's *also* broken for BE! It adds the following to head.S: +#ifdef __ARMEB_ +#define LOW_OFFSET 0x4 +#define HIGH_OFFSET 0x0 +#else +#define LOW_OFFSET 0x0 +#define HIGH_OFFSET 0x4 +#endif (spot the missing underscore). So I guess that means somebody should test whatever the resulting merge ends up being too... Will
On Mon, Oct 28, 2013 at 08:44:55AM +0000, Will Deacon wrote: > Hi Russell, > > On Mon, Oct 28, 2013 at 12:47:36AM +0000, Russell King - ARM Linux wrote: > > On Sat, Oct 19, 2013 at 08:51:35PM +0100, Ben Dooks wrote: > > > On 19/10/13 18:09, Will Deacon wrote: > > >> Do you think you could send another pull request please? > > > > > > Ok, sorted. > > > > Pulled, but there was a conflict. Please check this resolution (it's > > copy'n'pasted). I'll probably be in linux-next tomorrow in any case, > > but any mistake here can be fixed. > > This doesn't look quite right to me, but unfortunately I'm going be spending > most (all?) of today trying to catch a flight out of the UK. Hopefully Dave > or Ben can investigate further, but comments below. > > > diff --cc arch/arm/kernel/head.S > > index 54547947a4e9,a047acfa6b6d..000000000000 > > --- a/arch/arm/kernel/head.S > > +++ b/arch/arm/kernel/head.S > > @@@ -602,28 -586,26 +606,39 @@@ __fixup_a_pv_table > > b 2f > > 1: add r7, r3 > > ldrh ip, [r7, #2] > > + ARM_BE8(rev16 ip, ip) > > - and ip, 0x8f00 > > - orr ip, r6 @ mask in offset bits 31-24 > > + tst ip, #0x4000 > > + and ip, #0x8f00 > > + orrne ip, r6 @ mask in offset bits 31-24 > > + orreq ip, r0 @ mask in offset bits 7-0 > > + ARM_BE8(rev16 ip, ip) > > strh ip, [r7, #2] > > + ldrheq ip, [r7] > > + biceq ip, #0x20 > > + orreq ip, ip, r0, lsr #16 > > + strheq ip, [r7] > > There are new halfword accesses here without any conditional revs. Yes, I missed this one. > > + #ifdef CONFIG_CPU_ENDIAN_BE8 > > + @ in BE8, we load data in BE, but instructions still in LE > > + bic ip, ip, #0xff000000 > > - orr ip, ip, r6, lsl#24 > > ++ tst ip, #0x000f0000 @ check the rotation field > > Since that orr with shift has been removed, I think the masks for the BE > case are now incorrect... > > > ++ orrne ip, ip, r6, lsl #24 @ mask in offset bits 31-24 > > ++ biceq ip, ip, #0x00004000 @ clear bit 22 > > ++ orreq ip, ip, r0, lsl #24 @ mask in offset bits 7-0 Actually, look closer. It became the orrne here. > > + #else > > bic ip, ip, #0x000000ff > > - orr ip, ip, r6 @ mask in offset bits 31-24 > > + tst ip, #0xf00 @ check the rotation field > > + orrne ip, ip, r6 @ mask in offset bits 31-24 > > + biceq ip, ip, #0x400000 @ clear bit 22 > > ...which seems to be confirmed by the updated LE code (everything is off > by a byte). The LE code was left unaltered from Santosh's patch, so that should be correct. I just did an endian conversion to the BE case. > Somebody should probably sit down with the conflicting patch and port the BE > changes over. I think the relevant patch is "ARM: mm: Correct virt_to_phys > patching for 64 bit physical addresses". In fact, looking at *that* patch, > it's *also* broken for BE! It adds the following to head.S: > > +#ifdef __ARMEB_ > +#define LOW_OFFSET 0x4 > +#define HIGH_OFFSET 0x0 > +#else > +#define LOW_OFFSET 0x0 > +#define HIGH_OFFSET 0x4 > +#endif > > (spot the missing underscore). Yep, well spotted. Well, we have some time to get this all fixed, so I'm going to drop Ben's tree. I think we need to first commit a patch to fix the error in Santosh's patch.
Hi, On Monday 28 October 2013 02:23 PM, Russell King - ARM Linux wrote: > On Mon, Oct 28, 2013 at 08:44:55AM +0000, Will Deacon wrote: >> Hi Russell, >> >> On Mon, Oct 28, 2013 at 12:47:36AM +0000, Russell King - ARM Linux wrote: >>> On Sat, Oct 19, 2013 at 08:51:35PM +0100, Ben Dooks wrote: >>>> On 19/10/13 18:09, Will Deacon wrote: >>>>> Do you think you could send another pull request please? >>>> Ok, sorted. >>> Pulled, but there was a conflict. Please check this resolution (it's >>> copy'n'pasted). I'll probably be in linux-next tomorrow in any case, >>> but any mistake here can be fixed. >> This doesn't look quite right to me, but unfortunately I'm going be spending >> most (all?) of today trying to catch a flight out of the UK. Hopefully Dave >> or Ben can investigate further, but comments below. >> >>> diff --cc arch/arm/kernel/head.S >>> index 54547947a4e9,a047acfa6b6d..000000000000 >>> --- a/arch/arm/kernel/head.S >>> +++ b/arch/arm/kernel/head.S >>> @@@ -602,28 -586,26 +606,39 @@@ __fixup_a_pv_table >>> b 2f >>> 1: add r7, r3 >>> ldrh ip, [r7, #2] >>> + ARM_BE8(rev16 ip, ip) >>> - and ip, 0x8f00 >>> - orr ip, r6 @ mask in offset bits 31-24 >>> + tst ip, #0x4000 >>> + and ip, #0x8f00 >>> + orrne ip, r6 @ mask in offset bits 31-24 >>> + orreq ip, r0 @ mask in offset bits 7-0 >>> + ARM_BE8(rev16 ip, ip) >>> strh ip, [r7, #2] >>> + ldrheq ip, [r7] >>> + biceq ip, #0x20 >>> + orreq ip, ip, r0, lsr #16 >>> + strheq ip, [r7] >> There are new halfword accesses here without any conditional revs. > Yes, I missed this one. > >>> + #ifdef CONFIG_CPU_ENDIAN_BE8 >>> + @ in BE8, we load data in BE, but instructions still in LE >>> + bic ip, ip, #0xff000000 >>> - orr ip, ip, r6, lsl#24 >>> ++ tst ip, #0x000f0000 @ check the rotation field >> Since that orr with shift has been removed, I think the masks for the BE >> case are now incorrect... >> >>> ++ orrne ip, ip, r6, lsl #24 @ mask in offset bits 31-24 >>> ++ biceq ip, ip, #0x00004000 @ clear bit 22 >>> ++ orreq ip, ip, r0, lsl #24 @ mask in offset bits 7-0 > Actually, look closer. It became the orrne here. > >>> + #else >>> bic ip, ip, #0x000000ff >>> - orr ip, ip, r6 @ mask in offset bits 31-24 >>> + tst ip, #0xf00 @ check the rotation field >>> + orrne ip, ip, r6 @ mask in offset bits 31-24 >>> + biceq ip, ip, #0x400000 @ clear bit 22 >> ...which seems to be confirmed by the updated LE code (everything is off >> by a byte). > The LE code was left unaltered from Santosh's patch, so that should be > correct. I just did an endian conversion to the BE case. > >> Somebody should probably sit down with the conflicting patch and port the BE >> changes over. I think the relevant patch is "ARM: mm: Correct virt_to_phys >> patching for 64 bit physical addresses". In fact, looking at *that* patch, >> it's *also* broken for BE! It adds the following to head.S: >> >> +#ifdef __ARMEB_ >> +#define LOW_OFFSET 0x4 >> +#define HIGH_OFFSET 0x0 >> +#else >> +#define LOW_OFFSET 0x0 >> +#define HIGH_OFFSET 0x4 >> +#endif >> >> (spot the missing underscore). > Yep, well spotted. > > Well, we have some time to get this all fixed, so I'm going to drop > Ben's tree. I think we need to first commit a patch to fix the error > in Santosh's patch. Sorry, I will send a patch fix this missing underscore bug. Regards, Sricharan
On 28/10/13 08:53, Russell King - ARM Linux wrote: >> +#ifdef __ARMEB_ >> +#define LOW_OFFSET 0x4 >> +#define HIGH_OFFSET 0x0 >> +#else >> +#define LOW_OFFSET 0x0 >> +#define HIGH_OFFSET 0x4 >> +#endif >> >> (spot the missing underscore). > > Yep, well spotted. > > Well, we have some time to get this all fixed, so I'm going to drop > Ben's tree. I think we need to first commit a patch to fix the error > in Santosh's patch Ok, shall I look at re-basing to -rc6? I have a bug report with a change to the SCU reading for the SMP fix-up case which could do with fixing, so I could add this and send another pull request (complete with description this time).
Hi, On Tuesday 29 October 2013 10:39 AM, Victor Kamensky wrote: > Hi Sricharan, > > Another problem with f52bb72 commit is missing .align at > the end of __fixup_a_pv_table function. In case of thumb2 > kernel address at label 3 could be 2 bytes aligned and > cause unaligned access exception. > > diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S > index 2b3e981..8b03c2c 100644 > --- a/arch/arm/kernel/head.S > +++ b/arch/arm/kernel/head.S > @@ -662,6 +662,7 @@ ARM_BE8(rev ip, ip) > #endif > ENDPROC(__fixup_a_pv_table) > > + .align > 3: .long __pv_offset > > ENTRY(fixup_pv_table) > > It may work now because it is accidentally 4 bytes aligned > but it could change as code evolves. This happened to > me while I tried to work out how to deal with this code > in BE case (I am still working on that). Ok, then even this is missing i think. We did not see any issue because as you said it was aligned on 4 byte. I will fix this as well in the previous patch. Regards, Sricharan > Thanks, > Victor > > > On 28 October 2013 02:12, Sricharan R <r.sricharan@ti.com> wrote: >> Hi, >> On Monday 28 October 2013 02:23 PM, Russell King - ARM Linux wrote: >>> On Mon, Oct 28, 2013 at 08:44:55AM +0000, Will Deacon wrote: >>>> Hi Russell, >>>> >>>> On Mon, Oct 28, 2013 at 12:47:36AM +0000, Russell King - ARM Linux wrote: >>>>> On Sat, Oct 19, 2013 at 08:51:35PM +0100, Ben Dooks wrote: >>>>>> On 19/10/13 18:09, Will Deacon wrote: >>>>>>> Do you think you could send another pull request please? >>>>>> Ok, sorted. >>>>> Pulled, but there was a conflict. Please check this resolution (it's >>>>> copy'n'pasted). I'll probably be in linux-next tomorrow in any case, >>>>> but any mistake here can be fixed. >>>> This doesn't look quite right to me, but unfortunately I'm going be spending >>>> most (all?) of today trying to catch a flight out of the UK. Hopefully Dave >>>> or Ben can investigate further, but comments below. >>>> >>>>> diff --cc arch/arm/kernel/head.S >>>>> index 54547947a4e9,a047acfa6b6d..000000000000 >>>>> --- a/arch/arm/kernel/head.S >>>>> +++ b/arch/arm/kernel/head.S >>>>> @@@ -602,28 -586,26 +606,39 @@@ __fixup_a_pv_table >>>>> b 2f >>>>> 1: add r7, r3 >>>>> ldrh ip, [r7, #2] >>>>> + ARM_BE8(rev16 ip, ip) >>>>> - and ip, 0x8f00 >>>>> - orr ip, r6 @ mask in offset bits 31-24 >>>>> + tst ip, #0x4000 >>>>> + and ip, #0x8f00 >>>>> + orrne ip, r6 @ mask in offset bits 31-24 >>>>> + orreq ip, r0 @ mask in offset bits 7-0 >>>>> + ARM_BE8(rev16 ip, ip) >>>>> strh ip, [r7, #2] >>>>> + ldrheq ip, [r7] >>>>> + biceq ip, #0x20 >>>>> + orreq ip, ip, r0, lsr #16 >>>>> + strheq ip, [r7] >>>> There are new halfword accesses here without any conditional revs. >>> Yes, I missed this one. >>> >>>>> + #ifdef CONFIG_CPU_ENDIAN_BE8 >>>>> + @ in BE8, we load data in BE, but instructions still in LE >>>>> + bic ip, ip, #0xff000000 >>>>> - orr ip, ip, r6, lsl#24 >>>>> ++ tst ip, #0x000f0000 @ check the rotation field >>>> Since that orr with shift has been removed, I think the masks for the BE >>>> case are now incorrect... >>>> >>>>> ++ orrne ip, ip, r6, lsl #24 @ mask in offset bits 31-24 >>>>> ++ biceq ip, ip, #0x00004000 @ clear bit 22 >>>>> ++ orreq ip, ip, r0, lsl #24 @ mask in offset bits 7-0 >>> Actually, look closer. It became the orrne here. >>> >>>>> + #else >>>>> bic ip, ip, #0x000000ff >>>>> - orr ip, ip, r6 @ mask in offset bits 31-24 >>>>> + tst ip, #0xf00 @ check the rotation field >>>>> + orrne ip, ip, r6 @ mask in offset bits 31-24 >>>>> + biceq ip, ip, #0x400000 @ clear bit 22 >>>> ...which seems to be confirmed by the updated LE code (everything is off >>>> by a byte). >>> The LE code was left unaltered from Santosh's patch, so that should be >>> correct. I just did an endian conversion to the BE case. >>> >>>> Somebody should probably sit down with the conflicting patch and port the BE >>>> changes over. I think the relevant patch is "ARM: mm: Correct virt_to_phys >>>> patching for 64 bit physical addresses". In fact, looking at *that* patch, >>>> it's *also* broken for BE! It adds the following to head.S: >>>> >>>> +#ifdef __ARMEB_ >>>> +#define LOW_OFFSET 0x4 >>>> +#define HIGH_OFFSET 0x0 >>>> +#else >>>> +#define LOW_OFFSET 0x0 >>>> +#define HIGH_OFFSET 0x4 >>>> +#endif >>>> >>>> (spot the missing underscore). >>> Yep, well spotted. >>> >>> Well, we have some time to get this all fixed, so I'm going to drop >>> Ben's tree. I think we need to first commit a patch to fix the error >>> in Santosh's patch. >> Sorry, I will send a patch fix this missing underscore bug. >> >> Regards, >> Sricharan
On 28 October 2013 08:59, Ben Dooks <ben.dooks@codethink.co.uk> wrote: > On 28/10/13 08:53, Russell King - ARM Linux wrote: > >>> +#ifdef __ARMEB_ >>> +#define LOW_OFFSET 0x4 >>> +#define HIGH_OFFSET 0x0 >>> +#else >>> +#define LOW_OFFSET 0x0 >>> +#define HIGH_OFFSET 0x4 >>> +#endif >>> >>> (spot the missing underscore). >> >> >> Yep, well spotted. >> >> Well, we have some time to get this all fixed, so I'm going to drop >> Ben's tree. I think we need to first commit a patch to fix the error >> in Santosh's patch > > > Ok, shall I look at re-basing to -rc6? I just fetched rmk/for-next as of 10/29/2013 8pm PST. I see that big endian patch series is in. I believe conflict resolution 580dac83 in head.S is correct. I've added typo and .align fix as in [1] to head.S and tested BE image on TC2 in both ARM and THUMB2 modes - both boot and look fine to me. > I have a bug report with a change to the SCU reading for the SMP > fix-up case which could do with fixing, so I could add this and send > another pull request (complete with description this time). > IMHO it would be great if we fix SCU BE A9 SMP issue separately later, and keep BE changes that are already in rmk/for-next. Thanks, Victor [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/thread.html#207597 > -- > Ben Dooks http://www.codethink.co.uk/ > Senior Engineer Codethink - Providing Genius
On Tue, Oct 29, 2013 at 09:05:09PM -0700, Victor Kamensky wrote: > I just fetched rmk/for-next as of 10/29/2013 8pm PST. I see that big > endian patch series is in. I believe conflict resolution 580dac83 in head.S > is correct. I've added typo and .align fix as in [1] to head.S and > tested BE image on TC2 in both ARM and THUMB2 modes - both boot > and look fine to me. Although it's in for-next, I won't be pushing the series until we know that there are no known problems - so we need to get the patch you refer to in as well along side it.
On Mon, Oct 28, 2013 at 03:59:34PM +0000, Ben Dooks wrote: > On 28/10/13 08:53, Russell King - ARM Linux wrote: > >>> +#ifdef __ARMEB_ >>> +#define LOW_OFFSET 0x4 >>> +#define HIGH_OFFSET 0x0 >>> +#else >>> +#define LOW_OFFSET 0x0 >>> +#define HIGH_OFFSET 0x4 >>> +#endif >>> >>> (spot the missing underscore). >> >> Yep, well spotted. >> >> Well, we have some time to get this all fixed, so I'm going to drop >> Ben's tree. I think we need to first commit a patch to fix the error >> in Santosh's patch > > Ok, shall I look at re-basing to -rc6? > > I have a bug report with a change to the SCU reading for the SMP > fix-up case which could do with fixing, so I could add this and send > another pull request (complete with description this time). Rebasing on a later kernel from Linus doesn't do much good, because the conflicting changes aren't in Linus' tree, but are part of Santosh's patch set for high physical memory for LPAE systems. I think we're fine with knowing what the merge fixes are now. There's only one remaining problem left, which is a missing .align. Someone needs to send a patch for that. :)
On 30 October 2013 11:01, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Mon, Oct 28, 2013 at 03:59:34PM +0000, Ben Dooks wrote: >> On 28/10/13 08:53, Russell King - ARM Linux wrote: >> >>>> +#ifdef __ARMEB_ >>>> +#define LOW_OFFSET 0x4 >>>> +#define HIGH_OFFSET 0x0 >>>> +#else >>>> +#define LOW_OFFSET 0x0 >>>> +#define HIGH_OFFSET 0x4 >>>> +#endif >>>> >>>> (spot the missing underscore). >>> >>> Yep, well spotted. >>> >>> Well, we have some time to get this all fixed, so I'm going to drop >>> Ben's tree. I think we need to first commit a patch to fix the error >>> in Santosh's patch >> >> Ok, shall I look at re-basing to -rc6? >> >> I have a bug report with a change to the SCU reading for the SMP >> fix-up case which could do with fixing, so I could add this and send >> another pull request (complete with description this time). > > Rebasing on a later kernel from Linus doesn't do much good, because the > conflicting changes aren't in Linus' tree, but are part of Santosh's > patch set for high physical memory for LPAE systems. > > I think we're fine with knowing what the merge fixes are now. There's > only one remaining problem left, which is a missing .align. Someone > needs to send a patch for that. :) Sorry, I referenced [1], but actually Sricharan sent [2] which includes .align fix as well as fix for __ARMEB_ typo and marked it as V2 for [1]. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/thread.html#207597 [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/207809.html Is there anything else we need to do? Thanks, Victor