Nouveau - Nov 2016 - [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration

On Tuesday, November 8, 2016 10:46:07 AM CET Ilia Mirkin
wrote:
> > diff --git a/drivers/gpu/drm/nouveau/Kconfig
b/drivers/gpu/drm/nouveau/Kconfig
> > index 78631fb61adf..715cd6f4dc31 100644
> > --- a/drivers/gpu/drm/nouveau/Kconfig
> > +++ b/drivers/gpu/drm/nouveau/Kconfig
> > @@ -46,6 +46,14 @@ config NOUVEAU_DEBUG
> >           The paranoia and spam levels will add a lot of extra checks
which
> >           may potentially slow down driver operation.
> >
> > +config DRM_NOUVEAU_LED
> > +       bool "Support for logo LED"
> > +       depends on DRM_NOUVEAU && LEDS_CLASS
> > +       depends on !(DRM_NOUVEAU=y && LEDS_CLASS=m)
> > +       help
> > +         Say Y here to enabling controlling the brightness of the
> > +         LED behind NVIDIA logo on certain Titan cards.
> 
> This is a very odd restriction... could this be written as
> 
> depends on DRM_NOUVEAU
> select LEDS_CLASS
> 
> Or will that not flip the LEDS_CLASS from m to y in case
> DRM_NOUVEAU=y? If not, is there a way to cause that to happen?
Using 'select' on user-selectable symbols risks introducing
circular dependencies.
> Separately, perhaps we should just drop this LEDS_CLASS select into
> DRM_NOUVEAU? We've tended to avoid adding tons of options.
My first attempt was to add "depends on LEDS_CLASS || !LEDS_CLASS"
to DRM_NOUVEAU, which led to one long circle:

drivers/usb/Kconfig:39:error: recursive dependency detected!
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/usb/Kconfig:39:	symbol USB is selected by MOUSE_APPLETOUCH
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/input/mouse/Kconfig:187:	symbol MOUSE_APPLETOUCH depends on INPUT
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/input/Kconfig:8:	symbol INPUT is selected by VT
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/tty/Kconfig:12:	symbol VT is selected by FB_STI
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/video/fbdev/Kconfig:669:	symbol FB_STI depends on FB
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/video/fbdev/Kconfig:5:	symbol FB is selected by DRM_KMS_FB_HELPER
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/gpu/drm/Kconfig:42:	symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/gpu/drm/Kconfig:36:	symbol DRM_KMS_HELPER is selected by DRM_NOUVEAU
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/gpu/drm/nouveau/Kconfig:1:	symbol DRM_NOUVEAU depends on LEDS_CLASS
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/leds/Kconfig:16:	symbol LEDS_CLASS is selected by OMAP_DEBUG_LEDS
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
arch/arm/plat-omap/Kconfig:19:	symbol OMAP_DEBUG_LEDS depends on NEW_LEDS
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/leds/Kconfig:8:	symbol NEW_LEDS is selected by ATH9K_HTC
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/net/wireless/ath/ath9k/Kconfig:158:	symbol ATH9K_HTC depends on USB
Tue, 08 Nov 2016 11:49:44 +0100 build/0x3053A542_defconfig warnings
drivers/usb/Kconfig:39:error: recursive dependency detected!
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/usb/Kconfig:39:	symbol USB is selected by MOUSE_APPLETOUCH
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/input/mouse/Kconfig:187:	symbol MOUSE_APPLETOUCH depends on INPUT
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/input/Kconfig:8:	symbol INPUT is selected by VT
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/tty/Kconfig:12:	symbol VT is selected by FB_STI
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/video/fbdev/Kconfig:669:	symbol FB_STI depends on FB
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/video/fbdev/Kconfig:5:	symbol FB is selected by DRM_KMS_FB_HELPER
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/gpu/drm/Kconfig:42:	symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/gpu/drm/Kconfig:36:	symbol DRM_KMS_HELPER is selected by DRM_NOUVEAU
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/gpu/drm/nouveau/Kconfig:1:	symbol DRM_NOUVEAU depends on LEDS_CLASS
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/leds/Kconfig:16:	symbol LEDS_CLASS is selected by OMAP_DEBUG_LEDS
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
arch/arm/plat-omap/Kconfig:19:	symbol OMAP_DEBUG_LEDS depends on NEW_LEDS
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/leds/Kconfig:8:	symbol NEW_LEDS is selected by ATH9K_HTC
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/net/wireless/ath/ath9k/Kconfig:158:	symbol ATH9K_HTC depends on USB

The underlying problem is that we already have a number of other
symbols that either have "depends on LEDS_CLASS" or
"select LEDS_CLASS". To clean that up properly, we should either
make the symbol itself hidden and only select it from other drivers,
or use "depends on LEDS_CLASS" everywhere.

Another option is to use the IS_REACHABLE() macro instead of IS_ENABLED()
in the header file, to stub out the calls into the new file, but
that can be a bit confusing.

	Arnd

On Tue, Nov 08, 2016 at 04:52:49PM +0100, Arnd Bergmann
wrote:
> The underlying problem is that we already have a number of other
> symbols that either have "depends on LEDS_CLASS" or
> "select LEDS_CLASS". To clean that up properly, we should either
> make the symbol itself hidden and only select it from other drivers,
> or use "depends on LEDS_CLASS" everywhere.
> 
> Another option is to use the IS_REACHABLE() macro instead of IS_ENABLED()
> in the header file, to stub out the calls into the new file, but
> that can be a bit confusing.
Why don't you just add empty inline stubs for nouveau_led_init / _fini /
_suspend / _resume?

Thanks,

Lukas

On Tuesday, November 8, 2016 5:07:01 PM CET Lukas Wunner
wrote:
> On Tue, Nov 08, 2016 at 04:52:49PM +0100, Arnd Bergmann wrote:
> > The underlying problem is that we already have a number of other
> > symbols that either have "depends on LEDS_CLASS" or
> > "select LEDS_CLASS". To clean that up properly, we should
either
> > make the symbol itself hidden and only select it from other drivers,
> > or use "depends on LEDS_CLASS" everywhere.
> > 
> > Another option is to use the IS_REACHABLE() macro instead of
IS_ENABLED()
> > in the header file, to stub out the calls into the new file, but
> > that can be a bit confusing.
> 
> Why don't you just add empty inline stubs for nouveau_led_init / _fini
/
> _suspend / _resume?
> 
That's what I was suggesting:

diff --git a/drivers/gpu/drm/nouveau/nouveau_led.h
b/drivers/gpu/drm/nouveau/nouveau_led.h
index 9c9bb6ac938e..bc5f47cb516b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_led.h
+++ b/drivers/gpu/drm/nouveau/nouveau_led.h
@@ -35,21 +35,21 @@ struct nouveau_led {
 	struct led_classdev led;
 };
 
 static inline struct nouveau_led *
 nouveau_led(struct drm_device *dev)
 {
 	return nouveau_drm(dev)->led;
 }
 
 /* nouveau_led.c */
-#if IS_ENABLED(CONFIG_LEDS_CLASS)
+#if IS_REACHABLE(CONFIG_LEDS_CLASS)
 int  nouveau_led_init(struct drm_device *dev);
 void nouveau_led_suspend(struct drm_device *dev);
 void nouveau_led_resume(struct drm_device *dev);
 void nouveau_led_fini(struct drm_device *dev);
 #else
 static inline int  nouveau_led_init(struct drm_device *dev) { return 0; };
 static inline void nouveau_led_suspend(struct drm_device *dev) { };
 static inline void nouveau_led_resume(struct drm_device *dev) { };
 static inline void nouveau_led_fini(struct drm_device *dev) { };
 #endif

The downside is that now the nouveau_led_init() just won't be called
if CONFIG_LEDS_CLASS=m and CONFIG_DRM_NOUVEAU=y, which can be
surprising to users.

	Arnd