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

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

[Arnd Bergmann]

Hi everyone,

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

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

I have created workarounds for the Kconfig files, which now stop using
imply and do something else in each case. I don't know whether there was
a bug in the kconfig changes that has led to allowing configurations that
were not meant to be legal even with the new semantics, or if the Kconfig
files have simply become incorrect now and the tool works as expected.

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

       Arnd

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

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


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


-- 
2.26.0

[RFC 1/6] thunder: select PTP driver if possible

[Arnd Bergmann]

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 <arnd@arndb.de>
---
 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

[RFC 2/6] net/mlx5e: fix VXLAN dependency

[Arnd Bergmann]

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 <arnd@arndb.de>
---
 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

[RFC 3/6] LiquidIO VF: add dependency for PTP_1588_CLOCK

[Arnd Bergmann]

The 'imply' keyword no longer does what it was originally
introduced for:

lio_ethtool.c:(.text+0xba8): undefined reference to `ptp_clock_index'

Use a dependency instead.

Fixes: cd7aeb1f9706 ("LiquidIO VF: s/select/imply/ for PTP_1588_CLOCK")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 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 52806ef20d2d..e483c3001dc6 100644
--- a/drivers/net/ethernet/cavium/Kconfig
+++ b/drivers/net/ethernet/cavium/Kconfig
@@ -66,7 +66,7 @@ config LIQUIDIO
 	tristate "Cavium LiquidIO support"
 	depends on 64BIT && PCI
 	depends on PCI
-	imply PTP_1588_CLOCK
+	depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
 	select FW_LOADER
 	select LIBCRC32C
 	select NET_DEVLINK
-- 
2.26.0

[RFC 4/6] drm/bridge/sii8620: fix extcon dependency

[Arnd Bergmann]

Using 'imply' does not work here, it still cause the same build
failure:

arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_remove':
sil-sii8620.c:(.text+0x1b8): undefined reference to `extcon_unregister_notifier'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_probe':
sil-sii8620.c:(.text+0x27e8): undefined reference to `extcon_find_edev_by_node'
arm-linux-gnueabi-ld: sil-sii8620.c:(.text+0x2870): undefined reference to `extcon_register_notifier'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_extcon_work':
sil-sii8620.c:(.text+0x2908): undefined reference to `extcon_get_state'

I tried the usual 'depends on EXTCON || !EXTCON' logic, but that caused
a circular Kconfig dependency. Using IS_REACHABLE() is ugly but works.

Fixes: 7a109673899b ("drm/bridge/sii8620: add Kconfig dependency on extcon")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/bridge/Kconfig       | 1 -
 drivers/gpu/drm/bridge/sil-sii8620.c | 5 +++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index aaed2347ace9..78e5ba06acff 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -90,7 +90,6 @@ config DRM_SIL_SII8620
 	tristate "Silicon Image SII8620 HDMI/MHL bridge"
 	depends on OF
 	select DRM_KMS_HELPER
-	imply EXTCON
 	depends on RC_CORE || !RC_CORE
 	help
 	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 92acd336aa89..94b6c38e6855 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2330,7 +2330,8 @@ static int sii8620_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
-	ret = sii8620_extcon_init(ctx);
+	if (IS_REACHABLE(CONFIG_EXTCON))
+		ret = sii8620_extcon_init(ctx);
 	if (ret < 0) {
 		dev_err(ctx->dev, "failed to initialize EXTCON\n");
 		return ret;
@@ -2352,7 +2353,7 @@ static int sii8620_remove(struct i2c_client *client)
 {
 	struct sii8620 *ctx = i2c_get_clientdata(client);
 
-	if (ctx->extcon) {
+	if (IS_REACHABLE(CONFIG_EXTCON) && ctx->extcon) {
 		extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL,
 					   &ctx->extcon_nb);
 		flush_work(&ctx->extcon_wq);
-- 
2.26.0

[RFC 5/6] drm/rcar-du: fix selection of CMM driver

[Arnd Bergmann]

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 <arnd@arndb.de>
---
 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

[RFC 6/6] drm/rcar-du: fix lvds dependency

[Arnd Bergmann]

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 <arnd@arndb.de>
---
 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

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

[Nicolas Pitre]

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

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

[Saeed Mahameed]

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.

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

[Arnd Bergmann]

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

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

[Nicolas Pitre]

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

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

[Jason Gunthorpe]

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

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

[Jani Nikula]

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

Just chiming "me too" here.

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

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

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

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


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Saeed Mahameed]

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

Re: [RFC 4/6] drm/bridge/sii8620: fix extcon dependency

[Andrzej Hajda]

On 08.04.2020 22:27, Arnd Bergmann wrote:
> Using 'imply' does not work here, it still cause the same build
> failure:
>
> arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_remove':
> sil-sii8620.c:(.text+0x1b8): undefined reference to `extcon_unregister_notifier'
> arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_probe':
> sil-sii8620.c:(.text+0x27e8): undefined reference to `extcon_find_edev_by_node'
> arm-linux-gnueabi-ld: sil-sii8620.c:(.text+0x2870): undefined reference to `extcon_register_notifier'
> arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_extcon_work':
> sil-sii8620.c:(.text+0x2908): undefined reference to `extcon_get_state'
>
> I tried the usual 'depends on EXTCON || !EXTCON' logic, but that caused
> a circular Kconfig dependency. Using IS_REACHABLE() is ugly but works.

'depends on EXTCON || !EXTCON' seems to be proper solution, maybe would be better to try to solve circular dependencies issue.

Regards
Andrzej

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

[Geert Uytterhoeven]

Hi Saeed,

On Fri, Apr 10, 2020 at 4:41 AM Saeed Mahameed <saeedm@mellanox.com> wrote:
> On Thu, 2020-04-09 at 11:41 +0300, Jani Nikula wrote:
> > For example, you have two graphics drivers, one builtin and another
> > module. Then you have backlight as a module. Using IS_REACHABLE(),
> > backlight would work in one driver, but not the other. I'm sure there
> > is
> > the oddball person who finds this desirable, but the overwhelming
> > majority would just make the deps such that either you make all of
> > them
> > modules, or also require backlight to be builtin.
>
> the previous imply semantics handled this by forcing backlight to be
> built-in, which worked nicely.

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

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

Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Jason Gunthorpe]

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

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

[Saeed Mahameed]

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

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

[Jason Gunthorpe]

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

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

[Arnd Bergmann]

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

Re: [RFC 4/6] drm/bridge/sii8620: fix extcon dependency

[Arnd Bergmann]

On Fri, Apr 10, 2020 at 8:56 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
>
> On 08.04.2020 22:27, Arnd Bergmann wrote:
> > Using 'imply' does not work here, it still cause the same build
> > failure:
> >
> > arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_remove':
> > sil-sii8620.c:(.text+0x1b8): undefined reference to `extcon_unregister_notifier'
> > arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_probe':
> > sil-sii8620.c:(.text+0x27e8): undefined reference to `extcon_find_edev_by_node'
> > arm-linux-gnueabi-ld: sil-sii8620.c:(.text+0x2870): undefined reference to `extcon_register_notifier'
> > arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_extcon_work':
> > sil-sii8620.c:(.text+0x2908): undefined reference to `extcon_get_state'
> >
> > I tried the usual 'depends on EXTCON || !EXTCON' logic, but that caused
> > a circular Kconfig dependency. Using IS_REACHABLE() is ugly but works.
>
> 'depends on EXTCON || !EXTCON' seems to be proper solution, maybe would be better to try to solve circular dependencies issue.

I agree that would be nice, but I failed to come to a proper solution
here. FWIW, there
is one circular dependency that I managed to avoid by changing all
drivers that select FB_DDC
to depend on I2C rather than selecting it:

drivers/i2c/Kconfig:8:error: recursive dependency detected!
drivers/i2c/Kconfig:8: symbol I2C is selected by FB_DDC
drivers/video/fbdev/Kconfig:63: symbol FB_DDC depends on FB
drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER
drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_SIL_SII8620
drivers/gpu/drm/bridge/Kconfig:89: symbol DRM_SIL_SII8620 depends on EXTCON
drivers/extcon/Kconfig:2: symbol EXTCON is selected by CHARGER_MANAGER
drivers/power/supply/Kconfig:482: symbol CHARGER_MANAGER depends on POWER_SUPPLY
drivers/power/supply/Kconfig:2: symbol POWER_SUPPLY is selected by
HID_BATTERY_STRENGTH
drivers/hid/Kconfig:29: symbol HID_BATTERY_STRENGTH depends on HID
drivers/hid/Kconfig:8: symbol HID is selected by I2C_HID
drivers/hid/i2c-hid/Kconfig:5: symbol I2C_HID depends on I2C

After that, Kconfig crashes with a segfault:

drivers/video/fbdev/Kconfig:12:error: recursive dependency detected!
drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER
drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_SIL_SII8620
drivers/gpu/drm/bridge/Kconfig:89: symbol DRM_SIL_SII8620 depends on EXTCON
drivers/extcon/Kconfig:2: symbol EXTCON is selected by CHARGER_MANAGER
drivers/power/supply/Kconfig:482: symbol CHARGER_MANAGER depends on POWER_SUPPLY
drivers/power/supply/Kconfig:2: symbol POWER_SUPPLY is selected by HID_ASUS
drivers/hid/Kconfig:150: symbol HID_ASUS depends on LEDS_CLASS
drivers/leds/Kconfig:17: symbol LEDS_CLASS depends on NEW_LEDS
drivers/leds/Kconfig:9: symbol NEW_LEDS is selected by SENSORS_APPLESMC
drivers/hwmon/Kconfig:327: symbol SENSORS_APPLESMC depends on HWMON
drivers/hwmon/Kconfig:6: symbol HWMON is selected by EEEPC_LAPTOP
drivers/platform/x86/Kconfig:260: symbol EEEPC_LAPTOP depends on ACPI_VIDEO
make[3]: *** [/git/arm-soc/scripts/kconfig/Makefile:71: randconfig]
Segmentation fault (core dumped)

After changing EEEPC_LAPTOP and THINKPAD_ACPI to 'depends on HWMON' instead of
'select HWMON', I get this one:

drivers/video/fbdev/Kconfig:12:error: recursive dependency detected!
drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER
drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_SIL_SII8620
drivers/gpu/drm/bridge/Kconfig:89: symbol DRM_SIL_SII8620 depends on EXTCON
drivers/extcon/Kconfig:2: symbol EXTCON is selected by CHARGER_MANAGER
drivers/power/supply/Kconfig:482: symbol CHARGER_MANAGER depends on POWER_SUPPLY
drivers/power/supply/Kconfig:2: symbol POWER_SUPPLY is selected by HID_ASUS
drivers/hid/Kconfig:150: symbol HID_ASUS depends on LEDS_CLASS
drivers/leds/Kconfig:17: symbol LEDS_CLASS depends on NEW_LEDS
drivers/leds/Kconfig:9: symbol NEW_LEDS is selected by BACKLIGHT_ADP8860
drivers/video/backlight/Kconfig:316: symbol BACKLIGHT_ADP8860 depends
on BACKLIGHT_CLASS_DEVICE
drivers/video/backlight/Kconfig:143: symbol BACKLIGHT_CLASS_DEVICE is
selected by FB_BACKLIGHT
drivers/video/fbdev/Kconfig:187: symbol FB_BACKLIGHT depends on FB

Changing all drivers that select 'FB_BACKLIGHT' or 'BACKLIGHT_CLASS_DEVICE' to
'depends on BACKLIGHT_CLASS_DEVICE' gets it to build.

The steps each seem reasonable, in particular since they mostly clean
up the legacy
fbdev drivers to what they should have done anyway, but it is quite
invasive in the end.
Any other ideas?

       Arnd

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

[Jason Gunthorpe]

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

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

[Arnd Bergmann]

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

Re: [RFC 4/6] drm/bridge/sii8620: fix extcon dependency

[Daniel Vetter]

On Tue, Apr 14, 2020 at 5:05 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Apr 10, 2020 at 8:56 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >
> >
> > On 08.04.2020 22:27, Arnd Bergmann wrote:
> > > Using 'imply' does not work here, it still cause the same build
> > > failure:
> > >
> > > arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_remove':
> > > sil-sii8620.c:(.text+0x1b8): undefined reference to `extcon_unregister_notifier'
> > > arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_probe':
> > > sil-sii8620.c:(.text+0x27e8): undefined reference to `extcon_find_edev_by_node'
> > > arm-linux-gnueabi-ld: sil-sii8620.c:(.text+0x2870): undefined reference to `extcon_register_notifier'
> > > arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_extcon_work':
> > > sil-sii8620.c:(.text+0x2908): undefined reference to `extcon_get_state'
> > >
> > > I tried the usual 'depends on EXTCON || !EXTCON' logic, but that caused
> > > a circular Kconfig dependency. Using IS_REACHABLE() is ugly but works.
> >
> > 'depends on EXTCON || !EXTCON' seems to be proper solution, maybe would be better to try to solve circular dependencies issue.
>
> I agree that would be nice, but I failed to come to a proper solution
> here. FWIW, there
> is one circular dependency that I managed to avoid by changing all
> drivers that select FB_DDC
> to depend on I2C rather than selecting it:
>
> drivers/i2c/Kconfig:8:error: recursive dependency detected!
> drivers/i2c/Kconfig:8: symbol I2C is selected by FB_DDC
> drivers/video/fbdev/Kconfig:63: symbol FB_DDC depends on FB
> drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER
> drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
> drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_SIL_SII8620
> drivers/gpu/drm/bridge/Kconfig:89: symbol DRM_SIL_SII8620 depends on EXTCON
> drivers/extcon/Kconfig:2: symbol EXTCON is selected by CHARGER_MANAGER
> drivers/power/supply/Kconfig:482: symbol CHARGER_MANAGER depends on POWER_SUPPLY
> drivers/power/supply/Kconfig:2: symbol POWER_SUPPLY is selected by
> HID_BATTERY_STRENGTH
> drivers/hid/Kconfig:29: symbol HID_BATTERY_STRENGTH depends on HID
> drivers/hid/Kconfig:8: symbol HID is selected by I2C_HID
> drivers/hid/i2c-hid/Kconfig:5: symbol I2C_HID depends on I2C
>
> After that, Kconfig crashes with a segfault:
>
> drivers/video/fbdev/Kconfig:12:error: recursive dependency detected!
> drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER
> drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
> drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_SIL_SII8620
> drivers/gpu/drm/bridge/Kconfig:89: symbol DRM_SIL_SII8620 depends on EXTCON
> drivers/extcon/Kconfig:2: symbol EXTCON is selected by CHARGER_MANAGER
> drivers/power/supply/Kconfig:482: symbol CHARGER_MANAGER depends on POWER_SUPPLY
> drivers/power/supply/Kconfig:2: symbol POWER_SUPPLY is selected by HID_ASUS
> drivers/hid/Kconfig:150: symbol HID_ASUS depends on LEDS_CLASS
> drivers/leds/Kconfig:17: symbol LEDS_CLASS depends on NEW_LEDS
> drivers/leds/Kconfig:9: symbol NEW_LEDS is selected by SENSORS_APPLESMC
> drivers/hwmon/Kconfig:327: symbol SENSORS_APPLESMC depends on HWMON
> drivers/hwmon/Kconfig:6: symbol HWMON is selected by EEEPC_LAPTOP
> drivers/platform/x86/Kconfig:260: symbol EEEPC_LAPTOP depends on ACPI_VIDEO
> make[3]: *** [/git/arm-soc/scripts/kconfig/Makefile:71: randconfig]
> Segmentation fault (core dumped)
>
> After changing EEEPC_LAPTOP and THINKPAD_ACPI to 'depends on HWMON' instead of
> 'select HWMON', I get this one:
>
> drivers/video/fbdev/Kconfig:12:error: recursive dependency detected!
> drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER
> drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
> drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_SIL_SII8620
> drivers/gpu/drm/bridge/Kconfig:89: symbol DRM_SIL_SII8620 depends on EXTCON
> drivers/extcon/Kconfig:2: symbol EXTCON is selected by CHARGER_MANAGER
> drivers/power/supply/Kconfig:482: symbol CHARGER_MANAGER depends on POWER_SUPPLY
> drivers/power/supply/Kconfig:2: symbol POWER_SUPPLY is selected by HID_ASUS
> drivers/hid/Kconfig:150: symbol HID_ASUS depends on LEDS_CLASS
> drivers/leds/Kconfig:17: symbol LEDS_CLASS depends on NEW_LEDS
> drivers/leds/Kconfig:9: symbol NEW_LEDS is selected by BACKLIGHT_ADP8860
> drivers/video/backlight/Kconfig:316: symbol BACKLIGHT_ADP8860 depends
> on BACKLIGHT_CLASS_DEVICE
> drivers/video/backlight/Kconfig:143: symbol BACKLIGHT_CLASS_DEVICE is
> selected by FB_BACKLIGHT
> drivers/video/fbdev/Kconfig:187: symbol FB_BACKLIGHT depends on FB
>
> Changing all drivers that select 'FB_BACKLIGHT' or 'BACKLIGHT_CLASS_DEVICE' to
> 'depends on BACKLIGHT_CLASS_DEVICE' gets it to build.
>
> The steps each seem reasonable, in particular since they mostly clean
> up the legacy
> fbdev drivers to what they should have done anyway, but it is quite
> invasive in the end.
> Any other ideas?

Adding Jani, since iirc he looked at the entire backlight Kconfig
story before. I think there's some nonsense going on where in some
cases you don't get reasonable dummy functions where it just doesn't
make sense. Or something like that.

At least the entire select vs depends on backlight sounds eerily familiar.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Saeed Mahameed]

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.

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

[Arnd Bergmann]

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

Re: [RFC 5/6] drm/rcar-du: fix selection of CMM driver

[Laurent Pinchart]

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 <arnd@arndb.de>
> ---
>  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

Re: [RFC 5/6] drm/rcar-du: fix selection of CMM driver

[Arnd Bergmann]

On Tue, Apr 14, 2020 at 10:17 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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

Re: [RFC 5/6] drm/rcar-du: fix selection of CMM driver

[Laurent Pinchart]

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

Re: [RFC 5/6] drm/rcar-du: fix selection of CMM driver

[Arnd Bergmann]

On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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

Re: [RFC 4/6] drm/bridge/sii8620: fix extcon dependency

[Jani Nikula]

On Tue, 14 Apr 2020, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Apr 14, 2020 at 5:05 PM Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> On Fri, Apr 10, 2020 at 8:56 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> >
>> >
>> > On 08.04.2020 22:27, Arnd Bergmann wrote:
>> > > Using 'imply' does not work here, it still cause the same build
>> > > failure:
>> > >
>> > > arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_remove':
>> > > sil-sii8620.c:(.text+0x1b8): undefined reference to `extcon_unregister_notifier'
>> > > arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_probe':
>> > > sil-sii8620.c:(.text+0x27e8): undefined reference to `extcon_find_edev_by_node'
>> > > arm-linux-gnueabi-ld: sil-sii8620.c:(.text+0x2870): undefined reference to `extcon_register_notifier'
>> > > arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/sil-sii8620.o: in function `sii8620_extcon_work':
>> > > sil-sii8620.c:(.text+0x2908): undefined reference to `extcon_get_state'
>> > >
>> > > I tried the usual 'depends on EXTCON || !EXTCON' logic, but that caused
>> > > a circular Kconfig dependency. Using IS_REACHABLE() is ugly but works.
>> >
>> > 'depends on EXTCON || !EXTCON' seems to be proper solution, maybe would be better to try to solve circular dependencies issue.
>>
>> I agree that would be nice, but I failed to come to a proper solution
>> here. FWIW, there
>> is one circular dependency that I managed to avoid by changing all
>> drivers that select FB_DDC
>> to depend on I2C rather than selecting it:
>>
>> drivers/i2c/Kconfig:8:error: recursive dependency detected!
>> drivers/i2c/Kconfig:8: symbol I2C is selected by FB_DDC
>> drivers/video/fbdev/Kconfig:63: symbol FB_DDC depends on FB
>> drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER
>> drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
>> drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_SIL_SII8620
>> drivers/gpu/drm/bridge/Kconfig:89: symbol DRM_SIL_SII8620 depends on EXTCON
>> drivers/extcon/Kconfig:2: symbol EXTCON is selected by CHARGER_MANAGER
>> drivers/power/supply/Kconfig:482: symbol CHARGER_MANAGER depends on POWER_SUPPLY
>> drivers/power/supply/Kconfig:2: symbol POWER_SUPPLY is selected by
>> HID_BATTERY_STRENGTH
>> drivers/hid/Kconfig:29: symbol HID_BATTERY_STRENGTH depends on HID
>> drivers/hid/Kconfig:8: symbol HID is selected by I2C_HID
>> drivers/hid/i2c-hid/Kconfig:5: symbol I2C_HID depends on I2C
>>
>> After that, Kconfig crashes with a segfault:
>>
>> drivers/video/fbdev/Kconfig:12:error: recursive dependency detected!
>> drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER
>> drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
>> drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_SIL_SII8620
>> drivers/gpu/drm/bridge/Kconfig:89: symbol DRM_SIL_SII8620 depends on EXTCON
>> drivers/extcon/Kconfig:2: symbol EXTCON is selected by CHARGER_MANAGER
>> drivers/power/supply/Kconfig:482: symbol CHARGER_MANAGER depends on POWER_SUPPLY
>> drivers/power/supply/Kconfig:2: symbol POWER_SUPPLY is selected by HID_ASUS
>> drivers/hid/Kconfig:150: symbol HID_ASUS depends on LEDS_CLASS
>> drivers/leds/Kconfig:17: symbol LEDS_CLASS depends on NEW_LEDS
>> drivers/leds/Kconfig:9: symbol NEW_LEDS is selected by SENSORS_APPLESMC
>> drivers/hwmon/Kconfig:327: symbol SENSORS_APPLESMC depends on HWMON
>> drivers/hwmon/Kconfig:6: symbol HWMON is selected by EEEPC_LAPTOP
>> drivers/platform/x86/Kconfig:260: symbol EEEPC_LAPTOP depends on ACPI_VIDEO
>> make[3]: *** [/git/arm-soc/scripts/kconfig/Makefile:71: randconfig]
>> Segmentation fault (core dumped)
>>
>> After changing EEEPC_LAPTOP and THINKPAD_ACPI to 'depends on HWMON' instead of
>> 'select HWMON', I get this one:
>>
>> drivers/video/fbdev/Kconfig:12:error: recursive dependency detected!
>> drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER
>> drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
>> drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_SIL_SII8620
>> drivers/gpu/drm/bridge/Kconfig:89: symbol DRM_SIL_SII8620 depends on EXTCON
>> drivers/extcon/Kconfig:2: symbol EXTCON is selected by CHARGER_MANAGER
>> drivers/power/supply/Kconfig:482: symbol CHARGER_MANAGER depends on POWER_SUPPLY
>> drivers/power/supply/Kconfig:2: symbol POWER_SUPPLY is selected by HID_ASUS
>> drivers/hid/Kconfig:150: symbol HID_ASUS depends on LEDS_CLASS
>> drivers/leds/Kconfig:17: symbol LEDS_CLASS depends on NEW_LEDS
>> drivers/leds/Kconfig:9: symbol NEW_LEDS is selected by BACKLIGHT_ADP8860
>> drivers/video/backlight/Kconfig:316: symbol BACKLIGHT_ADP8860 depends
>> on BACKLIGHT_CLASS_DEVICE
>> drivers/video/backlight/Kconfig:143: symbol BACKLIGHT_CLASS_DEVICE is
>> selected by FB_BACKLIGHT
>> drivers/video/fbdev/Kconfig:187: symbol FB_BACKLIGHT depends on FB
>>
>> Changing all drivers that select 'FB_BACKLIGHT' or 'BACKLIGHT_CLASS_DEVICE' to
>> 'depends on BACKLIGHT_CLASS_DEVICE' gets it to build.
>>
>> The steps each seem reasonable, in particular since they mostly clean
>> up the legacy
>> fbdev drivers to what they should have done anyway, but it is quite
>> invasive in the end.
>> Any other ideas?
>
> Adding Jani, since iirc he looked at the entire backlight Kconfig
> story before. I think there's some nonsense going on where in some
> cases you don't get reasonable dummy functions where it just doesn't
> make sense. Or something like that.
>
> At least the entire select vs depends on backlight sounds eerily familiar.

TL;DR I'd ack a patch switching to depends.

I'm all for switching to "depends on BACKLIGHT_CLASS_DEVICE" throughout,
and I've sent patches to do just that years ago. Alas it was met with
opposition because if you have BACKLIGHT_CLASS_DEVICE=n as a starting
point, you won't even see your driver that depends on it in menuconfig.

I do think the menuconfig usability issue is a big part of the problem
here. You can't find and enable the options you want easily if the
dependencies aren't met. And you can't easily enable the dependencies
either. Select is an answer to that question. It's the wrong answer, but
it's *an* answer.

On another level, a lot of the problems could be avoided if we took heed
of the note in Documentation/kbuild/kconfig-language.rst:

  Note:
	select should be used with care. select will force
	a symbol to a value without visiting the dependencies.
	By abusing select you are able to select a symbol FOO even
	if FOO depends on BAR that is not set.
	In general use select only for non-visible symbols
	(no prompts anywhere) and for symbols with no dependencies.
	That will limit the usefulness but on the other hand avoid
	the illegal configurations all over.

Another part of the problem is that there is no warning or lint option
or anything to warn about this usage in config. You can easily add more
of these, and it'll break later in subtle ways. And we'll just keep
repeating the same discussion over and over again, some folks fixing
issues in kconfig, some folks using IS_REACHABLE() and eventually
hitting different kinds of problems. (Personally, I wouldn't very easily
accept a patch adding IS_REACHABLE() use in i915.)


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [RFC 5/6] drm/rcar-du: fix selection of CMM driver

[Geert Uytterhoeven]

Hi Arnd,

On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> 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 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC 5/6] drm/rcar-du: fix selection of CMM driver

[Arnd Bergmann]

On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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

Re: [RFC 5/6] drm/rcar-du: fix selection of CMM driver

[Arnd Bergmann]

On Wed, Apr 15, 2020 at 5:18 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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

Re: [RFC 5/6] drm/rcar-du: fix selection of CMM driver

[Laurent Pinchart]

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 <arnd@arndb.de> wrote:
> > On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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

Re: [RFC 5/6] drm/rcar-du: fix selection of CMM driver

[Arnd Bergmann]

On Wed, Apr 15, 2020 at 11:12 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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 <arnd@arndb.de> wrote:
> > > On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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

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

[Saeed Mahameed]

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

Re: [RFC 5/6] drm/rcar-du: fix selection of CMM driver

[Daniel Vetter]

On Wed, Apr 15, 2020 at 11:22 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Apr 15, 2020 at 11:12 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> 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 <arnd@arndb.de> wrote:
> > > > On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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

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

[Arnd Bergmann]

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

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

[Jani Nikula]

On Thu, 16 Apr 2020, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Apr 16, 2020 at 5:25 AM Saeed Mahameed <saeedm@mellanox.com> wrote:
>> BTW how about adding a new Kconfig option to hide the details of
>> ( BAR || !BAR) ? as Jason already explained and suggested, this will
>> make it easier for the users and developers to understand the actual
>> meaning behind this tristate weird condition.
>>
>> e.g have a new keyword:
>>      reach VXLAN
>> which will be equivalent to:
>>      depends on VXLAN && !VXLAN
>
> I'd love to see that, but I'm not sure what keyword is best. For your
> suggestion of "reach", that would probably do the job, but I'm not
> sure if this ends up being more or less confusing than what we have
> today.

Ah, perfect bikeshedding topic!

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

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

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Arnd Bergmann]

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

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

[Jason Gunthorpe]

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

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

[Nicolas Pitre]

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

Re: [RFC 5/6] drm/rcar-du: fix selection of CMM driver

[Laurent Pinchart]

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 <arnd@arndb.de> wrote:
> > > > > On Wed, Apr 15, 2020 at 4:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > On Wed, Apr 15, 2020 at 3:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > > > On Tue, Apr 14, 2020 at 10:52 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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

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

[Arnd Bergmann]

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

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

[Jason Gunthorpe]

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

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

[Jason Gunthorpe]

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

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

[Saeed Mahameed]

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.

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

[Andrzej Hajda]

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
>