diff mbox series

powerpc/prom_init: add __init markers to all functions

Message ID 1548903199-32695-1-git-send-email-yamada.masahiro@socionext.com (mailing list archive)
State Not Applicable
Headers show
Series powerpc/prom_init: add __init markers to all functions | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied
snowpatch_ozlabs/build-ppc64le success build succeeded & removed 0 sparse warning(s)
snowpatch_ozlabs/build-ppc64be success build succeeded & removed 0 sparse warning(s)
snowpatch_ozlabs/build-ppc64e success build succeeded & removed 0 sparse warning(s)
snowpatch_ozlabs/build-pmac32 fail build failed!
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 88 lines checked

Commit Message

Masahiro Yamada Jan. 31, 2019, 2:53 a.m. UTC 
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(-)

Comments

kernel test robot Jan. 31, 2019, 6:58 a.m. UTC | #1 
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
Michael Ellerman Feb. 5, 2019, 10:28 a.m. UTC | #2 
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
Masahiro Yamada Feb. 5, 2019, 1:57 p.m. UTC | #3 
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
Michael Ellerman Feb. 6, 2019, 11:37 a.m. UTC | #4 
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
diff mbox series

Patch

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 =