[PATCH] Documentation: kbuild: explain handling optional dependencies

[PATCH] Documentation: kbuild: explain handling optional dependencies

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

This problem frequently comes up in randconfig testing, with
drivers failing to link because of a dependency on an optional
feature.

The Kconfig language for this is very confusing, so try to
document it in "Kconfig hints" section.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 Documentation/kbuild/kconfig-language.rst | 26 +++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
index 858ed5d80defe..89dea587a469a 100644
--- a/Documentation/kbuild/kconfig-language.rst
+++ b/Documentation/kbuild/kconfig-language.rst
@@ -573,6 +573,32 @@ above, leading to:
 	bool "Support for foo hardware"
 	depends on ARCH_FOO_VENDOR || COMPILE_TEST
 
+Optional dependencies
+~~~~~~~~~~~~~~~~~~~~~
+
+Some drivers are able to optionally use a feature from another module
+or build cleanly with that module disabled, but cause a link failure
+when trying to use that loadable module from a built-in driver.
+
+The most common way to express this optional dependency in Kconfig logic
+uses the slighly counterintuitive
+
+  config FOO
+	bool "Support for foo hardware"
+	depends on BAR || !BAR
+
+This means that there is either a dependency on BAR that disallows
+the combination of FOO=y with BAR=m, or BAR is completely disabled.
+For a more formalized approach if there are multiple drivers that have
+the same dependency, a helper symbol can be used, like
+
+  config FOO
+	bool "Support for foo hardware"
+	depends on BAR_OPTIONAL
+
+  config BAR_OPTIONAL
+	def_tristate BAR || !BAR
+
 Kconfig recursive dependency limitations
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.39.2

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Javier Martinez Canillas]

Arnd Bergmann <arnd@kernel.org> writes:

Hello Arnd,

> From: Arnd Bergmann <arnd@arndb.de>
>
> This problem frequently comes up in randconfig testing, with
> drivers failing to link because of a dependency on an optional
> feature.
>
> The Kconfig language for this is very confusing, so try to
> document it in "Kconfig hints" section.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---

Thanks for writing this since as you mention that Kconfig idiom is not
intuitive. The docs looks great to me!

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Sakari Ailus]

On Wed, Sep 13, 2023 at 01:37:52PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> This problem frequently comes up in randconfig testing, with
> drivers failing to link because of a dependency on an optional
> feature.
> 
> The Kconfig language for this is very confusing, so try to
> document it in "Kconfig hints" section.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Nicolas Schier]

[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]

On Wed, Sep 13, 2023 at 01:37:52PM +0200 Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> This problem frequently comes up in randconfig testing, with
> drivers failing to link because of a dependency on an optional
> feature.
> 
> The Kconfig language for this is very confusing, so try to
> document it in "Kconfig hints" section.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---

Hi Arnd,

thanks for documenting this!  Three questions below:

>  Documentation/kbuild/kconfig-language.rst | 26 +++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
> index 858ed5d80defe..89dea587a469a 100644
> --- a/Documentation/kbuild/kconfig-language.rst
> +++ b/Documentation/kbuild/kconfig-language.rst
> @@ -573,6 +573,32 @@ above, leading to:
>  	bool "Support for foo hardware"
>  	depends on ARCH_FOO_VENDOR || COMPILE_TEST
>  
> +Optional dependencies
> +~~~~~~~~~~~~~~~~~~~~~
> +
> +Some drivers are able to optionally use a feature from another module
> +or build cleanly with that module disabled, but cause a link failure
> +when trying to use that loadable module from a built-in driver.
> +
> +The most common way to express this optional dependency in Kconfig logic
> +uses the slighly counterintuitive

slighly -> slightly

For better RST compliance: could you explicitly start the code block e.g. by
appending '::' as in "... counterintuitive::"?

> +
> +  config FOO
> +	bool "Support for foo hardware"
> +	depends on BAR || !BAR

are you sure that this is enough?  While testing, I needed to explicitly use
=y|=n:

    depends on BAR=y || BAR=n

to prevent FOO to be selectable iff BAR=m.

> +
> +This means that there is either a dependency on BAR that disallows
> +the combination of FOO=y with BAR=m, or BAR is completely disabled.

For me, this sentence is hard to parse (but I am not a native speaker); what
about something like this:

This means that FOO can only be enabled, iff BAR is either built-in or
completely disabled.  If BAR is built as a module, FOO cannot be enabled.

Kind regards,
Nicolas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Arnd Bergmann]

