diff mbox series

powerpc/prom_init: add __init markers to all functions

Message ID 1548903199-32695-1-git-send-email-yamada.masahiro@socionext.com
State Superseded
Headers show
Series powerpc/prom_init: add __init markers to all functions | expand

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(-)

-- 
2.7.4

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




-- 
Best Regards
Masahiro Yamada
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 =