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

Message ID 20200408202711.1198966-1-arnd@arndb.de
Headers show
Series
  • Regressions for "imply" behavior change
Related show

Message

Arnd Bergmann April 8, 2020, 8:27 p.m.
Hi everyone,

I've just restarted doing randconfig builds on top of mainline Linux and
found a couple of regressions with missing dependency from the recent
change in the "imply" keyword in Kconfig, presumably these two patches:

3a9dd3ecb207 kconfig: make 'imply' obey the direct dependency
def2fbffe62c kconfig: allow symbols implied by y to become m

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.

Please have a look at the cases I found, and what you think we should
do about them. I assume there are a couple more like these, the six
regressions here are what I found in the first 1000 randconfig builds
on the kernel.

       Arnd

Arnd Bergmann (6):
  thunder: select PTP driver if possible
  net/mlx5e: fix VXLAN dependency
  LiquidIO VF: add dependency for PTP_1588_CLOCK
  drm/bridge/sii8620: fix extcon dependency
  drm/rcar-du: fix selection of CMM driver
  drm/rcar-du: fix lvds dependency

 drivers/gpu/drm/bridge/Kconfig                  | 1 -
 drivers/gpu/drm/bridge/sil-sii8620.c            | 5 +++--
 drivers/gpu/drm/rcar-du/Kconfig                 | 7 ++-----
 drivers/net/ethernet/cavium/Kconfig             | 4 ++--
 drivers/net/ethernet/mellanox/mlx5/core/Kconfig | 2 +-
 5 files changed, 8 insertions(+), 11 deletions(-)


Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Saeed Mahameed <saeedm@mellanox.com>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-rdma@vger.kernel.org


-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Comments

Nicolas Pitre April 8, 2020, 8:38 p.m. | #1
On Wed, 8 Apr 2020, Arnd Bergmann wrote:

> Hi everyone,

> 

> I've just restarted doing randconfig builds on top of mainline Linux and

> found a couple of regressions with missing dependency from the recent

> change in the "imply" keyword in Kconfig, presumably these two patches:

> 

> 3a9dd3ecb207 kconfig: make 'imply' obey the direct dependency

> def2fbffe62c kconfig: allow symbols implied by y to become m

> 

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


Nicolas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Saeed Mahameed April 8, 2020, 8:46 p.m. | #2
On Wed, 2020-04-08 at 16:38 -0400, Nicolas Pitre wrote:
> On Wed, 8 Apr 2020, Arnd Bergmann wrote:

> 

> > Hi everyone,

> > 

> > I've just restarted doing randconfig builds on top of mainline

> > Linux and

> > found a couple of regressions with missing dependency from the

> > recent

> > change in the "imply" keyword in Kconfig, presumably these two

> > patches:

> > 

> > 3a9dd3ecb207 kconfig: make 'imply' obey the direct dependency

> > def2fbffe62c kconfig: allow symbols implied by y to become m

> > 

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

> 

> 


Yes i have a patch in mlx5-net branch converting IS_ENABLED to
IS_REACHABLE in mlx5, i will post it today.

Thanks,
Saeed.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Arnd Bergmann April 8, 2020, 8:49 p.m. | #3
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'

which avoids the case that IS_REACHABLE() works around badly.

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

      Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Nicolas Pitre April 8, 2020, 9:17 p.m. | #4
On Wed, 8 Apr 2020, 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.


Then this is a usage policy issue, not a code correctness issue.

The correctness issue is fixed with IS_REACHABLE(). If you want to 
enforce a usage policy then this goes in Kconfig.

But you still can do both.


Nicolas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jason Gunthorpe April 8, 2020, 10:42 p.m. | #5
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'


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

Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jani Nikula April 9, 2020, 8:41 a.m. | #6
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'

>

> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Saeed Mahameed April 10, 2020, 2:40 a.m. | #7
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.

> 

> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Geert Uytterhoeven April 10, 2020, 7:26 a.m. | #8
Hi Saeed,

On Fri, Apr 10, 2020 at 4:41 AM Saeed Mahameed <saeedm@mellanox.com> wrote:
> On Thu, 2020-04-09 at 11:41 +0300, Jani Nikula wrote:

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


Which may have worked fine for backlight, but not for other symbols
with dependencies that are not always met.

=> Use "select" to enable something unconditionally, but this can only
    be used if the target's dependencies are met.
=> Use "imply" to enable an optional feature conditionally.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jason Gunthorpe April 10, 2020, 5:13 p.m. | #9
On Fri, Apr 10, 2020 at 02:40:42AM +0000, Saeed Mahameed wrote:

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


Does this work if VXLAN = m ?

> 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


Does this trick work when vxlan is a bool not a tristate?

Why not just put the VXLAN || !VXLAN directly on MLX5_CORE?

Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Saeed Mahameed April 10, 2020, 7:04 p.m. | #10
On Fri, 2020-04-10 at 14:13 -0300, Jason Gunthorpe wrote:
> On Fri, Apr 10, 2020 at 02:40:42AM +0000, Saeed Mahameed wrote:

> 

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

> 

> Does this work if VXLAN = m ?


Yes, if VXLAN IS_REACHABLE to MLX5, mlx5_vxlan.o will be
compiled/linked.

> 

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

> 

> Does this trick work when vxlan is a bool not a tristate?

> 

> Why not just put the VXLAN || !VXLAN directly on MLX5_CORE?

> 


so force MLX5_CORE to n if vxlan is not reachable ? why ? mlx5_core can
perfectly live without vxlan .. all we need to know is if VXLAN is
supported and reachable, if so, then we want to also support vxlan in
mlx5 (i.e compile mlx5_vxlan.o) 

and how do we compile mlx5_vxlan.o wihout a single flag 
can i do in Makefile :
mlx5_core-$(VXLAN || !VXLAN) += mlx5_vxlan.o ?? 


> Jason

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jason Gunthorpe April 14, 2020, 1:29 p.m. | #11
On Fri, Apr 10, 2020 at 07:04:27PM +0000, Saeed Mahameed wrote:
> On Fri, 2020-04-10 at 14:13 -0300, Jason Gunthorpe wrote:

> > On Fri, Apr 10, 2020 at 02:40:42AM +0000, Saeed Mahameed wrote:

> > 

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

> > 

> > Does this work if VXLAN = m ?

> 

> Yes, if VXLAN IS_REACHABLE to MLX5, mlx5_vxlan.o will be

> compiled/linked.


So mlx5_core-m does the right thing somehow?

> > 

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

> > 

> > Does this trick work when vxlan is a bool not a tristate?

> > 

> > Why not just put the VXLAN || !VXLAN directly on MLX5_CORE?

> > 

> 

> so force MLX5_CORE to n if vxlan is not reachable ? 


IIRC that isn't what the expression does, if vxlan is 'n' then 
  n || !n == true

The other version of this is (m || VXLAN != m)

Basically all it does is prevent MLX5_CORE=y && VXLAN=m

> and how do we compile mlx5_vxlan.o wihout a single flag 

> can i do in Makefile :

> mlx5_core-$(VXLAN || !VXLAN) += mlx5_vxlan.o ?? 


No, you just use VXLAN directly, it will be m, n or y, but it won't be
m if mlx5_core is y

Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Arnd Bergmann April 14, 2020, 2:27 p.m. | #12
On Tue, Apr 14, 2020 at 3:29 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Fri, Apr 10, 2020 at 07:04:27PM +0000, Saeed Mahameed wrote:

> > On Fri, 2020-04-10 at 14:13 -0300, Jason Gunthorpe wrote:

> > > On Fri, Apr 10, 2020 at 02:40:42AM +0000, Saeed Mahameed wrote:

> > >

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

> > >

> > > Does this work if VXLAN = m ?

> >

> > Yes, if VXLAN IS_REACHABLE to MLX5, mlx5_vxlan.o will be

> > compiled/linked.

>

> So mlx5_core-m does the right thing somehow?


What happens with CONFIG_VXLAN=m is that the above turns into

mlx5_core-y := mlx5_core.o
mlx5_core-m += mlx5_vxlan.o

which in turn leads to mlx5_core.ko *not* containing mlx5_vxlan.o,
and in turn causing that link error against
mlx5_vxlan_create/mlx5_vxlan_destroy, unless the IS_ENABLED()
is changed to IS_REACHABLE().

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

> > >

> > > Does this trick work when vxlan is a bool not a tristate?

> > >

> > > Why not just put the VXLAN || !VXLAN directly on MLX5_CORE?

> > >

> >

> > so force MLX5_CORE to n if vxlan is not reachable ?

>

> IIRC that isn't what the expression does, if vxlan is 'n' then

>   n || !n == true


It forces MLX5_CORE to 'm' or 'n' but not 'y' if VXLAN=m,
but allows any option if VXLAN=y

> The other version of this is (m || VXLAN != m)


Right, that should be the same, but is less common.

I later found that I also needed this one for the same
kind of dependency on PTP:

--- a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
@@ -7,7 +7,7 @@ config MLX5_CORE
        tristate "Mellanox 5th generation network adapters (ConnectX
series) core driver"
        depends on PCI
        select NET_DEVLINK
-       imply PTP_1588_CLOCK
+       depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
        depends on VXLAN || !VXLAN
        imply MLXFW
        imply PCI_HYPERV_INTERFACE
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jason Gunthorpe April 14, 2020, 3:23 p.m. | #13
On Tue, Apr 14, 2020 at 04:27:41PM +0200, Arnd Bergmann wrote:
> On Tue, Apr 14, 2020 at 3:29 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:

> > On Fri, Apr 10, 2020 at 07:04:27PM +0000, Saeed Mahameed wrote:

> > > On Fri, 2020-04-10 at 14:13 -0300, Jason Gunthorpe wrote:

> > > > On Fri, Apr 10, 2020 at 02:40:42AM +0000, Saeed Mahameed wrote:

> > > >

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

> > > >

> > > > Does this work if VXLAN = m ?

> > >

> > > Yes, if VXLAN IS_REACHABLE to MLX5, mlx5_vxlan.o will be

> > > compiled/linked.

> >

> > So mlx5_core-m does the right thing somehow?

> 

> What happens with CONFIG_VXLAN=m is that the above turns into

> 

> mlx5_core-y := mlx5_core.o

> mlx5_core-m += mlx5_vxlan.o

> 

> which in turn leads to mlx5_core.ko *not* containing mlx5_vxlan.o,

> and in turn causing that link error against

> mlx5_vxlan_create/mlx5_vxlan_destroy, unless the IS_ENABLED()

> is changed to IS_REACHABLE().


What about the reverse if mlx5_core is 'm' and VLXAN is 'y'?

 mlx5_core-m := mlx5_core.o
 mlx5_core-y += mlx5_vxlan.o

Magically works out?

> > IIRC that isn't what the expression does, if vxlan is 'n' then

> >   n || !n == true

> 

> It forces MLX5_CORE to 'm' or 'n' but not 'y' if VXLAN=m,

> but allows any option if VXLAN=y


And any option if VXLAN=n ?

Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Arnd Bergmann April 14, 2020, 3:25 p.m. | #14
On Tue, Apr 14, 2020 at 5:23 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>

> On Tue, Apr 14, 2020 at 04:27:41PM +0200, Arnd Bergmann wrote:

> > On Tue, Apr 14, 2020 at 3:29 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:

> > > On Fri, Apr 10, 2020 at 07:04:27PM +0000, Saeed Mahameed wrote:

> > which in turn leads to mlx5_core.ko *not* containing mlx5_vxlan.o,

> > and in turn causing that link error against

> > mlx5_vxlan_create/mlx5_vxlan_destroy, unless the IS_ENABLED()

> > is changed to IS_REACHABLE().

>

> What about the reverse if mlx5_core is 'm' and VLXAN is 'y'?

>

>  mlx5_core-m := mlx5_core.o

>  mlx5_core-y += mlx5_vxlan.o

>

> Magically works out?


Yes, Kbuild takes care of that case.

> > > IIRC that isn't what the expression does, if vxlan is 'n' then

> > >   n || !n == true

> >

> > It forces MLX5_CORE to 'm' or 'n' but not 'y' if VXLAN=m,

> > but allows any option if VXLAN=y

>

> And any option if VXLAN=n ?


Correct.

      Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Saeed Mahameed April 14, 2020, 5:49 p.m. | #15
On Tue, 2020-04-14 at 17:25 +0200, Arnd Bergmann wrote:
> On Tue, Apr 14, 2020 at 5:23 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:

> > On Tue, Apr 14, 2020 at 04:27:41PM +0200, Arnd Bergmann wrote:

> > > On Tue, Apr 14, 2020 at 3:29 PM Jason Gunthorpe <jgg@ziepe.ca>

> > > wrote:

> > > > On Fri, Apr 10, 2020 at 07:04:27PM +0000, Saeed Mahameed wrote:

> > > which in turn leads to mlx5_core.ko *not* containing

> > > mlx5_vxlan.o,

