Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753820AbcKHQfk (ORCPT ); Tue, 8 Nov 2016 11:35:40 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35408 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750938AbcKHQfj (ORCPT ); Tue, 8 Nov 2016 11:35:39 -0500 MIME-Version: 1.0 In-Reply-To: References: <20161108135721.2142330-1-arnd@arndb.de> <12373137.2mgsQIhOfV@wuerfel> <20161108160701.GC6983@wunner.de> <7299445.oinG8ujhC7@wuerfel> From: Emil Velikov Date: Tue, 8 Nov 2016 16:35:36 +0000 Message-ID: Subject: Re: [Nouveau] [PATCH] drm/nouveau: fix LEDS_CLASS=m configuration To: Karol Herbst Cc: Arnd Bergmann , "nouveau@lists.freedesktop.org" , Ben Skeggs , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3440 Lines: 79 On 8 November 2016 at 16:21, Karol Herbst wrote: > 2016-11-08 17:12 GMT+01:00 Arnd Bergmann : >> 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. > > yeah, it will. I guess it is fine to force LEDS to y if nouveau is set > to y. The thinks I absolutely dislike is: > 1. auto hiding of features I _want_ to have, because I would have to > enable the dependencies first, which is like super annoying if there > are somewhere else > 2. preventing me from enabling something, cause the dependency is missing. > > We should clarify first if we actually want to enable those features > optionally, because there isn't much of a reason not to enable the > dependencies, except embedded systems. We have a lot more stuff where > we could add things like that: hwmon, debugfs, acpi, compat and maybe > there are even more things > Sounds like people may have missed the core part: This/earlier patch are required due to select "abuse" elsewhere in the kernel. The IS_REACHABLE/DRM_NOUVEAU_LED is patch to workaround things on nouveau side, with a proper one to remove/untangle the "select LEDS_CLASS". The latter will likely be slow/pain, since devs love to use select because it's convenient (and indeed it is). Thus [temporary] workaround on nouveau side will be good in the short term. Afaict, "forcing LEDS to y if nouveau is set to y" is going the opposite direction of what one should be going ;-) Regards, Emil