Re: [RFC PATCH 5/9] dt: deps: register drivers based on the initialization order based on DT

[Grant Likely]

On Mon, 12 May 2014 18:47:56 +0200, Alexander Holler <holler@xxxxxxxxxxxxx> wrote:
> The init system currently calls unknown functions with almost unknown
> functionality in an almost random order.

Correct, we've got a module system. Some would say that is a strength!
:-) That said, I don't object to optimizing to the optimal order when
possible.

> Fixing this is on a short-term basis is a bit tricky.
> 
> In order to register drivers with a deterministic order, a list of all
> available in-kernel drivers is needed. Unfortunately such a list doesn't
> exist, just a list of initcalls does exist.
> 
> The trick now is to first call special annotated initcalls (I call those
> "well done") for platform drivers, but not actualy starting those drivers
> by calling their probe function, but just collectiong their meta datas
> (struct platform_driver). After all those informations were collected,
> available the drivers will be started according to the previously
> determined order.

Why does the initcall level matter? Why not just let the initcalls
happen, capture the calls that register a driver, and then use that list
later?

> 
> The annotation of such platform drivers is necessary because it must be
> made sure that those drivers don't care if the probe is actually called in
> their initcall or later.
> 
> That means that all platform drivers which already do use
> 
> 	module_platform_driver() or
> 	module_platform_driver_probe()
> 
> don't need any modification because their initcall is known and already well
> done. But all platform drivers which do use
> 
> 	module_init() or
> 	*_initcall()
> 
> have to be reviewed if they are "well done". If they are, they need a change
> like
> 
> 	-module_init(foo_init);
> 	+well_done_platform_module_init(foo_init);
> 
> or
> 
> 	-subsys_initcall(foo_init);
> 	+well_done_platform_initcall(subsys, foo_init);
> 
> to become included in the deterministic order in which platform drivers
> will be initialized.
> 
> All other platform drivers will still be initialized in random order before
> platform drivers included in the deterministic order will be initialized.
> "Well done" drivers which don't appear in the order (because they don't appear
> in the DT) will be initialized after those which do appear in the order.
> 
> If CONFIG_OF_DEPENDENCIES is disabled, nothing is changed at all.
> 
> The long range target to fix the problem should be to include a list (array)
> of struct platform_driver in the kernel for all in-kernel platform drivers,
> instead of just initcalls. This will be easy if all platform drivers have
> become "well done".

How will that list be constructed? How will it account for multiple
platforms, each requiring a different init order?

> 
> Unfortunately there are some drivers which will need quiet some changes
> to become "well done". As an example for such an initcall look e.g. at
> drivers/tty/serial/8250/8250_core.c.
> 
> Signed-off-by: Alexander Holler <holler@xxxxxxxxxxxxx>
> ---
>  drivers/base/platform.c           | 13 +++++++
>  drivers/of/of_dependencies.c      | 79 +++++++++++++++++++++++++++++++++++++++
>  include/asm-generic/vmlinux.lds.h |  1 +
>  include/linux/init.h              | 19 ++++++++++
>  include/linux/of_dependencies.h   |  5 +++
>  include/linux/platform_device.h   | 16 ++++++--
>  init/main.c                       | 17 ++++++++-
>  7 files changed, 145 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index bc78848..b9c9b33 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -13,6 +13,7 @@
>  #include <linux/string.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_device.h>
> +#include <linux/of_dependencies.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/dma-mapping.h>
> @@ -541,6 +542,12 @@ int __platform_driver_register(struct platform_driver *drv,
>  	if (drv->shutdown)
>  		drv->driver.shutdown = platform_drv_shutdown;
>  
> +#ifdef CONFIG_OF_DEPENDENCIES
> +	if (of_init_is_recording())
> +		/* Just record the driver */
> +		return of_init_register_platform_driver(drv);
> +	else
> +#endif
>  	return driver_register(&drv->driver);
>  }
>  EXPORT_SYMBOL_GPL(__platform_driver_register);
> @@ -590,8 +597,14 @@ int __init_or_module platform_driver_probe(struct platform_driver *drv,
>  
>  	/* temporary section violation during probe() */
>  	drv->probe = probe;
> +
>  	retval = code = platform_driver_register(drv);
>  
> +#ifdef CONFIG_OF_DEPENDENCIES
> +	if (of_init_is_recording())
> +		/* Just record the driver */
> +		return retval;
> +#endif
>  	/*
>  	 * Fixup that section violation, being paranoid about code scanning
>  	 * the list of drivers in order to probe new devices.  Check to see
> diff --git a/drivers/of/of_dependencies.c b/drivers/of/of_dependencies.c
> index 7905172..4af62d5 100644
> --- a/drivers/of/of_dependencies.c
> +++ b/drivers/of/of_dependencies.c
> @@ -46,9 +46,12 @@ struct init_order {
>  	/* Used to keep track of parent devices in regard to the DT */
>  	uint32_t parent_by_phandle[MAX_DT_NODES+1];
>  	struct device *device_by_phandle[MAX_DT_NODES+1];
> +	struct platform_driver *platform_drivers[MAX_DT_NODES+1];
> +	unsigned count_drivers;
>  };
>  static struct init_order order __initdata;
>  
> +static bool is_recording;
>  
>  /* Copied from drivers/of/base.c (because it's lockless). */
>  static struct property * __init __of_find_property(const struct device_node *np,
> @@ -401,3 +404,79 @@ void __init of_init_free_order(void)
>  	order.count = 0;
>  	/* remove_new_phandles(); */
>  }
> +
> +void __init of_init_set_recording(bool recording)
> +{
> +	is_recording = recording;
> +}
> +
> +bool of_init_is_recording(void)
> +{
> +	return is_recording;
> +}
> +
> +int of_init_register_platform_driver(struct platform_driver *drv)
> +{
> +	BUG_ON(!is_recording);
> +	order.platform_drivers[order.count_drivers++] = drv;
> +	return 0;
> +}
> +
> +void __init of_init_register_drivers(void)
> +{
> +	unsigned i, j;
> +	int rc __maybe_unused;
> +
> +	BUG_ON(is_recording);
> +	/*
> +	 * Because we already have a list of devices and drivers together
> +	 * with their compatible strings, the below code could be speed up
> +	 * by replacing the functions which are walking through lists with
> +	 * something which uses trees or hashes to compare/search strings.
> +	 * These are of_driver_match_device() and driver_find() (the latter
> +	 * is called again in driver_register().
> +	 */
> +	for (i = 0; i < order.count; ++i) {
> +		struct device_node *node = order.order[i];
> +		struct device *dev = order.device_by_phandle[node->phandle];
> +
> +		for (j = 0; j < order.count_drivers; ++j) {
> +			struct platform_driver *drv = order.platform_drivers[j];
> +
> +			if (of_driver_match_device(dev, &drv->driver)) {
> +				if (!driver_find(drv->driver.name,
> +				    drv->driver.bus))
> +					platform_driver_register(drv);
> +				if (dev->parent)
> +					device_lock(dev->parent);
> +				rc = device_attach(dev);
> +				if (dev->parent)
> +					device_unlock(dev->parent);
> +				break;
> +			}
> +		}
> +		if (j >= order.count_drivers) {
> +			/*
> +			 * No driver in the initialization order matched,
> +			 * try to attach the device, maybe a driver already
> +			 * exists (e.g. loaded pre-smp).
> +			 */
> +			if (dev->parent)
> +				device_lock(dev->parent);
> +			rc = device_attach(dev);
> +			if (dev->parent)
> +				device_unlock(dev->parent);
> +		}
> +	}
> +	/*
> +	 * Now just register all drivers, including those not used through
> +	 * the initialization order (well-done drivers which aren't listed
> +	 * in the DT or blacklisted through of_init_create_devices()).
> +	 */
> +	for (j = 0; j < order.count_drivers; ++j) {
> +		struct platform_driver *drv = order.platform_drivers[j];
> +
> +		if (!driver_find(drv->driver.name, drv->driver.bus))
> +			platform_driver_register(drv);
> +	}
> +}
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index bc2121f..cedb3b0 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -633,6 +633,7 @@
>  		INIT_CALLS_LEVEL(rootfs)				\
>  		INIT_CALLS_LEVEL(6)					\
>  		INIT_CALLS_LEVEL(7)					\
> +		INIT_CALLS_LEVEL(8)					\
>  		VMLINUX_SYMBOL(__initcall_end) = .;
>  
>  #define CON_INITCALL							\
> diff --git a/include/linux/init.h b/include/linux/init.h
> index e168880..acb7dfa 100644
> --- a/include/linux/init.h
> +++ b/include/linux/init.h
> @@ -209,6 +209,23 @@ extern bool initcall_debug;
>  #define late_initcall(fn)		__define_initcall(fn, 7)
>  #define late_initcall_sync(fn)		__define_initcall(fn, 7s)
>  
> +/*
> + * A well_done_platform_module_init or well_done_platform_initcall
> + * only calls platform_driver_register() or platform_driver_probe()
> + * and ignores the return code. This is necessary because the
> + * actual calls to platform_driver_register() or platform_driver_probe()
> + * will be delayed when CONFIG_OF_DEPENDENCIES is enabled. This is done
> + * to sort those calls based on the dependencies in the DT (matched to the
> + * platform driver data).
> + */
> +#ifdef CONFIG_OF_DEPENDENCIES
> +#define well_done_platform_module_init(fn)	__define_initcall(fn, 8)
> +#define well_done_platform_initcall(leve, fn)	__define_initcall(fn, 8)
> +#else
> +#define well_done_platform_module_init(fn)	module_init(fn)
> +#define well_done_platform_initcall(level, fn)	level ## _initcall(fn)
> +#endif
> +
>  #define __initcall(fn) device_initcall(fn)
>  
>  #define __exitcall(fn) \
> @@ -289,6 +306,8 @@ void __init parse_early_options(char *cmdline);
>  #define rootfs_initcall(fn)		module_init(fn)
>  #define device_initcall(fn)		module_init(fn)
>  #define late_initcall(fn)		module_init(fn)
> +#define well_done_platform_initcall(fn)	module_init(fn)
> +#define well_done_platform_module_init(fn)	module_init(fn)
>  
>  #define console_initcall(fn)		module_init(fn)
>  #define security_initcall(fn)		module_init(fn)
> diff --git a/include/linux/of_dependencies.h b/include/linux/of_dependencies.h
> index e046ce2..8869162 100644
> --- a/include/linux/of_dependencies.h
> +++ b/include/linux/of_dependencies.h
> @@ -58,4 +58,9 @@ extern void of_init_create_devices(const struct of_device_id *matches,
>   */
>  extern void of_init_free_order(void);
>  
> +void of_init_set_recording(bool recording);
> +bool of_init_is_recording(void);
> +int of_init_register_platform_driver(struct platform_driver *drv);
> +void of_init_register_drivers(void);
> +
>  #endif	/* _LINUX_OF_DEPENDENCIES_H */
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 16f6654..b8559d9 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -215,9 +215,17 @@ static inline void platform_set_drvdata(struct platform_device *pdev,
>   * boilerplate.  Each module may only use this macro once, and
>   * calling it replaces module_init() and module_exit()
>   */
> -#define module_platform_driver(__platform_driver) \
> -	module_driver(__platform_driver, platform_driver_register, \
> -			platform_driver_unregister)
> +#define module_platform_driver(__driver) \
> +static int __init __driver##_init(void) \
> +{ \
> +	return platform_driver_register(&(__driver)); \
> +} \
> +well_done_platform_module_init(__driver##_init); \
> +static void __exit __driver##_exit(void) \
> +{ \
> +	platform_driver_unregister(&(__driver)); \
> +} \
> +module_exit(__driver##_exit);
>  
>  /* module_platform_driver_probe() - Helper macro for drivers that don't do
>   * anything special in module init/exit.  This eliminates a lot of
> @@ -230,7 +238,7 @@ static int __init __platform_driver##_init(void) \
>  	return platform_driver_probe(&(__platform_driver), \
>  				     __platform_probe);    \
>  } \
> -module_init(__platform_driver##_init); \
> +well_done_platform_module_init(__platform_driver##_init); \
>  static void __exit __platform_driver##_exit(void) \
>  { \
>  	platform_driver_unregister(&(__platform_driver)); \
> diff --git a/init/main.c b/init/main.c
> index 9c7fd4c..7591cd1 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -77,6 +77,7 @@
>  #include <linux/sched_clock.h>
>  #include <linux/context_tracking.h>
>  #include <linux/random.h>
> +#include <linux/of_dependencies.h>
>  
>  #include <asm/io.h>
>  #include <asm/bugs.h>
> @@ -720,6 +721,7 @@ extern initcall_t __initcall4_start[];
>  extern initcall_t __initcall5_start[];
>  extern initcall_t __initcall6_start[];
>  extern initcall_t __initcall7_start[];
> +extern initcall_t __initcall8_start[];
>  extern initcall_t __initcall_end[];
>  
>  static initcall_t *initcall_levels[] __initdata = {
> @@ -731,6 +733,7 @@ static initcall_t *initcall_levels[] __initdata = {
>  	__initcall5_start,
>  	__initcall6_start,
>  	__initcall7_start,
> +	__initcall8_start,
>  	__initcall_end,
>  };
>  
> @@ -744,6 +747,8 @@ static char *initcall_level_names[] __initdata = {
>  	"fs",
>  	"device",
>  	"late",
> +	/* must be the last level to become excluded in do_initcalls() */
> +	"well-done-platform-driver",
>  };
>  
>  static void __init do_initcall_level(int level)
> @@ -766,7 +771,7 @@ static void __init do_initcalls(void)
>  {
>  	int level;
>  
> -	for (level = 0; level < ARRAY_SIZE(initcall_levels) - 1; level++)
> +	for (level = 0; level < ARRAY_SIZE(initcall_levels) - 3; level++)
>  		do_initcall_level(level);
>  }
>  
> @@ -787,6 +792,16 @@ static void __init do_basic_setup(void)
>  	do_ctors();
>  	usermodehelper_enable();
>  	do_initcalls();
> +#ifdef CONFIG_OF_DEPENDENCIES
> +	/* collect a list of available platform drivers */
> +	of_init_set_recording(true);
> +	do_initcall_level(ARRAY_SIZE(initcall_levels) - 2);
> +	of_init_set_recording(false);
> +	/* probe available platform drivers with deterministic order */
> +	of_init_register_drivers();
> +	/* register late drivers */
> +	do_initcall_level(ARRAY_SIZE(initcall_levels) - 3);
> +#endif
>  	random_int_secret_init();
>  }
>  
> -- 
> 1.8.3.1
> 
> --
> 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/

--
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/