mbox series

[0/8] drm, fbdev: rework dependencies

Message ID 20200417155553.675905-1-arnd@arndb.de (mailing list archive)
Headers show
Series drm, fbdev: rework dependencies | expand

Message

Arnd Bergmann April 17, 2020, 3:55 p.m. UTC
I tried to fix up some dependencies after the sii8620 "imply EXTCON"
statementn broke, trying a few things but in the backing out a
change that would completely reverse the LEDS_CLASS selects into
a 'depends on'. 

However, what I got now are multiple changes that remove gratious
"selects" that lead to circular dependencies for sii8620 and others:

- Anything doing "select FB" is now gone, or becomes "depends on FB",

- DDC support depends on I2C instead of selecting it

- backlight class device support is never selected by framebuffer
  drivers but has proper dependencies

I have done thousands of randconfig build tests on this, but no
runtime tests.

Some of the 'depends on FOO || !FOO' statements could be simplified
into a new 'uses FOO' syntax based on a patch from Saeed Mahameed,
but I would for the moment treat that as a cleanup that can be done
later.

If we can agree on these changes, maybe someone can merge them
through the drm-misc tree.

Please review

       Arnd

Arnd Bergmann (8):
  fbdev: w100fb: clean up mach-pxa compile-time dependency
  fbdev/ARM: pxa: avoid selecting CONFIG_FB
  fbdev: rework FB_DDC dependencies
  drm/rcar: stop using 'imply' for dependencies
  drm/vmwgfx: make framebuffer support optional
  drm: decouple from CONFIG_FB
  fbdev: rework backlight dependencies
  drm/bridge/sii8620: fix extcon dependency

 arch/arm/configs/pxa_defconfig      |  3 ++
 arch/arm/mach-pxa/Kconfig           |  7 ---
 arch/arm/mach-pxa/eseries.c         | 14 +----
 arch/arm/mach-pxa/saar.c            |  2 +-
 arch/arm/mach-pxa/tavorevb.c        |  2 +-
 drivers/auxdisplay/Kconfig          |  1 +
 drivers/gpu/drm/Kconfig             |  5 +-
 drivers/gpu/drm/bridge/Kconfig      |  2 +-
 drivers/gpu/drm/mxsfb/Kconfig       |  1 -
 drivers/gpu/drm/rcar-du/Kconfig     | 23 +++++---
 drivers/gpu/drm/vmwgfx/Kconfig      | 17 +++---
 drivers/gpu/drm/vmwgfx/Makefile     |  4 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 35 +++++++-----
 drivers/gpu/drm/zte/Kconfig         |  1 -
 drivers/macintosh/Kconfig           |  1 +
 drivers/staging/fbtft/Kconfig       |  1 +
 drivers/staging/olpc_dcon/Kconfig   |  2 +-
 drivers/video/fbdev/Kconfig         | 31 ++++++++---
 drivers/video/fbdev/w100fb.c        | 84 +++++------------------------
 include/video/w100fb.h              |  6 +--
 20 files changed, 101 insertions(+), 141 deletions(-)

Comments

Daniel Vetter April 17, 2020, 5:14 p.m. UTC | #1
On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
> I tried to fix up some dependencies after the sii8620 "imply EXTCON"
> statementn broke, trying a few things but in the backing out a
> change that would completely reverse the LEDS_CLASS selects into
> a 'depends on'. 
> 
> However, what I got now are multiple changes that remove gratious
> "selects" that lead to circular dependencies for sii8620 and others:
> 
> - Anything doing "select FB" is now gone, or becomes "depends on FB",
> 
> - DDC support depends on I2C instead of selecting it
> 
> - backlight class device support is never selected by framebuffer
>   drivers but has proper dependencies
> 
> I have done thousands of randconfig build tests on this, but no
> runtime tests.
> 
> Some of the 'depends on FOO || !FOO' statements could be simplified
> into a new 'uses FOO' syntax based on a patch from Saeed Mahameed,
> but I would for the moment treat that as a cleanup that can be done
> later.
> 
> If we can agree on these changes, maybe someone can merge them
> through the drm-misc tree.
> 
> Please review

