| Message ID | 20201210034003.222297-1-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | RISC-V Kendryte K210 support improvements | expand |
Hi Damien, On Thu, Dec 10, 2020 at 4:41 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > Changes from v6: > * Annotate struct platform_driver variables with __refdata to avoid > section mismatch compilation errors Blindly following the advice from kernel test robot <lkp@intel.com> is not always a good idea: The variable k210_rst_driver references the function __init set_reset_devices() If the reference is valid then annotate the variable with or __refdata (see linux/init.h) or name the variable: If your driver's probe function is annotated with __init, you cannot have a pointer to it in the driver structure, as any binding done after the freeing of initmem will cause a crash. Adding the __refdata merely suppresses the warning, and won't avoid the crash. There are two solutions for this: 1. Remove the .probe pointer, and use platform_driver_probe() instead of platform_driver_register(). This guarantees the probe method will be called only once, before initmem is freed, but does mean that probe deferral cannot work. 2. Drop the __init annotation. This means the probe method can be called anytime, and supports both probe deferral and driver unbind/rebind cycles. Given the limited amount of RAM on k210, I think option 1 is preferred, but may require careful tuning of the initialization order using *_initcall*(), to make sure probe deferral won't ever be needed. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, 2020-12-10 at 11:04 +0100, Geert Uytterhoeven wrote: > Hi Damien, > > On Thu, Dec 10, 2020 at 4:41 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > > Changes from v6: > > * Annotate struct platform_driver variables with __refdata to avoid > > section mismatch compilation errors > > Blindly following the advice from kernel test robot <lkp@intel.com> is > not always a good idea: > > The variable k210_rst_driver references > the function __init set_reset_devices() > If the reference is valid then annotate the > variable with or __refdata (see linux/init.h) or name the variable: > > If your driver's probe function is annotated with __init, you cannot > have a pointer to it in the driver structure, as any binding done after > the freeing of initmem will cause a crash. Adding the __refdata merely > suppresses the warning, and won't avoid the crash. Hmm... I must be misunderstanding something here. free_initmem() is called from kernel_init() right before starting the user init process. That is late enough that all drivers are already probed and initialized. At least that is what I thought, especially considering that none of the k210 drivers can be modules and are all builtin. What am I missing here ? > There are two solutions for this: > 1. Remove the .probe pointer, and use platform_driver_probe() instead > of platform_driver_register(). > This guarantees the probe method will be called only once, before > initmem is freed, but does mean that probe deferral cannot work. > 2. Drop the __init annotation. > This means the probe method can be called anytime, and supports > both probe deferral and driver unbind/rebind cycles. > > Given the limited amount of RAM on k210, I think option 1 is preferred, > but may require careful tuning of the initialization order using > *_initcall*(), to make sure probe deferral won't ever be needed. I really would prefer to avoid the *_initcall() dance. That is not pretty. Simply removing the __init annotation, I see 766 pages free with "cat /proc/vmstat" right after boot, and the same number with __init too... So it does not look like the impact is significant. The biggest probe function is for the clock driver and this one seems fine with __init since it uses CLK_OF_DECLARE(). I do have a doubt about this now though. Looking at other drivers, all seem to have the __init annotation for their clock driver init function. So I think I will go with option 2. It is simpler and safer. We can always revisit and optimize later. I would prefer this series to land first :) > > Gr{oetje,eeting}s, > > Geert > > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Damien, On Thu, Dec 10, 2020 at 1:36 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > On Thu, 2020-12-10 at 11:04 +0100, Geert Uytterhoeven wrote: > > On Thu, Dec 10, 2020 at 4:41 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > > > Changes from v6: > > > * Annotate struct platform_driver variables with __refdata to avoid > > > section mismatch compilation errors > > > > Blindly following the advice from kernel test robot <lkp@intel.com> is > > not always a good idea: > > > > The variable k210_rst_driver references > > the function __init set_reset_devices() > > If the reference is valid then annotate the > > variable with or __refdata (see linux/init.h) or name the variable: > > > > If your driver's probe function is annotated with __init, you cannot > > have a pointer to it in the driver structure, as any binding done after > > the freeing of initmem will cause a crash. Adding the __refdata merely > > suppresses the warning, and won't avoid the crash. > > Hmm... I must be misunderstanding something here. free_initmem() is called from > kernel_init() right before starting the user init process. That is late enough > that all drivers are already probed and initialized. At least that is what I > thought, especially considering that none of the k210 drivers can be modules > and are all builtin. What am I missing here ? For these specific cases, binding is indeed unlikely to happen after free_initmem(). In the generic case that is not true. However, you can still trigger it manually by unbinding and rebinding the device manually through sysfs. > So I think I will go with option 2. It is simpler and safer. We can always > revisit and optimize later. I would prefer this series to land first :) Right. Correctness first, performance later. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 2020/12/10 22:23, Geert Uytterhoeven wrote: > Hi Damien, > > On Thu, Dec 10, 2020 at 1:36 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: >> On Thu, 2020-12-10 at 11:04 +0100, Geert Uytterhoeven wrote: >>> On Thu, Dec 10, 2020 at 4:41 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: >>>> Changes from v6: >>>> * Annotate struct platform_driver variables with __refdata to avoid >>>> section mismatch compilation errors >>> >>> Blindly following the advice from kernel test robot <lkp@intel.com> is >>> not always a good idea: >>> >>> The variable k210_rst_driver references >>> the function __init set_reset_devices() >>> If the reference is valid then annotate the >>> variable with or __refdata (see linux/init.h) or name the variable: >>> >>> If your driver's probe function is annotated with __init, you cannot >>> have a pointer to it in the driver structure, as any binding done after >>> the freeing of initmem will cause a crash. Adding the __refdata merely >>> suppresses the warning, and won't avoid the crash. >> >> Hmm... I must be misunderstanding something here. free_initmem() is called from >> kernel_init() right before starting the user init process. That is late enough >> that all drivers are already probed and initialized. At least that is what I >> thought, especially considering that none of the k210 drivers can be modules >> and are all builtin. What am I missing here ? > > For these specific cases, binding is indeed unlikely to happen after > free_initmem(). In the generic case that is not true. > However, you can still trigger it manually by unbinding and rebinding > the device manually through sysfs. Got it. Sending a v8 with the correction. Thanks ! > >> So I think I will go with option 2. It is simpler and safer. We can always >> revisit and optimize later. I would prefer this series to land first :) > > Right. Correctness first, performance later. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds >