| Message ID | 20200408202711.1198966-1-arnd@arndb.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | Regressions for "imply" behavior change | expand |
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
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.
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
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
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
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.
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. > >
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
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
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
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
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
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
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
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.
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
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
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
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.
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
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
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
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
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
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
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.
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 >
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