x86: apuv2: Fix softdep statement
diff mbox series

Message ID 20190629114136.45e90292@endymion
State Accepted, archived
Delegated to: Andy Shevchenko
Headers show
Series
  • x86: apuv2: Fix softdep statement
Related show

Commit Message

Jean Delvare June 29, 2019, 9:41 a.m. UTC
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(-)

Comments

Andy Shevchenko June 29, 2019, 11:13 a.m. UTC | #1
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
Jean Delvare July 1, 2019, 7:58 a.m. UTC | #2
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");
> >
> >
Lucas De Marchi July 1, 2019, 7:24 p.m. UTC | #3
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
Enrico Weigelt, metux IT consult July 3, 2019, 11:13 a.m. UTC | #4
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
Jean Delvare July 3, 2019, 12:09 p.m. UTC | #5
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.
Andy Shevchenko July 15, 2019, 9:51 a.m. UTC | #6
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.

Patch
diff mbox series

--- 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");