LinuxLists.cc - [RFC v1] tree-wide: remove "select FW

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Thu, May 21, 2015 at 01:45:49PM -0700, Luis R. Rodriguez wrote:
> The above history is just *one* set of reasons to remove all kernel
> "select FW_LOADER" entries. Another simple reason is that since
> FW_LOADER can only be disabled by an EXPERT and it defaults to y
> its now a superfluous select. We will ensure things build for you
> if using EXPERT but if you have selected EXPERT you should know
> what run time requirements you need.

Couple of notes only:

So the "depends" thing is a step backwards IMO because people would have
to go and enable FW_LOADER first. For the microcode loader, at least,
"select" is perfectly fine.

There's also the other aspect that this FW_LOADER thing ends up enabled
on 99.9% (!?) of the distro installations anyway, right? I.e., there's
always something needing firmware from userspace.

So the current "default y" behind EXPERT is probably the correct thing
anyway.

So yeah, send me the x86 microcode loader patch, I'll take it.

Thanks.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

2015-05-22 06:53:51

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 12:21:29AM +0200, Borislav Petkov wrote:
> On Thu, May 21, 2015 at 01:45:49PM -0700, Luis R. Rodriguez wrote:
> > The above history is just *one* set of reasons to remove all kernel
> > "select FW_LOADER" entries. Another simple reason is that since
> > FW_LOADER can only be disabled by an EXPERT and it defaults to y
> > its now a superfluous select. We will ensure things build for you
> > if using EXPERT but if you have selected EXPERT you should know
> > what run time requirements you need.
>
> Couple of notes only:

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...

Yap, just triggered one:

$ make randconfig; grep -E "(MICROCODE|FW_LOADER)" .config
scripts/kconfig/conf --randconfig Kconfig
KCONFIG_SEED=0xE14FBE97
warning: (USB_OTG_FSM && FSL_USB2_OTG && USB_MV_OTG) selects USB_OTG which has unmet direct dependencies (USB_SUPPORT && USB && PM)
warning: (USB_OTG_FSM && FSL_USB2_OTG && USB_MV_OTG) selects USB_OTG which has unmet direct dependencies (USB_SUPPORT && USB && PM)
warning: (SND_SOC_INTEL_BYTCR_RT5640_MACH && SND_SOC_INTEL_CHT_BSW_RT5672_MACH && SND_SOC_INTEL_CHT_BSW_RT5645_MACH) selects SND_SST_IPC_ACPI which has unmet direct dependencies (SOUND && !M68K && !UML && SND && SND_SOC && ACPI)
#
# configuration written to .config
#
CONFIG_MICROCODE=m
CONFIG_MICROCODE_INTEL=y
CONFIG_MICROCODE_AMD=y
CONFIG_MICROCODE_OLD_INTERFACE=y
# CONFIG_FW_LOADER is not set

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

2015-05-22 07:11:38

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

Hi Boris,

On Fri, May 22, 2015 at 8:53 AM, Borislav Petkov <[email protected]> wrote:
> On Fri, May 22, 2015 at 12:21:29AM +0200, Borislav Petkov wrote:
>> On Thu, May 21, 2015 at 01:45:49PM -0700, Luis R. Rodriguez wrote:
>> > The above history is just *one* set of reasons to remove all kernel
>> > "select FW_LOADER" entries. Another simple reason is that since
>> > FW_LOADER can only be disabled by an EXPERT and it defaults to y
>> > its now a superfluous select. We will ensure things build for you
>> > if using EXPERT but if you have selected EXPERT you should know
>> > what run time requirements you need.
>>
>> Couple of notes only:

> So the "depends" thing is a step backwards IMO because people would have
> to go and enable FW_LOADER first. For the microcode loader, at least,
> "select" is perfectly fine.

That's what the EXPERT protection is for.
FW_LOADER will always be enabled, unless people think they're smarter
than they are, and enable EXPERT, and disable FW_LOADER.

> 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...

If you expect to _boot_ randconfig kernels, you better disable EXPERT.
Still, your kernel can fail to boot due to some other critical dependency
(does your kernel support your CPU?).

If you want to boot randconfig kernels, create an allrandomconfig file
listing all options that must have a certain value:

$ cat allrandomconfig
CONFIG_EXPERT=n
CONFIG_MY_CRITICAL_FEATURE=y
...
$

and use "make randconfig KCONFIG_ALLCONFIG=1".

> ECO tip #101: Trim your mails when you reply.

You trimmed too much, so I had to readd it manually ;-)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2015-05-22 07:41:46

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 09:11:34AM +0200, Geert Uytterhoeven wrote:
> That's what the EXPERT protection is for.
> FW_LOADER will always be enabled, unless people think they're smarter
> than they are, and enable EXPERT, and disable FW_LOADER.

I know. I was just making the negative aspect of "depends" vs "select"
in that particular case, explicit. Just so that it is mentioned in the
conversation.

> If you want to boot randconfig kernels, create an allrandomconfig file
> listing all options that must have a certain value:
>
> $ cat allrandomconfig
> CONFIG_EXPERT=n
> CONFIG_MY_CRITICAL_FEATURE=y
> ...
> $
>
> and use "make randconfig KCONFIG_ALLCONFIG=1".

I do that but does everyone who builds randconfigs? I had to go and look
what variables Kbuild honors and prep an all.config and yadda yadda...

So maybe we should put that as a note somewhere for randconfig-building
people.

> You trimmed too much, so I had to readd it manually ;-)

Yeah, that's what mail threads are for.

People tend to reply to a huuge mail with one sentence, bury it
somewhere on page gazillion and one and not trim the rest. Antisocial
behavior, that. :-P

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

2015-05-22 07:45:49

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 9:41 AM, Borislav Petkov <[email protected]> wrote:
>> If you want to boot randconfig kernels, create an allrandomconfig file
>> listing all options that must have a certain value:
>>
>> $ cat allrandomconfig
>> CONFIG_EXPERT=n
>> CONFIG_MY_CRITICAL_FEATURE=y
>> ...
>> $
>>
>> and use "make randconfig KCONFIG_ALLCONFIG=1".
>
> I do that but does everyone who builds randconfigs? I had to go and look
> what variables Kbuild honors and prep an all.config and yadda yadda...