> > > and in turn causing that link error against

> > > mlx5_vxlan_create/mlx5_vxlan_destroy, unless the IS_ENABLED()

> > > is changed to IS_REACHABLE().

> > 

> > What about the reverse if mlx5_core is 'm' and VLXAN is 'y'?

> > 

> >  mlx5_core-m := mlx5_core.o

> >  mlx5_core-y += mlx5_vxlan.o

> > 

> > Magically works out?

> 

> Yes, Kbuild takes care of that case.

> 

> > > > IIRC that isn't what the expression does, if vxlan is 'n' then

> > > >   n || !n == true

> > > 

> > > It forces MLX5_CORE to 'm' or 'n' but not 'y' if VXLAN=m,

> > > but allows any option if VXLAN=y

> > 

> > And any option if VXLAN=n ?

> 

> Correct.

> 


Great !

Then bottom line we will change mlx5/Kconfig: to

depends on VXLAN || !VXLAN

This will force MLX5_CORE to m when necessary to make vxlan reachable
to mlx5_core.  So no need for explicit use of IS_REACHABLE().
in mlx5 there are 4 of these:

        imply PTP_1588_CLOCK
        imply VXLAN
        imply MLXFW
        imply PCI_HYPERV_INTERFACE


I will make a patch.

Thanks,
Saeed.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Arnd Bergmann April 14, 2020, 6:47 p.m. | #16
On Tue, Apr 14, 2020 at 7:49 PM Saeed Mahameed <saeedm@mellanox.com> wrote:
> On Tue, 2020-04-14 at 17:25 +0200, Arnd Bergmann wrote:

> > On Tue, Apr 14, 2020 at 5:23 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:

> > Correct.

> >

>

> Great !

>

> Then bottom line we will change mlx5/Kconfig: to

>

> depends on VXLAN || !VXLAN


Ok

> This will force MLX5_CORE to m when necessary to make vxlan reachable

> to mlx5_core.  So no need for explicit use of IS_REACHABLE().

> in mlx5 there are 4 of these:

>

>         imply PTP_1588_CLOCK

>         imply VXLAN

>         imply MLXFW

>         imply PCI_HYPERV_INTERFACE


As mentioned earlier, we do need to replace the 'imply PTP_1588_CLOCK'
with the same

         depends on PTP_1588_CLOCK || !PTP_1588_CLOCK

So far I have not seen problems for the other two options, so I assume they
are fine for now -- it seems to build just fine without PCI_HYPERV_INTERFACE,
and MLXFW has no other dependencies, meaning that 'imply' is the
same as 'select' here. Using 'select MLXFW' would make it clearer perhaps.

      Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Saeed Mahameed April 16, 2020, 3:25 a.m. | #17
On Tue, 2020-04-14 at 20:47 +0200, Arnd Bergmann wrote:
> On Tue, Apr 14, 2020 at 7:49 PM Saeed Mahameed <saeedm@mellanox.com>

> wrote:

> > On Tue, 2020-04-14 at 17:25 +0200, Arnd Bergmann wrote:

> > > On Tue, Apr 14, 2020 at 5:23 PM Jason Gunthorpe <jgg@ziepe.ca>

> > > wrote:

> > > Correct.

> > > 

> > 

> > Great !

> > 

> > Then bottom line we will change mlx5/Kconfig: to

> > 

> > depends on VXLAN || !VXLAN

> 

> Ok

> 


BTW how about adding a new Kconfig option to hide the details of 
( BAR || !BAR) ? as Jason already explained and suggested, this will
make it easier for the users and developers to understand the actual
meaning behind this tristate weird condition.

e.g have a new keyword:
     reach VXLAN
which will be equivalent to: 
     depends on VXLAN && !VXLAN

> > This will force MLX5_CORE to m when necessary to make vxlan

> > reachable

> > to mlx5_core.  So no need for explicit use of IS_REACHABLE().

> > in mlx5 there are 4 of these:

> > 

> >         imply PTP_1588_CLOCK

> >         imply VXLAN

> >         imply MLXFW

> >         imply PCI_HYPERV_INTERFACE

> 

> As mentioned earlier, we do need to replace the 'imply

> PTP_1588_CLOCK'

> with the same

> 

>          depends on PTP_1588_CLOCK || !PTP_1588_CLOCK

> 

> So far I have not seen problems for the other two options, so I

> assume they

> are fine for now -- it seems to build just fine without

