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

[Saeed Mahameed <saeedm@mellanox.com>]

On Thu, 2020-04-09 at 11:41 +0300, Jani Nikula wrote:
> On Wed, 08 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> 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@fluxnic.net>
> > > 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'
> > 

This assumes that the module using FOO has its own flag representing
FOO which is not always the case.

for example in mlx5 we use VXLAN config flag directly to compile VXLAN
related files:

mlx5/core/Makefile:

obj-$(CONFIG_MLX5_CORE) += mlx5_core.o

mlx5_core-y := mlx5_core.o
mlx5_core-$(VXLAN) += mlx5_vxlan.o

and in mlx5_main.o we do:
 
if (IS_ENABLED(VXLAN))
       mlx5_vxlan_init()

after the change in imply semantics:
our options are:

1) use IS_REACHABLE(VXLAN) instead of IS_ENABLED(VXLAN)

2) have MLX5_VXLAN in mlx5 Kconfig and use IS_ENABLED(MLX5_VXLAN) 
config MLX5_VXLAN
	depends on VXLAN || !VXLAN
	bool

So i understand that every one agree to use solution #2 ?

> > 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.
> 

the previous imply semantics handled this by forcing backlight to be
built-in, which worked nicely.

> 
> BR,
> Jani.
> 
>