On Wed, Sep 13, 2023, at 21:48, Nicolas Schier wrote:
> On Wed, Sep 13, 2023 at 01:37:52PM +0200 Arnd Bergmann wrote:
>
>>  Documentation/kbuild/kconfig-language.rst | 26 +++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>> 
>> diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
>> index 858ed5d80defe..89dea587a469a 100644
>> --- a/Documentation/kbuild/kconfig-language.rst
>> +++ b/Documentation/kbuild/kconfig-language.rst
>> @@ -573,6 +573,32 @@ above, leading to:
>>  	bool "Support for foo hardware"
>>  	depends on ARCH_FOO_VENDOR || COMPILE_TEST
>>  
>> +Optional dependencies
>> +~~~~~~~~~~~~~~~~~~~~~
>> +
>> +Some drivers are able to optionally use a feature from another module
>> +or build cleanly with that module disabled, but cause a link failure
>> +when trying to use that loadable module from a built-in driver.
>> +
>> +The most common way to express this optional dependency in Kconfig logic
>> +uses the slighly counterintuitive
>
> slighly -> slightly

Fixed, thanks

> For better RST compliance: could you explicitly start the code block e.g. by
> appending '::' as in "... counterintuitive::"?

Ok, done.

>> +
>> +  config FOO
>> +	bool "Support for foo hardware"
>> +	depends on BAR || !BAR
>
> are you sure that this is enough?  While testing, I needed to explicitly use
> =y|=n:
>
>     depends on BAR=y || BAR=n
>
> to prevent FOO to be selectable iff BAR=m.

I see my problem, I made a different mistake here. Your version
is correct for a 'bool' symbol as I had here, but the intention
of this was to make it work for tristate symbols, which are the
interesting case. I've fixed it up this way now, hope it now makes
sense to you:

--- a/Documentation/kbuild/kconfig-language.rst
+++ b/Documentation/kbuild/kconfig-language.rst
@@ -581,19 +581,19 @@ or build cleanly with that module disabled, but cause a link failure
 when trying to use that loadable module from a built-in driver.
 
 The most common way to express this optional dependency in Kconfig logic
-uses the slighly counterintuitive
+uses the slightly counterintuitive::
 
   config FOO
-       bool "Support for foo hardware"
+       tristate "Support for foo hardware"
        depends on BAR || !BAR
 
 This means that there is either a dependency on BAR that disallows
 the combination of FOO=y with BAR=m, or BAR is completely disabled.
 For a more formalized approach if there are multiple drivers that have
-the same dependency, a helper symbol can be used, like
+the same dependency, a helper symbol can be used, like::
 
   config FOO
-       bool "Support for foo hardware"
+       tristate "Support for foo hardware"
        depends on BAR_OPTIONAL
 
   config BAR_OPTIONAL

>> +This means that there is either a dependency on BAR that disallows
>> +the combination of FOO=y with BAR=m, or BAR is completely disabled.
>
> For me, this sentence is hard to parse (but I am not a native speaker); what
> about something like this:
>
> This means that FOO can only be enabled, iff BAR is either built-in or
> completely disabled.  If BAR is built as a module, FOO cannot be enabled.

That would describe the version you suggested, but that's a
different issue. Let me know if you still think it needs
clarification after fixing the example.

      Arnd

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Nicolas Schier]