Biggest concern I have is that usability of make menuconfig is horrible,
and it's very hard to find options that are hidden by depends on. You can
use the search interface, if you happen to know the option.

Once you've surmounted that bar, the next one is trying to find what
exactly you need to enable. Which again means endless of recursive
screaming at Kconfig files, since make menuconfig doesn't help you at all.

That's pretty much why we've never pushed this in, and instead done the
selects. I'm vary applying all this, since after after there'll be all the
screaming again and we have to back it out.

I think to embrace this without regrets what we need is:
- some way to list the hidden options
- some way to browse the depedencies of those hidden options

menuconfig cant do that, gconfig I cant build here (it's some old gtk2
thing, where do you even get the deps for that). xconfig also cant do
this easily, dependencies aren't linked.

So yeah not sure this is a good idea at all, until at least menuconfig can
cope.
-Daniel

> 
>        Arnd
> 
> Arnd Bergmann (8):
>   fbdev: w100fb: clean up mach-pxa compile-time dependency
>   fbdev/ARM: pxa: avoid selecting CONFIG_FB
>   fbdev: rework FB_DDC dependencies
>   drm/rcar: stop using 'imply' for dependencies
>   drm/vmwgfx: make framebuffer support optional
>   drm: decouple from CONFIG_FB
>   fbdev: rework backlight dependencies
>   drm/bridge/sii8620: fix extcon dependency
> 
>  arch/arm/configs/pxa_defconfig      |  3 ++
>  arch/arm/mach-pxa/Kconfig           |  7 ---
>  arch/arm/mach-pxa/eseries.c         | 14 +----
>  arch/arm/mach-pxa/saar.c            |  2 +-
>  arch/arm/mach-pxa/tavorevb.c        |  2 +-
>  drivers/auxdisplay/Kconfig          |  1 +
>  drivers/gpu/drm/Kconfig             |  5 +-
>  drivers/gpu/drm/bridge/Kconfig      |  2 +-
>  drivers/gpu/drm/mxsfb/Kconfig       |  1 -
>  drivers/gpu/drm/rcar-du/Kconfig     | 23 +++++---
>  drivers/gpu/drm/vmwgfx/Kconfig      | 17 +++---
>  drivers/gpu/drm/vmwgfx/Makefile     |  4 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 35 +++++++-----
>  drivers/gpu/drm/zte/Kconfig         |  1 -
>  drivers/macintosh/Kconfig           |  1 +
>  drivers/staging/fbtft/Kconfig       |  1 +
>  drivers/staging/olpc_dcon/Kconfig   |  2 +-
>  drivers/video/fbdev/Kconfig         | 31 ++++++++---
>  drivers/video/fbdev/w100fb.c        | 84 +++++------------------------
>  include/video/w100fb.h              |  6 +--
>  20 files changed, 101 insertions(+), 141 deletions(-)
> 
> -- 
> 2.26.0
> 
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Nicolas Pitre <nico@fluxnic.net>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> Cc: <masahiroy@kernel.org>
> Cc: <Laurent.pinchart@ideasonboard.com>
> Cc: <linux-renesas-soc@vger.kernel.org>,
> Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
> Cc: <kieran.bingham+renesas@ideasonboard.com>,
> Cc: <airlied@linux.ie>
> Cc: daniel@zonque.org
> Cc: haojian.zhuang@gmail.com
> Cc: robert.jarzmik@free.fr
> Cc: daniel@ffwll.ch
> Cc: marex@denx.de
> Cc: stefan@agner.ch
> Cc: linux-graphics-maintainer@vmware.com
> Cc: thellstrom@vmware.com
> Cc: jfrederich@gmail.com
> Cc: dsd@laptop.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-fbdev@vger.kernel.org
> Cc: geert@linux-m68k.org
Jason Gunthorpe April 17, 2020, 7:08 p.m. UTC | #2
On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
> > I tried to fix up some dependencies after the sii8620 "imply EXTCON"
> > statementn broke, trying a few things but in the backing out a
> > change that would completely reverse the LEDS_CLASS selects into
> > a 'depends on'. 
> > 
> > However, what I got now are multiple changes that remove gratious
> > "selects" that lead to circular dependencies for sii8620 and others:
> > 
> > - Anything doing "select FB" is now gone, or becomes "depends on FB",
> > 
> > - DDC support depends on I2C instead of selecting it
> > 
> > - backlight class device support is never selected by framebuffer
> >   drivers but has proper dependencies
> > 
> > I have done thousands of randconfig build tests on this, but no
> > runtime tests.
> > 
> > Some of the 'depends on FOO || !FOO' statements could be simplified
> > into a new 'uses FOO' syntax based on a patch from Saeed Mahameed,
> > but I would for the moment treat that as a cleanup that can be done
> > later.
> > 
> > If we can agree on these changes, maybe someone can merge them
> > through the drm-misc tree.
> > 
> > Please review
> 
> Biggest concern I have is that usability of make menuconfig is horrible,
> and it's very hard to find options that are hidden by depends on. You can
> use the search interface, if you happen to know the option.
> 
> Once you've surmounted that bar, the next one is trying to find what
> exactly you need to enable. Which again means endless of recursive
> screaming at Kconfig files, since make menuconfig doesn't help you at all.

