diff mbox

dell_smbios in 4.15 and keyboard backlight

Message ID 20180224222911.GC31352@fury (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Darren Hart Feb. 24, 2018, 10:29 p.m. UTC
On Fri, Feb 23, 2018 at 07:53:08PM +0000, Mario.Limonciello@dell.com wrote:
> 
> 
> > -----Original Message-----
> > From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> > Sent: Friday, February 23, 2018 1:41 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>; md@linux.it;
> > dvhart@infradead.org
> > Cc: pali.rohar@gmail.com; platform-driver-x86@vger.kernel.org
> > Subject: Re: dell_smbios in 4.15 and keyboard backlight
> > 
> > On Fri, 2018-02-23 at 19:19 +0000, Mario.Limonciello@dell.com wrote:
> > > > -----Original Message-----
> > > > From: Marco d'Itri [mailto:md@linux.it]
> > > > Sent: Friday, February 23, 2018 1:06 PM
> > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > > Cc: pali.rohar@gmail.com
> > > > Subject: Re: dell_smbios in 4.15 and keyboard backlight
> > > >
> > > > On Feb 23, Mario Limonciello <superm1@gmail.com> wrote:
> > > >
> > > > > Can you please email me on mario.limonciello@dell.com for this
> > > > > topic?
> > > > >
> > > > > I would be interested to know do you have dell-smbios-smm or
> > > > > dell-smbios-wmi not loading or compiling?
> > > >
> > > > Oops... The new modules have not been enabled yet by the Debian
> > > > kernel
> > > > team, so this explains why I could not find them.
> > > > Thank you for your help, I will get back to you if I will still have
> > > > troubles with the new kernel.
> > > >
> > >
> > > If you don't mind I'm going to CC this to the mailing list for
> > > discussion.  I think
> > > you identified a valid config problem.
> > >
> > > Andy, Darren,
> > >
> > > Marco (CC'ed) privately emailed Pali and I to discuss an issue that
> > > dell-laptop
> > > wasn't working properly for him and dell-smbios couldn't find a
> > > backend.
> > >
> > > I thought at first it was an issue of the race condition recently
> > > discussed but
> > > it's actually a case that the distro kernel he's using compiled:
> > >
> > > DELL_SMBIOS
> > > DELL_LAPTOP
> > >
> > > But didn't select DELL_SMBIOS_WMI or DELL_SMBIOS_SMM.
> > 
> > Distros have to enable whatever they want to.
> At least in this instance I'd hypothesize it's because these are new config
> options that default to off.
> 
> They probably had DELL_SMBIOS enabled before and carried that forward
> But there was nothing to transition them to make them turn on 
> DELL_SMBIOS_WMI or DELL_SMBIOS_SMM.
> 
> > 
> > Can it be the Dell model, that survives w/o one of above or even both?
> 
> The design as it exists to day is that dell-laptop needs dell-smbios but
> dell-smbios won't run unless it has a backend selected.
> 
> > 
> > > I remember hypothesizing about this with one of the earlier versions
> > > of the
> > > patch series but the decision was to just hide config DELL_SMBIOS as
> > > an invisible
> > > tristate.
> > 
> > 
> > > Because this is a real problem happening I think we need something
> > > like this:
> > >
> > > diff --git a/drivers/platform/x86/Kconfig
> > > b/drivers/platform/x86/Kconfig
> > > index 9a8f964..f2f548b 100644
> > > --- a/drivers/platform/x86/Kconfig
> > > +++ b/drivers/platform/x86/Kconfig
> > > @@ -107,6 +107,7 @@ config ASUS_LAPTOP
> > >
> > >  config DELL_SMBIOS
> > >         tristate
> > > +       depends on DELL_SMBIOS_WMI || DELL_SMBIOS_SMM
> > 
> > This will simple not work.
> You're right, I tried it and got this:

At least one problem with this is that DELL_LAPTOP "selects"
DELL_SMBIOS. We only "select" trivial CONFIG options without user
selectable dependencies - typically without a prompt, as is the case
with DELL_SMBIOS.

So, the real dependency here is for DELL_LAPTOP, which requires one of
DELL_SMBIOS_WMI or DELL_SMBIOS_SMM.

I don't think we want the user to have to pick one of these BEFORE they
can enable DELL_LAPTOP.

The current situation is a bit weird:

DELL_LAPTOP selects DELL_SMBIOS
DELL_SMBIOS_(WMI|SMM) select DELL_SMBIOS

But if we add the needful dependency, without WMI or SMM, DELL_LAPTOP doesn't appear.

> warning: (DELL_SMBIOS_WMI && DELL_SMBIOS_SMM && DELL_LAPTOP && DELL_WMI) selects DELL_SMBIOS which has unmet direct dependencies (X86 && X86_PLATFORM_DEVICES && (DELL_SMBIOS_WMI || DELL_SMBIOS_SMM))
> warning: (DELL_SMBIOS_WMI && DELL_SMBIOS_SMM && DELL_LAPTOP && DELL_WMI) selects DELL_SMBIOS which has unmet direct dependencies (X86 && X86_PLATFORM_DEVICES && (DELL_SMBIOS_WMI || DELL_SMBIOS_SMM))
> 
> but it still let me go forward.
> 
> Something like this maybe then to not let them even try to run dell-smbios?
> 
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index 8541cde..71489fe 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -556,6 +556,11 @@ static int __init dell_smbios_init(void)
>         const struct dmi_device *valid;
>         int ret;
>  
> +#if !defined(CONFIG_DELL_SMBIOS_WMI) && !defined(CONFIG_DELL_SMBIOS_SMM)
> +       pr_err("Missing SMBIOS backend.");
> +       return -ENODEV;
> +#endif
> +]

If we don't specify the dependency in the Kconfig this is an option...
although the user gets warned much later in the process than I think
we'd like.

Another option would be to make a Dell Laptop menu option, which makes
it clear there is *something* there, and populate with

Dell Laptop Extras
	Dell SMBIOS WMI calling interface (WMI | SMM Required)
	Dell SMBIOS SMM calling interface (WMI | SMM Required)
	Dell WMI notifications
	Dell Latitude freefall driver (ACPI SMO88XX)
	Dell Airplane Mode Switch driver

I think users might find this hierarchy useful anyway. By Making the
menuentry default to n, we satisfy Linus' concern, and we can also make
Dell Laptop Extras depend on DELL_SMBIOS_WMI || DELL_SMBIOS_SMM, and
make it and one of them default to M.

Maybe something like:


From a178faac18684ff216979fe151b6c0bbe1c8c83f Mon Sep 17 00:00:00 2001
Message-Id: <a178faac18684ff216979fe151b6c0bbe1c8c83f.1519511221.git.dvhart@infradead.org>
From: "Darren Hart (VMware)" <dvhart@infradead.org>
Date: Sat, 24 Feb 2018 14:22:30 -0800
Subject: [PATCH] platform/x86: Create Dell Laptop menuconfig

The current dependency mapping for:

A DELL_LAPTOP
B DELL_SMBIOS
C DELL_SMBIOS_WMI
D DELL_SMBIOS_SMM

Is non-intuitive and leads to accidental misconfiguration. Because A
depends on either C | D to function, but we can't default them to m or y
per Linus' recent comments [1], A will be hidden unless one or the other
is enabled.

Rather than ask the user to make than non-intuitive leap, create a
menuconfig entry for all Dell laptop drivers, which defaults to n,
satisfying Linus' concerns, while at the same time making it obvious
that there are things to enable for Dell Laptops. This also makes it
possible to set default to m for DELL_LAPTOP and DELL_SMBIOS_WMI, which
is the preferred configuration going forward. These will be n unless the
user explicitly enables the dell laptop menu.

1. https://lkml.org/lkml/2017/11/18/257

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/Kconfig | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Pali Rohár Feb. 24, 2018, 11 p.m. UTC | #1
On Saturday 24 February 2018 14:29:11 Darren Hart wrote:
> On Fri, Feb 23, 2018 at 07:53:08PM +0000, Mario.Limonciello@dell.com wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> > > Sent: Friday, February 23, 2018 1:41 PM
> > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>; md@linux.it;
> > > dvhart@infradead.org
> > > Cc: pali.rohar@gmail.com; platform-driver-x86@vger.kernel.org
> > > Subject: Re: dell_smbios in 4.15 and keyboard backlight
> > > 
> > > On Fri, 2018-02-23 at 19:19 +0000, Mario.Limonciello@dell.com wrote:
> > > > > -----Original Message-----
> > > > > From: Marco d'Itri [mailto:md@linux.it]
> > > > > Sent: Friday, February 23, 2018 1:06 PM
> > > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > > > Cc: pali.rohar@gmail.com
> > > > > Subject: Re: dell_smbios in 4.15 and keyboard backlight
> > > > >
> > > > > On Feb 23, Mario Limonciello <superm1@gmail.com> wrote:
> > > > >
> > > > > > Can you please email me on mario.limonciello@dell.com for this
> > > > > > topic?
> > > > > >
> > > > > > I would be interested to know do you have dell-smbios-smm or
> > > > > > dell-smbios-wmi not loading or compiling?
> > > > >
> > > > > Oops... The new modules have not been enabled yet by the Debian
> > > > > kernel
> > > > > team, so this explains why I could not find them.
> > > > > Thank you for your help, I will get back to you if I will still have
> > > > > troubles with the new kernel.
> > > > >
> > > >
> > > > If you don't mind I'm going to CC this to the mailing list for
> > > > discussion.  I think
> > > > you identified a valid config problem.
> > > >
> > > > Andy, Darren,
> > > >
> > > > Marco (CC'ed) privately emailed Pali and I to discuss an issue that
> > > > dell-laptop
> > > > wasn't working properly for him and dell-smbios couldn't find a
> > > > backend.
> > > >
> > > > I thought at first it was an issue of the race condition recently
> > > > discussed but
> > > > it's actually a case that the distro kernel he's using compiled:
> > > >
> > > > DELL_SMBIOS
> > > > DELL_LAPTOP
> > > >
> > > > But didn't select DELL_SMBIOS_WMI or DELL_SMBIOS_SMM.
> > > 
> > > Distros have to enable whatever they want to.
> > At least in this instance I'd hypothesize it's because these are new config
> > options that default to off.
> > 
> > They probably had DELL_SMBIOS enabled before and carried that forward
> > But there was nothing to transition them to make them turn on 
> > DELL_SMBIOS_WMI or DELL_SMBIOS_SMM.
> > 
> > > 
> > > Can it be the Dell model, that survives w/o one of above or even both?
> > 
> > The design as it exists to day is that dell-laptop needs dell-smbios but
> > dell-smbios won't run unless it has a backend selected.
> > 
> > > 
> > > > I remember hypothesizing about this with one of the earlier versions
> > > > of the
> > > > patch series but the decision was to just hide config DELL_SMBIOS as
> > > > an invisible
> > > > tristate.
> > > 
> > > 
> > > > Because this is a real problem happening I think we need something
> > > > like this:
> > > >
> > > > diff --git a/drivers/platform/x86/Kconfig
> > > > b/drivers/platform/x86/Kconfig
> > > > index 9a8f964..f2f548b 100644
> > > > --- a/drivers/platform/x86/Kconfig
> > > > +++ b/drivers/platform/x86/Kconfig
> > > > @@ -107,6 +107,7 @@ config ASUS_LAPTOP
> > > >
> > > >  config DELL_SMBIOS
> > > >         tristate
> > > > +       depends on DELL_SMBIOS_WMI || DELL_SMBIOS_SMM
> > > 
> > > This will simple not work.
> > You're right, I tried it and got this:
> 
> At least one problem with this is that DELL_LAPTOP "selects"
> DELL_SMBIOS. We only "select" trivial CONFIG options without user
> selectable dependencies - typically without a prompt, as is the case
> with DELL_SMBIOS.
> 
> So, the real dependency here is for DELL_LAPTOP, which requires one of
> DELL_SMBIOS_WMI or DELL_SMBIOS_SMM.
> 
> I don't think we want the user to have to pick one of these BEFORE they
> can enable DELL_LAPTOP.
> 
> The current situation is a bit weird:
> 
> DELL_LAPTOP selects DELL_SMBIOS
> DELL_SMBIOS_(WMI|SMM) select DELL_SMBIOS
> 
> But if we add the needful dependency, without WMI or SMM, DELL_LAPTOP doesn't appear.
> 
> > warning: (DELL_SMBIOS_WMI && DELL_SMBIOS_SMM && DELL_LAPTOP && DELL_WMI) selects DELL_SMBIOS which has unmet direct dependencies (X86 && X86_PLATFORM_DEVICES && (DELL_SMBIOS_WMI || DELL_SMBIOS_SMM))
> > warning: (DELL_SMBIOS_WMI && DELL_SMBIOS_SMM && DELL_LAPTOP && DELL_WMI) selects DELL_SMBIOS which has unmet direct dependencies (X86 && X86_PLATFORM_DEVICES && (DELL_SMBIOS_WMI || DELL_SMBIOS_SMM))
> > 
> > but it still let me go forward.
> > 
> > Something like this maybe then to not let them even try to run dell-smbios?
> > 
> > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> > index 8541cde..71489fe 100644
> > --- a/drivers/platform/x86/dell-smbios.c
> > +++ b/drivers/platform/x86/dell-smbios.c
> > @@ -556,6 +556,11 @@ static int __init dell_smbios_init(void)
> >         const struct dmi_device *valid;
> >         int ret;
> >  
> > +#if !defined(CONFIG_DELL_SMBIOS_WMI) && !defined(CONFIG_DELL_SMBIOS_SMM)
> > +       pr_err("Missing SMBIOS backend.");
> > +       return -ENODEV;
> > +#endif
> > +]
> 
> If we don't specify the dependency in the Kconfig this is an option...
> although the user gets warned much later in the process than I think
> we'd like.
> 
> Another option would be to make a Dell Laptop menu option, which makes
> it clear there is *something* there, and populate with
> 
> Dell Laptop Extras
> 	Dell SMBIOS WMI calling interface (WMI | SMM Required)
> 	Dell SMBIOS SMM calling interface (WMI | SMM Required)
> 	Dell WMI notifications
> 	Dell Latitude freefall driver (ACPI SMO88XX)
> 	Dell Airplane Mode Switch driver
> 
> I think users might find this hierarchy useful anyway. By Making the
> menuentry default to n, we satisfy Linus' concern, and we can also make
> Dell Laptop Extras depend on DELL_SMBIOS_WMI || DELL_SMBIOS_SMM, and
> make it and one of them default to M.
> 
> Maybe something like:
> 
> 
> From a178faac18684ff216979fe151b6c0bbe1c8c83f Mon Sep 17 00:00:00 2001
> Message-Id: <a178faac18684ff216979fe151b6c0bbe1c8c83f.1519511221.git.dvhart@infradead.org>
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
> Date: Sat, 24 Feb 2018 14:22:30 -0800
> Subject: [PATCH] platform/x86: Create Dell Laptop menuconfig
> 
> The current dependency mapping for:
> 
> A DELL_LAPTOP
> B DELL_SMBIOS
> C DELL_SMBIOS_WMI
> D DELL_SMBIOS_SMM
> 
> Is non-intuitive and leads to accidental misconfiguration. Because A
> depends on either C | D to function, but we can't default them to m or y
> per Linus' recent comments [1], A will be hidden unless one or the other
> is enabled.
> 
> Rather than ask the user to make than non-intuitive leap, create a
> menuconfig entry for all Dell laptop drivers, which defaults to n,
> satisfying Linus' concerns, while at the same time making it obvious
> that there are things to enable for Dell Laptops. This also makes it
> possible to set default to m for DELL_LAPTOP and DELL_SMBIOS_WMI, which
> is the preferred configuration going forward. These will be n unless the
> user explicitly enables the dell laptop menu.
> 
> 1. https://lkml.org/lkml/2017/11/18/257
> 
> Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
> ---
>  drivers/platform/x86/Kconfig | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index bc66a31..001f983 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -105,11 +105,22 @@ config ASUS_LAPTOP
>  
>  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
>  
> +menuconfig DELL_LAPTOP_EXTRAS
> +	bool "Dell Laptop Specific Device Drivers"
> +	default n
> +	---help---
> +	  Say Y here to get to see options for Dell Laptop specific device drivers
> +	  This option alone does not add any kernel code.
> +
> +	  If you say N, all options in this submenu will be skipped and disabled.
> +
> +if DELL_LAPTOP_EXTRAS
>  config DELL_SMBIOS
>  	tristate
>  
>  config DELL_SMBIOS_WMI
>  	tristate "Dell SMBIOS calling interface (WMI implementation)"
> +	default m
>  	depends on ACPI_WMI
>  	select DELL_WMI_DESCRIPTOR
>  	select DELL_SMBIOS
> @@ -135,12 +146,13 @@ config DELL_SMBIOS_SMM
>  
>  config DELL_LAPTOP
>  	tristate "Dell Laptop Extras"
> +	default m
>  	depends on DMI
>  	depends on BACKLIGHT_CLASS_DEVICE
>  	depends on ACPI_VIDEO || ACPI_VIDEO = n
>  	depends on RFKILL || RFKILL = n
>  	depends on SERIO_I8042
> -	select DELL_SMBIOS
> +	depends on DELL_SMBIOS_WMI || DELL_SMBIOS_SMM
>  	select POWER_SUPPLY
>  	select LEDS_CLASS
>  	select NEW_LEDS
> @@ -212,7 +224,7 @@ config DELL_RBTN
>  
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called dell-rbtn.
> -
> +endif # DELL_LAPTOP_EXTRAS
>  
>  config FUJITSU_LAPTOP
>  	tristate "Fujitsu Laptop Extras"
> -- 
> 2.9.5
> 
> 

And what would happen for existing configs which do not have
CONFIG_DELL_LAPTOP_EXTRAS yet, but have set e.g. CONFIG_DELL_LAPTOP set
to Y?

I think we should not break existing distribution configs which already
have CONFIG_DELL_LAPTOP set to Y or M.
Darren Hart Feb. 24, 2018, 11:07 p.m. UTC | #2
On February 24, 2018 3:00:24 PM PST, "Pali Rohár" <pali.rohar@gmail.com> wrote:
  
>And what would happen for existing configs which do not have
>CONFIG_DELL_LAPTOP_EXTRAS yet, but have set e.g. CONFIG_DELL_LAPTOP set
>to Y?
>
>I think we should not break existing distribution configs which already
>have CONFIG_DELL_LAPTOP set to Y or M.

We have to be able to change *something*. There is no stable CONFIG commitment, and it changes significantly with every release. This makes it much easier for them to get it right going forward. Having been in the distro config business myself, I see this as a very worthwhile trade-off.
Darren Hart Feb. 24, 2018, 11:35 p.m. UTC | #3
On Sat, Feb 24, 2018 at 03:07:47PM -0800, Darren Hart wrote:
> 
> 
> On February 24, 2018 3:00:24 PM PST, "Pali Rohár" <pali.rohar@gmail.com> wrote:
>   
> >And what would happen for existing configs which do not have
> >CONFIG_DELL_LAPTOP_EXTRAS yet, but have set e.g. CONFIG_DELL_LAPTOP set
> >to Y?
> >
> >I think we should not break existing distribution configs which already
> >have CONFIG_DELL_LAPTOP set to Y or M.
> 
> We have to be able to change *something*. There is no stable CONFIG commitment, and it changes significantly with every release. This makes it much easier for them to get it right going forward. Having been in the distro config business myself, I see this as a very worthwhile trade-off.
> 
> -- 
> Sent from my Android device with K-9 Mail. Please excuse my brevity.

Back at the laptop. People managing distro configs have to validate
their desired configs anyway, and we have tools in place to make such
changes obvious.  If they are using config fragments, they can use
scripts/kconfig/merge_config.sh which reports when the final config
differs from the input. If they are just using oldconfig, they can use
diff and review the changes.

The CONFIG space changes a lot, and while I agree we shouldn't make
gratuitous changes, I think this one makes a lot of sense.

In fact, I think moving moving x86 platform drivers Kconfig to a one
level hierarchy by vendor might be a really good way to both make it
easier to find things, as well as provide a default set of default=m
options, which are only enabled if their menuconfig entry is enabled,
simplifying the selection process.

If you have an Asus, or a Dell, or a Toshiba, etc. You just select
<VENDOR>_EXTRAS and have a pretty chance that it does "the right thing".
Limonciello, Mario Feb. 25, 2018, 1:39 a.m. UTC | #4
> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Saturday, February 24, 2018 4:29 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: andriy.shevchenko@linux.intel.com; md@linux.it; pali.rohar@gmail.com;
> platform-driver-x86@vger.kernel.org
> Subject: Re: dell_smbios in 4.15 and keyboard backlight
> 
> On Fri, Feb 23, 2018 at 07:53:08PM +0000, Mario.Limonciello@dell.com wrote:
> >
> >
> > > -----Original Message-----
> > > From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> > > Sent: Friday, February 23, 2018 1:41 PM
> > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>; md@linux.it;
> > > dvhart@infradead.org
> > > Cc: pali.rohar@gmail.com; platform-driver-x86@vger.kernel.org
> > > Subject: Re: dell_smbios in 4.15 and keyboard backlight
> > >
> > > On Fri, 2018-02-23 at 19:19 +0000, Mario.Limonciello@dell.com wrote:
> > > > > -----Original Message-----
> > > > > From: Marco d'Itri [mailto:md@linux.it]
> > > > > Sent: Friday, February 23, 2018 1:06 PM
> > > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > > > Cc: pali.rohar@gmail.com
> > > > > Subject: Re: dell_smbios in 4.15 and keyboard backlight
> > > > >
> > > > > On Feb 23, Mario Limonciello <superm1@gmail.com> wrote:
> > > > >
> > > > > > Can you please email me on mario.limonciello@dell.com for this
> > > > > > topic?
> > > > > >
> > > > > > I would be interested to know do you have dell-smbios-smm or
> > > > > > dell-smbios-wmi not loading or compiling?
> > > > >
> > > > > Oops... The new modules have not been enabled yet by the Debian
> > > > > kernel
> > > > > team, so this explains why I could not find them.
> > > > > Thank you for your help, I will get back to you if I will still have
> > > > > troubles with the new kernel.
> > > > >
> > > >
> > > > If you don't mind I'm going to CC this to the mailing list for
> > > > discussion.  I think
> > > > you identified a valid config problem.
> > > >
> > > > Andy, Darren,
> > > >
> > > > Marco (CC'ed) privately emailed Pali and I to discuss an issue that
> > > > dell-laptop
> > > > wasn't working properly for him and dell-smbios couldn't find a
> > > > backend.
> > > >
> > > > I thought at first it was an issue of the race condition recently
> > > > discussed but
> > > > it's actually a case that the distro kernel he's using compiled:
> > > >
> > > > DELL_SMBIOS
> > > > DELL_LAPTOP
> > > >
> > > > But didn't select DELL_SMBIOS_WMI or DELL_SMBIOS_SMM.
> > >
> > > Distros have to enable whatever they want to.
> > At least in this instance I'd hypothesize it's because these are new config
> > options that default to off.
> >
> > They probably had DELL_SMBIOS enabled before and carried that forward
> > But there was nothing to transition them to make them turn on
> > DELL_SMBIOS_WMI or DELL_SMBIOS_SMM.
> >
> > >
> > > Can it be the Dell model, that survives w/o one of above or even both?
> >
> > The design as it exists to day is that dell-laptop needs dell-smbios but
> > dell-smbios won't run unless it has a backend selected.
> >
> > >
> > > > I remember hypothesizing about this with one of the earlier versions
> > > > of the
> > > > patch series but the decision was to just hide config DELL_SMBIOS as
> > > > an invisible
> > > > tristate.
> > >
> > >
> > > > Because this is a real problem happening I think we need something
> > > > like this:
> > > >
> > > > diff --git a/drivers/platform/x86/Kconfig
> > > > b/drivers/platform/x86/Kconfig
> > > > index 9a8f964..f2f548b 100644
> > > > --- a/drivers/platform/x86/Kconfig
> > > > +++ b/drivers/platform/x86/Kconfig
> > > > @@ -107,6 +107,7 @@ config ASUS_LAPTOP
> > > >
> > > >  config DELL_SMBIOS
> > > >         tristate
> > > > +       depends on DELL_SMBIOS_WMI || DELL_SMBIOS_SMM
> > >
> > > This will simple not work.
> > You're right, I tried it and got this:
> 
> At least one problem with this is that DELL_LAPTOP "selects"
> DELL_SMBIOS. We only "select" trivial CONFIG options without user
> selectable dependencies - typically without a prompt, as is the case
> with DELL_SMBIOS.
> 
> So, the real dependency here is for DELL_LAPTOP, which requires one of
> DELL_SMBIOS_WMI or DELL_SMBIOS_SMM.
> 
> I don't think we want the user to have to pick one of these BEFORE they
> can enable DELL_LAPTOP.
> 
> The current situation is a bit weird:
> 
> DELL_LAPTOP selects DELL_SMBIOS
> DELL_SMBIOS_(WMI|SMM) select DELL_SMBIOS
> 
> But if we add the needful dependency, without WMI or SMM, DELL_LAPTOP doesn't
> appear.
> 
> > warning: (DELL_SMBIOS_WMI && DELL_SMBIOS_SMM && DELL_LAPTOP &&
> DELL_WMI) selects DELL_SMBIOS which has unmet direct dependencies (X86 &&
> X86_PLATFORM_DEVICES && (DELL_SMBIOS_WMI || DELL_SMBIOS_SMM))
> > warning: (DELL_SMBIOS_WMI && DELL_SMBIOS_SMM && DELL_LAPTOP &&
> DELL_WMI) selects DELL_SMBIOS which has unmet direct dependencies (X86 &&
> X86_PLATFORM_DEVICES && (DELL_SMBIOS_WMI || DELL_SMBIOS_SMM))
> >
> > but it still let me go forward.
> >
> > Something like this maybe then to not let them even try to run dell-smbios?
> >
> > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-
> smbios.c
> > index 8541cde..71489fe 100644
> > --- a/drivers/platform/x86/dell-smbios.c
> > +++ b/drivers/platform/x86/dell-smbios.c
> > @@ -556,6 +556,11 @@ static int __init dell_smbios_init(void)
> >         const struct dmi_device *valid;
> >         int ret;
> >
> > +#if !defined(CONFIG_DELL_SMBIOS_WMI) &&
> !defined(CONFIG_DELL_SMBIOS_SMM)
> > +       pr_err("Missing SMBIOS backend.");
> > +       return -ENODEV;
> > +#endif
> > +]
> 
> If we don't specify the dependency in the Kconfig this is an option...
> although the user gets warned much later in the process than I think
> we'd like.
> 
> Another option would be to make a Dell Laptop menu option, which makes
> it clear there is *something* there, and populate with
> 
> Dell Laptop Extras
> 	Dell SMBIOS WMI calling interface (WMI | SMM Required)
> 	Dell SMBIOS SMM calling interface (WMI | SMM Required)
> 	Dell WMI notifications
> 	Dell Latitude freefall driver (ACPI SMO88XX)
> 	Dell Airplane Mode Switch driver
> 
> I think users might find this hierarchy useful anyway. By Making the
> menuentry default to n, we satisfy Linus' concern, and we can also make
> Dell Laptop Extras depend on DELL_SMBIOS_WMI || DELL_SMBIOS_SMM, and
> make it and one of them default to M.
> 
> Maybe something like:
> 
> 
> From a178faac18684ff216979fe151b6c0bbe1c8c83f Mon Sep 17 00:00:00 2001
> Message-Id:
> <a178faac18684ff216979fe151b6c0bbe1c8c83f.1519511221.git.dvhart@infradea
> d.org>
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
> Date: Sat, 24 Feb 2018 14:22:30 -0800
> Subject: [PATCH] platform/x86: Create Dell Laptop menuconfig
> 
> The current dependency mapping for:
> 
> A DELL_LAPTOP
> B DELL_SMBIOS
> C DELL_SMBIOS_WMI
> D DELL_SMBIOS_SMM
> 
> Is non-intuitive and leads to accidental misconfiguration. Because A
> depends on either C | D to function, but we can't default them to m or y
> per Linus' recent comments [1], A will be hidden unless one or the other
> is enabled.
> 
> Rather than ask the user to make than non-intuitive leap, create a
> menuconfig entry for all Dell laptop drivers, which defaults to n,
> satisfying Linus' concerns, while at the same time making it obvious
> that there are things to enable for Dell Laptops. This also makes it
> possible to set default to m for DELL_LAPTOP and DELL_SMBIOS_WMI, which
> is the preferred configuration going forward. These will be n unless the
> user explicitly enables the dell laptop menu.

If this is the approach that's taken, then I think it's best to move "all" the platform/x86
Dell drivers under the umbrella.

dell-wmi, dell-wmi-descriptor, dell-rbtn etc.

> 
> 1. https://lkml.org/lkml/2017/11/18/257
> 
> Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
> ---
>  drivers/platform/x86/Kconfig | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index bc66a31..001f983 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -105,11 +105,22 @@ config ASUS_LAPTOP
> 
>  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
> 
> +menuconfig DELL_LAPTOP_EXTRAS
> +	bool "Dell Laptop Specific Device Drivers"
I think it would be better to abstract to "Dell specific device drivers"

Although they're valuable to laptops, everything except dell-laptop
functions on other format factors too.

> +	default n
> +	---help---
> +	  Say Y here to get to see options for Dell Laptop specific device drivers
> +	  This option alone does not add any kernel code.
> +
> +	  If you say N, all options in this submenu will be skipped and disabled.
> +
> +if DELL_LAPTOP_EXTRAS
>  config DELL_SMBIOS
>  	tristate
> 
>  config DELL_SMBIOS_WMI
>  	tristate "Dell SMBIOS calling interface (WMI implementation)"
> +	default m
>  	depends on ACPI_WMI
>  	select DELL_WMI_DESCRIPTOR
>  	select DELL_SMBIOS
> @@ -135,12 +146,13 @@ config DELL_SMBIOS_SMM
> 
>  config DELL_LAPTOP
>  	tristate "Dell Laptop Extras"
> +	default m
>  	depends on DMI
>  	depends on BACKLIGHT_CLASS_DEVICE
>  	depends on ACPI_VIDEO || ACPI_VIDEO = n
>  	depends on RFKILL || RFKILL = n
>  	depends on SERIO_I8042
> -	select DELL_SMBIOS
> +	depends on DELL_SMBIOS_WMI || DELL_SMBIOS_SMM

Have you given any thought to my other idea to also link all 3 into one module?

I think when combined with your proposal it would also satisfy the problem raised,
keep Linus happy, and let people configure only the parts they care about and
(hopefully) also fix the race condition at initialization by running SMM initialization
and WMI initialization immediately when dell-smbios is loaded.

I haven't tried it yet though.

>  	select POWER_SUPPLY
>  	select LEDS_CLASS
>  	select NEW_LEDS
> @@ -212,7 +224,7 @@ config DELL_RBTN
> 
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called dell-rbtn.
> -
> +endif # DELL_LAPTOP_EXTRAS
> 
>  config FUJITSU_LAPTOP
>  	tristate "Fujitsu Laptop Extras"
> --
> 2.9.5
> 
>
diff mbox

Patch

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index bc66a31..001f983 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -105,11 +105,22 @@  config ASUS_LAPTOP
 
 	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
 
+menuconfig DELL_LAPTOP_EXTRAS
+	bool "Dell Laptop Specific Device Drivers"
+	default n
+	---help---
+	  Say Y here to get to see options for Dell Laptop specific device drivers
+	  This option alone does not add any kernel code.
+
+	  If you say N, all options in this submenu will be skipped and disabled.
+
+if DELL_LAPTOP_EXTRAS
 config DELL_SMBIOS
 	tristate
 
 config DELL_SMBIOS_WMI
 	tristate "Dell SMBIOS calling interface (WMI implementation)"
+	default m
 	depends on ACPI_WMI
 	select DELL_WMI_DESCRIPTOR
 	select DELL_SMBIOS
@@ -135,12 +146,13 @@  config DELL_SMBIOS_SMM
 
 config DELL_LAPTOP
 	tristate "Dell Laptop Extras"
+	default m
 	depends on DMI
 	depends on BACKLIGHT_CLASS_DEVICE
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
 	depends on RFKILL || RFKILL = n
 	depends on SERIO_I8042
-	select DELL_SMBIOS
+	depends on DELL_SMBIOS_WMI || DELL_SMBIOS_SMM
 	select POWER_SUPPLY
 	select LEDS_CLASS
 	select NEW_LEDS
@@ -212,7 +224,7 @@  config DELL_RBTN
 
 	  To compile this driver as a module, choose M here: the module will
 	  be called dell-rbtn.
-
+endif # DELL_LAPTOP_EXTRAS
 
 config FUJITSU_LAPTOP
 	tristate "Fujitsu Laptop Extras"