> PCI_HYPERV_INTERFACE,

> and MLXFW has no other dependencies, meaning that 'imply' is the

> same as 'select' here. Using 'select MLXFW' would make it clearer

> perhaps.


No, I would like to avoid select and allow building mlx5 without MLXFW,
MLXFW already has a stub protected with IS_REACHABLE(), this is why we
don't have an issue with it.


> 

>       Arnd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Arnd Bergmann April 16, 2020, 7:20 a.m. | #18
On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <saeedm@mellanox.com> wrote:
>

> On Tue, 2020-04-14 at 20:47 +0200, Arnd Bergmann wrote:

> > On Tue, Apr 14, 2020 at 7:49 PM Saeed Mahameed <saeedm@mellanox.com>

> > wrote:

> > > On Tue, 2020-04-14 at 17:25 +0200, Arnd Bergmann wrote:

> > > > On Tue, Apr 14, 2020 at 5:23 PM Jason Gunthorpe <jgg@ziepe.ca>

> > > > wrote:

> > > > Correct.

> > > >

> > >

> > > Great !

> > >

> > > Then bottom line we will change mlx5/Kconfig: to

> > >

> > > depends on VXLAN || !VXLAN

> >

> > Ok

> >

>

> BTW how about adding a new Kconfig option to hide the details of

> ( BAR || !BAR) ? as Jason already explained and suggested, this will

> make it easier for the users and developers to understand the actual

> meaning behind this tristate weird condition.

>

> e.g have a new keyword:

>      reach VXLAN

> which will be equivalent to:

>      depends on VXLAN && !VXLAN


I'd love to see that, but I'm not sure what keyword is best. For your
suggestion of "reach", that would probably do the job, but I'm not
sure if this ends up being more or less confusing than what we have
today.

> > > This will force MLX5_CORE to m when necessary to make vxlan

> > > reachable

> > > to mlx5_core.  So no need for explicit use of IS_REACHABLE().

> > > in mlx5 there are 4 of these:

> > >

> > >         imply PTP_1588_CLOCK

> > >         imply VXLAN

> > >         imply MLXFW

> > >         imply PCI_HYPERV_INTERFACE

> >

> > As mentioned earlier, we do need to replace the 'imply

> > PTP_1588_CLOCK'

> > with the same

> >

> >          depends on PTP_1588_CLOCK || !PTP_1588_CLOCK

> >

> > So far I have not seen problems for the other two options, so I

> > assume they

> > are fine for now -- it seems to build just fine without

> > PCI_HYPERV_INTERFACE,

> > and MLXFW has no other dependencies, meaning that 'imply' is the

> > same as 'select' here. Using 'select MLXFW' would make it clearer

> > perhaps.

>

> No, I would like to avoid select and allow building mlx5 without MLXFW,

> MLXFW already has a stub protected with IS_REACHABLE(), this is why we

> don't have an issue with it.


So the 'imply MLXFW' should be dropped then?

        Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jani Nikula April 16, 2020, 10:17 a.m. | #19
On Thu, 16 Apr 2020, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <saeedm@mellanox.com> wrote:

>> BTW how about adding a new Kconfig option to hide the details of

>> ( BAR || !BAR) ? as Jason already explained and suggested, this will

>> make it easier for the users and developers to understand the actual

>> meaning behind this tristate weird condition.

>>

>> e.g have a new keyword:

>>      reach VXLAN

>> which will be equivalent to:

>>      depends on VXLAN && !VXLAN

>

> I'd love to see that, but I'm not sure what keyword is best. For your

> suggestion of "reach", that would probably do the job, but I'm not

> sure if this ends up being more or less confusing than what we have

> today.


Ah, perfect bikeshedding topic!

Perhaps "uses"? If the dependency is enabled it gets used as a
dependency.

Of course, this is all just talk until someone(tm) posts a patch
actually making the change. I've looked at the kconfig tool sources
before; not going to make the same mistake again.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Arnd Bergmann April 16, 2020, 12:38 p.m. | #20
On Thu, Apr 16, 2020 at 12:17 PM Jani Nikula
<jani.nikula@linux.intel.com> wrote:
>

> On Thu, 16 Apr 2020, Arnd Bergmann <arnd@arndb.de> wrote:

> > On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <saeedm@mellanox.com> wrote:

> >> BTW how about adding a new Kconfig option to hide the details of