[-- Attachment #1: Type: text/plain, Size: 4544 bytes --]

On Wed, Sep 13, 2023 at 09:55:36PM +0200 Arnd Bergmann wrote:
> On Wed, Sep 13, 2023, at 21:48, Nicolas Schier wrote:
> > On Wed, Sep 13, 2023 at 01:37:52PM +0200 Arnd Bergmann wrote:
> >
> >>  Documentation/kbuild/kconfig-language.rst | 26 +++++++++++++++++++++++
> >>  1 file changed, 26 insertions(+)
> >> 
> >> diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
> >> index 858ed5d80defe..89dea587a469a 100644
> >> --- a/Documentation/kbuild/kconfig-language.rst
> >> +++ b/Documentation/kbuild/kconfig-language.rst
> >> @@ -573,6 +573,32 @@ above, leading to:
> >>  	bool "Support for foo hardware"
> >>  	depends on ARCH_FOO_VENDOR || COMPILE_TEST
> >>  
> >> +Optional dependencies
> >> +~~~~~~~~~~~~~~~~~~~~~
> >> +
> >> +Some drivers are able to optionally use a feature from another module
> >> +or build cleanly with that module disabled, but cause a link failure
> >> +when trying to use that loadable module from a built-in driver.
> >> +
> >> +The most common way to express this optional dependency in Kconfig logic
> >> +uses the slighly counterintuitive
> >
> > slighly -> slightly
> 
> Fixed, thanks
> 
> > For better RST compliance: could you explicitly start the code block e.g. by
> > appending '::' as in "... counterintuitive::"?
> 
> Ok, done.
> 
> >> +
> >> +  config FOO
> >> +	bool "Support for foo hardware"
> >> +	depends on BAR || !BAR
> >
> > are you sure that this is enough?  While testing, I needed to explicitly use
> > =y|=n:
> >
> >     depends on BAR=y || BAR=n
> >
> > to prevent FOO to be selectable iff BAR=m.
> 
> I see my problem, I made a different mistake here. Your version
> is correct for a 'bool' symbol as I had here, but the intention
> of this was to make it work for tristate symbols, which are the
> interesting case. I've fixed it up this way now, hope it now makes
> sense to you:
> 
> --- a/Documentation/kbuild/kconfig-language.rst
> +++ b/Documentation/kbuild/kconfig-language.rst
> @@ -581,19 +581,19 @@ or build cleanly with that module disabled, but cause a link failure
>  when trying to use that loadable module from a built-in driver.
>  
>  The most common way to express this optional dependency in Kconfig logic
> -uses the slighly counterintuitive
> +uses the slightly counterintuitive::
>  
>    config FOO
> -       bool "Support for foo hardware"
> +       tristate "Support for foo hardware"
>         depends on BAR || !BAR

ah, thanks, tristate kconfig symbols are really more interesting.  But I am
still not sure, whether this works as proposed:

With the 'config FOO' above and

  config BAR
  	tristate "Support for bar feature"

kconfig allows me to choose between these:

BAR=y  => FOO={N/m/y}
BAR=m  => FOO={N/m}
BAR=n  => FOO={N/m/y}

But with

  config FOO
  	tristate "Support for foo hardware"
  	depends on !BAR=m

I can choose between:

BAR=y  => FOO={N/m/y}
BAR=m  => FOO is not selectable
BAR=n  => FOO={N/m/y}

(Re-checked with BAR=IPV6 and FOO=WIREGUARD; CONFIG_WIREGUARD as 'depends on
IPV6 || !IPV6' in its kconfig definition, and both are tristate kconfig
symbols.)

Thus, it seems to me, that the intuitive way is the way forward (and several
Kconfigs are out-of-date with regard to 'depends on !X=m'.  Or do I still miss
your point?

Kind regards,
Nicolas



>  This means that there is either a dependency on BAR that disallows
>  the combination of FOO=y with BAR=m, or BAR is completely disabled.
>  For a more formalized approach if there are multiple drivers that have
> -the same dependency, a helper symbol can be used, like
> +the same dependency, a helper symbol can be used, like::
>  
>    config FOO
> -       bool "Support for foo hardware"
> +       tristate "Support for foo hardware"
>         depends on BAR_OPTIONAL
>  
>    config BAR_OPTIONAL
> 
> >> +This means that there is either a dependency on BAR that disallows
> >> +the combination of FOO=y with BAR=m, or BAR is completely disabled.
> >
> > For me, this sentence is hard to parse (but I am not a native speaker); what
> > about something like this:
> >
> > This means that FOO can only be enabled, iff BAR is either built-in or
> > completely disabled.  If BAR is built as a module, FOO cannot be enabled.
> 
> That would describe the version you suggested, but that's a
> different issue. Let me know if you still think it needs
> clarification after fixing the example.
> 
>       Arnd

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Arnd Bergmann]

On Wed, Sep 13, 2023, at 22:34, Nicolas Schier wrote:
> On Wed, Sep 13, 2023 at 09:55:36PM +0200 Arnd Bergmann wrote:

>>    config FOO
>> -       bool "Support for foo hardware"
>> +       tristate "Support for foo hardware"
>>         depends on BAR || !BAR
>
> ah, thanks, tristate kconfig symbols are really more interesting.  But I am
> still not sure, whether this works as proposed:
>
> With the 'config FOO' above and
>
>   config BAR
>   	tristate "Support for bar feature"
>
> kconfig allows me to choose between these:
>
> BAR=y  => FOO={N/m/y}
> BAR=m  => FOO={N/m}
> BAR=n  => FOO={N/m/y}
>
> But with
>
>   config FOO
>   	tristate "Support for foo hardware"
>   	depends on !BAR=m
>
> I can choose between:
>
> BAR=y  => FOO={N/m/y}
> BAR=m  => FOO is not selectable
> BAR=n  => FOO={N/m/y}

That is indeed the point: if BAR=m, we want to be able to pick FOO=m
here, otherwise it is impossible to enabled everything as modules.

Another correct way to express the same thing as the first would
be 

config FOO
      tristate "Support for foo hardware"
      depends on !BAR=m || m

which I find even more confusing than the 'BAR || !BAR'
convention, though we have that in a couple of places.

I just found another variant that I had not seen before:

> (Re-checked with BAR=IPV6 and FOO=WIREGUARD; CONFIG_WIREGUARD as 'depends on
> IPV6 || !IPV6' in its kconfig definition, and both are tristate kconfig
> symbols.)
>
> Thus, it seems to me, that the intuitive way is the way forward (and several
> Kconfigs are out-of-date with regard to 'depends on !X=m'.  Or do I still miss
> your point?

I'm not sure what you mean here, but it appears that one of us
is missing the point ;-)

      Arnd

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Nicolas Schier]

