Message-ID: <20150522185207.GG40101@dtor-ws>
Date:	Fri, 22 May 2015 11:52:07 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	"Luis R. Rodriguez" <mcgrof@...e.com>
Cc:	Takashi Iwai <tiwai@...e.de>, Paul Bolle <pebolle@...cali.nl>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Borislav Petkov <bp@...en8.de>,
	Greg KH <gregkh@...uxfoundation.org>,
	"David S. Miller" <davem@...emloft.net>, clemens@...isch.de,
	JBottomley@...n.com, David Airlie <airlied@...ux.ie>,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Marcel Holtmann <marcel@...tmann.org>,
	"Gustavo F. Padovan" <gustavo@...ovan.org>,
	Johan Hedberg <johan.hedberg@...il.com>,
	Mikael Starvik <starvik@...s.com>,
	Jesper Nilsson <jesper.nilsson@...s.com>,
	Imre Kaloz <kaloz@...nwrt.org>, khalasa@...p.pl,
	Ohad Ben-Cohen <ohad@...ery.com>,
	Arnd Bergmann <arnd@...db.de>, 3chas3@...il.com,
	Jiri Slaby <jslaby@...e.cz>, Bryan Wu <cooloney@...il.com>,
	Richard Purdie <rpurdie@...ys.net>,
	Jacek Anaszewski <j.anaszewski@...sung.com>,
	mcgrof@...not-panic.com,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 08:19:24PM +0200, Luis R. Rodriguez wrote:
> On Fri, May 22, 2015 at 10:57:11AM -0700, Dmitry Torokhov wrote:
> > On Fri, May 22, 2015 at 10:43:12AM -0700, Luis R. Rodriguez wrote:
> > > On Fri, May 22, 2015 at 1:44 AM, Takashi Iwai <tiwai@...e.de> wrote:
> > > > At Fri, 22 May 2015 10:17:48 +0200,
> > > > Paul Bolle wrote:
> > > >>
> > > >> On Fri, 2015-05-22 at 09:11 +0200, Geert Uytterhoeven wrote:
> > > >> > On Fri, May 22, 2015 at 8:53 AM, Borislav Petkov <bp@...en8.de> wrote:
> > > >> > > One thing I forgot last night: what about randconfigs? All that
> > > >> > > functionality which selects FW_LOADER, won't boot anymore, right? I
> > > >> > > mean, there are provisions to build fine even with FW_LOADER unset but
> > > >> > > if you want to boot-test those kernels, you will artificially fail due
> > > >> > > to missing request_firmware* things...
> > > >>
> > > >> Luis also tried to explain to me that disabling FW_LOADER shouldn't make
> > > >> the build fail. (And, of course, we could decide to not care about
> > > >> randconfig builds that have EXPERT set. Maybe we could even special case
> > > >> EXPERT in randconfig. But that would make randconfig builds less useful.
> > > >> That's a separate issue, anyhow.)
> > > >
> > > > But FW_LOADER is a tristate, so it might be inconsistent if selected
> > > > randomly?  Luis' patch doesn't add depends but just removes select.
> > > 
> > > We could go both ways, either remove the "select" or replace it with
> > > "depends on". As you note keeping the "depends on" ensures run time
> > > sanity for the possible tristate mismatches, but this is an EXPERT
> > > concern. The crux of what option to go with is:
> > > 
> > >   Should we concern ourselves with run time configuration issues when
> > > folks enable EXPERT?
> > 
> > Yes.
> > 
> > dtor@...r-ws:~/kernel/master$ grep -r CONFIG_EXPERT /boot/config*
> > /boot/config-3.13.0-49-generic:CONFIG_EXPERT=y
> > /boot/config-3.13.0-52-generic:CONFIG_EXPERT=y
> > 
> > This is distro config and that is what many people use as a base for
> > their own configs.
> 
> Smells Ubuntu'ish, and hence ChromeOS'ish :)

Yes, this coming from Ubuntu, but ChromeOS is closer to Gentoo if
anything I'd say ;)

> 
> Either way SUSE kernels also have EXPERT=y as well.  Would not be surprised if
> other major distributions also have EXPERT=y.

Right, I believe Fedora has it set as well.

> 
> OK so we obviously care about EXPERT run time issues then, seems to kind
> of defeat the purpose of EXPERT though, no? Makes me wonder what major options
> are under EXPERT which most distros need.

Not major, rather tweaking the driver sets. Like expert V4L or HID
configs.

> Also, are there no other runtime
> configuration options tucked under EXPERT that we *do not* resolve right now?
> Or should all be taken care of? If not then we are being inconsistent.
> Both of these are side topics -- but since I have a feeling this might come
> up again, it may be worth evaluation.
> 
> Following on topic: such distro configs would never have FW_LOADER as n or
> m, though right? Is that sufficient for us to drop the select and not apply
> the "depends on" replacement ? Or do we want to stick to the "depend on"?

But the user who is not expecrt might drop it. The premise was "only
experts would have CONFIG_EXPERT and thus we do not care about
breakage". But if people use distro configs as a starting point they all
are "experts".

> 
> Note that not changing this means we *will* eventually run into the
> recursive dependency issue later, either with my FW API change patches
> or some other future feature. Likewise for any other kconfig option
> with similar semantics.
> 
> > > Without EXPERT all run time configurations are vetted to run as
> > > FW_LOADER defaults to y. If we go down the path of removing the select
> > > completely we'd be taking a position that we could at least ensure
> > > EXPERT builds will work, but we cannot vet for not run time sanity of
> > > such build. I favor simplicity so would prefer to nuke the select
> > > completely and if we're really concerned about EXPERT users tristate
> > > mismatch misconfiguration why not just replace tristate with bool for
> > > FW_LOADER. That would do us the service of simplifying that code a
> > > bit, and leave only in place one way for folks that enable EXPERT to
> > > shoot themselves in the foot with FW_LOADER?
> > 
> > I am afraid that we are moving into wrong direction here. Why don't we
> > look into Kconfig to teach it the difference between forced selection
> > and dependency instead?
> 
> That was what I originally hinted we should try to do. See the implications
> of the issue right now and my explanation of a whitelist [0], granted this
> is just a description, I tried looking at the code and quickly gave up,
> it was not clear how such a thing could be implemented :( I gave up after
> Paul assured me this could not be an issue, or rather that the logic on
> kconfig was correct.
> 
> FWIW, I think my original description of implications if this issue is not a
> real bug is a bit more clearer than the ones me and Paul ended up reviewing.
> The examples me and Paul reviewed are examples one can experiment if trying
> to address and fix the issue at hand in the most simplest cases.

So I see your GYM and LOCKER example, or better, the original FW &
CRYPTO warning and I wonder why do we actually go and check dependencies
of selected symbols when resolving given symbol. I.e. should we not
"cut" the select branches off and evaluate them on their own? I.e. when
we try to select a symbol we note the symbols it tries to select and
evaluate them on their own (selects still don't cascade down - right?)
and then evaluate "depends on" symbols?

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists