> -----Original Message----- > From: Darren Hart [mailto:dvhart@xxxxxxxxxxxxx] > Sent: Saturday, February 24, 2018 4:29 PM > To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx> > Cc: andriy.shevchenko@xxxxxxxxxxxxxxx; md@xxxxxxxx; pali.rohar@xxxxxxxxx; > platform-driver-x86@xxxxxxxxxxxxxxx > Subject: Re: dell_smbios in 4.15 and keyboard backlight > > 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@infradea > d.org> > 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. 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@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" 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 > >