[-- Attachment #1: Type: text/plain, Size: 1313 bytes --]

On Wed 13 Sep 2023 23:16:47 GMT, Arnd Bergmann wrote:
> On Wed, Sep 13, 2023, at 22:34, Nicolas Schier wrote:
> > On Wed, Sep 13, 2023 at 09:55:36PM +0200 Arnd Bergmann wrote:
> 
> >>    config FOO
> >> -       bool "Support for foo hardware"
> >> +       tristate "Support for foo hardware"
> >>         depends on BAR || !BAR
> >
> > ah, thanks, tristate kconfig symbols are really more interesting.  But I am
> > still not sure, whether this works as proposed:
> >
> > With the 'config FOO' above and
> >
> >   config BAR
> >   	tristate "Support for bar feature"
> >
> > kconfig allows me to choose between these:
> >
> > BAR=y  => FOO={N/m/y}
> > BAR=m  => FOO={N/m}
> > BAR=n  => FOO={N/m/y}
> >
> > But with
> >
> >   config FOO
> >   	tristate "Support for foo hardware"
> >   	depends on !BAR=m
> >
> > I can choose between:
> >
> > BAR=y  => FOO={N/m/y}
> > BAR=m  => FOO is not selectable
> > BAR=n  => FOO={N/m/y}
> 
> That is indeed the point: if BAR=m, we want to be able to pick FOO=m
> here, otherwise it is impossible to enabled everything as modules.

oh, I misinterpreted your very first sentence; thanks for clarifying it to me
and sorry for the noise.
With the minor fixes:

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>

Kind regards,
Nicolas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Arnd Bergmann]

On Thu, Sep 14, 2023, at 05:51, Nicolas Schier wrote:
> On Wed 13 Sep 2023 23:16:47 GMT, Arnd Bergmann wrote:
>> On Wed, Sep 13, 2023, at 22:34, Nicolas Schier wrote:
>> >
>> > BAR=y  => FOO={N/m/y}
>> > BAR=m  => FOO is not selectable
>> > BAR=n  => FOO={N/m/y}
>> 
>> That is indeed the point: if BAR=m, we want to be able to pick FOO=m
>> here, otherwise it is impossible to enabled everything as modules.
>
> oh, I misinterpreted your very first sentence; thanks for clarifying it to me
> and sorry for the noise.
> With the minor fixes:
>
> Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>
>

Ok, thanks!

I understand that the text is still confusing, so if anyone has an
idea for how to improve it further, let me know, otherwise
I'll send what I have now with type fixes as v2.

     Arnd

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Javier Martinez Canillas]

"Arnd Bergmann" <arnd@arndb.de> writes:

Hello Nicolas,

> On Wed, Sep 13, 2023, at 22:34, Nicolas Schier wrote:
>> On Wed, Sep 13, 2023 at 09:55:36PM +0200 Arnd Bergmann wrote:
>

[...]

