| Message ID | 20161108135721.2142330-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 8, 2016 at 8:56 AM, Arnd Bergmann <arnd@arndb.de> wrote: > The newly introduced LED handling for nouveau fails to link when the > driver is built-in but the LED subsystem is a loadable module: > > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_suspend': > tvnv17.c:(.text.nouveau_do_suspend+0x10): undefined reference to `nouveau_led_suspend' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_resume': > tvnv17.c:(.text.nouveau_do_resume+0xf0): undefined reference to `nouveau_led_resume' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_unload': > tvnv17.c:(.text.nouveau_drm_unload+0x34): undefined reference to `nouveau_led_fini' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_load': > tvnv17.c:(.text.nouveau_drm_load+0x7d0): undefined reference to `nouveau_led_init' > > This adds a separate Kconfig symbol for the LED support that > correctly tracks the dependencies. > > Fixes: 8d021d71b324 ("drm/nouveau/drm/nouveau: add a LED driver for the NVIDIA logo") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/gpu/drm/nouveau/Kbuild | 2 +- > drivers/gpu/drm/nouveau/Kconfig | 8 ++++++++ > drivers/gpu/drm/nouveau/nouveau_led.h | 2 +- > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild > index fde6e3656636..5e00e911daa6 100644 > --- a/drivers/gpu/drm/nouveau/Kbuild > +++ b/drivers/gpu/drm/nouveau/Kbuild > @@ -22,7 +22,7 @@ nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o > nouveau-y += nouveau_drm.o > nouveau-y += nouveau_hwmon.o > nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o > -nouveau-$(CONFIG_LEDS_CLASS) += nouveau_led.o > +nouveau-$(CONFIG_DRM_NOUVEAU_LED) += nouveau_led.o > nouveau-y += nouveau_nvif.o > nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o > nouveau-y += nouveau_usif.o # userspace <-> nvif > 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? Separately, perhaps we should just drop this LEDS_CLASS select into DRM_NOUVEAU? We've tended to avoid adding tons of options. Cheers, -ilia
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 08/11/16 15:56, Arnd Bergmann wrote: > The newly introduced LED handling for nouveau fails to link when the > driver is built-in but the LED subsystem is a loadable module: > > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_suspend': > tvnv17.c:(.text.nouveau_do_suspend+0x10): undefined reference to `nouveau_led_suspend' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_resume': > tvnv17.c:(.text.nouveau_do_resume+0xf0): undefined reference to `nouveau_led_resume' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_unload': > tvnv17.c:(.text.nouveau_drm_unload+0x34): undefined reference to `nouveau_led_fini' > drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_load': > tvnv17.c:(.text.nouveau_drm_load+0x7d0): undefined reference to `nouveau_led_init' > > This adds a separate Kconfig symbol for the LED support that > correctly tracks the dependencies. > > Fixes: 8d021d71b324 ("drm/nouveau/drm/nouveau: add a LED driver for the NVIDIA logo") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/gpu/drm/nouveau/Kbuild | 2 +- > drivers/gpu/drm/nouveau/Kconfig | 8 ++++++++ > drivers/gpu/drm/nouveau/nouveau_led.h | 2 +- > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild > index fde6e3656636..5e00e911daa6 100644 > --- a/drivers/gpu/drm/nouveau/Kbuild > +++ b/drivers/gpu/drm/nouveau/Kbuild > @@ -22,7 +22,7 @@ nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o > nouveau-y += nouveau_drm.o > nouveau-y += nouveau_hwmon.o > nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o > -nouveau-$(CONFIG_LEDS_CLASS) += nouveau_led.o > +nouveau-$(CONFIG_DRM_NOUVEAU_LED) += nouveau_led.o > nouveau-y += nouveau_nvif.o > nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o > nouveau-y += nouveau_usif.o # userspace <-> nvif > 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. Titan is a little too specific and inaccurate. How about high-end cards? As for the patch itself... I was going for the auto-magic approach but apparently failed at it :s Sorry... I guess what I wanted to do was to only enable the LED support if LEDS_CLASS=y. Anyway, your approach is cleaner because it makes it easy for the user to say if he/she wants to enable. I will have a closer look at it later. Thanks anyway for reporting the issue! Martin
2016-11-08 16:46 GMT+01:00 Ilia Mirkin <imirkin@alum.mit.edu>: > On Tue, Nov 8, 2016 at 8:56 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> The newly introduced LED handling for nouveau fails to link when the >> driver is built-in but the LED subsystem is a loadable module: >> >> drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_suspend': >> tvnv17.c:(.text.nouveau_do_suspend+0x10): undefined reference to `nouveau_led_suspend' >> drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_resume': >> tvnv17.c:(.text.nouveau_do_resume+0xf0): undefined reference to `nouveau_led_resume' >> drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_unload': >> tvnv17.c:(.text.nouveau_drm_unload+0x34): undefined reference to `nouveau_led_fini' >> drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_load': >> tvnv17.c:(.text.nouveau_drm_load+0x7d0): undefined reference to `nouveau_led_init' >> >> This adds a separate Kconfig symbol for the LED support that >> correctly tracks the dependencies. >> >> Fixes: 8d021d71b324 ("drm/nouveau/drm/nouveau: add a LED driver for the NVIDIA logo") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> --- >> drivers/gpu/drm/nouveau/Kbuild | 2 +- >> drivers/gpu/drm/nouveau/Kconfig | 8 ++++++++ >> drivers/gpu/drm/nouveau/nouveau_led.h | 2 +- >> 3 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild >> index fde6e3656636..5e00e911daa6 100644 >> --- a/drivers/gpu/drm/nouveau/Kbuild >> +++ b/drivers/gpu/drm/nouveau/Kbuild >> @@ -22,7 +22,7 @@ nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o >> nouveau-y += nouveau_drm.o >> nouveau-y += nouveau_hwmon.o >> nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o >> -nouveau-$(CONFIG_LEDS_CLASS) += nouveau_led.o >> +nouveau-$(CONFIG_DRM_NOUVEAU_LED) += nouveau_led.o >> nouveau-y += nouveau_nvif.o >> nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o >> nouveau-y += nouveau_usif.o # userspace <-> nvif >> 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? > > Separately, perhaps we should just drop this LEDS_CLASS select into > DRM_NOUVEAU? We've tended to avoid adding tons of options. > well, that would mean that you always need the LEDS_CLASS and maybe on a tegra system you don't want to, so the led stuff should stay completely optional. Don't know though. Alex, maybe you want to clarify which dependencies should stay optional? If nobody on your side care, then we won't care as well and only add switches if users actually request it. > Cheers, > > -ilia > _______________________________________________ > Nouveau mailing list > Nouveau@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau
diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild index fde6e3656636..5e00e911daa6 100644 --- a/drivers/gpu/drm/nouveau/Kbuild +++ b/drivers/gpu/drm/nouveau/Kbuild @@ -22,7 +22,7 @@ nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o nouveau-y += nouveau_drm.o nouveau-y += nouveau_hwmon.o nouveau-$(CONFIG_COMPAT) += nouveau_ioc32.o -nouveau-$(CONFIG_LEDS_CLASS) += nouveau_led.o +nouveau-$(CONFIG_DRM_NOUVEAU_LED) += nouveau_led.o nouveau-y += nouveau_nvif.o nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o nouveau-y += nouveau_usif.o # userspace <-> nvif 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. + config NOUVEAU_DEBUG_DEFAULT int "Default debug level" depends on DRM_NOUVEAU diff --git a/drivers/gpu/drm/nouveau/nouveau_led.h b/drivers/gpu/drm/nouveau/nouveau_led.h index 187ecdb82002..9c9bb6ac938e 100644 --- a/drivers/gpu/drm/nouveau/nouveau_led.h +++ b/drivers/gpu/drm/nouveau/nouveau_led.h @@ -42,7 +42,7 @@ nouveau_led(struct drm_device *dev) } /* nouveau_led.c */ -#if IS_ENABLED(CONFIG_LEDS_CLASS) +#if IS_ENABLED(CONFIG_DRM_NOUVEAU_LED) int nouveau_led_init(struct drm_device *dev); void nouveau_led_suspend(struct drm_device *dev); void nouveau_led_resume(struct drm_device *dev);
The newly introduced LED handling for nouveau fails to link when the driver is built-in but the LED subsystem is a loadable module: drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_suspend': tvnv17.c:(.text.nouveau_do_suspend+0x10): undefined reference to `nouveau_led_suspend' drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_do_resume': tvnv17.c:(.text.nouveau_do_resume+0xf0): undefined reference to `nouveau_led_resume' drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_unload': tvnv17.c:(.text.nouveau_drm_unload+0x34): undefined reference to `nouveau_led_fini' drivers/gpu/drm/nouveau/nouveau.o: In function `nouveau_drm_load': tvnv17.c:(.text.nouveau_drm_load+0x7d0): undefined reference to `nouveau_led_init' This adds a separate Kconfig symbol for the LED support that correctly tracks the dependencies. Fixes: 8d021d71b324 ("drm/nouveau/drm/nouveau: add a LED driver for the NVIDIA logo") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/gpu/drm/nouveau/Kbuild | 2 +- drivers/gpu/drm/nouveau/Kconfig | 8 ++++++++ drivers/gpu/drm/nouveau/nouveau_led.h | 2 +- 3 files changed, 10 insertions(+), 2 deletions(-)