> >> ( BAR || !BAR) ? as Jason already explained and suggested, this will

> >> make it easier for the users and developers to understand the actual

> >> meaning behind this tristate weird condition.

> >>

> >> e.g have a new keyword:

> >>      reach VXLAN

> >> which will be equivalent to:

> >>      depends on VXLAN && !VXLAN

> >

> > I'd love to see that, but I'm not sure what keyword is best. For your

> > suggestion of "reach", that would probably do the job, but I'm not

> > sure if this ends up being more or less confusing than what we have

> > today.

>

> Ah, perfect bikeshedding topic!

>

> Perhaps "uses"? If the dependency is enabled it gets used as a

> dependency.


That seems to be the best naming suggestion so far

> Of course, this is all just talk until someone(tm) posts a patch

> actually making the change. I've looked at the kconfig tool sources

> before; not going to make the same mistake again.


Right. OTOH whoever implements it gets to pick the color of the
bikeshed. ;-)

      Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jason Gunthorpe April 16, 2020, 2:52 p.m. | #21
On Thu, Apr 16, 2020 at 02:38:50PM +0200, Arnd Bergmann wrote:
> On Thu, Apr 16, 2020 at 12:17 PM Jani Nikula

> <jani.nikula@linux.intel.com> wrote:

> >

> > On Thu, 16 Apr 2020, Arnd Bergmann <arnd@arndb.de> wrote:

> > > On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <saeedm@mellanox.com> wrote:

> > >> BTW how about adding a new Kconfig option to hide the details of

> > >> ( BAR || !BAR) ? as Jason already explained and suggested, this will

> > >> make it easier for the users and developers to understand the actual

> > >> meaning behind this tristate weird condition.

> > >>

> > >> e.g have a new keyword:

> > >>      reach VXLAN

> > >> which will be equivalent to:

> > >>      depends on VXLAN && !VXLAN

> > >

> > > I'd love to see that, but I'm not sure what keyword is best. For your

> > > suggestion of "reach", that would probably do the job, but I'm not

> > > sure if this ends up being more or less confusing than what we have

> > > today.

> >

> > Ah, perfect bikeshedding topic!

> >

> > Perhaps "uses"? If the dependency is enabled it gets used as a

> > dependency.

> 

> That seems to be the best naming suggestion so far


Uses also  makes sense to me.

> > Of course, this is all just talk until someone(tm) posts a patch

> > actually making the change. I've looked at the kconfig tool sources

> > before; not going to make the same mistake again.

> 

> Right. OTOH whoever implements it gets to pick the color of the

> bikeshed. ;-)


I hope someone takes it up, especially now that imply, which
apparently used to do this, doesn't any more :)

Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Nicolas Pitre April 16, 2020, 3:12 p.m. | #22
On Thu, 16 Apr 2020, Arnd Bergmann wrote:

> On Thu, Apr 16, 2020 at 12:17 PM Jani Nikula

> <jani.nikula@linux.intel.com> wrote:

> >

> > On Thu, 16 Apr 2020, Arnd Bergmann <arnd@arndb.de> wrote:

> > > On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <saeedm@mellanox.com> wrote:

> > >> BTW how about adding a new Kconfig option to hide the details of

> > >> ( BAR || !BAR) ? as Jason already explained and suggested, this will

> > >> make it easier for the users and developers to understand the actual

> > >> meaning behind this tristate weird condition.

> > >>

> > >> e.g have a new keyword:

> > >>      reach VXLAN

> > >> which will be equivalent to:

> > >>      depends on VXLAN && !VXLAN

> > >

> > > I'd love to see that, but I'm not sure what keyword is best. For your

> > > suggestion of "reach", that would probably do the job, but I'm not

> > > sure if this ends up being more or less confusing than what we have

> > > today.

> >

> > Ah, perfect bikeshedding topic!

> >

> > Perhaps "uses"? If the dependency is enabled it gets used as a

> > dependency.

> 

> That seems to be the best naming suggestion so far


What I don't like about "uses" is that it doesn't convey the conditional 
dependency. It could be mistaken as being synonymous to "select".

What about "depends_if" ? The rationale is that this is actually a 
dependency, but only if the related symbol is set (i.e. not n or empty).

> Right. OTOH whoever implements it gets to pick the color of the

> bikeshed. ;-)


Absolutely. But some brainstorming can't hurt.