>> I can choose between:
>>
>> BAR=y  => FOO={N/m/y}
>> BAR=m  => FOO is not selectable
>> BAR=n  => FOO={N/m/y}
>
> That is indeed the point: if BAR=m, we want to be able to pick FOO=m
> here, otherwise it is impossible to enabled everything as modules.
>
> Another correct way to express the same thing as the first would
> be 
>
> config FOO
>       tristate "Support for foo hardware"
>       depends on !BAR=m || m
>
> which I find even more confusing than the 'BAR || !BAR'
> convention, though we have that in a couple of places.
>
> I just found another variant that I had not seen before:
>
>> (Re-checked with BAR=IPV6 and FOO=WIREGUARD; CONFIG_WIREGUARD as 'depends on
>> IPV6 || !IPV6' in its kconfig definition, and both are tristate kconfig
>> symbols.)
>>

Which is correct because WIREGUARD can be built with IPV6 disabled, but
if both options are enabled then WIREGUARD can only be built-in if the
IPV6 option is also built-in.

WIREGUARD must be a module if IPV6 is also a module, but can still be a
module if IPV6 is built-in.

In other words, what this idiom express is that the following configs
are possible:

 IPV6=n => WIREGUARD=y
 IPV6=n => WIREGUARD=m
 IPV6=y => WIREGUARD=y
 IPV6=y => WIREGUARD=m
 IPV6=m => WIREGUARD=m

but the following option is not possible:

 IPV6=m => WIREGUARD=y

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Masahiro Yamada]

On Thu, Sep 14, 2023 at 5:34 AM Nicolas Schier <nicolas@fjasle.eu> wrote:
>
> On Wed, Sep 13, 2023 at 09:55:36PM +0200 Arnd Bergmann wrote:
> > On Wed, Sep 13, 2023, at 21:48, Nicolas Schier wrote:
> > > On Wed, Sep 13, 2023 at 01:37:52PM +0200 Arnd Bergmann wrote:
> > >
> > >>  Documentation/kbuild/kconfig-language.rst | 26 +++++++++++++++++++++++
> > >>  1 file changed, 26 insertions(+)
> > >>
> > >> diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
> > >> index 858ed5d80defe..89dea587a469a 100644
> > >> --- a/Documentation/kbuild/kconfig-language.rst
> > >> +++ b/Documentation/kbuild/kconfig-language.rst
> > >> @@ -573,6 +573,32 @@ above, leading to:
> > >>    bool "Support for foo hardware"
> > >>    depends on ARCH_FOO_VENDOR || COMPILE_TEST
> > >>
> > >> +Optional dependencies
> > >> +~~~~~~~~~~~~~~~~~~~~~
> > >> +
> > >> +Some drivers are able to optionally use a feature from another module
> > >> +or build cleanly with that module disabled, but cause a link failure
> > >> +when trying to use that loadable module from a built-in driver.
> > >> +
> > >> +The most common way to express this optional dependency in Kconfig logic
> > >> +uses the slighly counterintuitive
> > >
> > > slighly -> slightly
> >
> > Fixed, thanks
> >
> > > For better RST compliance: could you explicitly start the code block e.g. by
> > > appending '::' as in "... counterintuitive::"?
> >
> > Ok, done.
> >
> > >> +
> > >> +  config FOO
> > >> +  bool "Support for foo hardware"
> > >> +  depends on BAR || !BAR
> > >
> > > are you sure that this is enough?  While testing, I needed to explicitly use
> > > =y|=n:
> > >
> > >     depends on BAR=y || BAR=n
> > >
> > > to prevent FOO to be selectable iff BAR=m.
> >
> > I see my problem, I made a different mistake here. Your version
> > is correct for a 'bool' symbol as I had here, but the intention
> > of this was to make it work for tristate symbols, which are the
> > interesting case. I've fixed it up this way now, hope it now makes
> > sense to you:
> >
> > --- a/Documentation/kbuild/kconfig-language.rst
> > +++ b/Documentation/kbuild/kconfig-language.rst
> > @@ -581,19 +581,19 @@ or build cleanly with that module disabled, but cause a link failure
> >  when trying to use that loadable module from a built-in driver.
> >
> >  The most common way to express this optional dependency in Kconfig logic
> > -uses the slighly counterintuitive
> > +uses the slightly counterintuitive::
> >
> >    config FOO
> > -       bool "Support for foo hardware"
> > +       tristate "Support for foo hardware"
> >         depends on BAR || !BAR
>
> ah, thanks, tristate kconfig symbols are really more interesting.


Both FOO and BAR MUST be tristate
to make this documentation sensible.


If FOO is bool type, "depends on BAR || !BAR"
becomes a no-op.

As you notice, FOO and BAR become independent of
each other.


You may wonder why.

Here, another unclear rule applies:

'depends on m' for a bool option is promoted to
'depends on y'.

