Re: [PATCH] ARM: tegra: paz00: fix __initdata placement

[Dmitry Torokhov]

On Mon, Jan 23, 2017 at 05:24:28PM +0100, Thierry Reding wrote:
> On Mon, Jan 23, 2017 at 11:04:22AM +0100, Marc Dietrich wrote:
> > Hello Dmitry,
> > 
> > Am Sonntag, 22. Januar 2017, 23:43:47 CET schrieb Dmitry Torokhov:
> > > To have expected effect the __initdata attribute should go after variable
> > > name and before initializer.`
> > > 
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > > ---
> > >  arch/arm/mach-tegra/board-paz00.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm/mach-tegra/board-paz00.c
> > > b/arch/arm/mach-tegra/board-paz00.c index 7478f6fb3664..ea6bff404161 100644
> > > --- a/arch/arm/mach-tegra/board-paz00.c
> > > +++ b/arch/arm/mach-tegra/board-paz00.c
> > > @@ -23,7 +23,7 @@
> > > 
> > >  #include "board.h"
> > > 
> > > -static struct property_entry __initdata wifi_rfkill_prop[] = {
> > > +static struct property_entry wifi_rfkill_prop[] __initdata = {
> > >  	PROPERTY_ENTRY_STRING("name", "wifi_rfkill"),
> > >  	PROPERTY_ENTRY_STRING("type", "wlan"),
> > >  	{ },
> > 
> > you are right according to the documentation, but objdump -s always shows that 
> > it gets put into the .rodata section. So this patch has no effect, because 
> > result is always same (and wrong). It's also possible that I'm doing something 
> > wrong :-) Btw, there are hundreds of such __initdata misplacement in the 
> > kernel.
> 
> Hmm... I get this:
> 
> 	$ objdump -t build/tegra20/arch/arm/mach-tegra/board-paz00.o
> 
> 	build/tegra20/arch/arm/mach-tegra/board-paz00.o:     file format
> 	elf32-little
> 
> 	SYMBOL TABLE:
> 	00000000 l    df *ABS*  00000000 board-paz00.c
> 	00000000 l    d  .text  00000000 .text
> 	00000000 l    d  .data  00000000 .data
> 	00000000 l    d  .bss   00000000 .bss
> 	00000000 l    d  .init.text     00000000 .init.text
> 	00000000 l       .init.text     00000000 $a
> 	00000000 l       .data  00000000 .LANCHOR1
> 	00000000 l       .init.data     00000000 .LANCHOR0
> 	00000000 l    d  .ARM.extab.init.text   00000000 .ARM.extab.init.text
> 	00000000 l    d  .ARM.exidx.init.text   00000000 .ARM.exidx.init.text
> 	00000000 l       .ARM.exidx.init.text   00000000 $d
> 	00000000 l       .data  00000000 $d
> 	00000000 l     O .data  000001c8 wifi_rfkill_device
> 	000001c8 l     O .data  00000048 wifi_gpio_lookup
> 	00000000 l    d  .rodata.str1.4 00000000 .rodata.str1.4
> 	00000000 l       .rodata.str1.4 00000000 $d
> 	00000000 l    d  .init.data     00000000 .init.data
> 	00000000 l       .init.data     00000000 $d
> 	00000000 l     O .init.data     00000048 wifi_rfkill_prop
> 	00000000 l    d  .debug_frame   00000000 .debug_frame
> 	00000010 l       .debug_frame   00000000 $d
> 	00000000 l    d  .debug_info    00000000 .debug_info
> 	00000000 l    d  .debug_abbrev  00000000 .debug_abbrev
> 	00000000 l    d  .debug_aranges 00000000 .debug_aranges
> 	00000000 l    d  .debug_ranges  00000000 .debug_ranges
> 	00000000 l    d  .debug_line    00000000 .debug_line
> 	00000000 l    d  .debug_str     00000000 .debug_str
> 	00000000 l    d  .note.GNU-stack        00000000 .note.GNU-stack
> 	00000000 l    d  .comment       00000000 .comment
> 	00000000 l    d  .ARM.attributes        00000000 .ARM.attributes
> 	00000000 g     F .init.text     00000030 tegra_paz00_wifikill_init
> 	00000000         *UND*  00000000 platform_device_add_properties
> 	00000000         *UND*  00000000 gpiod_add_lookup_table
> 	00000000         *UND*  00000000 platform_device_register
> 	00000000         *UND*  00000000 __aeabi_unwind_cpp_pr0
> 
> So it correctly ends up in .init.data for me. And it does so with or
> without the patch. GCCs documentation doesn't seem to say where exactly
> these attributes need to be put, though the examples given all have the
> attribute right before the =.
> 
> Oh... interestingly checkpatch does seem to have a check for this now,
> and it does recommend that __initdata be placed after wifi_rfkill_prop,
> so I'm going to apply this.

Ah, so I mis-remembered the rules and it seems that it does not really
matter if __initdata goes before or after variable name, it can't go
between "struct" and its name. From that checkpatch change introducing
the check:

    static struct __initdata samsung_pll_clock exynos4_plls[nr_plls] = { 

    does NOT put exynos4_plls in the .initdata section.  The __initdata marker
    can be virtually anywhere on the line, EXCEPT right after "struct".  The
    preferred location is before the "=" sign if there is one, or before the
    trailing ";" otherwise.

so if you are applying it you might want to change the wording as to the
effect of "making checkpatch happy" which wasn't my intention, but was
the outcome ;)

Thanks.

-- 
Dmitry