Re: [RFC 0/6] Regressions for "imply" behavior change

[Jani Nikula]

On Wed, 08 Apr 2020, Jason Gunthorpe <jgg@xxxxxxxx> wrote:
> On Wed, Apr 08, 2020 at 10:49:48PM +0200, Arnd Bergmann wrote:
>> On Wed, Apr 8, 2020 at 10:38 PM Nicolas Pitre <nico@xxxxxxxxxxx> wrote:
>> > On Wed, 8 Apr 2020, Arnd Bergmann wrote:
>> > > I have created workarounds for the Kconfig files, which now stop using
>> > > imply and do something else in each case. I don't know whether there was
>> > > a bug in the kconfig changes that has led to allowing configurations that
>> > > were not meant to be legal even with the new semantics, or if the Kconfig
>> > > files have simply become incorrect now and the tool works as expected.
>> >
>> > In most cases it is the code that has to be fixed. It typically does:
>> >
>> >         if (IS_ENABLED(CONFIG_FOO))
>> >                 foo_init();
>> >
>> > Where it should rather do:
>> >
>> >         if (IS_REACHABLE(CONFIG_FOO))
>> >                 foo_init();
>> >
>> > A couple of such patches have been produced and queued in their
>> > respective trees already.
>> 
>> I try to use IS_REACHABLE() only as a last resort, as it tends to
>> confuse users when a subsystem is built as a module and already
>> loaded but something relying on that subsystem does not use it.
>> 
>> In the six patches I made, I had to use IS_REACHABLE() once,
>> for the others I tended to use a Kconfig dependency like
>> 
>> 'depends on FOO || FOO=n'
>
> It is unfortunate kconfig doesn't have a language feature for this
> idiom, as the above is confounding without a lot of kconfig knowledge
>
>> I did come up with the IS_REACHABLE() macro originally, but that
>> doesn't mean I think it's a good idea to use it liberally ;-)
>
> It would be nice to have some uniform policy here
>
> I also don't like the IS_REACHABLE solution, it makes this more
> complicated, not less..

Just chiming "me too" here.

IS_REACHABLE() is not a solution, it's a hack to hide a dependency link
problem under the carpet, in a way that is difficult for the user to
debug and figure out.

The user thinks they've enabled a feature, but it doesn't get used
anyway, because a builtin depends on something that is a module and
therefore not reachable. Can someone please give me an example where
that kind of behaviour is desirable?

AFAICT IS_REACHABLE() is becoming more and more common in the kernel,
but arguably it's just making more undesirable configurations
possible. Configurations that should simply be blocked by using suitable
dependencies on the Kconfig level.

For example, you have two graphics drivers, one builtin and another
module. Then you have backlight as a module. Using IS_REACHABLE(),
backlight would work in one driver, but not the other. I'm sure there is
the oddball person who finds this desirable, but the overwhelming
majority would just make the deps such that either you make all of them
modules, or also require backlight to be builtin.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center