The ones who just build them probably not (unless they care about a specific
case), which increases build coverage, so that's good.

The ones who boot them probably do, else they'll find out the hard way...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2015-05-22 07:56:42

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 09:45:46AM +0200, Geert Uytterhoeven wrote:
> The ones who just build them probably not (unless they care about a
> specific case), which increases build coverage, so that's good.

Right.

> The ones who boot them probably do, else they'll find out the hard
> way...

Right, and this is what I wanted to avoid or at least save people the
trouble. Maybe randconfig builders should be expert enough to be able to
figure that out on their own.

:-)

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

2015-05-22 08:17:59

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, 2015-05-22 at 09:11 +0200, Geert Uytterhoeven wrote:
> On Fri, May 22, 2015 at 8:53 AM, Borislav Petkov <[email protected]> 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.)

> If you expect to _boot_ randconfig kernels, you better disable EXPERT.
> Still, your kernel can fail to boot due to some other critical dependency
> (does your kernel support your CPU?).
>
> If you want to boot randconfig kernels, create an allrandomconfig file
> listing all options that must have a certain value:
>
> $ cat allrandomconfig
> CONFIG_EXPERT=n
> CONFIG_MY_CRITICAL_FEATURE=y
> ...
> $
>
> and use "make randconfig KCONFIG_ALLCONFIG=1".

Booting (truly) randconfig builds is like booting allyesconfig or
allnoconfig builds: no one sane should do that, right?

Paul Bolle

2015-05-22 08:24:56

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 10:17 AM, Paul Bolle <[email protected]> wrote:
> Booting (truly) randconfig builds is like booting allyesconfig or
> allnoconfig builds: no one sane should do that, right?

allyesconfig should boot, modulo size limitations in boot loaders and
early kernel setup.

