Please revert 86c0baf123e474b6eb404798926ecf62b426bf3a

Please revert 86c0baf123e474b6eb404798926ecf62b426bf3a

[Russell King]

The change:

+#ifdef CONFIG_HOTPLUG_CPU
+#define __INIT		.section	".text","ax"
+#define __INITDATA	.section	".data","aw"
+#else
 #define __INIT		.section	".init.text","ax"
-#define __FINIT		.previous
 #define __INITDATA	.section	".init.data","aw"
+#endif
+#define __FINIT		.previous

breaks ARM, since the ARM startup code moves from the start of the image
to somewhere in the middle of the kernel image.  (That's trivially fixable.)

Besides that, this change is also wrong because not all assembly marked
__INIT needs to be moved into the standard .text segment when hotplug CPU
is enabled.  Ditto for __INITDATA.

One is left wondering why the original patch proposers didn't:

1. Invent __CPUINIT and __CPUINITDATA, to be consistent with __init vs
   __cpuinit.

2. copy other architecture maintainers.

Shall I invent __CPUINIT to mark data to always be thrown away, or are
the x86 folk going to withdraw this patch and do it properly?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: Please revert 86c0baf123e474b6eb404798926ecf62b426bf3a

[Prarit Bhargava]

Russell King wrote:
> The change:
>
> +#ifdef CONFIG_HOTPLUG_CPU
> +#define __INIT		.section	".text","ax"
> +#define __INITDATA	.section	".data","aw"
> +#else
>  #define __INIT		.section	".init.text","ax"
> -#define __FINIT		.previous
>  #define __INITDATA	.section	".init.data","aw"
> +#endif
> +#define __FINIT		.previous
>
>   

Something is not right here.... was this the patch that I submitted?  If 
so then I definitely screwed up pretty badly.

I thought I had done (sorry for the cut-and-paste):

 
+#ifdef CONFIG_HOTPLUG_CPU
+#define __CPUINIT	.section	".text","ax"
+#define __CPUINITDATA	.section	".data","aw"
+#else
+#define __CPUINIT	__INIT
+#define __CPUINITDATA	__INITDATA
+#endif


Maybe some wires got crossed along the submit route...  Russell, I'll 
submit a fix against the latest tree.

P.

Re: Please revert 86c0baf123e474b6eb404798926ecf62b426bf3a

[Linus Torvalds]

On Tue, 8 May 2007, Russell King wrote:
> 
> Shall I invent __CPUINIT to mark data to always be thrown away, or are
> the x86 folk going to withdraw this patch and do it properly?

I can certainly revert the patch, but it would probably be better if you 
were to just start adding the infrastructure you need and send it as a new 
patch instead (where the revert is part of it, and the __CPUINIT is 
additional). 

		Linus

Re: Please revert 86c0baf123e474b6eb404798926ecf62b426bf3a

[Prarit Bhargava]

Linus Torvalds wrote:
> On Tue, 8 May 2007, Russell King wrote:
>   
>> Shall I invent __CPUINIT to mark data to always be thrown away, or are
>> the x86 folk going to withdraw this patch and do it properly?
>>     
>
> I can certainly revert the patch, but it would probably be better if you 
> were to just start adding the infrastructure you need and send it as a new 
> patch instead (where the revert is part of it, and the __CPUINIT is 
> additional). 
>
>   

I've sent Russell a new patch to test off-list.  He or I will submit a 
fix here after he tests.

P.

> 		Linus
>   

Re: Please revert 86c0baf123e474b6eb404798926ecf62b426bf3a

[Russell King]

On Tue, May 08, 2007 at 11:19:46AM -0400, Prarit Bhargava wrote:
> Linus Torvalds wrote:
> >On Tue, 8 May 2007, Russell King wrote:
> >  
> >>Shall I invent __CPUINIT to mark data to always be thrown away, or are
> >>the x86 folk going to withdraw this patch and do it properly?
> >>    
> >
> >I can certainly revert the patch, but it would probably be better if you 
> >were to just start adding the infrastructure you need and send it as a new 
> >patch instead (where the revert is part of it, and the __CPUINIT is 
> >additional).
>
> I've sent Russell a new patch to test off-list.  He or I will submit a 
> fix here after he tests.

I'm not sure it'd make much sense for me to test it - I don't have x86
based test boxes here.

Assuming your intention is to revert this commit and replace it with your
patch which doesn't change the semantics of __INIT, it shouldn't cause me
any problems.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: Please revert 86c0baf123e474b6eb404798926ecf62b426bf3a

[Prarit Bhargava]

> I'm not sure it'd make much sense for me to test it - I don't have x86
> based test boxes here.
>
> Assuming your intention is to revert this commit and replace it with your
> patch which doesn't change the semantics of __INIT, it shouldn't cause me
> any problems.
>
>   

Okay -- I'll generate a new patch later this afternoon, test, and submit.

P.

Re: Please revert 86c0baf123e474b6eb404798926ecf62b426bf3a

[Russell King]

[-- Attachment #1: Type: text/plain, Size: 780 bytes --]

On Tue, May 08, 2007 at 02:49:28PM +0100, Russell King wrote:
> The change:
> 
> +#ifdef CONFIG_HOTPLUG_CPU
> +#define __INIT		.section	".text","ax"
> +#define __INITDATA	.section	".data","aw"
> +#else
>  #define __INIT		.section	".init.text","ax"
> -#define __FINIT		.previous
>  #define __INITDATA	.section	".init.data","aw"
> +#endif
> +#define __FINIT		.previous
> 
> breaks ARM, since the ARM startup code moves from the start of the image
> to somewhere in the middle of the kernel image.  (That's trivially fixable.)

Although I've worked around this, I still would like this fixed.
Prarit's produced a patch (mail attached) which should fix it,
can we get it merged please?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

[-- Attachment #2: Type: message/rfc822, Size: 3746 bytes --]

From: Prarit Bhargava <prarit@redhat.com>
To: linux-kernel@vger.kernel.org, akpm@linux-foundation.org, rmk+lkml@arm.linux.org.uk, dzickus@redhat.com
Cc: Prarit Bhargava <prarit@redhat.com>
Subject: [PATCH] Remove cpu hotplug defines for __INIT & __INITDATA
Date: Wed, 9 May 2007 13:25:32 -0400
Message-ID: <20070509172532.16700.33405.sendpatchset@prarit.boston.redhat.com>

Russell and Andrew,

After examining what was checked in and the code base I discovered that most
of 86c0baf123e474b6eb404798926ecf62b426bf3a wasn't necessary anymore....

So here's a patch that reverts the last part of that changeset:




Revert part of 86c0baf123e474b6eb404798926ecf62b426bf3a.

The kernel has moved forward to a state where the original change is not
necessary.  After porting forward, this final version of the patch was
applied and broke non-x86 architectures.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>

diff --git a/include/linux/init.h b/include/linux/init.h
index 8bc32bb..e007ae4 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -52,14 +52,9 @@
 #endif
 
 /* For assembly routines */
-#ifdef CONFIG_HOTPLUG_CPU
-#define __INIT		.section	".text","ax"
-#define __INITDATA	.section	".data","aw"
-#else
 #define __INIT		.section	".init.text","ax"
-#define __INITDATA	.section	".init.data","aw"
-#endif
 #define __FINIT		.previous
+#define __INITDATA	.section	".init.data","aw"
 
 #ifndef __ASSEMBLY__
 /*
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/