__setup_param(), unique_id and vdso_setup

__setup_param(), unique_id and vdso_setup

[Robert P. J. Day]

  just going through my outstanding list of kernel cleanup pedantry,
and i was reminded of this from include/linux/init.h:

=====
...
#define __setup(str, fn)                                        \
        __setup_param(str, fn, fn, 0)

/* NOTE: fn is as per module_param, not __setup!  Emits warning if fn
 * returns non-zero. */
#define early_param(str, fn)                                    \
        __setup_param(str, fn, fn, 1)
...
=====

  in short, both invocations of __setup_param() use identical second
and third parameters, and a tree-wide grep shows:

$ grep -rw __setup_param *
arch/x86/vdso/vdso32-setup.c:__setup_param("vdso=", vdso32_setup, vdso_setup, 0);
include/linux/init.h:#define __setup_param(str, unique_id, fn, early)                   \
include/linux/init.h:   __setup_param(str, fn, fn, 0)
include/linux/init.h:   __setup_param(str, fn, fn, 1)
include/linux/init.h:#define __setup_param(str, unique_id, fn)  /* nothing */
$

  so apart from that single exception involving "vdso", that macro
could be simplified to just get rid of that third parameter.  is there
something special about the vdso boot-time parm that *requires* it to
be the only boot-time parm in the entire kernel to have a different
unique id?  just curious.  or does that have to be preserved for
out-of-tree builds?

rday
--

========================================================================
Robert P. J. Day                               Waterloo, Ontario, CANADA

        Linux Consulting, Training and Annoying Kernel Pedantry.

Web page:                                          http://crashcourse.ca
Linked In:                             http://www.linkedin.com/in/rpjday
Twitter:                                       http://twitter.com/rpjday
========================================================================

Re: __setup_param(), unique_id and vdso_setup

[Sam Ravnborg]

On Wed, May 06, 2009 at 03:24:21PM -0400, Robert P. J. Day wrote:
> 
>   just going through my outstanding list of kernel cleanup pedantry,
> and i was reminded of this from include/linux/init.h:
> 
> =====
> ...
> #define __setup(str, fn)                                        \
>         __setup_param(str, fn, fn, 0)
> 
> /* NOTE: fn is as per module_param, not __setup!  Emits warning if fn
>  * returns non-zero. */
> #define early_param(str, fn)                                    \
>         __setup_param(str, fn, fn, 1)
> ...
> =====
> 
>   in short, both invocations of __setup_param() use identical second
> and third parameters, and a tree-wide grep shows:
> 
> $ grep -rw __setup_param *
> arch/x86/vdso/vdso32-setup.c:__setup_param("vdso=", vdso32_setup, vdso_setup, 0);
> include/linux/init.h:#define __setup_param(str, unique_id, fn, early)                   \
> include/linux/init.h:   __setup_param(str, fn, fn, 0)
> include/linux/init.h:   __setup_param(str, fn, fn, 1)
> include/linux/init.h:#define __setup_param(str, unique_id, fn)  /* nothing */
> $
> 
>   so apart from that single exception involving "vdso", that macro
> could be simplified to just get rid of that third parameter.  is there
> something special about the vdso boot-time parm that *requires* it to
> be the only boot-time parm in the entire kernel to have a different
> unique id?  just curious.  or does that have to be preserved for
> out-of-tree builds?
No - so please go ahead and clean it up.

Thanks,
	Sam

Re: __setup_param(), unique_id and vdso_setup

[Robert P. J. Day]

  regarding the sole remaining invocation of __setup_param() in the
entire tree that could possibly be removed, taking the definition of
__setup_param() with it:

On Sat, 9 May 2009, Sam Ravnborg wrote:

> On Wed, May 06, 2009 at 03:24:21PM -0400, Robert P. J. Day wrote:
> > ...
> > =====
> >
> >   in short, both invocations of __setup_param() use identical second
> > and third parameters, and a tree-wide grep shows:
> >
> > $ grep -rw __setup_param *
> > arch/x86/vdso/vdso32-setup.c:__setup_param("vdso=", vdso32_setup, vdso_setup, 0);
> > include/linux/init.h:#define __setup_param(str, unique_id, fn, early)                   \
> > include/linux/init.h:   __setup_param(str, fn, fn, 0)
> > include/linux/init.h:   __setup_param(str, fn, fn, 1)
> > include/linux/init.h:#define __setup_param(str, unique_id, fn)  /* nothing */
> > $
> >
> >   so apart from that single exception involving "vdso", that macro
> > could be simplified to just get rid of that third parameter.  is
> > there something special about the vdso boot-time parm that
> > *requires* it to be the only boot-time parm in the entire kernel
> > to have a different unique id?  just curious.  or does that have
> > to be preserved for out-of-tree builds?

> No - so please go ahead and clean it up.

  i'd be happy to, except i'm not sure how to resolve that last
invocation to no longer use __setup_param().  from
arch/x86/vdso/vdso32-setup.c:

=====
...
unsigned int __read_mostly vdso_enabled = VDSO_DEFAULT;

static int __init vdso_setup(char *s)
{
        vdso_enabled = simple_strtoul(s, NULL, 0);

        return 1;
}

/*
 * For consistency, the argument vdso32=[012] affects the 32-bit vDSO
 * behavior on both 64-bit and 32-bit kernels.
 * On 32-bit kernels, vdso=[012] means the same thing.
 */
__setup("vdso32=", vdso_setup);

#ifdef CONFIG_X86_32
__setup_param("vdso=", vdso32_setup, vdso_setup, 0);

EXPORT_SYMBOL_GPL(vdso_enabled);
#endif
...
=====

  if someone wants to resolve that file to remove the use of
__setup_param(), this cleanup should be done in two steps, anyway.
first, get rid of that final invocation, followed by removing the
definition from moduleparam.h.  suggestions?

rday

p.s.  there are only three architectures that appear to support a
boot-time parm of vdso (or some equivalent):

$ grep -rw __setup * | grep vdso
arch/sh/kernel/vsyscall/vsyscall.c:__setup("vdso=", vdso_setup);
arch/x86/vdso/vdso32-setup.c:__setup("vdso32=", vdso_setup);
arch/x86/vdso/vma.c:__setup("vdso=", vdso_setup);
arch/s390/kernel/vdso.c:__setup("vdso=", vdso_setup);
$

  that doesn't quite match with what one finds in
Documentation/kernel-parameters.txt:

        vdso=           [X86,SH]
                        vdso=2: enable compat VDSO (default with COMPAT_VDSO)
                        vdso=1: enable VDSO (default)
                        vdso=0: disable VDSO mapping

        vdso32=         [X86]
                        vdso32=2: enable compat VDSO (default with COMPAT_VDSO)
                        vdso32=1: enable 32-bit VDSO (default)
                        vdso32=0: disable 32-bit VDSO mapping

just an observation.
--
========================================================================
Robert P. J. Day                               Waterloo, Ontario, CANADA

        Linux Consulting, Training and Annoying Kernel Pedantry.

Web page:                                          http://crashcourse.ca
Linked In:                             http://www.linkedin.com/in/rpjday
Twitter:                                       http://twitter.com/rpjday
========================================================================