+1 on this. But this is a general kconfig problem, and not unique to
DRM, I've done this screaming for many different things now.. eg to
turn on every single RDMA driver.

I hackily delt with it by creating this rather insane script based on
the python kconfiglib to try and sort things out mostly automatically:

https://github.com/jgunthorpe/Kernel-Maintainer-Tools/blob/master/gj_tools/cmd_kconfig.py

It would be great if menuconfig had a key to say 'hey, really, turn
this on and everything it depends on, recursively'

Jason
Jani Nikula April 20, 2020, 8:14 a.m. UTC | #3
On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
>> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
>> > I tried to fix up some dependencies after the sii8620 "imply EXTCON"
>> > statementn broke, trying a few things but in the backing out a
>> > change that would completely reverse the LEDS_CLASS selects into
>> > a 'depends on'. 
>> > 
>> > However, what I got now are multiple changes that remove gratious
>> > "selects" that lead to circular dependencies for sii8620 and others:
>> > 
>> > - Anything doing "select FB" is now gone, or becomes "depends on FB",
>> > 
>> > - DDC support depends on I2C instead of selecting it
>> > 
>> > - backlight class device support is never selected by framebuffer
>> >   drivers but has proper dependencies
>> > 
>> > I have done thousands of randconfig build tests on this, but no
>> > runtime tests.
>> > 
>> > Some of the 'depends on FOO || !FOO' statements could be simplified
>> > into a new 'uses FOO' syntax based on a patch from Saeed Mahameed,
>> > but I would for the moment treat that as a cleanup that can be done
>> > later.
>> > 
>> > If we can agree on these changes, maybe someone can merge them
>> > through the drm-misc tree.
>> > 
>> > Please review
>> 
>> Biggest concern I have is that usability of make menuconfig is horrible,
>> and it's very hard to find options that are hidden by depends on. You can
>> use the search interface, if you happen to know the option.
>> 
>> Once you've surmounted that bar, the next one is trying to find what
>> exactly you need to enable. Which again means endless of recursive
>> screaming at Kconfig files, since make menuconfig doesn't help you at all.
>
> +1 on this. But this is a general kconfig problem, and not unique to
> DRM, I've done this screaming for many different things now.. eg to
> turn on every single RDMA driver.
>
> I hackily delt with it by creating this rather insane script based on
> the python kconfiglib to try and sort things out mostly automatically:
>
> https://github.com/jgunthorpe/Kernel-Maintainer-Tools/blob/master/gj_tools/cmd_kconfig.py
>
> It would be great if menuconfig had a key to say 'hey, really, turn
> this on and everything it depends on, recursively'

I'm really all for switching to using depends when that is the
semantically right thing to do. In many places using select is a hack to
make the UI simpler, and that's just plain wrong. We'll be doomed to
perpetual randconfig build failures and duct tape fixes.

I'm pretty tired of this, and I regularly ignore those duct tape fixes
to i915 backlight build issues on some bizarre configs that nobody will
ever use, and would not exist if depends were used throughout.

