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 <[email protected]>
Cc: Neil Armstrong <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Jonas Karlman <[email protected]>
Cc: Jernej Skrabec <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Kieran Bingham <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Saeed Mahameed <[email protected]>
Cc: Leon Romanovsky <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
--
2.26.0
The 'imply' statement does not prevent MLX5 to be built-in and fail
when VXLAN=m:
aarch64-linux-ld: drivers/net/ethernet/mellanox/mlx5/core/main.o: in function `mlx5_init_once':
main.c:(.text+0x7cc): undefined reference to `mlx5_vxlan_create'
main.c:(.text+0x958): undefined reference to `mlx5_vxlan_destroy'
Use a normal dependency instead.
Fixes: c5791ab0abec ("net/mlx5e: vxlan.c depends on CONFIG_VXLAN")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/net/ethernet/mellanox/mlx5/core/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
index 312e0a1ad43d..849b0be0ca9a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
@@ -8,7 +8,7 @@ config MLX5_CORE
depends on PCI
select NET_DEVLINK
imply PTP_1588_CLOCK
- imply VXLAN
+ depends on VXLAN || !VXLAN
imply MLXFW
imply PCI_HYPERV_INTERFACE
default n
--
2.26.0
On Wed, Apr 8, 2020 at 10:38 PM Nicolas Pitre <[email protected]> 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
The 'imply' selection means the driver can still be a loadable
module even if the main driver is built-in, leading to a link
error:
aarch64-linux-ld: drivers/net/ethernet/cavium/thunder/nicvf_main.o: in function `nicvf_remove':
nicvf_main.c:(.text+0x25c): undefined reference to `cavium_ptp_put'
aarch64-linux-ld: drivers/net/ethernet/cavium/thunder/nicvf_main.o: in function `nicvf_probe':
nicvf_main.c:(.text+0x3080): undefined reference to `cavium_ptp_get'
Use a 'select' statement instead.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/net/ethernet/cavium/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cavium/Kconfig b/drivers/net/ethernet/cavium/Kconfig
index 6a700d34019e..52806ef20d2d 100644
--- a/drivers/net/ethernet/cavium/Kconfig
+++ b/drivers/net/ethernet/cavium/Kconfig
@@ -27,7 +27,7 @@ config THUNDER_NIC_PF
config THUNDER_NIC_VF
tristate "Thunder Virtual function driver"
- imply CAVIUM_PTP
+ select CAVIUM_PTP if POSIX_TIMERS
depends on 64BIT && PCI
---help---
This driver supports Thunder's NIC virtual function
--
2.26.0
The 'imply' statement does not seem to have an effect, as it's
still possible to turn the CMM code into a loadable module
in a randconfig build, leading to a link error:
arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_enable':
rcar_du_crtc.c:(.text+0xad4): undefined reference to `rcar_lvds_clk_enable'
arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_disable':
rcar_du_crtc.c:(.text+0xd7c): undefined reference to `rcar_lvds_clk_disable'
arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_drv.o: in function `rcar_du_init':
rcar_du_drv.c:(.init.text+0x4): undefined reference to `rcar_du_of_init'
arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_encoder.o: in function `rcar_du_encoder_init':
Remove the 'imply', and instead use a silent symbol that defaults
to the correct setting.
Fixes: e08e934d6c28 ("drm: rcar-du: Add support for CMM")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/gpu/drm/rcar-du/Kconfig | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index 0919f1f159a4..5e35f5934d62 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -4,7 +4,6 @@ config DRM_RCAR_DU
depends on DRM && OF
depends on ARM || ARM64
depends on ARCH_RENESAS || COMPILE_TEST
- imply DRM_RCAR_CMM
imply DRM_RCAR_LVDS
select DRM_KMS_HELPER
select DRM_KMS_CMA_HELPER
@@ -15,9 +14,8 @@ config DRM_RCAR_DU
If M is selected the module will be called rcar-du-drm.
config DRM_RCAR_CMM
- tristate "R-Car DU Color Management Module (CMM) Support"
+ def_tristate DRM_RCAR_DU
depends on DRM && OF
- depends on DRM_RCAR_DU
help
Enable support for R-Car Color Management Module (CMM).
--
2.26.0
arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_enable':
rcar_du_crtc.c:(.text+0xad4): undefined reference to `rcar_lvds_clk_enable'
arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_disable':
rcar_du_crtc.c:(.text+0xd7c): undefined reference to `rcar_lvds_clk_disable'
arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_drv.o: in function `rcar_du_init':
rcar_du_drv.c:(.init.text+0x4): undefined reference to `rcar_du_of_init'
arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_encoder.o: in function `rcar_du_encoder_init':
rcar_du_encoder.c:(.text+0x7a): undefined reference to `rcar_lvds_dual_link'
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/gpu/drm/rcar-du/Kconfig | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index 5e35f5934d62..4bb879f02633 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -4,7 +4,6 @@ config DRM_RCAR_DU
depends on DRM && OF
depends on ARM || ARM64
depends on ARCH_RENESAS || COMPILE_TEST
- imply DRM_RCAR_LVDS
select DRM_KMS_HELPER
select DRM_KMS_CMA_HELPER
select DRM_GEM_CMA_HELPER
@@ -27,7 +26,7 @@ config DRM_RCAR_DW_HDMI
Enable support for R-Car Gen3 internal HDMI encoder.
config DRM_RCAR_LVDS
- tristate "R-Car DU LVDS Encoder Support"
+ def_tristate DRM_RCAR_DU
depends on DRM && DRM_BRIDGE && OF
select DRM_PANEL
select OF_FLATTREE
--
2.26.0
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, 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, 8 Apr 2020, Arnd Bergmann wrote:
> On Wed, Apr 8, 2020 at 10:38 PM Nicolas Pitre <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
On Thu, 2020-04-09 at 11:41 +0300, Jani Nikula wrote:
> On Wed, 08 Apr 2020, Jason Gunthorpe <[email protected]> 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 <[email protected]>
> > > 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 <[email protected]> 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 -- [email protected]
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
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 04:27:41PM +0200, Arnd Bergmann wrote:
> On Tue, Apr 14, 2020 at 3:29 PM Jason Gunthorpe <[email protected]> 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 10:17 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Arnd,
>
> Thank you for the patch.
>
> On Wed, Apr 08, 2020 at 10:27:10PM +0200, Arnd Bergmann wrote:
> > The 'imply' statement does not seem to have an effect, as it's
> > still possible to turn the CMM code into a loadable module
> > in a randconfig build, leading to a link error:
> >
> > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_enable':
> > rcar_du_crtc.c:(.text+0xad4): undefined reference to `rcar_lvds_clk_enable'
> > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_disable':
> > rcar_du_crtc.c:(.text+0xd7c): undefined reference to `rcar_lvds_clk_disable'
> > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_drv.o: in function `rcar_du_init':
> > rcar_du_drv.c:(.init.text+0x4): undefined reference to `rcar_du_of_init'
> > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_encoder.o: in function `rcar_du_encoder_init':
> >
> > Remove the 'imply', and instead use a silent symbol that defaults
> > to the correct setting.
>
> This will result in the CMM always being selected when DU is, increasing
> the kernel size even for devices that don't need it. I believe we need a
> better construct in Kconfig to fix this.
I had expected this to have the same meaning that we had before the
Kconfig change: whenever the dependencies are available, turn it on,
otherwise leave it disabled.
Can you describe what behavior you actually want instead?
> > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > @@ -4,7 +4,6 @@ config DRM_RCAR_DU
> > depends on DRM && OF
> > depends on ARM || ARM64
> > depends on ARCH_RENESAS || COMPILE_TEST
> > - imply DRM_RCAR_CMM
> > imply DRM_RCAR_LVDS
> > select DRM_KMS_HELPER
> > select DRM_KMS_CMA_HELPER
> > @@ -15,9 +14,8 @@ config DRM_RCAR_DU
> > If M is selected the module will be called rcar-du-drm.
> >
> > config DRM_RCAR_CMM
> > - tristate "R-Car DU Color Management Module (CMM) Support"
> > + def_tristate DRM_RCAR_DU
> > depends on DRM && OF
> > - depends on DRM_RCAR_DU
> > help
It would be easy enough to make this a visible 'bool' symbol and
build it into the rcu-du-drm.ko module itself. Would that help you?
Arnd
Hi Arnd,
On Tue, Apr 14, 2020 at 10:38:27PM +0200, Arnd Bergmann wrote:
> On Tue, Apr 14, 2020 at 10:17 PM Laurent Pinchart wrote:
> > On Wed, Apr 08, 2020 at 10:27:10PM +0200, Arnd Bergmann wrote:
> > > The 'imply' statement does not seem to have an effect, as it's
> > > still possible to turn the CMM code into a loadable module
> > > in a randconfig build, leading to a link error:
> > >
> > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_enable':
> > > rcar_du_crtc.c:(.text+0xad4): undefined reference to `rcar_lvds_clk_enable'
> > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_disable':
> > > rcar_du_crtc.c:(.text+0xd7c): undefined reference to `rcar_lvds_clk_disable'
> > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_drv.o: in function `rcar_du_init':
> > > rcar_du_drv.c:(.init.text+0x4): undefined reference to `rcar_du_of_init'
> > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_encoder.o: in function `rcar_du_encoder_init':
> > >
> > > Remove the 'imply', and instead use a silent symbol that defaults
> > > to the correct setting.
> >
> > This will result in the CMM always being selected when DU is, increasing
> > the kernel size even for devices that don't need it. I believe we need a
> > better construct in Kconfig to fix this.
>
> I had expected this to have the same meaning that we had before the
> Kconfig change: whenever the dependencies are available, turn it on,
> otherwise leave it disabled.
>
> Can you describe what behavior you actually want instead?
Doesn't "imply" mean it gets selected by default but can be manually
disabled ?
> > > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > > @@ -4,7 +4,6 @@ config DRM_RCAR_DU
> > > depends on DRM && OF
> > > depends on ARM || ARM64
> > > depends on ARCH_RENESAS || COMPILE_TEST
> > > - imply DRM_RCAR_CMM
> > > imply DRM_RCAR_LVDS
> > > select DRM_KMS_HELPER
> > > select DRM_KMS_CMA_HELPER
> > > @@ -15,9 +14,8 @@ config DRM_RCAR_DU
> > > If M is selected the module will be called rcar-du-drm.
> > >
> > > config DRM_RCAR_CMM
> > > - tristate "R-Car DU Color Management Module (CMM) Support"
> > > + def_tristate DRM_RCAR_DU
> > > depends on DRM && OF
> > > - depends on DRM_RCAR_DU
> > > help
>
> It would be easy enough to make this a visible 'bool' symbol and
> build it into the rcu-du-drm.ko module itself. Would that help you?
That could indeed simplify a few things. I wonder if it could introduce
a few small issues though (but likely nothing we can't fix). The two
that come to mind are the fact that the module would have two
MODULE_DESCRIPTION and MODULE_LICENSE entries (I have no idea if that
could cause breakages), and that it could make module unloading more
difficult as the CMM being used by the DU would increase the refcount on
the module. I think the latter could be worked around by manually
unbinding the DU device through sysfs before unloading the module (and I
can't say for sure that unloading the DU module is not broken today
*innocent and naive look* :-)).
--
Regards,
Laurent Pinchart
On Tue, Apr 14, 2020 at 3:29 PM Jason Gunthorpe <[email protected]> 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 5:23 PM Jason Gunthorpe <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > 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 <[email protected]> wrote:
> On Tue, 2020-04-14 at 17:25 +0200, Arnd Bergmann wrote:
> > On Tue, Apr 14, 2020 at 5:23 PM Jason Gunthorpe <[email protected]> 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
Hi Arnd,
Thank you for the patch.
On Wed, Apr 08, 2020 at 10:27:10PM +0200, Arnd Bergmann wrote:
> The 'imply' statement does not seem to have an effect, as it's
> still possible to turn the CMM code into a loadable module
> in a randconfig build, leading to a link error:
>
> arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_enable':
> rcar_du_crtc.c:(.text+0xad4): undefined reference to `rcar_lvds_clk_enable'
> arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_disable':
> rcar_du_crtc.c:(.text+0xd7c): undefined reference to `rcar_lvds_clk_disable'
> arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_drv.o: in function `rcar_du_init':
> rcar_du_drv.c:(.init.text+0x4): undefined reference to `rcar_du_of_init'
> arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_encoder.o: in function `rcar_du_encoder_init':
>
> Remove the 'imply', and instead use a silent symbol that defaults
> to the correct setting.
This will result in the CMM always being selected when DU is, increasing
the kernel size even for devices that don't need it. I believe we need a
better construct in Kconfig to fix this.
> Fixes: e08e934d6c28 ("drm: rcar-du: Add support for CMM")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/gpu/drm/rcar-du/Kconfig | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> index 0919f1f159a4..5e35f5934d62 100644
> --- a/drivers/gpu/drm/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> @@ -4,7 +4,6 @@ config DRM_RCAR_DU
> depends on DRM && OF
> depends on ARM || ARM64
> depends on ARCH_RENESAS || COMPILE_TEST
> - imply DRM_RCAR_CMM
> imply DRM_RCAR_LVDS
> select DRM_KMS_HELPER
> select DRM_KMS_CMA_HELPER
> @@ -15,9 +14,8 @@ config DRM_RCAR_DU
> If M is selected the module will be called rcar-du-drm.
>
> config DRM_RCAR_CMM
> - tristate "R-Car DU Color Management Module (CMM) Support"
> + def_tristate DRM_RCAR_DU
> depends on DRM && OF
> - depends on DRM_RCAR_DU
> help
> Enable support for R-Car Color Management Module (CMM).
>
--
Regards,
Laurent Pinchart
On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Arnd,
>
> On Tue, Apr 14, 2020 at 10:38:27PM +0200, Arnd Bergmann wrote:
> > On Tue, Apr 14, 2020 at 10:17 PM Laurent Pinchart wrote:
> > > On Wed, Apr 08, 2020 at 10:27:10PM +0200, Arnd Bergmann wrote:
> > > > The 'imply' statement does not seem to have an effect, as it's
> > > > still possible to turn the CMM code into a loadable module
> > > > in a randconfig build, leading to a link error:
> > > >
> > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_enable':
> > > > rcar_du_crtc.c:(.text+0xad4): undefined reference to `rcar_lvds_clk_enable'
> > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_disable':
> > > > rcar_du_crtc.c:(.text+0xd7c): undefined reference to `rcar_lvds_clk_disable'
> > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_drv.o: in function `rcar_du_init':
> > > > rcar_du_drv.c:(.init.text+0x4): undefined reference to `rcar_du_of_init'
> > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_encoder.o: in function `rcar_du_encoder_init':
> > > >
> > > > Remove the 'imply', and instead use a silent symbol that defaults
> > > > to the correct setting.
> > >
> > > This will result in the CMM always being selected when DU is, increasing
> > > the kernel size even for devices that don't need it. I believe we need a
> > > better construct in Kconfig to fix this.
> >
> > I had expected this to have the same meaning that we had before the
> > Kconfig change: whenever the dependencies are available, turn it on,
> > otherwise leave it disabled.
> >
> > Can you describe what behavior you actually want instead?
>
> Doesn't "imply" mean it gets selected by default but can be manually
> disabled ?
That may be what it means now (I still don't understand how it's defined
as of v5.7-rc1), but traditionally it was more like a 'select if all
dependencies
are met'.
> > > > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > > > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > > > @@ -4,7 +4,6 @@ config DRM_RCAR_DU
> > > > depends on DRM && OF
> > > > depends on ARM || ARM64
> > > > depends on ARCH_RENESAS || COMPILE_TEST
> > > > - imply DRM_RCAR_CMM
> > > > imply DRM_RCAR_LVDS
> > > > select DRM_KMS_HELPER
> > > > select DRM_KMS_CMA_HELPER
> > > > @@ -15,9 +14,8 @@ config DRM_RCAR_DU
> > > > If M is selected the module will be called rcar-du-drm.
> > > >
> > > > config DRM_RCAR_CMM
> > > > - tristate "R-Car DU Color Management Module (CMM) Support"
> > > > + def_tristate DRM_RCAR_DU
> > > > depends on DRM && OF
> > > > - depends on DRM_RCAR_DU
> > > > help
> >
> > It would be easy enough to make this a visible 'bool' symbol and
> > build it into the rcu-du-drm.ko module itself. Would that help you?
>
> That could indeed simplify a few things. I wonder if it could introduce
> a few small issues though (but likely nothing we can't fix). The two
> that come to mind are the fact that the module would have two
> MODULE_DESCRIPTION and MODULE_LICENSE entries (I have no idea if that
> could cause breakages), and that it could make module unloading more
> difficult as the CMM being used by the DU would increase the refcount on
> the module. I think the latter could be worked around by manually
> unbinding the DU device through sysfs before unloading the module (and I
> can't say for sure that unloading the DU module is not broken today
> *innocent and naive look* :-)).
In that case, a Makefile trick could also work, doing
ifdef CONFIG_DRM_RCAR_CMM
obj-$(CONFIG_DRM_RCAR_DU) += rcar-cmm.o
endif
Thereby making the cmm module have the same state (y or m) as
the du module whenever the option is enabled.
Arnd
Hi Arnd,
On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <[email protected]> wrote:
> On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart
> <[email protected]> wrote:
> > On Tue, Apr 14, 2020 at 10:38:27PM +0200, Arnd Bergmann wrote:
> > > On Tue, Apr 14, 2020 at 10:17 PM Laurent Pinchart wrote:
> > > > On Wed, Apr 08, 2020 at 10:27:10PM +0200, Arnd Bergmann wrote:
> > > > > The 'imply' statement does not seem to have an effect, as it's
> > > > > still possible to turn the CMM code into a loadable module
> > > > > in a randconfig build, leading to a link error:
> > > > >
> > > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_enable':
> > > > > rcar_du_crtc.c:(.text+0xad4): undefined reference to `rcar_lvds_clk_enable'
> > > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_crtc.o: in function `rcar_du_crtc_atomic_disable':
> > > > > rcar_du_crtc.c:(.text+0xd7c): undefined reference to `rcar_lvds_clk_disable'
> > > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_drv.o: in function `rcar_du_init':
> > > > > rcar_du_drv.c:(.init.text+0x4): undefined reference to `rcar_du_of_init'
> > > > > arm-linux-gnueabi-ld: drivers/gpu/drm/rcar-du/rcar_du_encoder.o: in function `rcar_du_encoder_init':
> > > > >
> > > > > Remove the 'imply', and instead use a silent symbol that defaults
> > > > > to the correct setting.
> > > >
> > > > This will result in the CMM always being selected when DU is, increasing
> > > > the kernel size even for devices that don't need it. I believe we need a
> > > > better construct in Kconfig to fix this.
> > >
> > > I had expected this to have the same meaning that we had before the
> > > Kconfig change: whenever the dependencies are available, turn it on,
> > > otherwise leave it disabled.
> > >
> > > Can you describe what behavior you actually want instead?
> >
> > Doesn't "imply" mean it gets selected by default but can be manually
> > disabled ?
>
> That may be what it means now (I still don't understand how it's defined
> as of v5.7-rc1), but traditionally it was more like a 'select if all
> dependencies are met'.
That's still what it is supposed to mean right now ;-)
Except that now it should correctly handle the modular case, too.
> > > > > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > > > > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > > > > @@ -4,7 +4,6 @@ config DRM_RCAR_DU
> > > > > depends on DRM && OF
> > > > > depends on ARM || ARM64
> > > > > depends on ARCH_RENESAS || COMPILE_TEST
> > > > > - imply DRM_RCAR_CMM
> > > > > imply DRM_RCAR_LVDS
> > > > > select DRM_KMS_HELPER
> > > > > select DRM_KMS_CMA_HELPER
> > > > > @@ -15,9 +14,8 @@ config DRM_RCAR_DU
> > > > > If M is selected the module will be called rcar-du-drm.
> > > > >
> > > > > config DRM_RCAR_CMM
> > > > > - tristate "R-Car DU Color Management Module (CMM) Support"
> > > > > + def_tristate DRM_RCAR_DU
> > > > > depends on DRM && OF
> > > > > - depends on DRM_RCAR_DU
> > > > > help
> > >
> > > It would be easy enough to make this a visible 'bool' symbol and
> > > build it into the rcu-du-drm.ko module itself. Would that help you?
> >
> > That could indeed simplify a few things. I wonder if it could introduce
> > a few small issues though (but likely nothing we can't fix). The two
> > that come to mind are the fact that the module would have two
> > MODULE_DESCRIPTION and MODULE_LICENSE entries (I have no idea if that
> > could cause breakages), and that it could make module unloading more
> > difficult as the CMM being used by the DU would increase the refcount on
> > the module. I think the latter could be worked around by manually
> > unbinding the DU device through sysfs before unloading the module (and I
> > can't say for sure that unloading the DU module is not broken today
> > *innocent and naive look* :-)).
>
> In that case, a Makefile trick could also work, doing
>
> ifdef CONFIG_DRM_RCAR_CMM
> obj-$(CONFIG_DRM_RCAR_DU) += rcar-cmm.o
> endif
>
> Thereby making the cmm module have the same state (y or m) as
> the du module whenever the option is enabled.
What about dropping the "imply DRM_RCAR_CMM", but defaulting to
enable CMM if DU is enabled?
config DRM_RCAR_CMM
tristate "R-Car DU Color Management Module (CMM) Support"
depends on DRM_RCAR_DU && OF
default DRM_RCAR_DU
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <[email protected]> wrote:
> On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <[email protected]> wrote:
> > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <[email protected]> wrote:
> > > Doesn't "imply" mean it gets selected by default but can be manually
> > > disabled ?
> >
> > That may be what it means now (I still don't understand how it's defined
> > as of v5.7-rc1), but traditionally it was more like a 'select if all
> > dependencies are met'.
>
> That's still what it is supposed to mean right now ;-)
> Except that now it should correctly handle the modular case, too.
Then there is a bug. If I run 'make menuconfig' now on a mainline kernel
and enable CONFIG_DRM_RCAR_DU, I can set
DRM_RCAR_CMM and DRM_RCAR_LVDS to 'y', 'n' or 'm' regardless
of whether CONFIG_DRM_RCAR_DU is 'm' or 'y'. The 'implies'
statement seems to be ignored entirely, except as reverse 'default'
setting.
> >
> > In that case, a Makefile trick could also work, doing
> >
> > ifdef CONFIG_DRM_RCAR_CMM
> > obj-$(CONFIG_DRM_RCAR_DU) += rcar-cmm.o
> > endif
> >
> > Thereby making the cmm module have the same state (y or m) as
> > the du module whenever the option is enabled.
>
> What about dropping the "imply DRM_RCAR_CMM", but defaulting to
> enable CMM if DU is enabled?
>
> config DRM_RCAR_CMM
> tristate "R-Car DU Color Management Module (CMM) Support"
> depends on DRM_RCAR_DU && OF
> default DRM_RCAR_DU
That doesn't work because it allows DRM_RCAR_DU=y with
DRM_RCAR_CMM=m, which causes a link failure.
Arnd
On Wed, Apr 15, 2020 at 5:18 PM Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <[email protected]> wrote:
> > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <[email protected]> wrote:
> > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <[email protected]> wrote:
> > > > Doesn't "imply" mean it gets selected by default but can be manually
> > > > disabled ?
> > >
> > > That may be what it means now (I still don't understand how it's defined
> > > as of v5.7-rc1), but traditionally it was more like a 'select if all
> > > dependencies are met'.
> >
> > That's still what it is supposed to mean right now ;-)
> > Except that now it should correctly handle the modular case, too.
>
> Then there is a bug. If I run 'make menuconfig' now on a mainline kernel
> and enable CONFIG_DRM_RCAR_DU, I can set
> DRM_RCAR_CMM and DRM_RCAR_LVDS to 'y', 'n' or 'm' regardless
> of whether CONFIG_DRM_RCAR_DU is 'm' or 'y'. The 'implies'
> statement seems to be ignored entirely, except as reverse 'default'
> setting.
Here is another version that should do what we want and is only
half-ugly. I can send that as a proper patch if it passes my testing
and nobody hates it too much.
Arnd
diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index 0919f1f159a4..d2fcec807dfa 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -4,8 +4,6 @@ config DRM_RCAR_DU
depends on DRM && OF
depends on ARM || ARM64
depends on ARCH_RENESAS || COMPILE_TEST
- imply DRM_RCAR_CMM
- imply DRM_RCAR_LVDS
select DRM_KMS_HELPER
select DRM_KMS_CMA_HELPER
select DRM_GEM_CMA_HELPER
@@ -14,13 +12,17 @@ config DRM_RCAR_DU
Choose this option if you have an R-Car chipset.
If M is selected the module will be called rcar-du-drm.
-config DRM_RCAR_CMM
- tristate "R-Car DU Color Management Module (CMM) Support"
- depends on DRM && OF
+config DRM_RCAR_USE_CMM
+ bool "R-Car DU Color Management Module (CMM) Support"
depends on DRM_RCAR_DU
+ default DRM_RCAR_DU
help
Enable support for R-Car Color Management Module (CMM).
+config DRM_RCAR_CMM
+ def_tristate DRM_RCAR_DU
+ depends on DRM_RCAR_USE_CMM
+
config DRM_RCAR_DW_HDMI
tristate "R-Car DU Gen3 HDMI Encoder Support"
depends on DRM && OF
@@ -28,15 +30,20 @@ config DRM_RCAR_DW_HDMI
help
Enable support for R-Car Gen3 internal HDMI encoder.
-config DRM_RCAR_LVDS
- tristate "R-Car DU LVDS Encoder Support"
- depends on DRM && DRM_BRIDGE && OF
+config DRM_RCAR_USE_LVDS
+ bool "R-Car DU LVDS Encoder Support"
+ depends on DRM_BRIDGE && OF
+ default DRM_RCAR_DU
select DRM_PANEL
select OF_FLATTREE
select OF_OVERLAY
help
Enable support for the R-Car Display Unit embedded LVDS encoders.
+config DRM_RCAR_LVDS
+ def_tristate DRM_RCAR_DU
+ depends on DRM_RCAR_USE_LVDS
+
config DRM_RCAR_VSP
bool "R-Car DU VSP Compositor Support" if ARM
default y if ARM64
Hi Arnd,
On Wed, Apr 15, 2020 at 09:07:14PM +0200, Arnd Bergmann wrote:
> On Wed, Apr 15, 2020 at 5:18 PM Arnd Bergmann <[email protected]> wrote:
> > On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <[email protected]> wrote:
> > > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <[email protected]> wrote:
> > > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <[email protected]> wrote:
> > > > > Doesn't "imply" mean it gets selected by default but can be manually
> > > > > disabled ?
> > > >
> > > > That may be what it means now (I still don't understand how it's defined
> > > > as of v5.7-rc1), but traditionally it was more like a 'select if all
> > > > dependencies are met'.
> > >
> > > That's still what it is supposed to mean right now ;-)
> > > Except that now it should correctly handle the modular case, too.
> >
> > Then there is a bug. If I run 'make menuconfig' now on a mainline kernel
> > and enable CONFIG_DRM_RCAR_DU, I can set
> > DRM_RCAR_CMM and DRM_RCAR_LVDS to 'y', 'n' or 'm' regardless
> > of whether CONFIG_DRM_RCAR_DU is 'm' or 'y'. The 'implies'
> > statement seems to be ignored entirely, except as reverse 'default'
> > setting.
>
> Here is another version that should do what we want and is only
> half-ugly. I can send that as a proper patch if it passes my testing
> and nobody hates it too much.
This may be a stupid question, but doesn't this really call for fixing
Kconfig ? This seems to be such a common pattern that requiring
constructs similar to the ones below will be a never-ending chase of
offenders.
> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> index 0919f1f159a4..d2fcec807dfa 100644
> --- a/drivers/gpu/drm/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> @@ -4,8 +4,6 @@ config DRM_RCAR_DU
> depends on DRM && OF
> depends on ARM || ARM64
> depends on ARCH_RENESAS || COMPILE_TEST
> - imply DRM_RCAR_CMM
> - imply DRM_RCAR_LVDS
> select DRM_KMS_HELPER
> select DRM_KMS_CMA_HELPER
> select DRM_GEM_CMA_HELPER
> @@ -14,13 +12,17 @@ config DRM_RCAR_DU
> Choose this option if you have an R-Car chipset.
> If M is selected the module will be called rcar-du-drm.
>
> -config DRM_RCAR_CMM
> - tristate "R-Car DU Color Management Module (CMM) Support"
> - depends on DRM && OF
> +config DRM_RCAR_USE_CMM
> + bool "R-Car DU Color Management Module (CMM) Support"
> depends on DRM_RCAR_DU
> + default DRM_RCAR_DU
> help
> Enable support for R-Car Color Management Module (CMM).
>
> +config DRM_RCAR_CMM
> + def_tristate DRM_RCAR_DU
> + depends on DRM_RCAR_USE_CMM
> +
> config DRM_RCAR_DW_HDMI
> tristate "R-Car DU Gen3 HDMI Encoder Support"
> depends on DRM && OF
> @@ -28,15 +30,20 @@ config DRM_RCAR_DW_HDMI
> help
> Enable support for R-Car Gen3 internal HDMI encoder.
>
> -config DRM_RCAR_LVDS
> - tristate "R-Car DU LVDS Encoder Support"
> - depends on DRM && DRM_BRIDGE && OF
> +config DRM_RCAR_USE_LVDS
> + bool "R-Car DU LVDS Encoder Support"
> + depends on DRM_BRIDGE && OF
> + default DRM_RCAR_DU
> select DRM_PANEL
> select OF_FLATTREE
> select OF_OVERLAY
> help
> Enable support for the R-Car Display Unit embedded LVDS encoders.
>
> +config DRM_RCAR_LVDS
> + def_tristate DRM_RCAR_DU
> + depends on DRM_RCAR_USE_LVDS
> +
> config DRM_RCAR_VSP
> bool "R-Car DU VSP Compositor Support" if ARM
> default y if ARM64
--
Regards,
Laurent Pinchart
On Wed, Apr 15, 2020 at 11:12 PM Laurent Pinchart
<[email protected]> wrote:
> On Wed, Apr 15, 2020 at 09:07:14PM +0200, Arnd Bergmann wrote:
> > On Wed, Apr 15, 2020 at 5:18 PM Arnd Bergmann <[email protected]> wrote:
> > > On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <[email protected]> wrote:
> > > > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <[email protected]> wrote:
> > > > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <[email protected]> wrote:
> > > > > > Doesn't "imply" mean it gets selected by default but can be manually
> > > > > > disabled ?
> > > > >
> > > > > That may be what it means now (I still don't understand how it's defined
> > > > > as of v5.7-rc1), but traditionally it was more like a 'select if all
> > > > > dependencies are met'.
> > > >
> > > > That's still what it is supposed to mean right now ;-)
> > > > Except that now it should correctly handle the modular case, too.
> > >
> > > Then there is a bug. If I run 'make menuconfig' now on a mainline kernel
> > > and enable CONFIG_DRM_RCAR_DU, I can set
> > > DRM_RCAR_CMM and DRM_RCAR_LVDS to 'y', 'n' or 'm' regardless
> > > of whether CONFIG_DRM_RCAR_DU is 'm' or 'y'. The 'implies'
> > > statement seems to be ignored entirely, except as reverse 'default'
> > > setting.
> >
> > Here is another version that should do what we want and is only
> > half-ugly. I can send that as a proper patch if it passes my testing
> > and nobody hates it too much.
>
> This may be a stupid question, but doesn't this really call for fixing
> Kconfig ? This seems to be such a common pattern that requiring
> constructs similar to the ones below will be a never-ending chase of
> offenders.
Maybe, I suppose the hardest part here would be to come up with
an appropriate name for the keyword ;-)
Any suggestions?
This specific issue is fairly rare though, in most cases the dependencies
are in the right order so a Kconfig symbol 'depends on' a second one
when the corresponding loadable module uses symbols from that second
module. The problem here is that the two are mixed up.
The much more common problem is the one where one needs to
wrong
config FOO
depends on BAR || !BAR
To ensure the dependency is either met or BAR is disabled, but
not FOO=y with BAR=m. If you have any suggestions for a keyword
for that thing, we can clean up hundreds of such instances.
Arnd
On Tue, 2020-04-14 at 20:47 +0200, Arnd Bergmann wrote:
> On Tue, Apr 14, 2020 at 7:49 PM Saeed Mahameed <[email protected]>
> wrote:
> > On Tue, 2020-04-14 at 17:25 +0200, Arnd Bergmann wrote:
> > > On Tue, Apr 14, 2020 at 5:23 PM Jason Gunthorpe <[email protected]>
> > > 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 Wed, Apr 15, 2020 at 11:22 PM Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Apr 15, 2020 at 11:12 PM Laurent Pinchart
> <[email protected]> wrote:
> > On Wed, Apr 15, 2020 at 09:07:14PM +0200, Arnd Bergmann wrote:
> > > On Wed, Apr 15, 2020 at 5:18 PM Arnd Bergmann <[email protected]> wrote:
> > > > On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <[email protected]> wrote:
> > > > > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <[email protected]> wrote:
> > > > > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <[email protected]> wrote:
> > > > > > > Doesn't "imply" mean it gets selected by default but can be manually
> > > > > > > disabled ?
> > > > > >
> > > > > > That may be what it means now (I still don't understand how it's defined
> > > > > > as of v5.7-rc1), but traditionally it was more like a 'select if all
> > > > > > dependencies are met'.
> > > > >
> > > > > That's still what it is supposed to mean right now ;-)
> > > > > Except that now it should correctly handle the modular case, too.
> > > >
> > > > Then there is a bug. If I run 'make menuconfig' now on a mainline kernel
> > > > and enable CONFIG_DRM_RCAR_DU, I can set
> > > > DRM_RCAR_CMM and DRM_RCAR_LVDS to 'y', 'n' or 'm' regardless
> > > > of whether CONFIG_DRM_RCAR_DU is 'm' or 'y'. The 'implies'
> > > > statement seems to be ignored entirely, except as reverse 'default'
> > > > setting.
> > >
> > > Here is another version that should do what we want and is only
> > > half-ugly. I can send that as a proper patch if it passes my testing
> > > and nobody hates it too much.
> >
> > This may be a stupid question, but doesn't this really call for fixing
> > Kconfig ? This seems to be such a common pattern that requiring
> > constructs similar to the ones below will be a never-ending chase of
> > offenders.
>
> Maybe, I suppose the hardest part here would be to come up with
> an appropriate name for the keyword ;-)
>
> Any suggestions?
>
> This specific issue is fairly rare though, in most cases the dependencies
> are in the right order so a Kconfig symbol 'depends on' a second one
> when the corresponding loadable module uses symbols from that second
> module. The problem here is that the two are mixed up.
>
> The much more common problem is the one where one needs to
> wrong
>
> config FOO
> depends on BAR || !BAR
>
> To ensure the dependency is either met or BAR is disabled, but
> not FOO=y with BAR=m. If you have any suggestions for a keyword
> for that thing, we can clean up hundreds of such instances.
Some ideas:
config FOO
can use BAR
maybe BAR
optional BAR
We should probably double-check that this is only ever used for when
both FOO and BAR are tri-state, since without that it doesn't make
much sense.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <[email protected]> wrote:
>
> On Tue, 2020-04-14 at 20:47 +0200, Arnd Bergmann wrote:
> > On Tue, Apr 14, 2020 at 7:49 PM Saeed Mahameed <[email protected]>
> > wrote:
> > > On Tue, 2020-04-14 at 17:25 +0200, Arnd Bergmann wrote:
> > > > On Tue, Apr 14, 2020 at 5:23 PM Jason Gunthorpe <[email protected]>
> > > > 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, Apr 16, 2020 at 12:17 PM Jani Nikula
<[email protected]> wrote:
>
> On Thu, 16 Apr 2020, Arnd Bergmann <[email protected]> wrote:
> > On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <[email protected]> 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
> <[email protected]> wrote:
> >
> > On Thu, 16 Apr 2020, Arnd Bergmann <[email protected]> wrote:
> > > On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <[email protected]> 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
> <[email protected]> wrote:
> >
> > On Thu, 16 Apr 2020, Arnd Bergmann <[email protected]> wrote:
> > > On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <[email protected]> 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 <[email protected]> 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 <[email protected]> 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, 16 Apr 2020, Arnd Bergmann <[email protected]> wrote:
> On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <[email protected]> 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
On Thu, Apr 16, 2020 at 08:51:14AM +0200, Daniel Vetter wrote:
> On Wed, Apr 15, 2020 at 11:22 PM Arnd Bergmann wrote:
> > On Wed, Apr 15, 2020 at 11:12 PM Laurent Pinchart wrote:
> > > On Wed, Apr 15, 2020 at 09:07:14PM +0200, Arnd Bergmann wrote:
> > > > On Wed, Apr 15, 2020 at 5:18 PM Arnd Bergmann <[email protected]> wrote:
> > > > > On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <[email protected]> wrote:
> > > > > > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <[email protected]> wrote:
> > > > > > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <[email protected]> wrote:
> > > > > > > > Doesn't "imply" mean it gets selected by default but can be manually
> > > > > > > > disabled ?
> > > > > > >
> > > > > > > That may be what it means now (I still don't understand how it's defined
> > > > > > > as of v5.7-rc1), but traditionally it was more like a 'select if all
> > > > > > > dependencies are met'.
> > > > > >
> > > > > > That's still what it is supposed to mean right now ;-)
> > > > > > Except that now it should correctly handle the modular case, too.
> > > > >
> > > > > Then there is a bug. If I run 'make menuconfig' now on a mainline kernel
> > > > > and enable CONFIG_DRM_RCAR_DU, I can set
> > > > > DRM_RCAR_CMM and DRM_RCAR_LVDS to 'y', 'n' or 'm' regardless
> > > > > of whether CONFIG_DRM_RCAR_DU is 'm' or 'y'. The 'implies'
> > > > > statement seems to be ignored entirely, except as reverse 'default'
> > > > > setting.
> > > >
> > > > Here is another version that should do what we want and is only
> > > > half-ugly. I can send that as a proper patch if it passes my testing
> > > > and nobody hates it too much.
> > >
> > > This may be a stupid question, but doesn't this really call for fixing
> > > Kconfig ? This seems to be such a common pattern that requiring
> > > constructs similar to the ones below will be a never-ending chase of
> > > offenders.
> >
> > Maybe, I suppose the hardest part here would be to come up with
> > an appropriate name for the keyword ;-)
> >
> > Any suggestions?
Would it make sense to fix the imply semantics ? Or are they use cases
for the current behaviour of imply ? "recommend" could be another
keyword. I think we should try to limit the number of keywords though,
as it would otherwise become quite messy.
> > This specific issue is fairly rare though, in most cases the dependencies
> > are in the right order so a Kconfig symbol 'depends on' a second one
> > when the corresponding loadable module uses symbols from that second
> > module. The problem here is that the two are mixed up.
> >
> > The much more common problem is the one where one needs to
> > wrong
> >
> > config FOO
> > depends on BAR || !BAR
> >
> > To ensure the dependency is either met or BAR is disabled, but
> > not FOO=y with BAR=m. If you have any suggestions for a keyword
> > for that thing, we can clean up hundreds of such instances.
>
> Some ideas:
>
> config FOO
> can use BAR
> maybe BAR
> optional BAR
Another idea,
depends optionally on BAR
> We should probably double-check that this is only ever used for when
> both FOO and BAR are tri-state, since without that it doesn't make
> much sense.
--
Regards,
Laurent Pinchart
On Thu, Apr 16, 2020 at 05:58:31PM +0200, Arnd Bergmann wrote:
> On Thu, Apr 16, 2020 at 4:52 PM Jason Gunthorpe <[email protected]> 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 <[email protected]> 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, 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
> > <[email protected]> wrote:
> > > On Thu, 16 Apr 2020, Arnd Bergmann <[email protected]> wrote:
> > > > On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <
> > > > [email protected]> 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 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
> > <[email protected]> wrote:
> > >
> > > On Thu, 16 Apr 2020, Arnd Bergmann <[email protected]> wrote:
> > > > On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <[email protected]> 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 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
>>> <[email protected]> wrote:
>>>> On Thu, 16 Apr 2020, Arnd Bergmann <[email protected]> wrote:
>>>>> On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <[email protected]> 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
>