| Message ID | 20191217102219.29223-1-info@metux.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: platform driver registering via initcall tables | expand |
On Tue, Dec 17, 2019 at 11:22:19AM +0100, Enrico Weigelt, metux IT consult wrote: > A large portion of platform drivers doesn't need their own init/exit > functions. At source level, the boilerplate is already replaced by > module_platform_driver() macro call, which creates this code under > the hood. But in the binary, the code is still there. > > This patch is an attempt to remove them it, by the same approach > already used for the init functions: collect pointers to the driver > structs in special sections, which are then processed by the init > code which already calls the init function vectors. For each level, > the structs are processed right after the init funcs, so we guarantee > the existing order, and explicit inits always come before the automatic > registering. No, what is so "special" about platform drivers that they require this? If anything, we should be moving _AWAY_ from platform drivers and use real bus drivers instead. > Downside of apprach: cluttering init code w/ a little bit knowledge > about driver related stuff (calls to platform_driver_register(), etc). Exactly, don't. > For now, only implemented for the built-in case (modules still go the > old route). The module case is a little bit trickier: either we have to > extend the module header (and modpost tool) or do some dynamic symbol > lookup. > > This patch is just a PoC for further discussions, not ready for mainline. > It also changes a few drivers, just for illustration. In case the general > approach is accepted, it will be cleaned up and splitted. Please no, I don't see why this is even needed. greg k-h
On 17.12.19 11:31, Greg KH wrote: Hi, > No, what is so "special" about platform drivers that they require this? Nothing, of course ;-) It's the the starting point for this PoC. The idea actually is doing this for all other driver types, too (eg. spi, pci, usb, ...). But they'll need their own tables, as different *_register() functions have to be called - just haven't implemented that yet. > If anything, we should be moving _AWAY_ from platform drivers and use > real bus drivers instead. That would be nice, but, unfortunately, we have lots of devices which aren't attached to any (probing-capable) bus. That's why we have things like oftree, etc. > Please no, I don't see why this is even needed. The idea is getting rid of all the init code, which all just does the same, just calls some *_register() function. --mtx --- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering info@metux.net -- +49-151-27565287
On Tue, Dec 17, 2019 at 02:44:39PM +0100, Enrico Weigelt, metux IT consult wrote: > On 17.12.19 11:31, Greg KH wrote: > > Hi, > > > No, what is so "special" about platform drivers that they require this? > > Nothing, of course ;-) > > It's the the starting point for this PoC. The idea actually is doing > this for all other driver types, too (eg. spi, pci, usb, ...). But > they'll need their own tables, as different *_register() functions have > to be called - just haven't implemented that yet. That's not needed, and you are going to break the implicit ordering we already have with link order. You are going to have to figure out what bus type the driver is, to determine what segment it was in, to figure out what was loaded before what. Not good. > > If anything, we should be moving _AWAY_ from platform drivers and use > > real bus drivers instead. > > That would be nice, but, unfortunately, we have lots of devices which > aren't attached to any (probing-capable) bus. That's why we have things > like oftree, etc. > > > Please no, I don't see why this is even needed. > > The idea is getting rid of all the init code, which all just does the > same, just calls some *_register() function. There's no need to get rid of it, what are you trying to save here? How can you be sure init order is still the same? thanks, greg k-h
On 17.12.19 15:06, Greg KH wrote: > That's not needed, and you are going to break the implicit ordering we > already have with link order. Ups, 10 points for you - I didn't consider that. > You are going to have to figure out what > bus type the driver is, to determine what segment it was in, to figure > out what was loaded before what. hmm, if it's just the ordering by bus type (but not within one bus type), then it shouldn't be the big deal to fix, as I'll need one table and register-loop per bus-type anyways. By the way: how is there init order ensured with dynamically loaded modules ? (for cases where there aren't explicit symbol dependencies) --mtx --- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering info@metux.net -- +49-151-27565287
On Tue, Dec 17, 2019 at 03:43:56PM +0100, Enrico Weigelt, metux IT consult wrote: > On 17.12.19 15:06, Greg KH wrote: > > > That's not needed, and you are going to break the implicit ordering we > > already have with link order. > > Ups, 10 points for you - I didn't consider that. > > > You are going to have to figure out what > > bus type the driver is, to determine what segment it was in, to figure > > out what was loaded before what. > > hmm, if it's just the ordering by bus type (but not within one bus > type), then it shouldn't be the big deal to fix, as I'll need one table > and register-loop per bus-type anyways. > > By the way: how is there init order ensured with dynamically loaded > modules ? (for cases where there aren't explicit symbol dependencies) See the recent work in the driver core for DT fixes for that very issue. greg k-h
diff --git a/drivers/gpio/gpio-amd-fch.c b/drivers/gpio/gpio-amd-fch.c index 4e44ba4d7423..2542d30258c6 100644 --- a/drivers/gpio/gpio-amd-fch.c +++ b/drivers/gpio/gpio-amd-fch.c @@ -183,8 +183,7 @@ static struct platform_driver amd_fch_gpio_driver = { }, .probe = amd_fch_gpio_probe, }; - -module_platform_driver(amd_fch_gpio_driver); +MODULE_PLATFORM_DRIVER(amd_fch_gpio_driver); MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@metux.net>"); MODULE_DESCRIPTION("AMD G-series FCH GPIO driver"); diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c index 6eb0a2f3f9de..0c107c11dd1d 100644 --- a/drivers/input/keyboard/gpio_keys_polled.c +++ b/drivers/input/keyboard/gpio_keys_polled.c @@ -383,7 +383,7 @@ static struct platform_driver gpio_keys_polled_driver = { .of_match_table = gpio_keys_polled_of_match, }, }; -module_platform_driver(gpio_keys_polled_driver); +MODULE_PLATFORM_DRIVER(gpio_keys_polled_driver); MODULE_LICENSE("GPL v2"); MODULE_AUTHOR("Gabor Juhos <juhosg@openwrt.org>"); diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c index a5c73f3d5f79..cf0ef4a9eb79 100644 --- a/drivers/leds/leds-gpio.c +++ b/drivers/leds/leds-gpio.c @@ -313,8 +313,7 @@ static struct platform_driver gpio_led_driver = { .of_match_table = of_gpio_leds_match, }, }; - -module_platform_driver(gpio_led_driver); +MODULE_PLATFORM_DRIVER(gpio_led_driver); MODULE_AUTHOR("Raphael Assenat <raph@8d.com>, Trent Piepho <tpiepho@freescale.com>"); MODULE_DESCRIPTION("GPIO LED driver"); diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index e00f41aa8ec4..5956f64db7c6 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -851,6 +851,25 @@ INIT_CALLS_LEVEL(7) \ __initcall_end = .; +#define INIT_DRVS_PLAT_LEVEL(level) \ + __initdrv_plat##level##_start = .; \ + KEEP(*(.initdrv_plat##level##.init)) \ + KEEP(*(.initdrv_plat##level##s.init)) \ + +#define INIT_DRVS \ + __initdrv_plat_start = .; \ + KEEP(*(.initdrv_plat_early.init)) \ + INIT_DRVS_PLAT_LEVEL(0) \ + INIT_DRVS_PLAT_LEVEL(1) \ + INIT_DRVS_PLAT_LEVEL(2) \ + INIT_DRVS_PLAT_LEVEL(3) \ + INIT_DRVS_PLAT_LEVEL(4) \ + INIT_DRVS_PLAT_LEVEL(5) \ + INIT_DRVS_PLAT_LEVEL(rootfs) \ + INIT_DRVS_PLAT_LEVEL(6) \ + INIT_DRVS_PLAT_LEVEL(7) \ + __initdrv_plat_end = .; + #define CON_INITCALL \ __con_initcall_start = .; \ KEEP(*(.con_initcall.init)) \ @@ -1022,6 +1041,7 @@ INIT_DATA \ INIT_SETUP(initsetup_align) \ INIT_CALLS \ + INIT_DRVS \ CON_INITCALL \ INIT_RAM_FS \ } diff --git a/include/linux/init.h b/include/linux/init.h index 212fc9e2f691..9aa1695bf69c 100644 --- a/include/linux/init.h +++ b/include/linux/init.h @@ -123,6 +123,11 @@ static inline initcall_t initcall_from_entry(initcall_entry_t *entry) { return offset_to_ptr(entry); } + +static inline struct platform_driver *initdrv_plat_from_entry(initcall_entry_t *entry) +{ + return offset_to_ptr(entry); +} #else typedef initcall_t initcall_entry_t; @@ -130,6 +135,11 @@ static inline initcall_t initcall_from_entry(initcall_entry_t *entry) { return *entry; } + +static inline struct platform_driver *initdrv_plat_from_entry(initcall_entry_t *entry) +{ + return *entry; +} #endif extern initcall_entry_t __con_initcall_start[], __con_initcall_end[]; @@ -191,13 +201,28 @@ extern bool initcall_debug; "__initcall_" #fn #id ": \n" \ ".long " #fn " - . \n" \ ".previous \n"); + +#define ___define_platform_driver(pd, id, __sec) \ + __ADDRESSABLE(pd) \ + asm(".section \"" #__sec ".init\", \"a\" \n" \ + "__plat_drv__" #pd #id ": \n" \ + ".long " #pd " - . \n" \ + ".previous \n"); + + #else #define ___define_initcall(fn, id, __sec) \ static initcall_t __initcall_##fn##id __used \ __attribute__((__section__(#__sec ".init"))) = fn; + +#define ___define_platform_driver(fn, id, __sec) \ + static initcall_t __initcall_##pd##id __used \ + __attribute__((__section__(#__sec ".init"))) = fn; + #endif #define __define_initcall(fn, id) ___define_initcall(fn, id, .initcall##id) +#define __define_platform_driver(fn, id) ___define_platform_driver(fn, id, .initdrv_plat##id) /* * Early initcalls run before initializing SMP. diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index 276a03c24691..3868b0e75f5f 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -244,6 +244,15 @@ static inline void platform_set_drvdata(struct platform_device *pdev, module_driver(__platform_driver, platform_driver_register, \ platform_driver_unregister) +#ifdef MODULE +#define MODULE_PLATFORM_DRIVER(__platform_driver) \ + module_driver(__platform_driver, platform_driver_register, \ + platform_driver_unregister) +#else +#define MODULE_PLATFORM_DRIVER(__platform_driver) \ + __define_platform_driver(__platform_driver, 6) +#endif + /* builtin_platform_driver() - Helper macro for builtin drivers that * don't do anything special in driver init. This eliminates some * boilerplate. Each driver may only use this macro once, and diff --git a/init/main.c b/init/main.c index ec3a1463ac69..754b68111b53 100644 --- a/init/main.c +++ b/init/main.c @@ -94,6 +94,7 @@ #include <linux/jump_label.h> #include <linux/mem_encrypt.h> #include <linux/file.h> +#include <linux/platform_device.h> #include <asm/io.h> #include <asm/bugs.h> @@ -955,6 +956,22 @@ int __init_or_module do_one_initcall(initcall_t fn) return ret; } +struct platform_driver; + +int __init_or_module do_one_initdrv_plat(struct platform_driver *pd) +{ + int rc; + + rc = platform_driver_register(pd); + if (rc) + pr_warn("init: failed registering platform driver: %s: %d\n", + pd->driver.name, rc); + else + pr_info("init: registered platform driver: %s\n", + pd->driver.name); + + return rc; +} extern initcall_entry_t __initcall_start[]; extern initcall_entry_t __initcall0_start[]; @@ -979,6 +996,29 @@ static initcall_entry_t *initcall_levels[] __initdata = { __initcall_end, }; +extern initcall_entry_t __initdrv_plat_start[]; +extern initcall_entry_t __initdrv_plat0_start[]; +extern initcall_entry_t __initdrv_plat1_start[]; +extern initcall_entry_t __initdrv_plat2_start[]; +extern initcall_entry_t __initdrv_plat3_start[]; +extern initcall_entry_t __initdrv_plat4_start[]; +extern initcall_entry_t __initdrv_plat5_start[]; +extern initcall_entry_t __initdrv_plat6_start[]; +extern initcall_entry_t __initdrv_plat7_start[]; +extern initcall_entry_t __initdrv_plat_end[]; + +static initcall_entry_t *initdrv_plat_levels[] __initdata = { + __initdrv_plat0_start, + __initdrv_plat1_start, + __initdrv_plat2_start, + __initdrv_plat3_start, + __initdrv_plat4_start, + __initdrv_plat5_start, + __initdrv_plat6_start, + __initdrv_plat7_start, + __initdrv_plat_end, +}; + /* Keep these in sync with initcalls in include/linux/init.h */ static const char *initcall_level_names[] __initdata = { "pure", @@ -1005,6 +1045,9 @@ static void __init do_initcall_level(int level) trace_initcall_level(initcall_level_names[level]); for (fn = initcall_levels[level]; fn < initcall_levels[level+1]; fn++) do_one_initcall(initcall_from_entry(fn)); + + for (fn = initdrv_plat_levels[level]; fn < initdrv_plat_levels[level+1]; fn++) + do_one_initdrv_plat(initdrv_plat_from_entry(fn)); } static void __init do_initcalls(void) diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index 6e892c93d104..7fc8871c4da6 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -957,7 +957,7 @@ static void check_section(const char *modname, struct elf_info *elf, #define ALL_INIT_SECTIONS INIT_SECTIONS, ALL_XXXINIT_SECTIONS #define ALL_EXIT_SECTIONS EXIT_SECTIONS, ALL_XXXEXIT_SECTIONS -#define DATA_SECTIONS ".data", ".data.rel" +#define DATA_SECTIONS ".data", ".data.rel", ".data.platdrv" #define TEXT_SECTIONS ".text", ".text.unlikely", ".sched.text", \ ".kprobes.text", ".cpuidle.text" #define OTHER_TEXT_SECTIONS ".ref.text", ".head.text", ".spinlock.text", \
A large portion of platform drivers doesn't need their own init/exit functions. At source level, the boilerplate is already replaced by module_platform_driver() macro call, which creates this code under the hood. But in the binary, the code is still there. This patch is an attempt to remove them it, by the same approach already used for the init functions: collect pointers to the driver structs in special sections, which are then processed by the init code which already calls the init function vectors. For each level, the structs are processed right after the init funcs, so we guarantee the existing order, and explicit inits always come before the automatic registering. Downside of apprach: cluttering init code w/ a little bit knowledge about driver related stuff (calls to platform_driver_register(), etc). For now, only implemented for the built-in case (modules still go the old route). The module case is a little bit trickier: either we have to extend the module header (and modpost tool) or do some dynamic symbol lookup. This patch is just a PoC for further discussions, not ready for mainline. It also changes a few drivers, just for illustration. In case the general approach is accepted, it will be cleaned up and splitted. 2DO: * check for potential arch specific issues (-> um ?) * implement loadable module case * add other driver types (eg. spi, pci, ...) * design a practical way for converting drivers step by step (or shall we just brutally change module_platform_driver() ?) Please let me know what you think about this. happy hacking --mtx --- drivers/gpio/gpio-amd-fch.c | 3 +-- drivers/input/keyboard/gpio_keys_polled.c | 2 +- drivers/leds/leds-gpio.c | 3 +-- include/asm-generic/vmlinux.lds.h | 20 ++++++++++++++ include/linux/init.h | 25 ++++++++++++++++++ include/linux/platform_device.h | 9 +++++++ init/main.c | 43 +++++++++++++++++++++++++++++++ scripts/mod/modpost.c | 2 +- 8 files changed, 101 insertions(+), 6 deletions(-)