I'm fine with select but only when it's restricted to symbols that have
no dependencies of their own and have no UI. This is in line with
Documentation/kbuild/kconfig-language.rst. Not enforcing this is another
Kconfig tool shortcoming.

See also my reply to Sam [1].

BR,
Jani.


[1] https://lore.kernel.org/dri-devel/871roi37qe.fsf@intel.com/
Arnd Bergmann April 20, 2020, 2:03 p.m. UTC | #4
On Mon, Apr 20, 2020 at 10:14 AM Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
> >> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
> >> >
> >> > If we can agree on these changes, maybe someone can merge them
> >> > through the drm-misc tree.
> >> >
> >> > Please review
> >>
> >> Biggest concern I have is that usability of make menuconfig is horrible,

No doubt about that, but that seems to be unrelated to the cleanup.

> >> and it's very hard to find options that are hidden by depends on. You can
> >> use the search interface, if you happen to know the option.
> >>
> >> Once you've surmounted that bar, the next one is trying to find what
> >> exactly you need to enable. Which again means endless of recursive
> >> screaming at Kconfig files, since make menuconfig doesn't help you at all.

The changes I'm doing are mostly for fbdev, which is currently the
odd one out. Most kernel subsystems today follow the documented
recommendations and only use 'depends on' for things they
depend on.

Having fbdev be the exception causes two problems:

- It does not make kconfig any easier to use overall, just less consistent
  when it is the only thing that implicitly turns on dependencies and
  for everything else one still has to look up what the dependencies are.

- Most of the problems with circular dependencies come from mixing
  the two methods, and most of the cases where they have caused
  problems in the past involve fbdev in some way.

I also doubt switching lots of 'depends on' to 'select' all over Kconfig
would improve the situation on a global level. It would simplify the
problem of turning something on without understanding the what it
does, but in turn it makes it harder to turn off something else.

E.g. today it is hard to turn off fbdev because that is selected by a
number of (partly unrelated) options, but there was a recent discussion
about getting distros to stop enabling fbdev out of security concerns.

> I'm really all for switching to using depends when that is the
> semantically right thing to do. In many places using select is a hack to
> make the UI simpler, and that's just plain wrong. We'll be doomed to
> perpetual randconfig build failures and duct tape fixes.
>
> I'm pretty tired of this, and I regularly ignore those duct tape fixes
> to i915 backlight build issues on some bizarre configs that nobody will
> ever use, and would not exist if depends were used throughout.
>
> I'm fine with select but only when it's restricted to symbols that have
> no dependencies of their own and have no UI. This is in line with
> Documentation/kbuild/kconfig-language.rst. Not enforcing this is another
> Kconfig tool shortcoming.

Agreed, that is generally a good rule.

      Arnd
Daniel Vetter April 21, 2020, 12:27 p.m. UTC | #5
On Mon, Apr 20, 2020 at 04:03:23PM +0200, Arnd Bergmann wrote:
> On Mon, Apr 20, 2020 at 10:14 AM Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
> > On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
> > >> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
> > >> >
> > >> > If we can agree on these changes, maybe someone can merge them
> > >> > through the drm-misc tree.
> > >> >
> > >> > Please review
> > >>
> > >> Biggest concern I have is that usability of make menuconfig is horrible,
> 
> No doubt about that, but that seems to be unrelated to the cleanup.
> 
> > >> and it's very hard to find options that are hidden by depends on. You can
> > >> use the search interface, if you happen to know the option.
> > >>
> > >> Once you've surmounted that bar, the next one is trying to find what
> > >> exactly you need to enable. Which again means endless of recursive
> > >> screaming at Kconfig files, since make menuconfig doesn't help you at all.
> 
> The changes I'm doing are mostly for fbdev, which is currently the
> odd one out. Most kernel subsystems today follow the documented
> recommendations and only use 'depends on' for things they
> depend on.
> 
> Having fbdev be the exception causes two problems:
> 
> - It does not make kconfig any easier to use overall, just less consistent
>   when it is the only thing that implicitly turns on dependencies and
>   for everything else one still has to look up what the dependencies are.
> 
> - Most of the problems with circular dependencies come from mixing
>   the two methods, and most of the cases where they have caused
>   problems in the past involve fbdev in some way.
> 
> I also doubt switching lots of 'depends on' to 'select' all over Kconfig
> would improve the situation on a global level. It would simplify the
> problem of turning something on without understanding the what it
> does, but in turn it makes it harder to turn off something else.
> 
> E.g. today it is hard to turn off fbdev because that is selected by a
> number of (partly unrelated) options, but there was a recent discussion
> about getting distros to stop enabling fbdev out of security concerns.

I've done some history digging, this is the patch that started this all:

commit d2f59357700487a8b944f4f7777d1e97cf5ea2ed
Author: Ingo Molnar <mingo@elte.hu>
Date:   Thu Feb 5 16:03:34 2009 +0100

    drm/i915: select framebuffer support automatically

I.e. driver gets disabled because a new config is added which isn't
enabled. System doesn't boot, maintainer gets angry regression report,
select hack gets added.

Note on the specific example the code has been reworked enough that even
if you'd upgrade the kernel all that would get disabled now is the fbdev
emulation on top of drm drivers, not any of the drm drivers.

The above says we should have an automatic system for at least oldconfig
(but would be nice in menuconfig too), since "break user's kernel on
upgrade" isn't an option. And without that select is going to come back
somewhere and make a huge nasty mess: We're definitely not going to
fix Kconfig when fixing a regression in -rc kernels.

So in theory no need to convince me that select is terrible. Practice
disagrees unfortunately.
-Daniel

> 
> > I'm really all for switching to using depends when that is the
> > semantically right thing to do. In many places using select is a hack to
> > make the UI simpler, and that's just plain wrong. We'll be doomed to
> > perpetual randconfig build failures and duct tape fixes.
> >
> > I'm pretty tired of this, and I regularly ignore those duct tape fixes
> > to i915 backlight build issues on some bizarre configs that nobody will
> > ever use, and would not exist if depends were used throughout.
> >
> > I'm fine with select but only when it's restricted to symbols that have
> > no dependencies of their own and have no UI. This is in line with
> > Documentation/kbuild/kconfig-language.rst. Not enforcing this is another
> > Kconfig tool shortcoming.
> 
> Agreed, that is generally a good rule.
> 
>       Arnd
Jani Nikula April 21, 2020, 12:58 p.m. UTC | #6
On Tue, 21 Apr 2020, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Apr 20, 2020 at 04:03:23PM +0200, Arnd Bergmann wrote:
>> On Mon, Apr 20, 2020 at 10:14 AM Jani Nikula
>> <jani.nikula@linux.intel.com> wrote:
>> > On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>> > > On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
>> > >> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
>> > >> >
>> > >> > If we can agree on these changes, maybe someone can merge them
>> > >> > through the drm-misc tree.
>> > >> >
>> > >> > Please review
>> > >>
>> > >> Biggest concern I have is that usability of make menuconfig is horrible,
>> 
>> No doubt about that, but that seems to be unrelated to the cleanup.
>> 
>> > >> and it's very hard to find options that are hidden by depends on. You can
>> > >> use the search interface, if you happen to know the option.
>> > >>
>> > >> Once you've surmounted that bar, the next one is trying to find what
>> > >> exactly you need to enable. Which again means endless of recursive
>> > >> screaming at Kconfig files, since make menuconfig doesn't help you at all.
>> 
>> The changes I'm doing are mostly for fbdev, which is currently the
>> odd one out. Most kernel subsystems today follow the documented
>> recommendations and only use 'depends on' for things they
>> depend on.
>> 
>> Having fbdev be the exception causes two problems:
>> 
>> - It does not make kconfig any easier to use overall, just less consistent
>>   when it is the only thing that implicitly turns on dependencies and
>>   for everything else one still has to look up what the dependencies are.
>> 
>> - Most of the problems with circular dependencies come from mixing
>>   the two methods, and most of the cases where they have caused
>>   problems in the past involve fbdev in some way.
>> 
>> I also doubt switching lots of 'depends on' to 'select' all over Kconfig
>> would improve the situation on a global level. It would simplify the
>> problem of turning something on without understanding the what it
>> does, but in turn it makes it harder to turn off something else.
>> 
>> E.g. today it is hard to turn off fbdev because that is selected by a
>> number of (partly unrelated) options, but there was a recent discussion
>> about getting distros to stop enabling fbdev out of security concerns.
>
> I've done some history digging, this is the patch that started this all:
>
> commit d2f59357700487a8b944f4f7777d1e97cf5ea2ed
> Author: Ingo Molnar <mingo@elte.hu>
> Date:   Thu Feb 5 16:03:34 2009 +0100
>
>     drm/i915: select framebuffer support automatically
>
> I.e. driver gets disabled because a new config is added which isn't
> enabled. System doesn't boot, maintainer gets angry regression report,
> select hack gets added.