(No, commit 486df8bc4627bdfc ("m68k: Increase initial mapping to 8 or 16 MiB
if possible") is not good enough ;-)

$ size linux-m68k-allyesconfig/vmlinux
text data bss dec hex filename
42540590 2438292 1645336 46624218 2c76dda linux-m68k-allyesconfig/vmlinux
$

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2015-05-22 08:26:59

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 10:17:48AM +0200, Paul Bolle wrote:
> Luis also tried to explain to me that disabling FW_LOADER shouldn't
> make the build fail.

It doesn't. There are #else provisions in the header which define empty
functions returning -EINVAL and so on.

> Booting (truly) randconfig builds is like booting allyesconfig or
> allnoconfig builds: no one sane should do that, right?

Why not? It doesn't hurt to try to boot them in a guest. It sometimes
triggers valid issues.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

2015-05-22 08:32:48

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, 2015-05-22 at 08:53 +0200, Borislav Petkov 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...
>
> Yap, just triggered one:
>
> $ make randconfig; grep -E "(MICROCODE|FW_LOADER)" .config
> scripts/kconfig/conf --randconfig Kconfig
> KCONFIG_SEED=0xE14FBE97
> warning: (USB_OTG_FSM && FSL_USB2_OTG && USB_MV_OTG) selects USB_OTG which has unmet direct dependencies (USB_SUPPORT && USB && PM)
> warning: (USB_OTG_FSM && FSL_USB2_OTG && USB_MV_OTG) selects USB_OTG which has unmet direct dependencies (USB_SUPPORT && USB && PM)
> warning: (SND_SOC_INTEL_BYTCR_RT5640_MACH && SND_SOC_INTEL_CHT_BSW_RT5672_MACH && SND_SOC_INTEL_CHT_BSW_RT5645_MACH) selects SND_SST_IPC_ACPI which has unmet direct dependencies (SOUND && !M68K && !UML && SND && SND_SOC && ACPI)

Those missing dependencies look unrelated to not having FW_LOADER set.
Did I miss something here?

Thanks,

Paul Bolle

2015-05-22 08:44:18

by Takashi Iwai

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

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 <[email protected]> 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.

Takashi

2015-05-22 09:06:34

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 10:32:39AM +0200, Paul Bolle wrote:
> Those missing dependencies look unrelated to not having FW_LOADER set.
> Did I miss something here?

I just pasted what appeared on the console here. You can ignore them.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

2015-05-22 09:06:50

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, 2015-05-22 at 10:44 +0200, Takashi Iwai wrote:
> At Fri, 22 May 2015 10:17:48 +0200,
> Paul Bolle wrote:
> > 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.

include/linux/firmware.h contains:
#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE)
int request_firmware(const struct firmware **fw, const char *name,
struct device *device);
[...]
#else
static inline int request_firmware(const struct firmware **fw,
const char *name,
struct device *device)
{
return -EINVAL;
}
[...]
#endif

So I _think_ the build should be fine.

But, in case of built-in users of request_firmware() and friends,
actually using that build might not go as expected. But if you set
EXPERT and disable FW_LOADER, or as you point out, set it to 'm', you
own the pieces when things break, don't you?

But now I'm doing Luis' job. I didn't sign off on that patch! So let's
hope Luis thought of all the corner cases.

Paul Bolle

2015-05-22 09:20:24

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Thu, 2015-05-21 at 13:45 -0700, Luis R. Rodriguez wrote:
> Paul also insists that *there is no reason* to think that current logic
> of the kconfig tools, as it is currently implemented, is flawed [2].

> [1] https://lkml.org/lkml/2015/5/8/748

(It should be noted that Russell King taught me the approach I took
there. Before Russell did that I found dependency errors involving
recursive selects, ie most of them, rather baffling.

Any flaws in that message are entirely mine, of course.)

> [2] https://lkml.org/lkml/2015/5/18/835

"insist" is pushing things a bit too far. "claims" or "stated"?

Paul Bolle

2015-05-22 09:56:44

by Takashi Iwai

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

At Fri, 22 May 2015 11:06:37 +0200,
Paul Bolle wrote:
>
> On Fri, 2015-05-22 at 10:44 +0200, Takashi Iwai wrote:
> > At Fri, 22 May 2015 10:17:48 +0200,
> > Paul Bolle wrote:
> > > 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.
>
> include/linux/firmware.h contains:
> #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE)
> int request_firmware(const struct firmware **fw, const char *name,
> struct device *device);
> [...]
> #else
> static inline int request_firmware(const struct firmware **fw,
> const char *name,
> struct device *device)
> {
> return -EINVAL;
> }
> [...]
> #endif
>
> So I _think_ the build should be fine.

Ah, OK, that would work.

> But, in case of built-in users of request_firmware() and friends,
> actually using that build might not go as expected. But if you set
> EXPERT and disable FW_LOADER, or as you point out, set it to 'm', you
> own the pieces when things break, don't you?

Yeah, I'm not against it, too. My concern is only about the silent
breakage.

Thinking of this again, I also concluded that removing the mostly
superfluous "select FW_LOADER" would make things easier in the end.

> But now I'm doing Luis' job. I didn't sign off on that patch! So let's
> hope Luis thought of all the corner cases.

Heh, let's hope we all reach to a happy end.

thanks,

Takashi

2015-05-22 17:43:34

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 1:44 AM, Takashi Iwai <[email protected]> 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 <[email protected]> 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?

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?

Luis

2015-05-22 17:57:21

by Dmitry Torokhov

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

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 <[email protected]> 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 <[email protected]> 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@dtor-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.

>
> 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?

Thanks.

--
Dmitry

2015-05-22 18:21:35

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

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 <[email protected]> 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 <[email protected]> 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@dtor-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 :)

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

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. 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"?

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.

[0] https://lkml.org/lkml/2015/5/12/597

Luis

2015-05-22 18:52:15

by Dmitry Torokhov

[permalink] [raw]

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 <[email protected]> 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 <[email protected]> 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@dtor-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

2015-05-22 19:28:23

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

Adding Josh, as I know he seems to be on an EXPERT crusade to tuck
away system calls for tinyconfig, so likely can chime in for some
insane EXPERT runtime issue that users can run into.

On Fri, May 22, 2015 at 11:52:07AM -0700, Dmitry Torokhov wrote:
> On Fri, May 22, 2015 at 08:19:24PM +0200, Luis R. Rodriguez wrote:
> > 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.

Driver tweaks seems rather harmless compared to "trust me I know
what I'm doing" even for all run time cases.

So the question still stands -- are there any kconfig options which
depend on EXPERT which enable folks to screw up run time (not just driver
tweaks? If so, would it be good to separate the two types of expert
config options, one for tweaks, another for run time possible conflicts?

Reason I ask this is -- IMO we should not treat the possible EXPERT run time
conflicts of FW_LOADER specially and try to be consistent, if we are ensuring
EXPERT run time conflicts will work with FW_LOADER why not for others? If
we want the option to let an EXPERT kconfig option enable run time conflicts
maybe we need to split things up further.

> > 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 expert 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".

Replacing "select FW_LOADER" with "depends on FW_LOADER", although still pegged
with EXPERT, would not allow EXPERT users (most folks we've determined now) to
disable FW_LOADER if a driver was selected that needed FW_LOADER, in fact I
could not also create a tristate conflict of say MICROCODE set to y and
FW_LOADER to m. Please let me know if I've missed something with testing this
though.

So unless I'm missing a corner case, it would seem replacing "select FW_LOADER"
with "depends on FW_LOADER" is a reasonable option so far. The subtleties lie
then now in since EXPERT is used by most distributions, everyone is an expert,
and:

a) Do we want to be consistent about ensuring run time sanity of EXPERT?
b) Should we split EXPERT into tweak / possible run time break options?

> > 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'm no kconfig expert and I wondered the same.

Perhaps Paul can elaborate.

> 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?

2015-05-22 19:30:29

by Austin S Hemmelgarn

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On 2015-05-22 14:52, Dmitry Torokhov wrote:
> 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 <[email protected]> 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 <[email protected]> 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@dtor-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 ;)
>
Technically, ChromeOS _is_ Gentoo, just with a bunch of third party
patches and configuration layered on top. IMHO though, they really do
have a good excuse for enabling CONFIG_EXPERT (namely, they have
absolute control over pretty much the entire system, and run on
specialized hardware).
>>
>> 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.
>
I'm pretty certain that Fedora does, and that Arch does 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.
>
Of the people I know personally who build their own kernels, about 80%
have EXPERT=y for exactly this reason, namely, they want finer control
over driver options, and don't do anything to the options under the
EXPERT menu. Personally this is one of two reasons that I use EXPERT=y,
the other being disabling the handful of deprecated syscalls at the top
of the list (and disabling AIO when I'm building embedded systems).
>> 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".
>
Based on this, maybe the EXPERT stuff should be subdivided into driver
options, and core API options under separate config options.

Attachments:

smime.p7s (2.90 kB)
S/MIME Cryptographic Signature

2015-05-22 20:12:37

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 9:30 PM, Austin S Hemmelgarn
<[email protected]> wrote:
>>> 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.
>>
> I'm pretty certain that Fedora does, and that Arch does as well.

Perhaps they just use allmodconfig, which enables EXPERT ;-)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2015-05-22 20:22:24

