| Message ID | 20190629114136.45e90292@endymion (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
| Series | x86: apuv2: Fix softdep statement | expand |
On Sat, Jun 29, 2019 at 12:41 PM Jean Delvare <jdelvare@suse.de> wrote: > > Only one MODULE_SOFTDEP statement is allowed per module. Multiple > dependencies must be expressed in a single statement. Some module init utils even do not support softdep. Nevertheless, the message is somewhat misleading. It's not "only one allowed" — this is not true, it's "only first will be served". This is how I read kmod sources. And perhaps better to fix them? At least I would rather support somelike MODULE_SOFTDEP("pre: ..."); MODULE_SOFTDEP("post: ..."); > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Cc: "Enrico Weigelt, metux IT consult" <info@metux.net> > Cc: Darren Hart <dvhart@infradead.org> > Cc: Andy Shevchenko <andy@infradead.org> > --- > drivers/platform/x86/pcengines-apuv2.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > --- linux-5.1.orig/drivers/platform/x86/pcengines-apuv2.c 2019-05-06 02:42:58.000000000 +0200 > +++ linux-5.1/drivers/platform/x86/pcengines-apuv2.c 2019-06-29 11:37:48.062005738 +0200 > @@ -255,6 +255,4 @@ MODULE_DESCRIPTION("PC Engines APUv2/APU > MODULE_LICENSE("GPL"); > MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table); > MODULE_ALIAS("platform:pcengines-apuv2"); > -MODULE_SOFTDEP("pre: platform:" AMD_FCH_GPIO_DRIVER_NAME); > -MODULE_SOFTDEP("pre: platform:leds-gpio"); > -MODULE_SOFTDEP("pre: platform:gpio_keys_polled"); > +MODULE_SOFTDEP("pre: platform:" AMD_FCH_GPIO_DRIVER_NAME " platform:leds-gpio platform:gpio_keys_polled"); > > > -- > Jean Delvare > SUSE L3 Support -- With Best Regards, Andy Shevchenko
Hi Andy, On Sat, 29 Jun 2019 14:13:02 +0300, Andy Shevchenko wrote: > On Sat, Jun 29, 2019 at 12:41 PM Jean Delvare <jdelvare@suse.de> wrote: > > > > Only one MODULE_SOFTDEP statement is allowed per module. Multiple > > dependencies must be expressed in a single statement. > > Some module init utils even do not support softdep. And? > Nevertheless, the message is somewhat misleading. It's not "only one > allowed" — this is not true, it's "only first will be served". > This is how I read kmod sources. What practical difference does it make? > And perhaps better to fix them? It's not considered a bug, as it is already possible to have multiple dependencies listed, you only have to put them in the same statement. There are several other MODULE_* macros which also can be used only once per module (MODULE_LICENSE, MODULE_DESCRIPTION) so I see nothing fundamentally wrong with MODULE_SOFTDEP following the same model. The example provided clearly illustrates how multiple dependencies should be declared. One possible improvement would be to add a comment explicitly stating that this macro can only be used once per module. > At least I would rather support somelike > MODULE_SOFTDEP("pre: ..."); > MODULE_SOFTDEP("post: ..."); Feel free to implement this on your copious spare time if you think there is any actual value in this change. Personally I'm not sure and I just want to get the (driver) bug fixed. Fixing the driver is more simple and easier to backport if needed. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > > Cc: "Enrico Weigelt, metux IT consult" <info@metux.net> > > Cc: Darren Hart <dvhart@infradead.org> > > Cc: Andy Shevchenko <andy@infradead.org> > > --- > > drivers/platform/x86/pcengines-apuv2.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > --- linux-5.1.orig/drivers/platform/x86/pcengines-apuv2.c 2019-05-06 02:42:58.000000000 +0200 > > +++ linux-5.1/drivers/platform/x86/pcengines-apuv2.c 2019-06-29 11:37:48.062005738 +0200 > > @@ -255,6 +255,4 @@ MODULE_DESCRIPTION("PC Engines APUv2/APU > > MODULE_LICENSE("GPL"); > > MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table); > > MODULE_ALIAS("platform:pcengines-apuv2"); > > -MODULE_SOFTDEP("pre: platform:" AMD_FCH_GPIO_DRIVER_NAME); > > -MODULE_SOFTDEP("pre: platform:leds-gpio"); > > -MODULE_SOFTDEP("pre: platform:gpio_keys_polled"); > > +MODULE_SOFTDEP("pre: platform:" AMD_FCH_GPIO_DRIVER_NAME " platform:leds-gpio platform:gpio_keys_polled"); > > > >
On Sat, Jun 29, 2019 at 4:13 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sat, Jun 29, 2019 at 12:41 PM Jean Delvare <jdelvare@suse.de> wrote: > > > > Only one MODULE_SOFTDEP statement is allowed per module. Multiple > > dependencies must be expressed in a single statement. > > Some module init utils even do not support softdep. not related to the patch in question... and module init tools has long been replaced by kmod. > > Nevertheless, the message is somewhat misleading. It's not "only one > allowed" — this is not true, it's "only first will be served". > This is how I read kmod sources. > > And perhaps better to fix them? > At least I would rather support somelike > MODULE_SOFTDEP("pre: ..."); > MODULE_SOFTDEP("post: ..."); it is a bug, because multiple softdep statements like you suggest has never been supported. Either by kmod or module-init-tools. What it actually does is to replace the previous one. depmod will look into the ELF and generate a /lib/modules/$(uname -r)/modules.softdep file. That config file is loaded by kmod as any other config file with the same prio as others in /lib. Implementing what you ask would probably be done with MODULE_SOFTDEP_PRE() / MODULE_SOFTDEP_POST() and let the build system do the right thing. Not supported today and not easily backportable. Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> Lucas De Marchi > > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > > Cc: "Enrico Weigelt, metux IT consult" <info@metux.net> > > Cc: Darren Hart <dvhart@infradead.org> > > Cc: Andy Shevchenko <andy@infradead.org> > > --- > > drivers/platform/x86/pcengines-apuv2.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > --- linux-5.1.orig/drivers/platform/x86/pcengines-apuv2.c 2019-05-06 02:42:58.000000000 +0200 > > +++ linux-5.1/drivers/platform/x86/pcengines-apuv2.c 2019-06-29 11:37:48.062005738 +0200 > > @@ -255,6 +255,4 @@ MODULE_DESCRIPTION("PC Engines APUv2/APU > > MODULE_LICENSE("GPL"); > > MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table); > > MODULE_ALIAS("platform:pcengines-apuv2"); > > -MODULE_SOFTDEP("pre: platform:" AMD_FCH_GPIO_DRIVER_NAME); > > -MODULE_SOFTDEP("pre: platform:leds-gpio"); > > -MODULE_SOFTDEP("pre: platform:gpio_keys_polled"); > > +MODULE_SOFTDEP("pre: platform:" AMD_FCH_GPIO_DRIVER_NAME " platform:leds-gpio platform:gpio_keys_polled"); > > > > > > -- > > Jean Delvare > > SUSE L3 Support > > > > -- > With Best Regards, > Andy Shevchenko
On 29.06.19 11:41, Jean Delvare wrote: > Only one MODULE_SOFTDEP statement is allowed per module. Multiple > dependencies must be expressed in a single statement. Thanks for the report. I'll give it a test on actual target, when I'm back from travel. I recall some strange problems w/ module load order, maybe that's exactly the missing piece. BTW: just curious whether you happen to be located in Nuremberg ? (maybe we could have a coffee some day ;-)). --mtx
On Wed, 3 Jul 2019 13:13:03 +0200, Enrico Weigelt, metux IT consult wrote: > On 29.06.19 11:41, Jean Delvare wrote: > > Only one MODULE_SOFTDEP statement is allowed per module. Multiple > > dependencies must be expressed in a single statement. > > Thanks for the report. I'll give it a test on actual target, when I'm > back from travel. > > I recall some strange problems w/ module load order, maybe that's > exactly the missing piece. Thanks. > BTW: just curious whether you happen to be located in Nuremberg ? > (maybe we could have a coffee some day ;-)). Nope, I work from France. I have not been in Nuremberg for many years now, as most of my team is in Prague now so that's where I meet them.
On Mon, Jul 1, 2019 at 10:58 AM Jean Delvare <jdelvare@suse.de> wrote: > On Sat, 29 Jun 2019 14:13:02 +0300, Andy Shevchenko wrote: > > On Sat, Jun 29, 2019 at 12:41 PM Jean Delvare <jdelvare@suse.de> wrote: > > > > > > Only one MODULE_SOFTDEP statement is allowed per module. Multiple > > > dependencies must be expressed in a single statement. > > > > Some module init utils even do not support softdep. > > And? And either change will not have an effect on those environments. > > Nevertheless, the message is somewhat misleading. It's not "only one > > allowed" — this is not true, it's "only first will be served". > > This is how I read kmod sources. > > What practical difference does it make? The precision in the description of the root cause. Misleading messages may give a wrong picture on what's going on. > > And perhaps better to fix them? > > It's not considered a bug, as it is already possible to have multiple > dependencies listed, you only have to put them in the same statement. > There are several other MODULE_* macros which also can be used only > once per module (MODULE_LICENSE, MODULE_DESCRIPTION) so I see nothing > fundamentally wrong with MODULE_SOFTDEP following the same model. The > example provided clearly illustrates how multiple dependencies should > be declared. One possible improvement would be to add a comment > explicitly stating that this macro can only be used once per module. > > > At least I would rather support somelike > > MODULE_SOFTDEP("pre: ..."); > > MODULE_SOFTDEP("post: ..."); > > Feel free to implement this on your copious spare time if you think > there is any actual value in this change. Personally I'm not sure and I > just want to get the (driver) bug fixed. Fixing the driver is more > simple and easier to backport if needed. Yes, I agree, and your code looks good to me. Thank you for doing this. I would prefer to see a clear commit message.
--- linux-5.1.orig/drivers/platform/x86/pcengines-apuv2.c 2019-05-06 02:42:58.000000000 +0200 +++ linux-5.1/drivers/platform/x86/pcengines-apuv2.c 2019-06-29 11:37:48.062005738 +0200 @@ -255,6 +255,4 @@ MODULE_DESCRIPTION("PC Engines APUv2/APU MODULE_LICENSE("GPL"); MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table); MODULE_ALIAS("platform:pcengines-apuv2"); -MODULE_SOFTDEP("pre: platform:" AMD_FCH_GPIO_DRIVER_NAME); -MODULE_SOFTDEP("pre: platform:leds-gpio"); -MODULE_SOFTDEP("pre: platform:gpio_keys_polled"); +MODULE_SOFTDEP("pre: platform:" AMD_FCH_GPIO_DRIVER_NAME " platform:leds-gpio platform:gpio_keys_polled");
Only one MODULE_SOFTDEP statement is allowed per module. Multiple dependencies must be expressed in a single statement. Signed-off-by: Jean Delvare <jdelvare@suse.de> Cc: "Enrico Weigelt, metux IT consult" <info@metux.net> Cc: Darren Hart <dvhart@infradead.org> Cc: Andy Shevchenko <andy@infradead.org> --- drivers/platform/x86/pcengines-apuv2.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)