Gotta love a good commit message from a decade ago.

First, it says it's a migration helper. And that the problem
specifically is that the user has a working config *without* FB enabled
as a starting point.

Now, if the starting point for a new config *now* is less than ten years
old, and it had i915 enabled, it'll also have FB enabled. Because
select. The migration part has done its job, and I think we should be
good to make some progress.

The commit message also notes the problems of select.

BR,
Jani.


> Note on the specific example the code has been reworked enough that even
> if you'd upgrade the kernel all that would get disabled now is the fbdev
> emulation on top of drm drivers, not any of the drm drivers.
>
> The above says we should have an automatic system for at least oldconfig
> (but would be nice in menuconfig too), since "break user's kernel on
> upgrade" isn't an option. And without that select is going to come back
> somewhere and make a huge nasty mess: We're definitely not going to
> fix Kconfig when fixing a regression in -rc kernels.
>
> So in theory no need to convince me that select is terrible. Practice
> disagrees unfortunately.
> -Daniel
>
>> 
>> > I'm really all for switching to using depends when that is the
>> > semantically right thing to do. In many places using select is a hack to
>> > make the UI simpler, and that's just plain wrong. We'll be doomed to
>> > perpetual randconfig build failures and duct tape fixes.
>> >
>> > I'm pretty tired of this, and I regularly ignore those duct tape fixes
>> > to i915 backlight build issues on some bizarre configs that nobody will
>> > ever use, and would not exist if depends were used throughout.
>> >
>> > I'm fine with select but only when it's restricted to symbols that have
>> > no dependencies of their own and have no UI. This is in line with
>> > Documentation/kbuild/kconfig-language.rst. Not enforcing this is another
>> > Kconfig tool shortcoming.
>> 
>> Agreed, that is generally a good rule.
>> 
>>       Arnd
Geert Uytterhoeven April 21, 2020, 1:05 p.m. UTC | #7
Hi Jani,

