diff mbox

linux-next regression on ARM926

Message ID 20110718112757.GV23270@n2100.arm.linux.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Russell King - ARM Linux July 18, 2011, 11:27 a.m. UTC 
On Mon, Jul 18, 2011 at 01:02:01PM +0200, Linus Walleij wrote:
> Hi Dave,
> 
> do you have any hints on how to resolve this build error in the -next
> tree:
> 
>   LD      .tmp_vmlinux1
> arch/arm/mm/built-in.o:(.init.data+0xe0): undefined reference to
> `cpu_arm926_do_suspend'
> arch/arm/mm/built-in.o:(.init.data+0xe4): undefined reference to
> `cpu_arm926_do_resume'
> make[2]: *** [.tmp_vmlinux1] Error 1
> make[1]: *** [sub-make] Error 2
> make[1]: Leaving directory `/home/linus/linux-next'
> make: *** [build] Error 2
> 
> This is while building the U300, I can't really tell if the error is on my
> (U300) side or in the recent patches to the proc_arm926 stuff?
> It seems all ARM926 SoCs were affected.

Hmm.

That happens because without CONFIG_PM_SLEEP, we do this:

#define cpu_arm926_do_suspend   0
#define cpu_arm926_do_resume    0

whereas the macro assembler does this:

        .word   cpu_\name\()_do_suspend
        .word   cpu_\name\()_do_resume

and this means that neither the preprocessor nor the assembler can tie
these two together.

One solution would be to put an #ifdef CONFIG_PM_SLEEP around that in
mm/proc-macros.S to select .word 0 instead, and get rid of the #else
in the individual proc-*.S files - something like this (untested):

Comments

Linus Walleij July 18, 2011, 3:40 p.m. UTC | #1 
On Mon, Jul 18, 2011 at 1:27 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> One solution would be to put an #ifdef CONFIG_PM_SLEEP around that in
> mm/proc-macros.S to select .word 0 instead, and get rid of the #else
> in the individual proc-*.S files - something like this (untested):

It works like a charm for U300. Thanks Russell!
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
tip-bot for Dave Martin July 19, 2011, 10:27 a.m. UTC | #2 
On Mon, Jul 18, 2011 at 12:27 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jul 18, 2011 at 01:02:01PM +0200, Linus Walleij wrote:
>> Hi Dave,
>>
>> do you have any hints on how to resolve this build error in the -next
>> tree:
>>
>>   LD      .tmp_vmlinux1
>> arch/arm/mm/built-in.o:(.init.data+0xe0): undefined reference to
>> `cpu_arm926_do_suspend'
>> arch/arm/mm/built-in.o:(.init.data+0xe4): undefined reference to
>> `cpu_arm926_do_resume'
>> make[2]: *** [.tmp_vmlinux1] Error 1
>> make[1]: *** [sub-make] Error 2
>> make[1]: Leaving directory `/home/linus/linux-next'
>> make: *** [build] Error 2
>>
>> This is while building the U300, I can't really tell if the error is on my
>> (U300) side or in the recent patches to the proc_arm926 stuff?
>> It seems all ARM926 SoCs were affected.
>
> Hmm.
>
> That happens because without CONFIG_PM_SLEEP, we do this:
>
> #define cpu_arm926_do_suspend   0
> #define cpu_arm926_do_resume    0
>
> whereas the macro assembler does this:
>
>        .word   cpu_\name\()_do_suspend
>        .word   cpu_\name\()_do_resume
>
> and this means that neither the preprocessor nor the assembler can tie
> these two together.
>
> One solution would be to put an #ifdef CONFIG_PM_SLEEP around that in
> mm/proc-macros.S to select .word 0 instead, and get rid of the #else
> in the individual proc-*.S files - something like this (untested):
>
> diff --git a/arch/arm/mm/proc-arm926.S b/arch/arm/mm/proc-arm926.S
> index b2f9bde..2bbcf05 100644
> --- a/arch/arm/mm/proc-arm926.S
> +++ b/arch/arm/mm/proc-arm926.S
> @@ -421,9 +421,6 @@ ENTRY(cpu_arm926_do_resume)
>                     PMD_SECT_CACHEABLE | PMD_BIT4 | PMD_SECT_AP_WRITE
>        b       cpu_resume_mmu
>  ENDPROC(cpu_arm926_do_resume)
> -#else
> -#define cpu_arm926_do_suspend  0
> -#define cpu_arm926_do_resume   0
>  #endif
>
>        __CPUINIT
> diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
> index 4ae9b44..307a4de 100644
> --- a/arch/arm/mm/proc-macros.S
> +++ b/arch/arm/mm/proc-macros.S
> @@ -276,8 +276,13 @@ ENTRY(\name\()_processor_functions)
>
>        .if \suspend
>        .word   cpu_\name\()_suspend_size
> +#ifdef CONFIG_PM_SLEEP
>        .word   cpu_\name\()_do_suspend
>        .word   cpu_\name\()_do_resume
> +#else
> +       .word   0
> +       .word   0
> +#endif
>        .else
>        .word   0
>        .word   0
>
>