by Dmitry Torokhov

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 09:28:14PM +0200, Luis R. Rodriguez wrote:
> Adding Josh, as I know he seems to be on an EXPERT crusade to tuck
> away system calls for tinyconfig, so likely can chime in for some
> insane EXPERT runtime issue that users can run into.
>
> On Fri, May 22, 2015 at 11:52:07AM -0700, Dmitry Torokhov wrote:
> > On Fri, May 22, 2015 at 08:19:24PM +0200, Luis R. Rodriguez wrote:
> > > 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.
>
> Driver tweaks seems rather harmless compared to "trust me I know
> what I'm doing" even for all run time cases.
>
> So the question still stands -- are there any kconfig options which
> depend on EXPERT which enable folks to screw up run time (not just driver
> tweaks? If so, would it be good to separate the two types of expert
> config options, one for tweaks, another for run time possible conflicts?
>
> Reason I ask this is -- IMO we should not treat the possible EXPERT run time
> conflicts of FW_LOADER specially and try to be consistent, if we are ensuring
> EXPERT run time conflicts will work with FW_LOADER why not for others? If
> we want the option to let an EXPERT kconfig option enable run time conflicts
> maybe we need to split things up further.
>
> > > 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 expert 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".
>
> Replacing "select FW_LOADER" with "depends on FW_LOADER", although still pegged
> with EXPERT, would not allow EXPERT users (most folks we've determined now) to
> disable FW_LOADER if a driver was selected that needed FW_LOADER, in fact I
> could not also create a tristate conflict of say MICROCODE set to y and
> FW_LOADER to m. Please let me know if I've missed something with testing this
> though.
>
> So unless I'm missing a corner case, it would seem replacing "select FW_LOADER"
> with "depends on FW_LOADER" is a reasonable option so far. The subtleties lie
> then now in since EXPERT is used by most distributions, everyone is an expert,
> and:
>
> a) Do we want to be consistent about ensuring run time sanity of EXPERT?
> b) Should we split EXPERT into tweak / possible run time break options?
>
> > > 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'm no kconfig expert

Me neither ;)

> and I wondered the same.
>
> Perhaps Paul can elaborate.
>
> > 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?

By the way, to resolve your FW loader issue can you simply select CRYPTO
instead of depending on it? I believe that certain core facilities (such
as FW loaders, Crypto, etc) should be normally selected rather than
depended upon, just because of workflow. If user works with menuconfig
and has core facility disable he won't event be presented with an
option. So one has to perform several "passes" over menuconfig to slect
everything that is needed.

Thanks.

--
Dmitry

2015-05-22 21:44:16

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 09:28:14PM +0200, Luis R. Rodriguez wrote:
> Adding Josh, as I know he seems to be on an EXPERT crusade to tuck
> away system calls for tinyconfig, so likely can chime in for some
> insane EXPERT runtime issue that users can run into.

Thanks.

> On Fri, May 22, 2015 at 11:52:07AM -0700, Dmitry Torokhov wrote:
> > On Fri, May 22, 2015 at 08:19:24PM +0200, Luis R. Rodriguez wrote:
> > > 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.
>
> Driver tweaks seems rather harmless compared to "trust me I know
> what I'm doing" even for all run time cases.
>
> So the question still stands -- are there any kconfig options which
> depend on EXPERT which enable folks to screw up run time (not just driver
> tweaks? If so, would it be good to separate the two types of expert
> config options, one for tweaks, another for run time possible conflicts?
>
> Reason I ask this is -- IMO we should not treat the possible EXPERT run time
> conflicts of FW_LOADER specially and try to be consistent, if we are ensuring
> EXPERT run time conflicts will work with FW_LOADER why not for others? If
> we want the option to let an EXPERT kconfig option enable run time conflicts
> maybe we need to split things up further.

I don't think it should be possible, even with EXPERT, to disable an
option that a driver needs at runtime without disabling that driver. If
the driver *optionally* needs something, but can work without it, it
shouldn't have "depends" or "select".

> > > 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 expert 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".
>
> Replacing "select FW_LOADER" with "depends on FW_LOADER", although still pegged
> with EXPERT, would not allow EXPERT users (most folks we've determined now) to
> disable FW_LOADER if a driver was selected that needed FW_LOADER, in fact I
> could not also create a tristate conflict of say MICROCODE set to y and
> FW_LOADER to m. Please let me know if I've missed something with testing this
> though.
>
> So unless I'm missing a corner case, it would seem replacing "select FW_LOADER"
> with "depends on FW_LOADER" is a reasonable option so far. The subtleties lie
> then now in since EXPERT is used by most distributions, everyone is an expert,
> and:
>
> a) Do we want to be consistent about ensuring run time sanity of EXPERT?
> b) Should we split EXPERT into tweak / possible run time break options?

"depends on FW_LOADER" and "select FW_LOADER" are equivalent from the
point of view of what configs they allow; they differ in how the
dependency is shown in UIs. "select" means you can always see the
drivers, and you can't turn off FW_LOADER until you turn off all of the
drivers. "depends" means that if you disable FW_LOADER all the drivers
that depend on it disappear, and you can't turn them on unless you turn
on FW_LOADER (which however is always on when !EXPERT).

Either way, if you have FW_LOADER and a pile of drivers using it
enabled, you have to disable all the drivers before disabling FW_LOADER.
The behavior only differs when you have FW_LOADER disabled, which is a
sufficiently uncommon case that it doesn't matter deeply which approach
you take. Personally, if FW_LOADER doesn't have any dependencies, I'd
favor the select, since it avoids the case of "wait, where is the driver
I expected and why can't I find it?".

However, from the sounds of things, you want to add a dependency to
FW_LOADER. So in that case, yeah, switch to depends rather than
rewriting Kconfig, unless you're feeling really ambitious.

> > > 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.

By "recursive dependency issue", I'm guessing that someone wants to make
FW_LOADER depend on something else, which breaks because "select"
doesn't know how to recurse?

Ideally, someone should teach Kconfig to handle recursive dependencies,
but in the meantime using "depends" makes sense.

- Josh Triplett

2015-05-22 22:22:10

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 02:43:57PM -0700, [email protected] wrote:
> On Fri, May 22, 2015 at 09:28:14PM +0200, Luis R. Rodriguez wrote:
> > So the question still stands -- are there any kconfig options which
> > depend on EXPERT which enable folks to screw up run time (not just driver
> > tweaks?
>
> I don't think it should be possible

OK, thanks!

> > So unless I'm missing a corner case, it would seem replacing "select FW_LOADER"
> > with "depends on FW_LOADER" is a reasonable option so far. The subtleties lie
> > then now in since EXPERT is used by most distributions, everyone is an expert,
> > and:
> >
> > a) Do we want to be consistent about ensuring run time sanity of EXPERT?
> > b) Should we split EXPERT into tweak / possible run time break options?
>
> "depends on FW_LOADER" and "select FW_LOADER" are equivalent from the
> point of view of what configs they allow; they differ in how the
> dependency is shown in UIs.
>
> However, from the sounds of things, you want to add a dependency to
> FW_LOADER.

