| Message ID | 20201210034003.222297-1-damien.lemoal@wdc.com |
|---|---|
| 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
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