On Tue, Apr 21, 2020 at 2:58 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 21 Apr 2020, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Apr 20, 2020 at 04:03:23PM +0200, Arnd Bergmann wrote:
> >> On Mon, Apr 20, 2020 at 10:14 AM Jani Nikula
> >> <jani.nikula@linux.intel.com> wrote:
> >> > On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >> > > On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
> >> > >> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
> >> > >> >
> >> > >> > If we can agree on these changes, maybe someone can merge them
> >> > >> > through the drm-misc tree.
> >> > >> >
> >> > >> > Please review
> >> > >>
> >> > >> Biggest concern I have is that usability of make menuconfig is horrible,
> >>
> >> No doubt about that, but that seems to be unrelated to the cleanup.
> >>
> >> > >> and it's very hard to find options that are hidden by depends on. You can
> >> > >> use the search interface, if you happen to know the option.
> >> > >>
> >> > >> Once you've surmounted that bar, the next one is trying to find what
> >> > >> exactly you need to enable. Which again means endless of recursive
> >> > >> screaming at Kconfig files, since make menuconfig doesn't help you at all.
> >>
> >> The changes I'm doing are mostly for fbdev, which is currently the
> >> odd one out. Most kernel subsystems today follow the documented
> >> recommendations and only use 'depends on' for things they
> >> depend on.
> >>
> >> Having fbdev be the exception causes two problems:
> >>
> >> - It does not make kconfig any easier to use overall, just less consistent
> >>   when it is the only thing that implicitly turns on dependencies and
> >>   for everything else one still has to look up what the dependencies are.
> >>
> >> - Most of the problems with circular dependencies come from mixing
> >>   the two methods, and most of the cases where they have caused
> >>   problems in the past involve fbdev in some way.
> >>
> >> I also doubt switching lots of 'depends on' to 'select' all over Kconfig
> >> would improve the situation on a global level. It would simplify the
> >> problem of turning something on without understanding the what it
> >> does, but in turn it makes it harder to turn off something else.
> >>
> >> E.g. today it is hard to turn off fbdev because that is selected by a
> >> number of (partly unrelated) options, but there was a recent discussion
> >> about getting distros to stop enabling fbdev out of security concerns.
> >
> > I've done some history digging, this is the patch that started this all:
> >
> > commit d2f59357700487a8b944f4f7777d1e97cf5ea2ed
> > Author: Ingo Molnar <mingo@elte.hu>
> > Date:   Thu Feb 5 16:03:34 2009 +0100
> >
> >     drm/i915: select framebuffer support automatically
> >
> > I.e. driver gets disabled because a new config is added which isn't
> > enabled. System doesn't boot, maintainer gets angry regression report,
> > select hack gets added.
>
> Gotta love a good commit message from a decade ago.
>
> First, it says it's a migration helper. And that the problem
> specifically is that the user has a working config *without* FB enabled
> as a starting point.
>
> Now, if the starting point for a new config *now* is less than ten years
> old, and it had i915 enabled, it'll also have FB enabled. Because
> select. The migration part has done its job, and I think we should be
> good to make some progress.

It will indeed work with "make oldconfig", as an old config with
CONFIG_DRM_I915 enabled will have CONFIG_FB set.

However, that is not true when starting with a defconfig that has
CONFIG_DRM_I915 enabled: such a defconfig will not have CONFIG_FB set,
due to the trimming process when creating a minimal defconfig.

Hence when making the change from "select FB" to "depends on FB", you
have to make sure to update the affected defconfigs, too:

$ git grep CONFIG_DRM_I915 -- "*defconfig*"
arch/x86/configs/i386_defconfig:CONFIG_DRM_I915=y
arch/x86/configs/x86_64_defconfig:CONFIG_DRM_I915=y

Gr{oetje,eeting}s,

                        Geert