Nope the recursive case was that a new component which depends on
FW_LOADER ended up selecting CRYPTO, and it turns out another driver
which depends on FW_LOADER also depended on CRYPTO so that in turn
creates a recursive dependency.

> So in that case, yeah, switch to depends rather than
> rewriting Kconfig, unless you're feeling really ambitious.

OK so switching to "depends on" is the winning contender so far.

> > > > 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.
>
> By "recursive dependency issue", I'm guessing that someone wants to make
> FW_LOADER depend on something else, which breaks because "select"
> doesn't know how to recurse?

Kind of, the issue actually was a new component which depends on FW_LOADER
and has crypto dependencies. Since the qat crypto driver selects FW_LOADER
but also has a set of crypto dependencies that creates a recursive dependency
loop.

> Ideally, someone should teach Kconfig to handle recursive dependencies,
> but in the meantime using "depends" makes sense.

Doesn't sound like anyone is that enthusiastic about that.

So... shall we just switch to "depends on" tree-wide then for FW_LOADER?

Luis

2015-05-22 22:35:42

by Herbert Xu

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Sat, May 23, 2015 at 12:22:00AM +0200, Luis R. Rodriguez wrote:
> Kind of, the issue actually was a new component which depends on FW_LOADER
> and has crypto dependencies. Since the qat crypto driver selects FW_LOADER
> but also has a set of crypto dependencies that creates a recursive dependency
> loop.

Actually, how about making FW_SIG select FW_LOADER instead of
depending on it? I think this should break the cycle.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2015-05-22 23:02:57

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 3:33 PM, Herbert Xu <[email protected]> wrote:
> On Sat, May 23, 2015 at 12:22:00AM +0200, Luis R. Rodriguez wrote:
>> Kind of, the issue actually was a new component which depends on FW_LOADER
>> and has crypto dependencies. Since the qat crypto driver selects FW_LOADER
>> but also has a set of crypto dependencies that creates a recursive dependency
>> loop.
>
> Actually, how about making FW_SIG select FW_LOADER instead of
> depending on it? I think this should break the cycle.

Indeed, it does. Kind of odd, but works - and well if others run into
the recursive issue then we have two diverging solutions now:

a) Either swap all "select FOO" to "depends on FOO" or,
b) Change the offending "depends on FOO" to "select FOO"

So sticking to one seems to make Kconfig happy for recursive
dependency solving for now...

Luis

2015-05-22 23:55:03

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, May 22, 2015 at 04:02:33PM -0700, Luis R. Rodriguez wrote:
> On Fri, May 22, 2015 at 3:33 PM, Herbert Xu <[email protected]> wrote:
> > On Sat, May 23, 2015 at 12:22:00AM +0200, Luis R. Rodriguez wrote:
> >> Kind of, the issue actually was a new component which depends on FW_LOADER
> >> and has crypto dependencies. Since the qat crypto driver selects FW_LOADER
> >> but also has a set of crypto dependencies that creates a recursive dependency
> >> loop.
> >
> > Actually, how about making FW_SIG select FW_LOADER instead of
> > depending on it? I think this should break the cycle.
>
> Indeed, it does. Kind of odd, but works - and well if others run into
> the recursive issue then we have two diverging solutions now:
>
> a) Either swap all "select FOO" to "depends on FOO" or,
> b) Change the offending "depends on FOO" to "select FOO"
>
> So sticking to one seems to make Kconfig happy for recursive
> dependency solving for now...

For simplicity and avoidance of massive tree-wide patches, (b) seems
preferable in this case.

Long-term, I think ideally we should have *every* visible Kconfig option
always pulled in by "depends on" rather than "select", with visibility
and recursion handled by smarter tools. That said, meddle not in the
internals of Kconfig, for it has many unshorn yaks (and yaccs).

- Josh Triplett

2015-05-23 07:14:35

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Sat, May 23, 2015 at 1:02 AM, Luis R. Rodriguez <[email protected]> wrote:
> On Fri, May 22, 2015 at 3:33 PM, Herbert Xu <[email protected]> wrote:
>> On Sat, May 23, 2015 at 12:22:00AM +0200, Luis R. Rodriguez wrote:
>>> Kind of, the issue actually was a new component which depends on FW_LOADER
>>> and has crypto dependencies. Since the qat crypto driver selects FW_LOADER
>>> but also has a set of crypto dependencies that creates a recursive dependency
>>> loop.
>>
>> Actually, how about making FW_SIG select FW_LOADER instead of
>> depending on it? I think this should break the cycle.
>
> Indeed, it does. Kind of odd, but works - and well if others run into
> the recursive issue then we have two diverging solutions now:
>
> a) Either swap all "select FOO" to "depends on FOO" or,
> b) Change the offending "depends on FOO" to "select FOO"
>
> So sticking to one seems to make Kconfig happy for recursive
> dependency solving for now...

Yes, that works, too.

Perhaps Kconfig should start warning if it encounters a symbol
that is both selected and being depended on, even without detecting an
explicit recursive dependency?
That's gonna give tons of warnings, though...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2015-05-25 09:28:08

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, 2015-05-22 at 22:12 +0200, Geert Uytterhoeven wrote:
> On Fri, May 22, 2015 at 9:30 PM, Austin S Hemmelgarn
> <[email protected]> wrote:
> >>> 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.
> >>
> > I'm pretty certain that Fedora does, and that Arch does as well.

Fedora doesn't set EXPERT, and as far as I can see, never has. Same goes
for EMBEDDED, which is the predecessor of EXPERT.

If EXPERT really turns out to be set on most distributions it will have
gone the way of EXPERIMENTAL: a warning sign that lost its impact
through overuse. Perhaps that's the way these things go. But I don't
think that has happened with EXPERT, not yet.

Paul Bolle

2015-05-25 10:55:29

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, 2015-05-22 at 14:43 -0700, [email protected] wrote:
> Ideally, someone should teach Kconfig to handle recursive dependencies,

I'm probably reading too much in this remark, but how should it handle
that other than returning an error because the configuration it's fed
makes it run in circles?

Paul Bolle

2015-05-25 10:59:41

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, 2015-05-22 at 21:28 +0200, Luis R. Rodriguez wrote:
> > 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'm no kconfig expert and I wondered the same.
>
> Perhaps Paul can elaborate.

Because both the dependencies of a symbol and the select statements on
that symbol influence the set of values that is possible for that
symbol.

Paul Bolle

2015-05-25 11:10:23

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Fri, 2015-05-22 at 11:52 -0700, Dmitry Torokhov wrote:
> selects still don't cascade down - right?

select statements trigger the select statements of the selected symbol,
if that's what you mean.

Paul Bolle

2015-05-25 17:55:22

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Mon, May 25, 2015 at 12:55:17PM +0200, Paul Bolle wrote:
> On Fri, 2015-05-22 at 14:43 -0700, [email protected] wrote:
> > Ideally, someone should teach Kconfig to handle recursive dependencies,
>
> I'm probably reading too much in this remark, but how should it handle
> that other than returning an error because the configuration it's fed
> makes it run in circles?

I don't mean cyclic dependencies (for which Kconfig should just report
an error, ideally including the full list of symbols forming the cycle).

I mean that Kconfig should do recursive dependency resolution. If B
depends on A, and C depends on B, I should be able to turn on C
directly and have B and A enabled.

As an intermediate measure, it'd be *really* handy to be able to browse
in the curses UI directly from a symbol to the symbols it depends on to
quickly enable/disable them, rather than having to look at the list of
dependencies of a symbol, search for that symbol, remember the path the
search showed, and browse there manually.

- Josh Triplett

2015-05-25 18:25:42

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Mon, 2015-05-25 at 10:54 -0700, Josh Triplett wrote:
> I don't mean cyclic dependencies (for which Kconfig should just report
> an error, ideally including the full list of symbols forming the cycle).
>
> I mean that Kconfig should do recursive dependency resolution.

(My English might be letting me down here. Or my grasp of elementary
logic, for that matter. Recursive and cyclic are not really
interchangeable? Should Kconfig perhaps report a "Cyclic dependency
error"? )

> If B
> depends on A, and C depends on B, I should be able to turn on C
> directly and have B and A enabled.

And how should kconfig handle, say:
- B depends on (A || D)
- C depends on B

Should (B && D) be enabled or (B && A)? This is not meant as a
rhetorical question. But I do fear the complications for the choices
this idea might encounter are, at best, not worth the effort.

> As an intermediate measure, it'd be *really* handy to be able to browse
> in the curses UI directly from a symbol to the symbols it depends on to
> quickly enable/disable them, rather than having to look at the list of
> dependencies of a symbol, search for that symbol, remember the path the
> search showed, and browse there manually.

Probably. I'd rather not think about the UI involved when the symbol(s)
that should be enabled is (are) two or more levels down the chain. And
the odd "stacked" UI for searches in the curses tool adds to the fun.
But then again, I'm a the-glass-is-half-empty person.

Paul Bolle

2015-05-25 19:07:10

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Mon, May 25, 2015 at 08:25:35PM +0200, Paul Bolle wrote:
> On Mon, 2015-05-25 at 10:54 -0700, Josh Triplett wrote:
> > I don't mean cyclic dependencies (for which Kconfig should just report
> > an error, ideally including the full list of symbols forming the cycle).
> >
> > I mean that Kconfig should do recursive dependency resolution.
>
> (My English might be letting me down here. Or my grasp of elementary
> logic, for that matter. Recursive and cyclic are not really
> interchangeable? Should Kconfig perhaps report a "Cyclic dependency
> error"? )

I do think that'd be a clearer message, yes.

For clarity: "recursive dependency resolution" as in "recursive
resolution of dependencies", not "resolution of recursive dependencies".

> > If B
> > depends on A, and C depends on B, I should be able to turn on C
> > directly and have B and A enabled.
>
> And how should kconfig handle, say:
> - B depends on (A || D)
> - C depends on B
>
> Should (B && D) be enabled or (B && A)? This is not meant as a
> rhetorical question. But I do fear the complications for the choices
> this idea might encounter are, at best, not worth the effort.

That's exactly the kind of problem that makes this difficult to do, and
has thus stopped anyone from working on it. A fully general solution
requires a system that can also solve arbitrary logic problems, which is
not necessarily a feature.

The typical choice made by package management systems is to enable B and
A, preferring the leftmost dependency of B. That's what Debian's apt
does, in the absence of conflicts.

For an initial solution, I'd suggest just recursing through "select"
dependencies; if C has "select B", and B has "select A", enabling C
should enable B and A. Since you can't "select A || D", that seems
straightforward enough. You can "select A if X", but for that, just
bail out if the value of the expression X is changed by any of the
select statements.

> > As an intermediate measure, it'd be *really* handy to be able to browse
> > in the curses UI directly from a symbol to the symbols it depends on to
> > quickly enable/disable them, rather than having to look at the list of
> > dependencies of a symbol, search for that symbol, remember the path the
> > search showed, and browse there manually.
>
> Probably. I'd rather not think about the UI involved when the symbol(s)
> that should be enabled is (are) two or more levels down the chain. And
> the odd "stacked" UI for searches in the curses tool adds to the fun.
> But then again, I'm a the-glass-is-half-empty person.

I'm more thinking that if you're looking at an option, you should be
able to hit a key to see all the symbols that option depends on in a
list, then navigate to one of those symbols, and be taken to the menu
containing that symbol with the cursor on that symbol. (You could then
hit a key to go "back" to where you were.) It's not perfect, but it's
an improvement.

I'd also suggest that the search mechanism should produce a list that
can be browsed with the arrow keys, with enter taking you to the symbol.

- Josh Triplett

2015-05-25 19:07:55

by James Bottomley

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Mon, 2015-05-25 at 10:54 -0700, Josh Triplett wrote:
> On Mon, May 25, 2015 at 12:55:17PM +0200, Paul Bolle wrote:
> > On Fri, 2015-05-22 at 14:43 -0700, [email protected] wrote:
> > > Ideally, someone should teach Kconfig to handle recursive dependencies,
> >
> > I'm probably reading too much in this remark, but how should it handle
> > that other than returning an error because the configuration it's fed
> > makes it run in circles?
>
> I don't mean cyclic dependencies (for which Kconfig should just report
> an error, ideally including the full list of symbols forming the cycle).
>
> I mean that Kconfig should do recursive dependency resolution. If B
> depends on A, and C depends on B, I should be able to turn on C
> directly and have B and A enabled.

That's really hard in practise you have to make any symbol that selects
something depend on the dependencies of the selected symbol. You can't
do this without involving a SAT solver. A guy promised to do this a
couple of years ago, but the patches never materialised. However, they
may exist somewhere if someone wants to take a look at completing it.

James

> As an intermediate measure, it'd be *really* handy to be able to browse
> in the curses UI directly from a symbol to the symbols it depends on to
> quickly enable/disable them, rather than having to look at the list of
> dependencies of a symbol, search for that symbol, remember the path the
> search showed, and browse there manually.
>
> - Josh Triplett

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-05-25 19:17:08

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Mon, 2015-05-25 at 12:06 -0700, Josh Triplett wrote:
> On Mon, May 25, 2015 at 08:25:35PM +0200, Paul Bolle wrote:
> > On Mon, 2015-05-25 at 10:54 -0700, Josh Triplett wrote:
> For clarity: "recursive dependency resolution" as in "recursive
> resolution of dependencies", not "resolution of recursive dependencies".

I feel a headache coming up.

> For an initial solution, I'd suggest just recursing through "select"
> dependencies; if C has "select B", and B has "select A", enabling C
> should enable B and A. Since you can't "select A || D", that seems
> straightforward enough. You can "select A if X", but for that, just
> bail out if the value of the expression X is changed by any of the
> select statements.

Actually, kconfig already recurses through select statements. (I haven't
checked, but I'd expect it to handle the "select A if X" case correctly
while doing the recursive select dance.)

Thanks,

Paul Bolle

2015-05-25 19:29:57

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Mon, May 25, 2015 at 07:07:14PM +0000, James Bottomley wrote:
> On Mon, 2015-05-25 at 10:54 -0700, Josh Triplett wrote:
> > On Mon, May 25, 2015 at 12:55:17PM +0200, Paul Bolle wrote:
> > > On Fri, 2015-05-22 at 14:43 -0700, [email protected] wrote:
> > > > Ideally, someone should teach Kconfig to handle recursive dependencies,
> > >
> > > I'm probably reading too much in this remark, but how should it handle
> > > that other than returning an error because the configuration it's fed
> > > makes it run in circles?
> >
> > I don't mean cyclic dependencies (for which Kconfig should just report
> > an error, ideally including the full list of symbols forming the cycle).
> >
> > I mean that Kconfig should do recursive dependency resolution. If B
> > depends on A, and C depends on B, I should be able to turn on C
> > directly and have B and A enabled.
>
> That's really hard in practise you have to make any symbol that selects
> something depend on the dependencies of the selected symbol. You can't
> do this without involving a SAT solver. A guy promised to do this a
> couple of years ago, but the patches never materialised. However, they
> may exist somewhere if someone wants to take a look at completing it.

You can't do this in the completely general case without a SAT solver.
However, I think it's possible to help the user a bit more than than
Kconfig currently does without introducing a general dependency solver.

In particular, it'd be nice to have an easy way to see at a glance
"can't enable C because it depends on B", together with an easy way to
get to B to enable it to get to C. User-guided dependency resolution
seems like an improvement over no dependency resolution.

- Josh Triplett

2015-05-25 21:30:18

by James Bottomley

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Mon, 2015-05-25 at 12:29 -0700, Josh Triplett wrote:
> On Mon, May 25, 2015 at 07:07:14PM +0000, James Bottomley wrote:
> > On Mon, 2015-05-25 at 10:54 -0700, Josh Triplett wrote:
> > > On Mon, May 25, 2015 at 12:55:17PM +0200, Paul Bolle wrote:
> > > > On Fri, 2015-05-22 at 14:43 -0700, [email protected] wrote:
> > > > > Ideally, someone should teach Kconfig to handle recursive dependencies,
> > > >
> > > > I'm probably reading too much in this remark, but how should it handle
> > > > that other than returning an error because the configuration it's fed
> > > > makes it run in circles?
> > >
> > > I don't mean cyclic dependencies (for which Kconfig should just report
> > > an error, ideally including the full list of symbols forming the cycle).
> > >
> > > I mean that Kconfig should do recursive dependency resolution. If B
> > > depends on A, and C depends on B, I should be able to turn on C
> > > directly and have B and A enabled.
> >
> > That's really hard in practise you have to make any symbol that selects
> > something depend on the dependencies of the selected symbol. You can't
> > do this without involving a SAT solver. A guy promised to do this a
> > couple of years ago, but the patches never materialised. However, they
> > may exist somewhere if someone wants to take a look at completing it.
>
> You can't do this in the completely general case without a SAT solver.
> However, I think it's possible to help the user a bit more than than
> Kconfig currently does without introducing a general dependency solver.

Well the logically simplest way of doing that it to require that a
selected symbol can have no dependencies (but may select others) and to
make this what Kconfig enforces ... of course, our current Kconfig tree
violates this all over the place.

