On Saturday 24 February 2018 14:29:11 Darren Hart wrote: > On Fri, Feb 23, 2018 at 07:53:08PM +0000, Mario.Limonciello@xxxxxxxx wrote: > > > > > > > -----Original Message----- > > > From: Andy Shevchenko [mailto:andriy.shevchenko@xxxxxxxxxxxxxxx] > > > Sent: Friday, February 23, 2018 1:41 PM > > > To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>; md@xxxxxxxx; > > > dvhart@xxxxxxxxxxxxx > > > Cc: pali.rohar@xxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx > > > Subject: Re: dell_smbios in 4.15 and keyboard backlight > > > > > > On Fri, 2018-02-23 at 19:19 +0000, Mario.Limonciello@xxxxxxxx wrote: > > > > > -----Original Message----- > > > > > From: Marco d'Itri [mailto:md@xxxxxxxx] > > > > > Sent: Friday, February 23, 2018 1:06 PM > > > > > To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx> > > > > > Cc: pali.rohar@xxxxxxxxx > > > > > Subject: Re: dell_smbios in 4.15 and keyboard backlight > > > > > > > > > > On Feb 23, Mario Limonciello <superm1@xxxxxxxxx> wrote: > > > > > > > > > > > Can you please email me on mario.limonciello@xxxxxxxx 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@xxxxxxxxxxxxx> > From: "Darren Hart (VMware)" <dvhart@xxxxxxxxxxxxx> > 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@xxxxxxxxxxxxx> > --- > 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. -- Pali Rohár pali.rohar@xxxxxxxxx
Attachment:
signature.asc
Description: PGP signature