Daniel Vetter April 21, 2020, 1:10 p.m. UTC | #8
On Tue, Apr 21, 2020 at 3:05 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Jani,
>
> On Tue, Apr 21, 2020 at 2:58 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Tue, 21 Apr 2020, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Mon, Apr 20, 2020 at 04:03:23PM +0200, Arnd Bergmann wrote:
> > >> On Mon, Apr 20, 2020 at 10:14 AM Jani Nikula
> > >> <jani.nikula@linux.intel.com> wrote:
> > >> > On Fri, 17 Apr 2020, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >> > > On Fri, Apr 17, 2020 at 07:14:53PM +0200, Daniel Vetter wrote:
> > >> > >> On Fri, Apr 17, 2020 at 05:55:45PM +0200, Arnd Bergmann wrote:
> > >> > >> >
> > >> > >> > If we can agree on these changes, maybe someone can merge them
> > >> > >> > through the drm-misc tree.
> > >> > >> >
> > >> > >> > Please review
> > >> > >>
> > >> > >> Biggest concern I have is that usability of make menuconfig is horrible,
> > >>
> > >> No doubt about that, but that seems to be unrelated to the cleanup.
> > >>
> > >> > >> and it's very hard to find options that are hidden by depends on. You can
> > >> > >> use the search interface, if you happen to know the option.
> > >> > >>
> > >> > >> Once you've surmounted that bar, the next one is trying to find what
> > >> > >> exactly you need to enable. Which again means endless of recursive
> > >> > >> screaming at Kconfig files, since make menuconfig doesn't help you at all.
> > >>
> > >> The changes I'm doing are mostly for fbdev, which is currently the
> > >> odd one out. Most kernel subsystems today follow the documented
> > >> recommendations and only use 'depends on' for things they
> > >> depend on.
> > >>
> > >> Having fbdev be the exception causes two problems:
> > >>
> > >> - It does not make kconfig any easier to use overall, just less consistent
> > >>   when it is the only thing that implicitly turns on dependencies and
> > >>   for everything else one still has to look up what the dependencies are.
> > >>
> > >> - Most of the problems with circular dependencies come from mixing
> > >>   the two methods, and most of the cases where they have caused
> > >>   problems in the past involve fbdev in some way.
> > >>
> > >> I also doubt switching lots of 'depends on' to 'select' all over Kconfig
> > >> would improve the situation on a global level. It would simplify the
> > >> problem of turning something on without understanding the what it
> > >> does, but in turn it makes it harder to turn off something else.
> > >>
> > >> E.g. today it is hard to turn off fbdev because that is selected by a
> > >> number of (partly unrelated) options, but there was a recent discussion
> > >> about getting distros to stop enabling fbdev out of security concerns.
> > >
> > > I've done some history digging, this is the patch that started this all:
> > >
> > > commit d2f59357700487a8b944f4f7777d1e97cf5ea2ed
> > > Author: Ingo Molnar <mingo@elte.hu>
> > > Date:   Thu Feb 5 16:03:34 2009 +0100
> > >
> > >     drm/i915: select framebuffer support automatically
> > >
> > > I.e. driver gets disabled because a new config is added which isn't
> > > enabled. System doesn't boot, maintainer gets angry regression report,
> > > select hack gets added.
> >
> > Gotta love a good commit message from a decade ago.
> >
> > First, it says it's a migration helper. And that the problem
> > specifically is that the user has a working config *without* FB enabled
> > as a starting point.
> >
> > Now, if the starting point for a new config *now* is less than ten years
> > old, and it had i915 enabled, it'll also have FB enabled. Because
> > select. The migration part has done its job, and I think we should be
> > good to make some progress.
>
> It will indeed work with "make oldconfig", as an old config with
> CONFIG_DRM_I915 enabled will have CONFIG_FB set.
>
> However, that is not true when starting with a defconfig that has
> CONFIG_DRM_I915 enabled: such a defconfig will not have CONFIG_FB set,
> due to the trimming process when creating a minimal defconfig.
>
> Hence when making the change from "select FB" to "depends on FB", you
> have to make sure to update the affected defconfigs, too:
>
> $ git grep CONFIG_DRM_I915 -- "*defconfig*"
> arch/x86/configs/i386_defconfig:CONFIG_DRM_I915=y
> arch/x86/configs/x86_64_defconfig:CONFIG_DRM_I915=y

To clarify what I was aiming for with my mail: I'm not worried about
fbdev here, I'm just worried that this will come back, and we'll grow
select somewhere else until it's become a big & totally horrible mess.
I think a lot of the backlight selects have also grown because of
this, so this isn't just a one-off I think.

If Arnd is happy to play "Kconfig select" whack-a-mole ever once in a
while (and deal with the intermediate compile horrors while everyone
upgrades) I'm ok with this landing. Just not terribly happy if the
underlying issue isn't fixed.
-Daniel

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
Jani Nikula April 21, 2020, 1:25 p.m. UTC | #9
On Tue, 21 Apr 2020, Daniel Vetter <daniel@ffwll.ch> wrote:
> To clarify what I was aiming for with my mail: I'm not worried about
> fbdev here, I'm just worried that this will come back, and we'll grow
> select somewhere else until it's become a big & totally horrible mess.
> I think a lot of the backlight selects have also grown because of
> this, so this isn't just a one-off I think.
>
> If Arnd is happy to play "Kconfig select" whack-a-mole ever once in a
> while (and deal with the intermediate compile horrors while everyone
> upgrades) I'm ok with this landing. Just not terribly happy if the
> underlying issue isn't fixed.

And I'll keep ignoring the IS_REACHABLE() patches that make i915 build
with a config that should not exist. ;)

BR,
Jani.