ARM: tegra: paz00: fix __initdata placement

PATCH In review Dmitry Torokhov 3 participants 6 messages Jan 23, 2017 – Jan 25, 2017

Subsystems (3)

Arm (32-bit) Arm sub-architectures Tegra architecture support

lore

Share & tools

lore

Dmitry Torokhov

author

2017-01-23 07:43 UTC #0 lore raw

PATCH 1/1 ARM: tegra: paz00: fix __initdata placement

To have expected effect the __initdata attribute should go after variable name and before initializer.`

Signed-off-by Dmitry Torokhov <[email protected]>

---
 arch/arm/mach-tegra/board-paz00.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mach-tegra/board-paz00.c +1 -1

index 7478f6fb3664..ea6bff404161 100644
@@ -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"),
 	{ },

Signature · 5 lines

2.11.0.483.g087da7b7c-goog


-- 
Dmitry

Marc Dietrich

2017-01-23 10:04 UTC #1 lore raw

Hello Dmitry,

Am Sonntag, 22. Januar 2017, 23:43:47 CET schrieb Dmitry Torokhov:

Quoted 13 lines

To have expected effect the __initdata attribute should go after variable name and before initializer.`
Signed-off-by Dmitry Torokhov <[email protected]>
---
 arch/arm/mach-tegra/board-paz00.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/arch/arm/mach-tegra/board-paz00.c index 7478f6fb3664..ea6bff404161 100644

arch/arm/mach-tegra/board-paz00.c line 29

Quoted 8 lines

arch/arm/mach-tegra/board-paz00.c +1 -1

@@ -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.

Marc
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170123/9ac10752/attachment.sig>

Thierry Reding

2017-01-23 16:24 UTC #2 lore raw

Quoted 19 lines · Marc Dietrich

Hello Dmitry,
Am Sonntag, 22. Januar 2017, 23:43:47 CET schrieb Dmitry Torokhov:
Quoted 16 lines
To have expected effect the __initdata attribute should go after variable name and before initializer.`
Signed-off-by Dmitry Torokhov <[email protected]>
---
 arch/arm/mach-tegra/board-paz00.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/arch/arm/mach-tegra/board-paz00.c index 7478f6fb3664..ea6bff404161 100644
arch/arm/mach-tegra/board-paz00.c +0 -0
@@ -23,7 +23,7 @@
 
 #include "board.h"
 

Quoted 11 lines

arch/arm/mach-tegra/board-paz00.c line 29
Quoted 5 lines
arch/arm/mach-tegra/board-paz00.c +1 -1
-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.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170123/712e14aa/attachment.sig>

Marc Dietrich

2017-01-24 09:08 UTC #3 lore raw

Hello Thierry,

Am Montag, 23. Januar 2017, 17:24:28 CET schrieb Thierry Reding:

Quoted 80 lines · Marc Dietrich

Quoted 25 lines

Hello Dmitry,
Am Sonntag, 22. Januar 2017, 23:43:47 CET schrieb Dmitry Torokhov:
Quoted 22 lines
To have expected effect the __initdata attribute should go after variable
name and before initializer.`
Signed-off-by Dmitry Torokhov <[email protected]>
---
 arch/arm/mach-tegra/board-paz00.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/arch/arm/mach-tegra/board-paz00.c index 7478f6fb3664..ea6bff404161 100644
arch/arm/mach-tegra/board-paz00.c +1 -1
@@ -23,7 +23,7 @@
 
 #include "board.h"
 
-static struct property_entry __initdata wifi_rfkill_prop[] = {
+static struct property_entry wifi_rfkill_prop[] __initdata = {
 

Quoted 9 lines

Quoted 3 lines
arch/arm/mach-tegra/board-paz00.c +0 -0
 	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

Quoted 4 lines

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 =.

ok, objdump -s is maybe not appropriate to display such things.

Contents of section .init.data:

 0000 2c000000 0c000000 00010000 00000000  ,...............
 0010 34000000 00000000 40000000 05000000  4....... at .......
 0020 00010000 00000000 48000000 00000000  ........H.......
 0030 00000000 00000000 00000000 00000000  ................
 0040 00000000 00000000                    ........        
Contents of section .rodata.str1.4:
 0000 72666b69 6c6c5f67 70696f00 74656772  rfkill_gpio.tegr
 0010 612d6770 696f0000 72657365 74000000  a-gpio..reset...
 0020 73687574 646f776e 00000000 6e616d65  shutdown....name
 0030 00000000 77696669 5f72666b 696c6c00  ....wifi_rfkill.
 0040 74797065 00000000 776c616e 00        type....wlan.

In fact, .init.data seems to contain relocations to .rodata.str1.4, which "-s" does not resolve.

Quoted 3 lines

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.

Btw, where is your tree for 4.11? git.kernel.org shows no action since November. Please also include https://patchwork.ozlabs.org/patch/704367/ if you haven't done yet. I forgot to add a #stable tag for 4.9 and 4.10.

Dmitry Torokhov

author

2017-01-24 18:27 UTC #4 lore raw

In reply to Thierry Reding

Quoted 76 lines · Thierry Reding

Quoted 19 lines · Marc Dietrich

Hello Dmitry,
Am Sonntag, 22. Januar 2017, 23:43:47 CET schrieb Dmitry Torokhov:
Quoted 16 lines
To have expected effect the __initdata attribute should go after variable name and before initializer.`
Signed-off-by Dmitry Torokhov <[email protected]>
---
 arch/arm/mach-tegra/board-paz00.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/arch/arm/mach-tegra/board-paz00.c index 7478f6fb3664..ea6bff404161 100644
arch/arm/mach-tegra/board-paz00.c +0 -0
@@ -23,7 +23,7 @@
 
 #include "board.h"
 

Quoted 11 lines

arch/arm/mach-tegra/board-paz00.c line 29
Quoted 5 lines
arch/arm/mach-tegra/board-paz00.c +1 -1
-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

Quoted 8 lines

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.

Signature · 1 line

Dmitry

Thierry Reding

2017-01-25 08:11 UTC #5 lore raw

In reply to Dmitry Torokhov

Quoted 7 lines · Dmitry Torokhov

To have expected effect the __initdata attribute should go after variable name and before initializer.`
Signed-off-by Dmitry Torokhov <[email protected]>
---
 arch/arm/mach-tegra/board-paz00.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Applied with an updated commit message.

Thanks,
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170125/df9c6362/attachment.sig>