https://github.com/torvalds/linux/blob/v6.5/scripts/kconfig/symbol.c#L214





-- 
Best Regards
Masahiro Yamada

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Jani Nikula]

On Wed, 13 Sep 2023, Arnd Bergmann <arnd@kernel.org> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> This problem frequently comes up in randconfig testing, with
> drivers failing to link because of a dependency on an optional
> feature.
>
> The Kconfig language for this is very confusing, so try to
> document it in "Kconfig hints" section.

Thanks for doing this.

>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  Documentation/kbuild/kconfig-language.rst | 26 +++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
> index 858ed5d80defe..89dea587a469a 100644
> --- a/Documentation/kbuild/kconfig-language.rst
> +++ b/Documentation/kbuild/kconfig-language.rst
> @@ -573,6 +573,32 @@ above, leading to:
>  	bool "Support for foo hardware"
>  	depends on ARCH_FOO_VENDOR || COMPILE_TEST
>  
> +Optional dependencies
> +~~~~~~~~~~~~~~~~~~~~~
> +
> +Some drivers are able to optionally use a feature from another module
> +or build cleanly with that module disabled, but cause a link failure
> +when trying to use that loadable module from a built-in driver.
> +
> +The most common way to express this optional dependency in Kconfig logic
> +uses the slighly counterintuitive
> +
> +  config FOO
> +	bool "Support for foo hardware"
> +	depends on BAR || !BAR

	depends on BAR || BAR=n

seems to be an alternative that's about as common:

$ git grep "depends on \([A-Z0-9_]\+\) || \!\1" | wc -l
109
$ git grep "depends on \([A-Z0-9_]\+\) || \1=n" | wc -l
107

Maybe worth mentioning both?


BR,
Jani.


> +
> +This means that there is either a dependency on BAR that disallows
> +the combination of FOO=y with BAR=m, or BAR is completely disabled.
> +For a more formalized approach if there are multiple drivers that have
> +the same dependency, a helper symbol can be used, like
> +
> +  config FOO
> +	bool "Support for foo hardware"
> +	depends on BAR_OPTIONAL
> +
> +  config BAR_OPTIONAL
> +	def_tristate BAR || !BAR
> +
>  Kconfig recursive dependency limitations
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

-- 
Jani Nikula, Intel

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Arnd Bergmann]

On Thu, Sep 14, 2023, at 15:42, Jani Nikula wrote:
> On Wed, 13 Sep 2023, Arnd Bergmann <arnd@kernel.org> wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>>  
>> +Optional dependencies
>> +~~~~~~~~~~~~~~~~~~~~~
>> +
>> +Some drivers are able to optionally use a feature from another module
>> +or build cleanly with that module disabled, but cause a link failure
>> +when trying to use that loadable module from a built-in driver.
>> +
>> +The most common way to express this optional dependency in Kconfig logic
>> +uses the slighly counterintuitive
>> +
>> +  config FOO
>> +	bool "Support for foo hardware"
>> +	depends on BAR || !BAR
>
> 	depends on BAR || BAR=n
>
> seems to be an alternative that's about as common:
>
> $ git grep "depends on \([A-Z0-9_]\+\) || \!\1" | wc -l
> 109
> $ git grep "depends on \([A-Z0-9_]\+\) || \1=n" | wc -l
> 107
>
> Maybe worth mentioning both?

I fear that would add more confusion than it avoids:
"!BAR" is actually different from "BAR=n", but
"BAR || !BAR" is the same as "BAR || BAR=n" here, and
trying to explain this in the documentation would either
make it incorrect or unhelpfully complicated.

      Arnd

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Jani Nikula]

On Thu, 14 Sep 2023, "Arnd Bergmann" <arnd@arndb.de> wrote:
> On Thu, Sep 14, 2023, at 15:42, Jani Nikula wrote:
>> On Wed, 13 Sep 2023, Arnd Bergmann <arnd@kernel.org> wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>  
>>> +Optional dependencies
>>> +~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +Some drivers are able to optionally use a feature from another module
>>> +or build cleanly with that module disabled, but cause a link failure
>>> +when trying to use that loadable module from a built-in driver.
>>> +
>>> +The most common way to express this optional dependency in Kconfig logic
>>> +uses the slighly counterintuitive
>>> +
>>> +  config FOO
>>> +	bool "Support for foo hardware"
>>> +	depends on BAR || !BAR
>>
>> 	depends on BAR || BAR=n
>>
>> seems to be an alternative that's about as common:
>>
>> $ git grep "depends on \([A-Z0-9_]\+\) || \!\1" | wc -l
>> 109
>> $ git grep "depends on \([A-Z0-9_]\+\) || \1=n" | wc -l
>> 107
>>
>> Maybe worth mentioning both?
>
> I fear that would add more confusion than it avoids:
> "!BAR" is actually different from "BAR=n", but