Nicolas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Arnd Bergmann April 16, 2020, 3:58 p.m. | #23
On Thu, Apr 16, 2020 at 4:52 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Thu, Apr 16, 2020 at 02:38:50PM +0200, Arnd Bergmann wrote:

> > On Thu, Apr 16, 2020 at 12:17 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:

> > > Of course, this is all just talk until someone(tm) posts a patch

> > > actually making the change. I've looked at the kconfig tool sources

> > > before; not going to make the same mistake again.

> >

> > Right. OTOH whoever implements it gets to pick the color of the

> > bikeshed. ;-)

>

> I hope someone takes it up, especially now that imply, which

> apparently used to do this, doesn't any more :)


The old 'imply' was something completely different, it was more of a
'try to select if you can so we can assume it's there, but give up
if it can only be a module and we need it to be built-in".

        Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jason Gunthorpe April 16, 2020, 6:05 p.m. | #24
On Thu, Apr 16, 2020 at 05:58:31PM +0200, Arnd Bergmann wrote:
> On Thu, Apr 16, 2020 at 4:52 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:

> > On Thu, Apr 16, 2020 at 02:38:50PM +0200, Arnd Bergmann wrote:

> > > On Thu, Apr 16, 2020 at 12:17 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:

> > > > Of course, this is all just talk until someone(tm) posts a patch

> > > > actually making the change. I've looked at the kconfig tool sources

> > > > before; not going to make the same mistake again.

> > >

> > > Right. OTOH whoever implements it gets to pick the color of the

> > > bikeshed. ;-)

> >

> > I hope someone takes it up, especially now that imply, which

> > apparently used to do this, doesn't any more :)

> 

> The old 'imply' was something completely different, it was more of a

> 'try to select if you can so we can assume it's there, but give up

> if it can only be a module and we need it to be built-in".


But it seems to have done this as a side-effect, and drivers were
relying on that, otherwise this series wouldn't exist..

Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jason Gunthorpe April 16, 2020, 6:21 p.m. | #25
On Thu, Apr 16, 2020 at 11:12:56AM -0400, Nicolas Pitre wrote:
> On Thu, 16 Apr 2020, Arnd Bergmann wrote:

> 

> > On Thu, Apr 16, 2020 at 12:17 PM Jani Nikula

> > <jani.nikula@linux.intel.com> wrote:

> > >

> > > On Thu, 16 Apr 2020, Arnd Bergmann <arnd@arndb.de> wrote:

> > > > On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <saeedm@mellanox.com> wrote:

> > > >> BTW how about adding a new Kconfig option to hide the details of

> > > >> ( BAR || !BAR) ? as Jason already explained and suggested, this will

> > > >> make it easier for the users and developers to understand the actual

> > > >> meaning behind this tristate weird condition.

> > > >>

> > > >> e.g have a new keyword:

> > > >>      reach VXLAN

> > > >> which will be equivalent to:

> > > >>      depends on VXLAN && !VXLAN

> > > >

> > > > I'd love to see that, but I'm not sure what keyword is best. For your

> > > > suggestion of "reach", that would probably do the job, but I'm not

> > > > sure if this ends up being more or less confusing than what we have

> > > > today.

> > >

> > > Ah, perfect bikeshedding topic!

> > >

> > > Perhaps "uses"? If the dependency is enabled it gets used as a

> > > dependency.

> > 

> > That seems to be the best naming suggestion so far

> 

> What I don't like about "uses" is that it doesn't convey the conditional 

> dependency. It could be mistaken as being synonymous to "select".

> 

> What about "depends_if" ? The rationale is that this is actually a

> dependency, but only if the related symbol is set (i.e. not n or empty).


I think that stretches the common understanding of 'depends' a bit too
far.. A depends where the target can be N is just too strange.

Somthing incorporating 'optional' seems like a better choice
'optionally uses' seems particularly clear and doesn't overload
existing works like depends or select

Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Saeed Mahameed April 16, 2020, 6:38 p.m. | #26
On Thu, 2020-04-16 at 11:52 -0300, Jason Gunthorpe wrote:
> On Thu, Apr 16, 2020 at 02:38:50PM +0200, Arnd Bergmann wrote:

> > On Thu, Apr 16, 2020 at 12:17 PM Jani Nikula

> > <jani.nikula@linux.intel.com> wrote:

> > > On Thu, 16 Apr 2020, Arnd Bergmann <arnd@arndb.de> wrote:

> > > > On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <

> > > > saeedm@mellanox.com> wrote:

> > > > > BTW how about adding a new Kconfig option to hide the details

> > > > > of

> > > > > ( BAR || !BAR) ? as Jason already explained and suggested,

> > > > > this will

> > > > > make it easier for the users and developers to understand the

> > > > > actual

> > > > > meaning behind this tristate weird condition.

> > > > > 

> > > > > e.g have a new keyword:

> > > > >      reach VXLAN

> > > > > which will be equivalent to:

> > > > >      depends on VXLAN && !VXLAN

> > > > 

> > > > I'd love to see that, but I'm not sure what keyword is best.

> > > > For your

> > > > suggestion of "reach", that would probably do the job, but I'm

> > > > not

> > > > sure if this ends up being more or less confusing than what we

> > > > have

> > > > today.

> > > 

> > > Ah, perfect bikeshedding topic!

> > > 

> > > Perhaps "uses"? If the dependency is enabled it gets used as a

> > > dependency.

> > 

> > That seems to be the best naming suggestion so far

> 

> Uses also  makes sense to me.

> 

> > > Of course, this is all just talk until someone(tm) posts a patch

> > > actually making the change. I've looked at the kconfig tool

> > > sources

> > > before; not going to make the same mistake again.

> > 

> > Right. OTOH whoever implements it gets to pick the color of the

> > bikeshed. ;-)

> 

> I hope someone takes it up, especially now that imply, which

> apparently used to do this, doesn't any more :)

> 


Well, I have a patch since yesterday.. i though You and Arnd didn't
like the idea, so i dropped the whole thing :), but apparently you just
didn't like the name of the new option. 

"uses" seems like a good name .. 

I will post the patch.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Andrzej Hajda April 16, 2020, 7:56 p.m. | #27
On 16.04.2020 20:21, Jason Gunthorpe wrote:
> On Thu, Apr 16, 2020 at 11:12:56AM -0400, Nicolas Pitre wrote:

>> On Thu, 16 Apr 2020, Arnd Bergmann wrote:

>>

>>> On Thu, Apr 16, 2020 at 12:17 PM Jani Nikula

>>> <jani.nikula@linux.intel.com> wrote:

>>>> On Thu, 16 Apr 2020, Arnd Bergmann <arnd@arndb.de> wrote:

>>>>> On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <saeedm@mellanox.com> wrote:

>>>>>> BTW how about adding a new Kconfig option to hide the details of

>>>>>> ( BAR || !BAR) ? as Jason already explained and suggested, this will

>>>>>> make it easier for the users and developers to understand the actual

>>>>>> meaning behind this tristate weird condition.

>>>>>>

>>>>>> e.g have a new keyword:

>>>>>>       reach VXLAN

>>>>>> which will be equivalent to:

>>>>>>       depends on VXLAN && !VXLAN

>>>>> I'd love to see that, but I'm not sure what keyword is best. For your

>>>>> suggestion of "reach", that would probably do the job, but I'm not

>>>>> sure if this ends up being more or less confusing than what we have

>>>>> today.

>>>> Ah, perfect bikeshedding topic!

>>>>

>>>> Perhaps "uses"? If the dependency is enabled it gets used as a

>>>> dependency.

>>> That seems to be the best naming suggestion so far

>> What I don't like about "uses" is that it doesn't convey the conditional

>> dependency. It could be mistaken as being synonymous to "select".

>>

>> What about "depends_if" ? The rationale is that this is actually a

>> dependency, but only if the related symbol is set (i.e. not n or empty).

> I think that stretches the common understanding of 'depends' a bit too

> far.. A depends where the target can be N is just too strange.

>

> Somthing incorporating 'optional' seems like a better choice

> 'optionally uses' seems particularly clear and doesn't overload

> existing works like depends or select



I think the whole misunderstanding with imply is that ppl try to use it 
as weak 'depend on' but it is weak 'select' - ie it enforces value of 
implied symbol in contrast to 'depends' which enforces value of current 
symbol.

So if we want to add new symbol 'weak depend' it would be good to stress 
out that difference.

Moreover name imply is already cryptic, adding another keyword which for 
sure will be cryptic (as there are no natural candidates) will 
complicate things more.

Maybe adding some decorator would be better, like optionally or weak, 
for example:

optionally depends on X

optionally select Y

Even more readable:

depends on X if on

depends on Y if enabled


Regards

Andrzej


>

> Jason

>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel