Messages in this thread Patch in this message | | | From | Masahiro Yamada <> | Date | Tue, 5 Feb 2019 22:57:29 +0900 | Subject | Re: [PATCH] powerpc/prom_init: add __init markers to all functions |
| |
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
| |