*head explodes*

> "BAR || !BAR" is the same as "BAR || BAR=n" here, and
> trying to explain this in the documentation would either
> make it incorrect or unhelpfully complicated.

Fair enough.


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Masahiro Yamada]

On Thu, Sep 14, 2023 at 11:57 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Sep 14, 2023, at 15:42, Jani Nikula wrote:
> > On Wed, 13 Sep 2023, Arnd Bergmann <arnd@kernel.org> wrote:
> >> From: Arnd Bergmann <arnd@arndb.de>
> >>
> >> +Optional dependencies
> >> +~~~~~~~~~~~~~~~~~~~~~
> >> +
> >> +Some drivers are able to optionally use a feature from another module
> >> +or build cleanly with that module disabled, but cause a link failure
> >> +when trying to use that loadable module from a built-in driver.
> >> +
> >> +The most common way to express this optional dependency in Kconfig logic
> >> +uses the slighly counterintuitive
> >> +
> >> +  config FOO
> >> +    bool "Support for foo hardware"
> >> +    depends on BAR || !BAR
> >
> >       depends on BAR || BAR=n
> >
> > seems to be an alternative that's about as common:
> >
> > $ git grep "depends on \([A-Z0-9_]\+\) || \!\1" | wc -l
> > 109
> > $ git grep "depends on \([A-Z0-9_]\+\) || \1=n" | wc -l
> > 107
> >
> > Maybe worth mentioning both?
>
> I fear that would add more confusion than it avoids:
> "!BAR" is actually different from "BAR=n", but
> "BAR || !BAR" is the same as "BAR || BAR=n" here, and
> trying to explain this in the documentation would either
> make it incorrect or unhelpfully complicated.



The rules are already explained in line 231-278
of Documentation/kbuild/kconfig-language.rst


y, m, n are internally 2, 1, 0.

!A returns (2 - A).
A=B returns 2 if the equation is true, 0 otherwise.
A||B returns max(A,B)


Given those in my mind, this is simple math.

For each case of BAR=y, =m, =n,

BAR               2       1       0
!BAR              0       1       2
BAR=n             0       0       2
BAR||!BAR         2       1       2
BAR||BAR=n        2       1       2
BAR!=m||m         2       1       2


So, the last three are equivalent.
They are equally complicated and confusing, though.


After all, what we are doing is to create this matrix:

          |   WIREGUARD
          |   y     m     n
----------------------------
        y |   O     O     O
IPV6    m |   X     O     O
        n |   O     O     O



It is unclear why WIREGUARD must be entirely disabled
just because of the optional feature being modular.



My preference is to use IS_REACHABLE(CONFIG_IPV6)
instead of IS_ENABLED(CONFIG_IPV6)
under drivers/net/wireguard, then
get rid of "depends on IPV6 || !IPV6)




If you want to make it clearer on the Kconfig level,
perhaps the following is also possible.


config WIREGUARD
       tristate "WireGuard"

config WIREGUARD_IPV6
       def_bool y
       depends on WIREGUARD
       depends on IPV6 >= WIREGUARD

config IPV6
       tristate "IPV6"



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Arnd Bergmann]

On Thu, Sep 14, 2023, at 19:23, Masahiro Yamada wrote:
> On Thu, Sep 14, 2023 at 11:57 PM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thu, Sep 14, 2023, at 15:42, Jani Nikula wrote:
>
> It is unclear why WIREGUARD must be entirely disabled
> just because of the optional feature being modular.

I don't think anyone is asking for that, and the current
"depends on IPV6 || !IPV6" seems fine here, and is consistent
with dozens of other symbols.

> My preference is to use IS_REACHABLE(CONFIG_IPV6)
> instead of IS_ENABLED(CONFIG_IPV6)
> under drivers/net/wireguard, then
> get rid of "depends on IPV6 || !IPV6)

My feeling is that this would be significantly worse from a
usability point of view even if it made it a little easier
for maintainers:

When a user selects both IPV6 and WIREGUARD, they expect
to be able to use them together, and a normal user setting
WIREGUARD=y would have a hard time figuring out why that
leads it becoming IPv4-only.