The intended meaning of "suspend=1" for define_processor_functions was
"this cpu can do suspend" -- but this makes sense only if
CONFIG_PM_SLEEP is enabled.  Where processors define their suspend
functions unconditionally, that isn't a problem.  But processors
shouldn't be required (or even encouraged) to define those functions
if the kernel doesn't have suspend support at all.

So the above fix looks entirely sensible to me.

I'm offline for the next day or two, but I trust Linus' test -- so if you like:

Acked-by: Dave Martin <dave.martin@linaro.org>


We could rename the suspend argument to something like "can_suspend"
to make things clearer, but that would hit a lot of files again, so I
don't think resultant the churn would really be justified.

Cheers
---Dave
Russell King - ARM Linux July 19, 2011, 11:09 a.m. UTC | #3 
On Tue, Jul 19, 2011 at 11:27:38AM +0100, Dave Martin wrote:
> On Mon, Jul 18, 2011 at 12:27 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Jul 18, 2011 at 01:02:01PM +0200, Linus Walleij wrote:
> >> Hi Dave,
> >>
> >> do you have any hints on how to resolve this build error in the -next
> >> tree:
> >>
> >>   LD      .tmp_vmlinux1
> >> arch/arm/mm/built-in.o:(.init.data+0xe0): undefined reference to
> >> `cpu_arm926_do_suspend'
> >> arch/arm/mm/built-in.o:(.init.data+0xe4): undefined reference to
> >> `cpu_arm926_do_resume'
> >> make[2]: *** [.tmp_vmlinux1] Error 1
> >> make[1]: *** [sub-make] Error 2
> >> make[1]: Leaving directory `/home/linus/linux-next'
> >> make: *** [build] Error 2
> >>
> >> This is while building the U300, I can't really tell if the error is on my
> >> (U300) side or in the recent patches to the proc_arm926 stuff?
> >> It seems all ARM926 SoCs were affected.
> >
> > Hmm.
> >
> > That happens because without CONFIG_PM_SLEEP, we do this:
> >
> > #define cpu_arm926_do_suspend   0
> > #define cpu_arm926_do_resume    0
> >
> > whereas the macro assembler does this:
> >
> >        .word   cpu_\name\()_do_suspend
> >        .word   cpu_\name\()_do_resume
> >
> > and this means that neither the preprocessor nor the assembler can tie
> > these two together.
> >
> > One solution would be to put an #ifdef CONFIG_PM_SLEEP around that in
> > mm/proc-macros.S to select .word 0 instead, and get rid of the #else
> > in the individual proc-*.S files - something like this (untested):
> >
> > diff --git a/arch/arm/mm/proc-arm926.S b/arch/arm/mm/proc-arm926.S
> > index b2f9bde..2bbcf05 100644
> > --- a/arch/arm/mm/proc-arm926.S
> > +++ b/arch/arm/mm/proc-arm926.S
> > @@ -421,9 +421,6 @@ ENTRY(cpu_arm926_do_resume)
> >                     PMD_SECT_CACHEABLE | PMD_BIT4 | PMD_SECT_AP_WRITE
> >        b       cpu_resume_mmu
> >  ENDPROC(cpu_arm926_do_resume)
> > -#else
> > -#define cpu_arm926_do_suspend  0
> > -#define cpu_arm926_do_resume   0
> >  #endif
> >
> >        __CPUINIT
> > diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
> > index 4ae9b44..307a4de 100644
> > --- a/arch/arm/mm/proc-macros.S
> > +++ b/arch/arm/mm/proc-macros.S
> > @@ -276,8 +276,13 @@ ENTRY(\name\()_processor_functions)
> >
> >        .if \suspend
> >        .word   cpu_\name\()_suspend_size
> > +#ifdef CONFIG_PM_SLEEP
> >        .word   cpu_\name\()_do_suspend
> >        .word   cpu_\name\()_do_resume
> > +#else
> > +       .word   0
> > +       .word   0
> > +#endif
> >        .else
> >        .word   0
> >        .word   0
> >
> >
> 
> The intended meaning of "suspend=1" for define_processor_functions was
> "this cpu can do suspend" -- but this makes sense only if
> CONFIG_PM_SLEEP is enabled.  Where processors define their suspend
> functions unconditionally, that isn't a problem.  But processors
> shouldn't be required (or even encouraged) to define those functions
> if the kernel doesn't have suspend support at all.
> 
> So the above fix looks entirely sensible to me.
> 
> I'm offline for the next day or two, but I trust Linus' test -- so if you like:
> 
> Acked-by: Dave Martin <dave.martin@linaro.org>

We need to fix up the other proc-*.S files to remove the #else clause too,
so the above patch was just supposed to be an example...
tip-bot for Dave Martin July 20, 2011, 8:35 a.m. UTC | #4 
On Tue, Jul 19, 2011 at 12:09 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jul 19, 2011 at 11:27:38AM +0100, Dave Martin wrote:
>> On Mon, Jul 18, 2011 at 12:27 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Mon, Jul 18, 2011 at 01:02:01PM +0200, Linus Walleij wrote:
>> >> Hi Dave,
>> >>
>> >> do you have any hints on how to resolve this build error in the -next
>> >> tree:
>> >>
>> >>   LD      .tmp_vmlinux1
>> >> arch/arm/mm/built-in.o:(.init.data+0xe0): undefined reference to
>> >> `cpu_arm926_do_suspend'
>> >> arch/arm/mm/built-in.o:(.init.data+0xe4): undefined reference to
>> >> `cpu_arm926_do_resume'
>> >> make[2]: *** [.tmp_vmlinux1] Error 1
>> >> make[1]: *** [sub-make] Error 2
>> >> make[1]: Leaving directory `/home/linus/linux-next'
>> >> make: *** [build] Error 2
>> >>
>> >> This is while building the U300, I can't really tell if the error is on my
>> >> (U300) side or in the recent patches to the proc_arm926 stuff?
>> >> It seems all ARM926 SoCs were affected.
>> >
>> > Hmm.
>> >
>> > That happens because without CONFIG_PM_SLEEP, we do this:
>> >
>> > #define cpu_arm926_do_suspend   0
>> > #define cpu_arm926_do_resume    0
>> >
>> > whereas the macro assembler does this:
>> >
>> >        .word   cpu_\name\()_do_suspend
>> >        .word   cpu_\name\()_do_resume
>> >
>> > and this means that neither the preprocessor nor the assembler can tie
>> > these two together.
>> >
>> > One solution would be to put an #ifdef CONFIG_PM_SLEEP around that in
>> > mm/proc-macros.S to select .word 0 instead, and get rid of the #else
>> > in the individual proc-*.S files - something like this (untested):
>> >
>> > diff --git a/arch/arm/mm/proc-arm926.S b/arch/arm/mm/proc-arm926.S
>> > index b2f9bde..2bbcf05 100644
>> > --- a/arch/arm/mm/proc-arm926.S
>> > +++ b/arch/arm/mm/proc-arm926.S
>> > @@ -421,9 +421,6 @@ ENTRY(cpu_arm926_do_resume)
>> >                     PMD_SECT_CACHEABLE | PMD_BIT4 | PMD_SECT_AP_WRITE
>> >        b       cpu_resume_mmu
>> >  ENDPROC(cpu_arm926_do_resume)
>> > -#else
>> > -#define cpu_arm926_do_suspend  0
>> > -#define cpu_arm926_do_resume   0
>> >  #endif
>> >
>> >        __CPUINIT
>> > diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
>> > index 4ae9b44..307a4de 100644
>> > --- a/arch/arm/mm/proc-macros.S
>> > +++ b/arch/arm/mm/proc-macros.S
>> > @@ -276,8 +276,13 @@ ENTRY(\name\()_processor_functions)
>> >
>> >        .if \suspend
>> >        .word   cpu_\name\()_suspend_size
>> > +#ifdef CONFIG_PM_SLEEP
>> >        .word   cpu_\name\()_do_suspend
>> >        .word   cpu_\name\()_do_resume
>> > +#else
>> > +       .word   0
>> > +       .word   0
>> > +#endif
>> >        .else
>> >        .word   0
>> >        .word   0
>> >
>> >
>>
>> The intended meaning of "suspend=1" for define_processor_functions was
>> "this cpu can do suspend" -- but this makes sense only if
>> CONFIG_PM_SLEEP is enabled.  Where processors define their suspend
>> functions unconditionally, that isn't a problem.  But processors
>> shouldn't be required (or even encouraged) to define those functions
>> if the kernel doesn't have suspend support at all.
>>
>> So the above fix looks entirely sensible to me.
>>
>> I'm offline for the next day or two, but I trust Linus' test -- so if you like:
>>
>> Acked-by: Dave Martin <dave.martin@linaro.org>
>
> We need to fix up the other proc-*.S files to remove the #else clause too,
> so the above patch was just supposed to be an example...
>