> In particular, it'd be nice to have an easy way to see at a glance
> "can't enable C because it depends on B", together with an easy way to
> get to B to enable it to get to C. User-guided dependency resolution
> seems like an improvement over no dependency resolution.

Sure, but it's mostly people like us doing it and find . -name Kconfig\*
-exec grep -3 <symbol> {} /dev/null \; usually does the trick for me.

James

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-05-26 00:47:50

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Mon, May 25, 2015 at 09:29:51PM +0000, James Bottomley wrote:
> On Mon, 2015-05-25 at 12:29 -0700, Josh Triplett wrote:
> > On Mon, May 25, 2015 at 07:07:14PM +0000, James Bottomley wrote:
> > > On Mon, 2015-05-25 at 10:54 -0700, Josh Triplett wrote:
> > > > On Mon, May 25, 2015 at 12:55:17PM +0200, Paul Bolle wrote:
> > > > > On Fri, 2015-05-22 at 14:43 -0700, [email protected] wrote:
> > > > > > Ideally, someone should teach Kconfig to handle recursive dependencies,
> > > > >
> > > > > I'm probably reading too much in this remark, but how should it handle
> > > > > that other than returning an error because the configuration it's fed
> > > > > makes it run in circles?
> > > >
> > > > I don't mean cyclic dependencies (for which Kconfig should just report
> > > > an error, ideally including the full list of symbols forming the cycle).
> > > >
> > > > I mean that Kconfig should do recursive dependency resolution. If B
> > > > depends on A, and C depends on B, I should be able to turn on C
> > > > directly and have B and A enabled.
> > >
> > > That's really hard in practise you have to make any symbol that selects
> > > something depend on the dependencies of the selected symbol. You can't
> > > do this without involving a SAT solver. A guy promised to do this a
> > > couple of years ago, but the patches never materialised. However, they
> > > may exist somewhere if someone wants to take a look at completing it.
> >
> > You can't do this in the completely general case without a SAT solver.
> > However, I think it's possible to help the user a bit more than than
> > Kconfig currently does without introducing a general dependency solver.
>
> Well the logically simplest way of doing that it to require that a
> selected symbol can have no dependencies (but may select others) and to
> make this what Kconfig enforces ... of course, our current Kconfig tree
> violates this all over the place.

Or enforce that it has no disabled dependencies.

> > In particular, it'd be nice to have an easy way to see at a glance
> > "can't enable C because it depends on B", together with an easy way to
> > get to B to enable it to get to C. User-guided dependency resolution
> > seems like an improvement over no dependency resolution.
>
> Sure, but it's mostly people like us doing it and find . -name Kconfig\*
> -exec grep -3 <symbol> {} /dev/null \; usually does the trick for me.

That's not particularly convenient from the middle of menuconfig when
configuring a kernel. It'd be nice to be able to navigate in one step
from a symbol to its dependencies within the menuconfig interface.

- Josh Triplett

2015-05-26 01:33:55

by James Bottomley

[permalink] [raw]

Subject: Re: [RFC v1] tree-wide: remove "select FW_LOADER" uses

On Mon, 2015-05-25 at 17:47 -0700, Josh Triplett wrote:
> On Mon, May 25, 2015 at 09:29:51PM +0000, James Bottomley wrote:
> > On Mon, 2015-05-25 at 12:29 -0700, Josh Triplett wrote:
> > > On Mon, May 25, 2015 at 07:07:14PM +0000, James Bottomley wrote:
> > > > On Mon, 2015-05-25 at 10:54 -0700, Josh Triplett wrote:
> > > > > On Mon, May 25, 2015 at 12:55:17PM +0200, Paul Bolle wrote:
> > > > > > On Fri, 2015-05-22 at 14:43 -0700, [email protected] wrote:
> > > > > > > Ideally, someone should teach Kconfig to handle recursive dependencies,
> > > > > >
> > > > > > I'm probably reading too much in this remark, but how should it handle
> > > > > > that other than returning an error because the configuration it's fed
> > > > > > makes it run in circles?
> > > > >
> > > > > I don't mean cyclic dependencies (for which Kconfig should just report
> > > > > an error, ideally including the full list of symbols forming the cycle).
> > > > >
> > > > > I mean that Kconfig should do recursive dependency resolution. If B
> > > > > depends on A, and C depends on B, I should be able to turn on C
> > > > > directly and have B and A enabled.
> > > >
> > > > That's really hard in practise you have to make any symbol that selects
> > > > something depend on the dependencies of the selected symbol. You can't
> > > > do this without involving a SAT solver. A guy promised to do this a
> > > > couple of years ago, but the patches never materialised. However, they
> > > > may exist somewhere if someone wants to take a look at completing it.
> > >
> > > You can't do this in the completely general case without a SAT solver.
> > > However, I think it's possible to help the user a bit more than than
> > > Kconfig currently does without introducing a general dependency solver.
> >
> > Well the logically simplest way of doing that it to require that a
> > selected symbol can have no dependencies (but may select others) and to
> > make this what Kconfig enforces ... of course, our current Kconfig tree
> > violates this all over the place.
>
> Or enforce that it has no disabled dependencies.

That's what doesn't work: once you depend on a symbol that selects
something else, you have to evaluate the satisfiability condition again
recursively ... and that's a SAT solver. Unfortunately, you need either
a heuristic that makes the logic chains simple or resign yourself to
doing it properly.

> > > In particular, it'd be nice to have an easy way to see at a glance
> > > "can't enable C because it depends on B", together with an easy way to
> > > get to B to enable it to get to C. User-guided dependency resolution
> > > seems like an improvement over no dependency resolution.
> >
> > Sure, but it's mostly people like us doing it and find . -name Kconfig\*
> > -exec grep -3 <symbol> {} /dev/null \; usually does the trick for me.
>
> That's not particularly convenient from the middle of menuconfig when
> configuring a kernel. It'd be nice to be able to navigate in one step
> from a symbol to its dependencies within the menuconfig interface.

Well, it works for me but, sure, do something you prefer ... I use make
config rather than menuconfig anyway.

James

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-05-26 08:29:16