From: Arnd Bergmann <arnd@arndb.de>
To: Ilia Mirkin <imirkin@alum.mit.edu>
Cc: Ben Skeggs <bskeggs@redhat.com>,
        "nouveau@lists.freedesktop.org" <nouveau@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration
Date: Tue, 08 Nov 2016 16:52:49 +0100
Message-ID: <12373137.2mgsQIhOfV@wuerfel>
User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; )
In-Reply-To: <CAKb7UviRQ5_7nAeQ1zfacsiM1f13gohHRHeDZaD+ZOXM8srFKw@mail.gmail.com>
References: <20161108135721.2142330-1-arnd@arndb.de> <CAKb7UviRQ5_7nAeQ1zfacsiM1f13gohHRHeDZaD+ZOXM8srFKw@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 7004
Lines: 127

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