Hmmm, I'll take a closer look at the implications ... but
unfortunately I'm not going to be able to do much until Thursday.

Cheers
---Dave
diff mbox

Patch

diff --git a/arch/arm/mm/proc-arm926.S b/arch/arm/mm/proc-arm926.S
index b2f9bde..2bbcf05 100644
--- a/arch/arm/mm/proc-arm926.S
+++ b/arch/arm/mm/proc-arm926.S
@@ -421,9 +421,6 @@  ENTRY(cpu_arm926_do_resume)
 		     PMD_SECT_CACHEABLE | PMD_BIT4 | PMD_SECT_AP_WRITE
 	b	cpu_resume_mmu
 ENDPROC(cpu_arm926_do_resume)
-#else
-#define cpu_arm926_do_suspend	0
-#define cpu_arm926_do_resume	0
 #endif
 
 	__CPUINIT
diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
index 4ae9b44..307a4de 100644
--- a/arch/arm/mm/proc-macros.S
+++ b/arch/arm/mm/proc-macros.S
@@ -276,8 +276,13 @@  ENTRY(\name\()_processor_functions)
 
 	.if \suspend
 	.word	cpu_\name\()_suspend_size
+#ifdef CONFIG_PM_SLEEP
 	.word	cpu_\name\()_do_suspend
 	.word	cpu_\name\()_do_resume
+#else
+	.word	0
+	.word	0
+#endif
 	.else
 	.word	0
 	.word	0