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

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

2016-11-08 17:12 GMT+01:00 Arnd Bergmann <arnd at
arndb.de>:
> 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
>
>         Arnd
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

On 8 November 2016 at 16:21, Karol Herbst <karolherbst at gmail.com>
wrote:
> 2016-11-08 17:12 GMT+01:00 Arnd Bergmann <arnd at arndb.de>:
>> 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