* [PATCH] powerpc/prom_init: add __init markers to all functions @ 2019-01-31 2:53 Masahiro Yamada 2019-01-31 6:58 ` kbuild test robot 2019-02-05 10:28 ` Michael Ellerman 0 siblings, 2 replies; 5+ messages in thread From: Masahiro Yamada @ 2019-01-31 2:53 UTC (permalink / raw) To: linuxppc-dev, Michael Ellerman Cc: Mathieu Malaterre, Aneesh Kumar K.V, linux-kernel, Masahiro Yamada, Paul Mackerras It is fragile to rely on the compiler's optimization to avoid the section mismatch. Some functions may not be necessarily inlined when the compiler's inlining heuristic changes. Add __init markers consistently. As for prom_getprop() and prom_getproplen(), they are marked as 'inline', so inlining is guaranteed because PowerPC never enables CONFIG_OPTIMIZE_INLINING. However, it would be better to leave the inlining decision to the compiler. I replaced 'inline' with __init. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- arch/powerpc/kernel/prom_init.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c index f33ff41..85b0719 100644 --- a/arch/powerpc/kernel/prom_init.c +++ b/arch/powerpc/kernel/prom_init.c @@ -501,19 +501,19 @@ static int __init prom_next_node(phandle *nodep) } } -static inline int prom_getprop(phandle node, const char *pname, +static int __init prom_getprop(phandle node, const char *pname, void *value, size_t valuelen) { return call_prom("getprop", 4, 1, node, ADDR(pname), (u32)(unsigned long) value, (u32) valuelen); } -static inline int prom_getproplen(phandle node, const char *pname) +static int __init prom_getproplen(phandle node, const char *pname) { return call_prom("getproplen", 2, 1, node, ADDR(pname)); } -static void add_string(char **str, const char *q) +static void __init add_string(char **str, const char *q) { char *p = *str; @@ -523,7 +523,7 @@ static void add_string(char **str, const char *q) *str = p; } -static char *tohex(unsigned int x) +static char __init *tohex(unsigned int x) { static const char digits[] __initconst = "0123456789abcdef"; static char result[9] __prombss; @@ -570,7 +570,7 @@ static int __init prom_setprop(phandle node, const char *nodename, #define islower(c) ('a' <= (c) && (c) <= 'z') #define toupper(c) (islower(c) ? ((c) - 'a' + 'A') : (c)) -static unsigned long prom_strtoul(const char *cp, const char **endp) +static unsigned long __init prom_strtoul(const char *cp, const char **endp) { unsigned long result = 0, base = 10, value; @@ -595,7 +595,7 @@ static unsigned long prom_strtoul(const char *cp, const char **endp) return result; } -static unsigned long prom_memparse(const char *ptr, const char **retptr) +static unsigned long __init prom_memparse(const char *ptr, const char **retptr) { unsigned long ret = prom_strtoul(ptr, retptr); int shift = 0; @@ -2924,7 +2924,7 @@ static void __init fixup_device_tree_pasemi(void) prom_setprop(iob, name, "device_type", "isa", sizeof("isa")); } #else /* !CONFIG_PPC_PASEMI_NEMO */ -static inline void fixup_device_tree_pasemi(void) { } +static inline void __init fixup_device_tree_pasemi(void) { } #endif static void __init fixup_device_tree(void) @@ -2986,15 +2986,15 @@ static void __init prom_check_initrd(unsigned long r3, unsigned long r4) #ifdef CONFIG_PPC64 #ifdef CONFIG_RELOCATABLE -static void reloc_toc(void) +static void __init reloc_toc(void) { } -static void unreloc_toc(void) +static void __init unreloc_toc(void) { } #else -static void __reloc_toc(unsigned long offset, unsigned long nr_entries) +static void __init __reloc_toc(unsigned long offset, unsigned long nr_entries) { unsigned long i; unsigned long *toc_entry; @@ -3008,7 +3008,7 @@ static void __reloc_toc(unsigned long offset, unsigned long nr_entries) } } -static void reloc_toc(void) +static void __init reloc_toc(void) { unsigned long offset = reloc_offset(); unsigned long nr_entries = @@ -3019,7 +3019,7 @@ static void reloc_toc(void) mb(); } -static void unreloc_toc(void) +static void __init unreloc_toc(void) { unsigned long offset = reloc_offset(); unsigned long nr_entries = -- 2.7.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/prom_init: add __init markers to all functions 2019-01-31 2:53 [PATCH] powerpc/prom_init: add __init markers to all functions Masahiro Yamada @ 2019-01-31 6:58 ` kbuild test robot 2019-02-05 10:28 ` Michael Ellerman 1 sibling, 0 replies; 5+ messages in thread From: kbuild test robot @ 2019-01-31 6:58 UTC (permalink / raw) To: Masahiro Yamada Cc: Mathieu Malaterre, linux-kernel, Masahiro Yamada, Paul Mackerras, kbuild-all, Aneesh Kumar K.V, linuxppc-dev [-- Attachment #1: Type: text/plain, Size: 1463 bytes --] Hi Masahiro, I love your patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v5.0-rc4 next-20190130] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Masahiro-Yamada/powerpc-prom_init-add-__init-markers-to-all-functions/20190131-134035 config: powerpc-mpc837x_mds_defconfig (attached as .config) compiler: powerpc-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=8.2.0 make.cross ARCH=powerpc All errors (new ones prefixed by >>): >> arch/powerpc/kernel/prom_init.c:511:19: error: 'prom_getproplen' defined but not used [-Werror=unused-function] static int __init prom_getproplen(phandle node, const char *pname) ^~~~~~~~~~~~~~~ cc1: all warnings being treated as errors vim +/prom_getproplen +511 arch/powerpc/kernel/prom_init.c 510 > 511 static int __init prom_getproplen(phandle node, const char *pname) 512 { 513 return call_prom("getproplen", 2, 1, node, ADDR(pname)); 514 } 515 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 15157 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/prom_init: add __init markers to all functions 2019-01-31 2:53 [PATCH] powerpc/prom_init: add __init markers to all functions Masahiro Yamada 2019-01-31 6:58 ` kbuild test robot @ 2019-02-05 10:28 ` Michael Ellerman 2019-02-05 13:57 ` Masahiro Yamada 1 sibling, 1 reply; 5+ messages in thread From: Michael Ellerman @ 2019-02-05 10:28 UTC (permalink / raw) To: Masahiro Yamada, linuxppc-dev Cc: Mathieu Malaterre, Aneesh Kumar K.V, linux-kernel, Masahiro Yamada, Paul Mackerras Masahiro Yamada <yamada.masahiro@socionext.com> writes: > It is fragile to rely on the compiler's optimization to avoid the > section mismatch. Some functions may not be necessarily inlined > when the compiler's inlining heuristic changes. > > Add __init markers consistently. > > As for prom_getprop() and prom_getproplen(), they are marked as > 'inline', so inlining is guaranteed because PowerPC never enables > CONFIG_OPTIMIZE_INLINING. However, it would be better to leave the > inlining decision to the compiler. I replaced 'inline' with __init. I'm going to drop that part because it breaks the build in some configurations (as reported by the build robot). > diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c > index f33ff41..85b0719 100644 > --- a/arch/powerpc/kernel/prom_init.c > +++ b/arch/powerpc/kernel/prom_init.c > @@ -501,19 +501,19 @@ static int __init prom_next_node(phandle *nodep) > } > } > > -static inline int prom_getprop(phandle node, const char *pname, > +static int __init prom_getprop(phandle node, const char *pname, > void *value, size_t valuelen) > { > return call_prom("getprop", 4, 1, node, ADDR(pname), > (u32)(unsigned long) value, (u32) valuelen); > } > > -static inline int prom_getproplen(phandle node, const char *pname) > +static int __init prom_getproplen(phandle node, const char *pname) > { > return call_prom("getproplen", 2, 1, node, ADDR(pname)); > } > > -static void add_string(char **str, const char *q) > +static void __init add_string(char **str, const char *q) > { > char *p = *str; > > @@ -523,7 +523,7 @@ static void add_string(char **str, const char *q) > *str = p; > } > > -static char *tohex(unsigned int x) > +static char __init *tohex(unsigned int x) > { > static const char digits[] __initconst = "0123456789abcdef"; > static char result[9] __prombss; > @@ -570,7 +570,7 @@ static int __init prom_setprop(phandle node, const char *nodename, > #define islower(c) ('a' <= (c) && (c) <= 'z') > #define toupper(c) (islower(c) ? ((c) - 'a' + 'A') : (c)) > > -static unsigned long prom_strtoul(const char *cp, const char **endp) > +static unsigned long __init prom_strtoul(const char *cp, const char **endp) > { > unsigned long result = 0, base = 10, value; > > @@ -595,7 +595,7 @@ static unsigned long prom_strtoul(const char *cp, const char **endp) > return result; > } > > -static unsigned long prom_memparse(const char *ptr, const char **retptr) > +static unsigned long __init prom_memparse(const char *ptr, const char **retptr) > { > unsigned long ret = prom_strtoul(ptr, retptr); > int shift = 0; > @@ -2924,7 +2924,7 @@ static void __init fixup_device_tree_pasemi(void) > prom_setprop(iob, name, "device_type", "isa", sizeof("isa")); > } > #else /* !CONFIG_PPC_PASEMI_NEMO */ > -static inline void fixup_device_tree_pasemi(void) { } > +static inline void __init fixup_device_tree_pasemi(void) { } I don't think we need __init for an empty static inline. > #endif > > static void __init fixup_device_tree(void) > @@ -2986,15 +2986,15 @@ static void __init prom_check_initrd(unsigned long r3, unsigned long r4) > > #ifdef CONFIG_PPC64 > #ifdef CONFIG_RELOCATABLE > -static void reloc_toc(void) > +static void __init reloc_toc(void) > { > } > > -static void unreloc_toc(void) > +static void __init unreloc_toc(void) > { > } Those should be empty static inlines, I'll fix them up. > #else > -static void __reloc_toc(unsigned long offset, unsigned long nr_entries) > +static void __init __reloc_toc(unsigned long offset, unsigned long nr_entries) > { > unsigned long i; > unsigned long *toc_entry; > @@ -3008,7 +3008,7 @@ static void __reloc_toc(unsigned long offset, unsigned long nr_entries) > } > } > > -static void reloc_toc(void) > +static void __init reloc_toc(void) > { > unsigned long offset = reloc_offset(); > unsigned long nr_entries = > @@ -3019,7 +3019,7 @@ static void reloc_toc(void) > mb(); > } > > -static void unreloc_toc(void) > +static void __init unreloc_toc(void) > { > unsigned long offset = reloc_offset(); > unsigned long nr_entries = cheers ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/prom_init: add __init markers to all functions 2019-02-05 10:28 ` Michael Ellerman @ 2019-02-05 13:57 ` Masahiro Yamada 2019-02-06 11:37 ` Michael Ellerman 0 siblings, 1 reply; 5+ messages in thread From: Masahiro Yamada @ 2019-02-05 13:57 UTC (permalink / raw) To: Michael Ellerman Cc: Mathieu Malaterre, Linux Kernel Mailing List, Paul Mackerras, Aneesh Kumar K.V, linuxppc-dev On Tue, Feb 5, 2019 at 7:33 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > > Masahiro Yamada <yamada.masahiro@socionext.com> writes: > > > It is fragile to rely on the compiler's optimization to avoid the > > section mismatch. Some functions may not be necessarily inlined > > when the compiler's inlining heuristic changes. > > > > Add __init markers consistently. > > > > As for prom_getprop() and prom_getproplen(), they are marked as > > 'inline', so inlining is guaranteed because PowerPC never enables > > CONFIG_OPTIMIZE_INLINING. However, it would be better to leave the > > inlining decision to the compiler. I replaced 'inline' with __init. > > I'm going to drop that part because it breaks the build in some > configurations (as reported by the build robot). If you drop this part, my motivation for this patch is lost. My motivation is to allow all architectures to enable CONFIG_OPTIMIZE_INLINING. (Currently, only x86 can enable it, but I see nothing arch-dependent in this feature.) When I tested it in 0-day bot, it reported section mismatches from prom_getprop() and prom_getproplen(). So, I want to fix the section mismatches without relying on 'inline'. My suggestion is this: static int __init __maybe_unused prom_getproplen(phandle node, const char *pname) { return call_prom("getproplen", 2, 1, node, ADDR(pname)); } It is true you can use the side-effect of 'inline' to hide the unused function warnings, but I prefer as less inline markers as possible in *.c files. > > diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c > > index f33ff41..85b0719 100644 > > --- a/arch/powerpc/kernel/prom_init.c > > +++ b/arch/powerpc/kernel/prom_init.c > > @@ -501,19 +501,19 @@ static int __init prom_next_node(phandle *nodep) > > } > > } > > > > -static inline int prom_getprop(phandle node, const char *pname, > > +static int __init prom_getprop(phandle node, const char *pname, > > void *value, size_t valuelen) > > { > > return call_prom("getprop", 4, 1, node, ADDR(pname), > > (u32)(unsigned long) value, (u32) valuelen); > > } > > > > -static inline int prom_getproplen(phandle node, const char *pname) > > +static int __init prom_getproplen(phandle node, const char *pname) > > { > > return call_prom("getproplen", 2, 1, node, ADDR(pname)); > > } > > > > -static void add_string(char **str, const char *q) > > +static void __init add_string(char **str, const char *q) > > { > > char *p = *str; > > > > @@ -523,7 +523,7 @@ static void add_string(char **str, const char *q) > > *str = p; > > } > > > > -static char *tohex(unsigned int x) > > +static char __init *tohex(unsigned int x) > > { > > static const char digits[] __initconst = "0123456789abcdef"; > > static char result[9] __prombss; > > @@ -570,7 +570,7 @@ static int __init prom_setprop(phandle node, const char *nodename, > > #define islower(c) ('a' <= (c) && (c) <= 'z') > > #define toupper(c) (islower(c) ? ((c) - 'a' + 'A') : (c)) > > > > -static unsigned long prom_strtoul(const char *cp, const char **endp) > > +static unsigned long __init prom_strtoul(const char *cp, const char **endp) > > { > > unsigned long result = 0, base = 10, value; > > > > @@ -595,7 +595,7 @@ static unsigned long prom_strtoul(const char *cp, const char **endp) > > return result; > > } > > > > -static unsigned long prom_memparse(const char *ptr, const char **retptr) > > +static unsigned long __init prom_memparse(const char *ptr, const char **retptr) > > { > > unsigned long ret = prom_strtoul(ptr, retptr); > > int shift = 0; > > @@ -2924,7 +2924,7 @@ static void __init fixup_device_tree_pasemi(void) > > prom_setprop(iob, name, "device_type", "isa", sizeof("isa")); > > } > > #else /* !CONFIG_PPC_PASEMI_NEMO */ > > -static inline void fixup_device_tree_pasemi(void) { } > > +static inline void __init fixup_device_tree_pasemi(void) { } > > I don't think we need __init for an empty static inline. I prefer 'static __init' to 'static inline', but I can drop this if you are uncomfortable with it. My work will not be blocked by this. > > #endif > > > > static void __init fixup_device_tree(void) > > @@ -2986,15 +2986,15 @@ static void __init prom_check_initrd(unsigned long r3, unsigned long r4) > > > > #ifdef CONFIG_PPC64 > > #ifdef CONFIG_RELOCATABLE > > -static void reloc_toc(void) > > +static void __init reloc_toc(void) > > { > > } > > > > -static void unreloc_toc(void) > > +static void __init unreloc_toc(void) > > { > > } > > Those should be empty static inlines, I'll fix them up. As I said above, I believe 'static inline' is mostly useful in headers, but this is up to you. BTW, I have v2 in hand already. Do you need it if it is convenient for you? I added __init to enter_prom() as well, but you may not be comfortable with replacing inline with __init. > > #else > > -static void __reloc_toc(unsigned long offset, unsigned long nr_entries) > > +static void __init __reloc_toc(unsigned long offset, unsigned long nr_entries) > > { > > unsigned long i; > > unsigned long *toc_entry; > > @@ -3008,7 +3008,7 @@ static void __reloc_toc(unsigned long offset, unsigned long nr_entries) > > } > > } > > > > -static void reloc_toc(void) > > +static void __init reloc_toc(void) > > { > > unsigned long offset = reloc_offset(); > > unsigned long nr_entries = > > @@ -3019,7 +3019,7 @@ static void reloc_toc(void) > > mb(); > > } > > > > -static void unreloc_toc(void) > > +static void __init unreloc_toc(void) > > { > > unsigned long offset = reloc_offset(); > > unsigned long nr_entries = > > > cheers -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/prom_init: add __init markers to all functions 2019-02-05 13:57 ` Masahiro Yamada @ 2019-02-06 11:37 ` Michael Ellerman 0 siblings, 0 replies; 5+ messages in thread From: Michael Ellerman @ 2019-02-06 11:37 UTC (permalink / raw) To: Masahiro Yamada Cc: Mathieu Malaterre, Linux Kernel Mailing List, Paul Mackerras, Aneesh Kumar K.V, linuxppc-dev Masahiro Yamada <yamada.masahiro@socionext.com> writes: > On Tue, Feb 5, 2019 at 7:33 PM Michael Ellerman <mpe@ellerman.id.au> wrote: >> >> Masahiro Yamada <yamada.masahiro@socionext.com> writes: >> >> > It is fragile to rely on the compiler's optimization to avoid the >> > section mismatch. Some functions may not be necessarily inlined >> > when the compiler's inlining heuristic changes. >> > >> > Add __init markers consistently. >> > >> > As for prom_getprop() and prom_getproplen(), they are marked as >> > 'inline', so inlining is guaranteed because PowerPC never enables >> > CONFIG_OPTIMIZE_INLINING. However, it would be better to leave the >> > inlining decision to the compiler. I replaced 'inline' with __init. >> >> I'm going to drop that part because it breaks the build in some >> configurations (as reported by the build robot). > > > If you drop this part, my motivation for this patch is lost. That's no good then :) > My motivation is to allow all architectures to enable > CONFIG_OPTIMIZE_INLINING. > (Currently, only x86 can enable it, but I see nothing arch-dependent > in this feature.) Hmm OK. > When I tested it in 0-day bot, it reported > section mismatches from prom_getprop() and prom_getproplen(). > > So, I want to fix the section mismatches without > relying on 'inline'. > > My suggestion is this: > > static int __init __maybe_unused prom_getproplen(phandle node, > const char *pname) > { > return call_prom("getproplen", 2, 1, node, ADDR(pname)); > } Yeah I guess that works. My concern was whether it generates any code when it's unused, but it seems at least with modern GCC it doesn't. >> > diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c >> > index f33ff41..85b0719 100644 >> > --- a/arch/powerpc/kernel/prom_init.c >> > +++ b/arch/powerpc/kernel/prom_init.c >> > @@ -501,19 +501,19 @@ static int __init prom_next_node(phandle *nodep) >> > } >> > } >> > >> > -static inline int prom_getprop(phandle node, const char *pname, >> > +static int __init prom_getprop(phandle node, const char *pname, >> > void *value, size_t valuelen) >> > { >> > return call_prom("getprop", 4, 1, node, ADDR(pname), >> > (u32)(unsigned long) value, (u32) valuelen); >> > } >> > >> > -static inline int prom_getproplen(phandle node, const char *pname) >> > +static int __init prom_getproplen(phandle node, const char *pname) >> > { >> > return call_prom("getproplen", 2, 1, node, ADDR(pname)); >> > } >> > >> > -static void add_string(char **str, const char *q) >> > +static void __init add_string(char **str, const char *q) >> > { >> > char *p = *str; >> > >> > @@ -523,7 +523,7 @@ static void add_string(char **str, const char *q) >> > *str = p; >> > } >> > >> > -static char *tohex(unsigned int x) >> > +static char __init *tohex(unsigned int x) >> > { >> > static const char digits[] __initconst = "0123456789abcdef"; >> > static char result[9] __prombss; >> > @@ -570,7 +570,7 @@ static int __init prom_setprop(phandle node, const char *nodename, >> > #define islower(c) ('a' <= (c) && (c) <= 'z') >> > #define toupper(c) (islower(c) ? ((c) - 'a' + 'A') : (c)) >> > >> > -static unsigned long prom_strtoul(const char *cp, const char **endp) >> > +static unsigned long __init prom_strtoul(const char *cp, const char **endp) >> > { >> > unsigned long result = 0, base = 10, value; >> > >> > @@ -595,7 +595,7 @@ static unsigned long prom_strtoul(const char *cp, const char **endp) >> > return result; >> > } >> > >> > -static unsigned long prom_memparse(const char *ptr, const char **retptr) >> > +static unsigned long __init prom_memparse(const char *ptr, const char **retptr) >> > { >> > unsigned long ret = prom_strtoul(ptr, retptr); >> > int shift = 0; >> > @@ -2924,7 +2924,7 @@ static void __init fixup_device_tree_pasemi(void) >> > prom_setprop(iob, name, "device_type", "isa", sizeof("isa")); >> > } >> > #else /* !CONFIG_PPC_PASEMI_NEMO */ >> > -static inline void fixup_device_tree_pasemi(void) { } >> > +static inline void __init fixup_device_tree_pasemi(void) { } >> >> I don't think we need __init for an empty static inline. > > I prefer 'static __init' to 'static inline', > but I can drop this if you are uncomfortable with it. I guess I'm just used to empty stubs being static inline, but it doesn't really matter, as long as the compiler generates no code for them. >> > static void __init fixup_device_tree(void) >> > @@ -2986,15 +2986,15 @@ static void __init prom_check_initrd(unsigned long r3, unsigned long r4) >> > >> > #ifdef CONFIG_PPC64 >> > #ifdef CONFIG_RELOCATABLE >> > -static void reloc_toc(void) >> > +static void __init reloc_toc(void) >> > { >> > } >> > >> > -static void unreloc_toc(void) >> > +static void __init unreloc_toc(void) >> > { >> > } >> >> Those should be empty static inlines, I'll fix them up. > > As I said above, I believe 'static inline' is mostly useful in headers, > but this is up to you. No I think you've convinced me. > BTW, I have v2 in hand already. > Do you need it if it is convenient for you? Yes please send it. > I added __init to enter_prom() as well, > but you may not be comfortable with > replacing inline with __init. That's fine. I'd forgotten the 64-bit version was in assembly. We should really move it to a separate file and put it in init.text too. cheers ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-06 11:39 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-31 2:53 [PATCH] powerpc/prom_init: add __init markers to all functions Masahiro Yamada 2019-01-31 6:58 ` kbuild test robot 2019-02-05 10:28 ` Michael Ellerman 2019-02-05 13:57 ` Masahiro Yamada 2019-02-06 11:37 ` Michael Ellerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).