Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial PowerController (SPC) support

From: Nicolas Pitre
Date: Wed Jun 12 2013 - 23:26:26 EST


On Thu, 13 Jun 2013, Samuel Ortiz wrote:

> > +static struct vexpress_spc_drvdata *info;
> > +static u32 *vexpress_spc_config_data;
> > +static struct vexpress_config_bridge *vexpress_spc_config_bridge;
> > +static struct vexpress_config_func *opp_func, *perf_func;
> > +
> > +static int vexpress_spc_load_result = -EAGAIN;
> As I said, quite static...
>
> > +static bool __init __vexpress_spc_check_loaded(void);
> > +/*
> > + * Pointer spc_check_loaded is swapped after init hence it is safe
> > + * to initialize it to a function in the __init section
> > + */
> > +static bool (*spc_check_loaded)(void) __refdata = &__vexpress_spc_check_loaded;
> > +
> > +static bool __init __vexpress_spc_check_loaded(void)
> > +{
> > + if (vexpress_spc_load_result == -EAGAIN)
> > + vexpress_spc_load_result = vexpress_spc_init();
> > + spc_check_loaded = &vexpress_spc_initialized;
> > + return vexpress_spc_initialized();
> > +}
> > +
> > +/*
> > + * Function exported to manage early_initcall ordering.
> > + * SPC code is needed very early in the boot process
> > + * to bring CPUs out of reset and initialize power
> > + * management back-end. After boot swap pointers to
> > + * make the functionality check available to loadable
> > + * modules, when early boot init functions have been
> > + * already freed from kernel address space.
> > + */
> > +bool vexpress_spc_check_loaded(void)
> > +{
> > + return spc_check_loaded();
> > +}
> > +EXPORT_SYMBOL_GPL(vexpress_spc_check_loaded);
> That one and the previous function look really nasty to me.
> The simple fact that you need a static variable in your code to check if
> your module is loaded sounds really fishy.

Maybe a bit of context might explain why things are done this way.

This code is, amongst other things, needed to properly bring up
secondary CPUs during boot. This means that it has to be initialized at
the early_initcall level before SMP is initialized. So that rules out
most of the device driver niceties which are not initialized yet, and
that also means that this can't be made into a loadable module.

To make things even more subtle, this code is needed to implement the
backend for the MCPM layer through which the actual bringup of secondary
CPUs is done. So that MCPM backend is itself also initialized at the
early_initcall level, but we don't know and can't enforce the order
those different things will be initialized. So the MCPM backend calls
vexpress_spc_check_loaded() to initialize it if it has not been
initialized yet, or return false if there is no SPC on the booted
system.

Yet this code is also the entry point for some operating point changes
requested by the cpufreq driver, and that one can be modular. And the
cpufreq driver also has to test for the presence of a SPC via
vexpress_spc_check_loaded().

So that's the main reason why there is a static state variable, and why
there is a switch of function pointed by spc_check_loaded to let the
init code go after boot and still be able to be referenced by modules
after boot.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/