> If you want to make it clearer on the Kconfig level,
> perhaps the following is also possible.
>
>
> config WIREGUARD
>        tristate "WireGuard"
>
> config WIREGUARD_IPV6
>        def_bool y
>        depends on WIREGUARD
>        depends on IPV6 >= WIREGUARD
>
> config IPV6
>        tristate "IPV6"

That has the same downside, with the added problem
of also confusing kernel developers with the '>='
Kconfig syntax, which IMHO makes no sense unless one
knows way too much about Kconfig internals.

      Arnd

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Jani Nikula]

On Fri, 15 Sep 2023, "Arnd Bergmann" <arnd@arndb.de> wrote:
> On Thu, Sep 14, 2023, at 19:23, Masahiro Yamada wrote:
>> On Thu, Sep 14, 2023 at 11:57 PM Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Thu, Sep 14, 2023, at 15:42, Jani Nikula wrote:
>>
>> It is unclear why WIREGUARD must be entirely disabled
>> just because of the optional feature being modular.
>
> I don't think anyone is asking for that, and the current
> "depends on IPV6 || !IPV6" seems fine here, and is consistent
> with dozens of other symbols.
>
>> My preference is to use IS_REACHABLE(CONFIG_IPV6)
>> instead of IS_ENABLED(CONFIG_IPV6)
>> under drivers/net/wireguard, then
>> get rid of "depends on IPV6 || !IPV6)
>
> My feeling is that this would be significantly worse from a
> usability point of view even if it made it a little easier
> for maintainers:
>
> When a user selects both IPV6 and WIREGUARD, they expect
> to be able to use them together, and a normal user setting
> WIREGUARD=y would have a hard time figuring out why that
> leads it becoming IPv4-only.

I think IS_REACHABLE() is in most cases just plain wrong, and should
only be used as the last resort.

I think the kconfig should express what the dependencies are, and you
should get kconfig or build errors if you get it wrong, and not paper
over them with IS_REACHABLE().

Configuring the kernel is hard enough, and IS_REACHABLE() silences the
issues with no messages to the user at any point. If the user gets it
wrong, it just doesn't work like they expect, they have no clues why,
and they have to peruse the kernel source to figure it out. (Or, more
likely, file a bug and waste the kernel developer/maintainer time to get
the configuration right.)

IS_REACHABLE() considered harmful.


BR,
Jani.


>
>> If you want to make it clearer on the Kconfig level,
>> perhaps the following is also possible.
>>
>>
>> config WIREGUARD
>>        tristate "WireGuard"
>>
>> config WIREGUARD_IPV6
>>        def_bool y
>>        depends on WIREGUARD
>>        depends on IPV6 >= WIREGUARD
>>
>> config IPV6
>>        tristate "IPV6"
>
> That has the same downside, with the added problem
> of also confusing kernel developers with the '>='
> Kconfig syntax, which IMHO makes no sense unless one
> knows way too much about Kconfig internals.
>
>       Arnd

-- 
Jani Nikula, Intel

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Arnd Bergmann]

On Fri, Sep 15, 2023, at 09:34, Jani Nikula wrote:
>
> IS_REACHABLE() considered harmful.

Absolutely agreed, and I'm sorry I introduced it in the
first place in commit 9b174527e7b75 ("[media] Add and use
IS_REACHABLE macro").

At the time, it was only used by drivers/media, which used
to have a lot of open-coded instances of it and a lot of
wrong checks.

Having a formal syntax for it was an improvement for
drivers/media since it was more broken before, but it's
usually a mistake to use it when there is another solution,
and we probably should have tried harder to fix the
dependencies in drivers/media at the time.

      Arnd

Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Randy Dunlap]

On 9/15/23 00:44, Arnd Bergmann wrote:
> On Fri, Sep 15, 2023, at 09:34, Jani Nikula wrote:
>>
>> IS_REACHABLE() considered harmful.
> 

+1 absolutely.

> Absolutely agreed, and I'm sorry I introduced it in the
> first place in commit 9b174527e7b75 ("[media] Add and use
> IS_REACHABLE macro").
> 
> At the time, it was only used by drivers/media, which used
> to have a lot of open-coded instances of it and a lot of
> wrong checks.
> 
> Having a formal syntax for it was an improvement for
> drivers/media since it was more broken before, but it's
> usually a mistake to use it when there is another solution,
> and we probably should have tried harder to fix the
> dependencies in drivers/media at the time.
> 
>       Arnd

-- 
~Randy