[PATCH v3 1/3] platform/x86: dell-smbios: Correct some style warnings

[PATCH v3 1/3] platform/x86: dell-smbios: Correct some style warnings

[Mario Limonciello]

WARNING: function definition argument 'struct calling_interface_buffer *'
should also have an identifier name
+       int (*call_fn)(struct calling_interface_buffer *);

WARNING: Block comments use * on subsequent lines
+       /* 4 bytes of table header, plus 7 bytes of Dell header,
	plus at least
+          6 bytes of entry */

WARNING: Block comments use a trailing */ on a separate line
+          6 bytes of entry */

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/platform/x86/dell-smbios.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 8541cde..76b9d75 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -36,7 +36,7 @@ static DEFINE_MUTEX(smbios_mutex);
 struct smbios_device {
 	struct list_head list;
 	struct device *device;
-	int (*call_fn)(struct calling_interface_buffer *);
+	int (*call_fn)(struct calling_interface_buffer *arg);
 };
 
 struct smbios_call {
@@ -352,8 +352,10 @@ static void __init parse_da_table(const struct dmi_header *dm)
 	struct calling_interface_structure *table =
 		container_of(dm, struct calling_interface_structure, header);
 
-	/* 4 bytes of table header, plus 7 bytes of Dell header, plus at least
-	   6 bytes of entry */
+	/*
+	 * 4 bytes of table header, plus 7 bytes of Dell header
+	 * plus at least 6 bytes of entry
+	 */
 
 	if (dm->length < 17)
 		return;
-- 
2.7.4

[PATCH v3 2/3] platform/x86: dell-smbios: Rename dell-smbios source to dell-smbios-base

[Mario Limonciello]

This is being done to faciliate a later change to link all the dell-smbios
drivers together.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/platform/x86/Makefile                              | 1 +
 drivers/platform/x86/{dell-smbios.c => dell-smbios-base.c} | 0
 2 files changed, 1 insertion(+)
 rename drivers/platform/x86/{dell-smbios.c => dell-smbios-base.c} (100%)

diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index c388608..940b118 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_MSI_LAPTOP)	+= msi-laptop.o
 obj-$(CONFIG_ACPI_CMPC)		+= classmate-laptop.o
 obj-$(CONFIG_COMPAL_LAPTOP)	+= compal-laptop.o
 obj-$(CONFIG_DELL_SMBIOS)	+= dell-smbios.o
+dell-smbios-objs		:= dell-smbios-base.o
 obj-$(CONFIG_DELL_SMBIOS_WMI)	+= dell-smbios-wmi.o
 obj-$(CONFIG_DELL_SMBIOS_SMM)	+= dell-smbios-smm.o
 obj-$(CONFIG_DELL_LAPTOP)	+= dell-laptop.o
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios-base.c
similarity index 100%
rename from drivers/platform/x86/dell-smbios.c
rename to drivers/platform/x86/dell-smbios-base.c
-- 
2.7.4

[PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-* modules together

[Mario Limonciello]

Some race conditions were raised due to dell-smbios and its backends
not being ready by the time that a consumer would call one of the
exported methods.

To avoid this problem, guarantee that all initialization has been
done by linking them all together and running init for them all.

As part of this change the Kconfig needs to be adjusted so that
CONFIG_DELL_SMBIOS_SMM and CONFIG_DELL_SMBIOS_WMI are boolean
rather than modules.

CONFIG_DELL_SMBIOS is a visually selectable option again and both
CONFIG_DELL_SMBIOS_WMI and CONFIG_DELL_SMBIOS_SMM are optional.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
Changes from v2:
 * Move rest of #ifdef out of source files
 * Only compile units as necessary

 drivers/platform/x86/Kconfig            | 11 ++++++++---
 drivers/platform/x86/Makefile           |  4 ++--
 drivers/platform/x86/dell-smbios-base.c | 21 ++++++++++++++++++++-
 drivers/platform/x86/dell-smbios-smm.c  | 18 ++++--------------
 drivers/platform/x86/dell-smbios-wmi.c  | 14 ++++----------
 drivers/platform/x86/dell-smbios.h      | 27 ++++++++++++++++++++++++++-
 6 files changed, 64 insertions(+), 31 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 9a8f964..0c0897e 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -106,10 +106,15 @@ config ASUS_LAPTOP
 	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
 
 config DELL_SMBIOS
-	tristate
+	tristate "Dell SMBIOS driver"
+	---help---
+	This provides support for the Dell SMBIOS calling interface.
+	If you have a Dell computer you should enable this option.
+
+	Be sure to select at least one backend for it to work properly.
 
 config DELL_SMBIOS_WMI
-	tristate "Dell SMBIOS calling interface (WMI implementation)"
+	bool "Dell SMBIOS driver WMI support"
 	depends on ACPI_WMI
 	select DELL_WMI_DESCRIPTOR
 	select DELL_SMBIOS
@@ -122,7 +127,7 @@ config DELL_SMBIOS_WMI
 	it just won't load.
 
 config DELL_SMBIOS_SMM
-	tristate "Dell SMBIOS calling interface (SMM implementation)"
+	bool "Dell SMBIOS driver SMM support"
 	depends on DCDBAS
 	select DELL_SMBIOS
 	---help---
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 940b118..2ba6cb7 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -14,8 +14,8 @@ obj-$(CONFIG_ACPI_CMPC)		+= classmate-laptop.o
 obj-$(CONFIG_COMPAL_LAPTOP)	+= compal-laptop.o
 obj-$(CONFIG_DELL_SMBIOS)	+= dell-smbios.o
 dell-smbios-objs		:= dell-smbios-base.o
-obj-$(CONFIG_DELL_SMBIOS_WMI)	+= dell-smbios-wmi.o
-obj-$(CONFIG_DELL_SMBIOS_SMM)	+= dell-smbios-smm.o
+dell-smbios-$(CONFIG_DELL_SMBIOS_WMI)	+= dell-smbios-wmi.o
+dell-smbios-$(CONFIG_DELL_SMBIOS_SMM)	+= dell-smbios-smm.o
 obj-$(CONFIG_DELL_LAPTOP)	+= dell-laptop.o
 obj-$(CONFIG_DELL_WMI)		+= dell-wmi.o
 obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.o
diff --git a/drivers/platform/x86/dell-smbios-base.c b/drivers/platform/x86/dell-smbios-base.c
index 76b9d75..5bcf8a1 100644
--- a/drivers/platform/x86/dell-smbios-base.c
+++ b/drivers/platform/x86/dell-smbios-base.c
@@ -556,7 +556,7 @@ static void free_group(struct platform_device *pdev)
 static int __init dell_smbios_init(void)
 {
 	const struct dmi_device *valid;
-	int ret;
+	int ret, wmi, smm;
 
 	valid = dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL);
 	if (!valid) {
@@ -591,8 +591,24 @@ static int __init dell_smbios_init(void)
 	if (ret)
 		goto fail_create_group;
 
+	/* register backends */
+	wmi = init_dell_smbios_wmi();
+	if (wmi)
+		pr_debug("Failed to initialize WMI backend: %d\n", wmi);
+	smm = init_dell_smbios_smm();
+	if (smm)
+		pr_debug("Failed to initialize SMM backend: %d\n", smm);
+	if (wmi && smm) {
+		pr_err("No SMBIOS backends available (wmi: %d, smm: %d)\n",
+			wmi, smm);
+		goto fail_sysfs;
+	}
+
 	return 0;
 
+fail_sysfs:
+	free_group(platform_device);
+
 fail_create_group:
 	platform_device_del(platform_device);
 
@@ -609,6 +625,8 @@ static int __init dell_smbios_init(void)
 
 static void __exit dell_smbios_exit(void)
 {
+	exit_dell_smbios_wmi();
+	exit_dell_smbios_smm();
 	mutex_lock(&smbios_mutex);
 	if (platform_device) {
 		free_group(platform_device);
@@ -625,5 +643,6 @@ module_exit(dell_smbios_exit);
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
 MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>");
 MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
+MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
 MODULE_DESCRIPTION("Common functions for kernel modules using Dell SMBIOS");
 MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-smbios-smm.c b/drivers/platform/x86/dell-smbios-smm.c
index 89f65c4..e9e9da5 100644
--- a/drivers/platform/x86/dell-smbios-smm.c
+++ b/drivers/platform/x86/dell-smbios-smm.c
@@ -58,7 +58,7 @@ static const struct dmi_system_id dell_device_table[] __initconst = {
 };
 MODULE_DEVICE_TABLE(dmi, dell_device_table);
 
-static void __init parse_da_table(const struct dmi_header *dm)
+static void parse_da_table(const struct dmi_header *dm)
 {
 	struct calling_interface_structure *table =
 		container_of(dm, struct calling_interface_structure, header);
@@ -73,7 +73,7 @@ static void __init parse_da_table(const struct dmi_header *dm)
 	da_command_code = table->cmdIOCode;
 }
 
-static void __init find_cmd_address(const struct dmi_header *dm, void *dummy)
+static void find_cmd_address(const struct dmi_header *dm, void *dummy)
 {
 	switch (dm->type) {
 	case 0xda: /* Calling interface */
@@ -128,7 +128,7 @@ static bool test_wsmt_enabled(void)
 	return false;
 }
 
-static int __init dell_smbios_smm_init(void)
+int init_dell_smbios_smm(void)
 {
 	int ret;
 	/*
@@ -176,7 +176,7 @@ static int __init dell_smbios_smm_init(void)
 	return ret;
 }
 
-static void __exit dell_smbios_smm_exit(void)
+void exit_dell_smbios_smm(void)
 {
 	if (platform_device) {
 		dell_smbios_unregister_device(&platform_device->dev);
@@ -184,13 +184,3 @@ static void __exit dell_smbios_smm_exit(void)
 		free_page((unsigned long)buffer);
 	}
 }
-
-subsys_initcall(dell_smbios_smm_init);
-module_exit(dell_smbios_smm_exit);
-
-MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
-MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>");
-MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
-MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
-MODULE_DESCRIPTION("Dell SMBIOS communications over SMI");
-MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-smbios-wmi.c
index 609557a..fbefedb 100644
--- a/drivers/platform/x86/dell-smbios-wmi.c
+++ b/drivers/platform/x86/dell-smbios-wmi.c
@@ -228,7 +228,7 @@ static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
 	{ },
 };
 
-static void __init parse_b1_table(const struct dmi_header *dm)
+static void parse_b1_table(const struct dmi_header *dm)
 {
 	struct misc_bios_flags_structure *flags =
 	container_of(dm, struct misc_bios_flags_structure, header);
@@ -242,7 +242,7 @@ static void __init parse_b1_table(const struct dmi_header *dm)
 		wmi_supported = 1;
 }
 
-static void __init find_b1(const struct dmi_header *dm, void *dummy)
+static void find_b1(const struct dmi_header *dm, void *dummy)
 {
 	switch (dm->type) {
 	case 0xb1: /* misc bios flags */
@@ -261,7 +261,7 @@ static struct wmi_driver dell_smbios_wmi_driver = {
 	.filter_callback = dell_smbios_wmi_filter,
 };
 
-static int __init init_dell_smbios_wmi(void)
+int init_dell_smbios_wmi(void)
 {
 	dmi_walk(find_b1, NULL);
 
@@ -271,15 +271,9 @@ static int __init init_dell_smbios_wmi(void)
 	return wmi_driver_register(&dell_smbios_wmi_driver);
 }
 
-static void __exit exit_dell_smbios_wmi(void)
+void exit_dell_smbios_wmi(void)
 {
 	wmi_driver_unregister(&dell_smbios_wmi_driver);
 }
 
-module_init(init_dell_smbios_wmi);
-module_exit(exit_dell_smbios_wmi);
-
 MODULE_ALIAS("wmi:" DELL_WMI_SMBIOS_GUID);
-MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
-MODULE_DESCRIPTION("Dell SMBIOS communications over WMI");
-MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 138d478..d8adaf9 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -75,4 +75,29 @@ int dell_laptop_register_notifier(struct notifier_block *nb);
 int dell_laptop_unregister_notifier(struct notifier_block *nb);
 void dell_laptop_call_notifier(unsigned long action, void *data);
 
-#endif
+/* for the supported backends */
+#ifdef CONFIG_DELL_SMBIOS_WMI
+int init_dell_smbios_wmi(void);
+void exit_dell_smbios_wmi(void);
+#else /* CONFIG_DELL_SMBIOS_WMI */
+static inline int init_dell_smbios_wmi(void)
+{
+	return -ENODEV;
+}
+static inline void exit_dell_smbios_wmi(void)
+{}
+#endif /* CONFIG_DELL_SMBIOS_WMI */
+
+#ifdef CONFIG_DELL_SMBIOS_SMM
+int init_dell_smbios_smm(void);
+void exit_dell_smbios_smm(void);
+#else /* CONFIG_DELL_SMBIOS_SMM */
+static inline int init_dell_smbios_smm(void)
+{
+	return -ENODEV;
+}
+static inline void exit_dell_smbios_smm(void)
+{}
+#endif /* CONFIG_DELL_SMBIOS_SMM */
+
+#endif /* _DELL_SMBIOS_H_ */
-- 
2.7.4

Re: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-* modules together

[Darren Hart]

On Tue, Feb 27, 2018 at 12:23:04PM -0600, Mario Limonciello wrote:
> Some race conditions were raised due to dell-smbios and its backends
> not being ready by the time that a consumer would call one of the
> exported methods.
> 
> To avoid this problem, guarantee that all initialization has been
> done by linking them all together and running init for them all.
> 
> As part of this change the Kconfig needs to be adjusted so that
> CONFIG_DELL_SMBIOS_SMM and CONFIG_DELL_SMBIOS_WMI are boolean
> rather than modules.
> 
> CONFIG_DELL_SMBIOS is a visually selectable option again and both
> CONFIG_DELL_SMBIOS_WMI and CONFIG_DELL_SMBIOS_SMM are optional.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
> Changes from v2:
>  * Move rest of #ifdef out of source files
>  * Only compile units as necessary
> 
>  drivers/platform/x86/Kconfig            | 11 ++++++++---
>  drivers/platform/x86/Makefile           |  4 ++--
>  drivers/platform/x86/dell-smbios-base.c | 21 ++++++++++++++++++++-
>  drivers/platform/x86/dell-smbios-smm.c  | 18 ++++--------------
>  drivers/platform/x86/dell-smbios-wmi.c  | 14 ++++----------
>  drivers/platform/x86/dell-smbios.h      | 27 ++++++++++++++++++++++++++-
>  6 files changed, 64 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 9a8f964..0c0897e 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -106,10 +106,15 @@ config ASUS_LAPTOP
>  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
>  
>  config DELL_SMBIOS
> -	tristate
> +	tristate "Dell SMBIOS driver"
> +	---help---
> +	This provides support for the Dell SMBIOS calling interface.
> +	If you have a Dell computer you should enable this option.
> +
> +	Be sure to select at least one backend for it to work properly.

In order to make the default sane, I would suggest making the above a menu
(default n), nesting the two options below, and making WMI a default=y. This
satisfies Linus' requirement by defaulting everything to n, but enables a sane
default IFF the DELL_SMBIOS menu is enabled.

Alternatively, all of the above can be put under the "Dell Extras" menu I
proposed, and DELL_SMBIOS can default to m and DELL_SMBIOS_WMI can default to y.

>  
> +	/* register backends */
> +	wmi = init_dell_smbios_wmi();
> +	if (wmi)
> +		pr_debug("Failed to initialize WMI backend: %d\n", wmi);
> +	smm = init_dell_smbios_smm();
> +	if (smm)
> +		pr_debug("Failed to initialize SMM backend: %d\n", smm);
> +	if (wmi && smm) {
> +		pr_err("No SMBIOS backends available (wmi: %d, smm: %d)\n",
> +			wmi, smm);
> +		goto fail_sysfs;
> +	}
> +

I like the approach of building a library instead of a bunch of separate
modules.

-- 
Darren Hart
VMware Open Source Technology Center

RE: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-* modules together

[Mario.Limonciello]

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Thursday, March 1, 2018 11:49 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; pali.rohar@gmail.com;
> LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org
> Subject: Re: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-*
> modules together
> 
> On Tue, Feb 27, 2018 at 12:23:04PM -0600, Mario Limonciello wrote:
> > Some race conditions were raised due to dell-smbios and its backends
> > not being ready by the time that a consumer would call one of the
> > exported methods.
> >
> > To avoid this problem, guarantee that all initialization has been
> > done by linking them all together and running init for them all.
> >
> > As part of this change the Kconfig needs to be adjusted so that
> > CONFIG_DELL_SMBIOS_SMM and CONFIG_DELL_SMBIOS_WMI are boolean
> > rather than modules.
> >
> > CONFIG_DELL_SMBIOS is a visually selectable option again and both
> > CONFIG_DELL_SMBIOS_WMI and CONFIG_DELL_SMBIOS_SMM are optional.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > ---
> > Changes from v2:
> >  * Move rest of #ifdef out of source files
> >  * Only compile units as necessary
> >
> >  drivers/platform/x86/Kconfig            | 11 ++++++++---
> >  drivers/platform/x86/Makefile           |  4 ++--
> >  drivers/platform/x86/dell-smbios-base.c | 21 ++++++++++++++++++++-
> >  drivers/platform/x86/dell-smbios-smm.c  | 18 ++++--------------
> >  drivers/platform/x86/dell-smbios-wmi.c  | 14 ++++----------
> >  drivers/platform/x86/dell-smbios.h      | 27 ++++++++++++++++++++++++++-
> >  6 files changed, 64 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 9a8f964..0c0897e 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -106,10 +106,15 @@ config ASUS_LAPTOP
> >  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
> >
> >  config DELL_SMBIOS
> > -	tristate
> > +	tristate "Dell SMBIOS driver"
> > +	---help---
> > +	This provides support for the Dell SMBIOS calling interface.
> > +	If you have a Dell computer you should enable this option.
> > +
> > +	Be sure to select at least one backend for it to work properly.
> 
> In order to make the default sane, I would suggest making the above a menu
> (default n), nesting the two options below, and making WMI a default=y. This
> satisfies Linus' requirement by defaulting everything to n, but enables a sane
> default IFF the DELL_SMBIOS menu is enabled.
> 
> Alternatively, all of the above can be put under the "Dell Extras" menu I
> proposed, and DELL_SMBIOS can default to m and DELL_SMBIOS_WMI can default
> to y.

I'm on board with moving everything to a Dell extras menu.  There are so many
drivers now, it really seems like the best way to make sure that people get them.

You'll have to actively go out of your way to turn off one of the sub-drivers then
to get into the situation that was described.

Since you did the original patch for dell extras nested in a mail would you mind 
sending that as a follow up second patch on top of my patch?

I think it's probably too much to do it all in this same patch itself.
Alternatively I can re-send my series with it before my 3/3, up to you.

> 
> >
> > +	/* register backends */
> > +	wmi = init_dell_smbios_wmi();
> > +	if (wmi)
> > +		pr_debug("Failed to initialize WMI backend: %d\n", wmi);
> > +	smm = init_dell_smbios_smm();
> > +	if (smm)
> > +		pr_debug("Failed to initialize SMM backend: %d\n", smm);
> > +	if (wmi && smm) {
> > +		pr_err("No SMBIOS backends available (wmi: %d, smm: %d)\n",
> > +			wmi, smm);
> > +		goto fail_sysfs;
> > +	}
> > +
> 
> I like the approach of building a library instead of a bunch of separate
> modules.
> 
> --
> Darren Hart
> VMware Open Source Technology Center

Re: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-* modules together

[Darren Hart]

On Thu, Mar 01, 2018 at 08:54:31PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Darren Hart [mailto:dvhart@infradead.org]
> > Sent: Thursday, March 1, 2018 11:49 AM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; pali.rohar@gmail.com;
> > LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org
> > Subject: Re: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-*
> > modules together
> > 
> > On Tue, Feb 27, 2018 at 12:23:04PM -0600, Mario Limonciello wrote:
> > > Some race conditions were raised due to dell-smbios and its backends
> > > not being ready by the time that a consumer would call one of the
> > > exported methods.
> > >
> > > To avoid this problem, guarantee that all initialization has been
> > > done by linking them all together and running init for them all.
> > >
> > > As part of this change the Kconfig needs to be adjusted so that
> > > CONFIG_DELL_SMBIOS_SMM and CONFIG_DELL_SMBIOS_WMI are boolean
> > > rather than modules.
> > >
> > > CONFIG_DELL_SMBIOS is a visually selectable option again and both
> > > CONFIG_DELL_SMBIOS_WMI and CONFIG_DELL_SMBIOS_SMM are optional.
> > >
> > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > ---
> > > Changes from v2:
> > >  * Move rest of #ifdef out of source files
> > >  * Only compile units as necessary
> > >
> > >  drivers/platform/x86/Kconfig            | 11 ++++++++---
> > >  drivers/platform/x86/Makefile           |  4 ++--
> > >  drivers/platform/x86/dell-smbios-base.c | 21 ++++++++++++++++++++-
> > >  drivers/platform/x86/dell-smbios-smm.c  | 18 ++++--------------
> > >  drivers/platform/x86/dell-smbios-wmi.c  | 14 ++++----------
> > >  drivers/platform/x86/dell-smbios.h      | 27 ++++++++++++++++++++++++++-
> > >  6 files changed, 64 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > index 9a8f964..0c0897e 100644
> > > --- a/drivers/platform/x86/Kconfig
> > > +++ b/drivers/platform/x86/Kconfig
> > > @@ -106,10 +106,15 @@ config ASUS_LAPTOP
> > >  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
> > >
> > >  config DELL_SMBIOS
> > > -	tristate
> > > +	tristate "Dell SMBIOS driver"
> > > +	---help---
> > > +	This provides support for the Dell SMBIOS calling interface.
> > > +	If you have a Dell computer you should enable this option.
> > > +
> > > +	Be sure to select at least one backend for it to work properly.
> > 
> > In order to make the default sane, I would suggest making the above a menu
> > (default n), nesting the two options below, and making WMI a default=y. This
> > satisfies Linus' requirement by defaulting everything to n, but enables a sane
> > default IFF the DELL_SMBIOS menu is enabled.
> > 
> > Alternatively, all of the above can be put under the "Dell Extras" menu I
> > proposed, and DELL_SMBIOS can default to m and DELL_SMBIOS_WMI can default
> > to y.
> 
> I'm on board with moving everything to a Dell extras menu.  There are so many
> drivers now, it really seems like the best way to make sure that people get them.
> 
> You'll have to actively go out of your way to turn off one of the sub-drivers then
> to get into the situation that was described.
> 
> Since you did the original patch for dell extras nested in a mail would you mind 
> sending that as a follow up second patch on top of my patch?
> 
> I think it's probably too much to do it all in this same patch itself.

Yup, easier for me to wrangle that all on my end. I'll look at your 3 again with
that in mind and let you know if I think any changes are needed, or just update
mine to apply on top.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-* modules together

[Darren Hart]

On Tue, Feb 27, 2018 at 12:23:04PM -0600, Mario Limonciello wrote:
> Some race conditions were raised due to dell-smbios and its backends
> not being ready by the time that a consumer would call one of the
> exported methods.
> 
> To avoid this problem, guarantee that all initialization has been
> done by linking them all together and running init for them all.
> 
> As part of this change the Kconfig needs to be adjusted so that
> CONFIG_DELL_SMBIOS_SMM and CONFIG_DELL_SMBIOS_WMI are boolean
> rather than modules.
> 
> CONFIG_DELL_SMBIOS is a visually selectable option again and both
> CONFIG_DELL_SMBIOS_WMI and CONFIG_DELL_SMBIOS_SMM are optional.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
> Changes from v2:
>  * Move rest of #ifdef out of source files
>  * Only compile units as necessary
> 
>  drivers/platform/x86/Kconfig            | 11 ++++++++---
>  drivers/platform/x86/Makefile           |  4 ++--
>  drivers/platform/x86/dell-smbios-base.c | 21 ++++++++++++++++++++-
>  drivers/platform/x86/dell-smbios-smm.c  | 18 ++++--------------
>  drivers/platform/x86/dell-smbios-wmi.c  | 14 ++++----------
>  drivers/platform/x86/dell-smbios.h      | 27 ++++++++++++++++++++++++++-
>  6 files changed, 64 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 9a8f964..0c0897e 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -106,10 +106,15 @@ config ASUS_LAPTOP
>  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
>  
>  config DELL_SMBIOS
> -	tristate
> +	tristate "Dell SMBIOS driver"
> +	---help---
> +	This provides support for the Dell SMBIOS calling interface.
> +	If you have a Dell computer you should enable this option.
> +
> +	Be sure to select at least one backend for it to work properly.
>  
>  config DELL_SMBIOS_WMI
> -	tristate "Dell SMBIOS calling interface (WMI implementation)"
> +	bool "Dell SMBIOS driver WMI support"

Would you be happy with using "backend" here and for SMM?

	bool "Dell SMBIOS driver WMI backend"

It makes it obvious what the help text is referring to by "select at
least one backend".

Happy to just make this change locally, no need to resubmit.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-* modules together

[Darren Hart]

On Tue, Feb 27, 2018 at 12:23:04PM -0600, Mario Limonciello wrote:
> Some race conditions were raised due to dell-smbios and its backends
> not being ready by the time that a consumer would call one of the
> exported methods.
> 
> To avoid this problem, guarantee that all initialization has been
> done by linking them all together and running init for them all.
> 
> As part of this change the Kconfig needs to be adjusted so that
> CONFIG_DELL_SMBIOS_SMM and CONFIG_DELL_SMBIOS_WMI are boolean
> rather than modules.
> 
> CONFIG_DELL_SMBIOS is a visually selectable option again and both
> CONFIG_DELL_SMBIOS_WMI and CONFIG_DELL_SMBIOS_SMM are optional.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
> Changes from v2:
>  * Move rest of #ifdef out of source files
>  * Only compile units as necessary
> 
>  drivers/platform/x86/Kconfig            | 11 ++++++++---
>  drivers/platform/x86/Makefile           |  4 ++--
>  drivers/platform/x86/dell-smbios-base.c | 21 ++++++++++++++++++++-
>  drivers/platform/x86/dell-smbios-smm.c  | 18 ++++--------------
>  drivers/platform/x86/dell-smbios-wmi.c  | 14 ++++----------
>  drivers/platform/x86/dell-smbios.h      | 27 ++++++++++++++++++++++++++-
>  6 files changed, 64 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 9a8f964..0c0897e 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -106,10 +106,15 @@ config ASUS_LAPTOP
>  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
>  
>  config DELL_SMBIOS
> -	tristate
> +	tristate "Dell SMBIOS driver"
> +	---help---
> +	This provides support for the Dell SMBIOS calling interface.
> +	If you have a Dell computer you should enable this option.
> +
> +	Be sure to select at least one backend for it to work properly.
>  
>  config DELL_SMBIOS_WMI
> -	tristate "Dell SMBIOS calling interface (WMI implementation)"
> +	bool "Dell SMBIOS driver WMI support"

Oops, the help text still says Y or M for this and SMM - I'll correct locally.

-- 
Darren Hart
VMware Open Source Technology Center

RE: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-* modules together

[Mario.Limonciello]

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Friday, March 2, 2018 6:22 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; pali.rohar@gmail.com; LKML
> <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org
> Subject: Re: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-*
> modules together
> 
> On Tue, Feb 27, 2018 at 12:23:04PM -0600, Mario Limonciello wrote:
> > Some race conditions were raised due to dell-smbios and its backends
> > not being ready by the time that a consumer would call one of the
> > exported methods.
> >
> > To avoid this problem, guarantee that all initialization has been
> > done by linking them all together and running init for them all.
> >
> > As part of this change the Kconfig needs to be adjusted so that
> > CONFIG_DELL_SMBIOS_SMM and CONFIG_DELL_SMBIOS_WMI are boolean
> > rather than modules.
> >
> > CONFIG_DELL_SMBIOS is a visually selectable option again and both
> > CONFIG_DELL_SMBIOS_WMI and CONFIG_DELL_SMBIOS_SMM are optional.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > ---
> > Changes from v2:
> >  * Move rest of #ifdef out of source files
> >  * Only compile units as necessary
> >
> >  drivers/platform/x86/Kconfig            | 11 ++++++++---
> >  drivers/platform/x86/Makefile           |  4 ++--
> >  drivers/platform/x86/dell-smbios-base.c | 21 ++++++++++++++++++++-
> >  drivers/platform/x86/dell-smbios-smm.c  | 18 ++++--------------
> >  drivers/platform/x86/dell-smbios-wmi.c  | 14 ++++----------
> >  drivers/platform/x86/dell-smbios.h      | 27 ++++++++++++++++++++++++++-
> >  6 files changed, 64 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 9a8f964..0c0897e 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -106,10 +106,15 @@ config ASUS_LAPTOP
> >  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
> >
> >  config DELL_SMBIOS
> > -	tristate
> > +	tristate "Dell SMBIOS driver"
> > +	---help---
> > +	This provides support for the Dell SMBIOS calling interface.
> > +	If you have a Dell computer you should enable this option.
> > +
> > +	Be sure to select at least one backend for it to work properly.
> >
> >  config DELL_SMBIOS_WMI
> > -	tristate "Dell SMBIOS calling interface (WMI implementation)"
> > +	bool "Dell SMBIOS driver WMI support"
> 
> Would you be happy with using "backend" here and for SMM?
> 
> 	bool "Dell SMBIOS driver WMI backend"
> 
> It makes it obvious what the help text is referring to by "select at
> least one backend".
> 
> Happy to just make this change locally, no need to resubmit.
> 

Yeah, fine by me.

Thanks,

Re: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-* modules together

[Darren Hart]

On Tue, Feb 27, 2018 at 12:23:04PM -0600, Mario Limonciello wrote:
> Some race conditions were raised due to dell-smbios and its backends
> not being ready by the time that a consumer would call one of the
> exported methods.
> 
> To avoid this problem, guarantee that all initialization has been
> done by linking them all together and running init for them all.
> 
> As part of this change the Kconfig needs to be adjusted so that
> CONFIG_DELL_SMBIOS_SMM and CONFIG_DELL_SMBIOS_WMI are boolean
> rather than modules.
> 
> CONFIG_DELL_SMBIOS is a visually selectable option again and both
> CONFIG_DELL_SMBIOS_WMI and CONFIG_DELL_SMBIOS_SMM are optional.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
> Changes from v2:
>  * Move rest of #ifdef out of source files
>  * Only compile units as necessary
> 
>  drivers/platform/x86/Kconfig            | 11 ++++++++---
>  drivers/platform/x86/Makefile           |  4 ++--
>  drivers/platform/x86/dell-smbios-base.c | 21 ++++++++++++++++++++-
>  drivers/platform/x86/dell-smbios-smm.c  | 18 ++++--------------
>  drivers/platform/x86/dell-smbios-wmi.c  | 14 ++++----------
>  drivers/platform/x86/dell-smbios.h      | 27 ++++++++++++++++++++++++++-
>  6 files changed, 64 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 9a8f964..0c0897e 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -106,10 +106,15 @@ config ASUS_LAPTOP
>  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
>  
>  config DELL_SMBIOS
> -	tristate
> +	tristate "Dell SMBIOS driver"
> +	---help---
> +	This provides support for the Dell SMBIOS calling interface.
> +	If you have a Dell computer you should enable this option.
> +
> +	Be sure to select at least one backend for it to work properly.
>  
>  config DELL_SMBIOS_WMI
> -	tristate "Dell SMBIOS calling interface (WMI implementation)"
> +	bool "Dell SMBIOS driver WMI support"
>  	depends on ACPI_WMI
>  	select DELL_WMI_DESCRIPTOR
>  	select DELL_SMBIOS

Blarg, this also violates the kconfig-language guidance for using select
only for non-visible symbols with no dependencies.

I have a patch that works, but I don't love it. We can remove the prompt
from DELL_SMBIOS, and make DELL_SMBIOS_WMI|SMM "depend on" DELL_SMBIOS.
The options just appear sort of randomly when either DELL_LAPTOP or
DELL_WMI are selected - but they can default to y, which is nice.

I have a subsequent patch which makes DELL_SMBIOS_WMI|SMM "depend on"
DELL_LAPTOP, which makes this look a lot nicer. These just appear under
DELL_LAPTOP as bool options. The problem with this approach is DELL_WMI
also requires DELL_SMBIOS and DELL_WMI doesn't currently depend on
DELL_LAPTOP. One option is to make it depend on DELL_LAPTOP as it seems
unlikely for them to ever be used separately.  I don't love this either.
Thoughts?

Perhaps the compromise is leave the DELL_SMBIOS prompt. This doesn't
have the negative consequences the kconfig-languages is trying to guard
against, and it makes it obvious what the DELL_SMBIOS_* options are -
and we can still default them to y. See below.

This seems like it might be the least bad option at the moment. I'll
keep thinking on it a bit...

From: "Darren Hart (VMware)" <dvhart@infradead.org>
Date: Fri, 2 Mar 2018 17:40:32 -0800
Subject: [PATCH] platform/x86: Allow for SMBIOS backend defaults

Avoid accidental configurations by setting default y for DELL_SMBIOS
backends. Avoid this impacting the default build size, by making them
dependent on DELL_SMBIOS, so they only appear when DELL_SMBIOS is
manually selected, or by DELL_LAPTOP or DELL_WMI.

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

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index e3f682c..022c837 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -115,9 +115,10 @@ config DELL_SMBIOS
 
 config DELL_SMBIOS_WMI
 	bool "Dell SMBIOS driver WMI backend"
+	default y
 	depends on ACPI_WMI
 	select DELL_WMI_DESCRIPTOR
-	select DELL_SMBIOS
+	depends on DELL_SMBIOS
 	---help---
 	This provides an implementation for the Dell SMBIOS calling interface
 	communicated over ACPI-WMI.
@@ -128,8 +129,9 @@ config DELL_SMBIOS_WMI
 
 config DELL_SMBIOS_SMM
 	bool "Dell SMBIOS driver SMM backend"
+	default y
 	depends on DCDBAS
-	select DELL_SMBIOS
+	depends on DELL_SMBIOS
 	---help---
 	This provides an implementation for the Dell SMBIOS calling interface
 	communicated over SMI/SMM.
-- 
2.9.5


-- 
Darren Hart
VMware Open Source Technology Center

RE: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-* modules together

[Mario.Limonciello]

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Friday, March 2, 2018 8:11 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; pali.rohar@gmail.com; LKML
> <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org
> Subject: Re: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-*
> modules together
> 
> On Tue, Feb 27, 2018 at 12:23:04PM -0600, Mario Limonciello wrote:
> > Some race conditions were raised due to dell-smbios and its backends
> > not being ready by the time that a consumer would call one of the
> > exported methods.
> >
> > To avoid this problem, guarantee that all initialization has been
> > done by linking them all together and running init for them all.
> >
> > As part of this change the Kconfig needs to be adjusted so that
> > CONFIG_DELL_SMBIOS_SMM and CONFIG_DELL_SMBIOS_WMI are boolean
> > rather than modules.
> >
> > CONFIG_DELL_SMBIOS is a visually selectable option again and both
> > CONFIG_DELL_SMBIOS_WMI and CONFIG_DELL_SMBIOS_SMM are optional.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > ---
> > Changes from v2:
> >  * Move rest of #ifdef out of source files
> >  * Only compile units as necessary
> >
> >  drivers/platform/x86/Kconfig            | 11 ++++++++---
> >  drivers/platform/x86/Makefile           |  4 ++--
> >  drivers/platform/x86/dell-smbios-base.c | 21 ++++++++++++++++++++-
> >  drivers/platform/x86/dell-smbios-smm.c  | 18 ++++--------------
> >  drivers/platform/x86/dell-smbios-wmi.c  | 14 ++++----------
> >  drivers/platform/x86/dell-smbios.h      | 27 ++++++++++++++++++++++++++-
> >  6 files changed, 64 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 9a8f964..0c0897e 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -106,10 +106,15 @@ config ASUS_LAPTOP
> >  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
> >
> >  config DELL_SMBIOS
> > -	tristate
> > +	tristate "Dell SMBIOS driver"
> > +	---help---
> > +	This provides support for the Dell SMBIOS calling interface.
> > +	If you have a Dell computer you should enable this option.
> > +
> > +	Be sure to select at least one backend for it to work properly.
> >
> >  config DELL_SMBIOS_WMI
> > -	tristate "Dell SMBIOS calling interface (WMI implementation)"
> > +	bool "Dell SMBIOS driver WMI support"
> >  	depends on ACPI_WMI
> >  	select DELL_WMI_DESCRIPTOR
> >  	select DELL_SMBIOS
> 
> Blarg, this also violates the kconfig-language guidance for using select
> only for non-visible symbols with no dependencies.
> 
> I have a patch that works, but I don't love it. We can remove the prompt
> from DELL_SMBIOS, and make DELL_SMBIOS_WMI|SMM "depend on"
> DELL_SMBIOS.
> The options just appear sort of randomly when either DELL_LAPTOP or
> DELL_WMI are selected - but they can default to y, which is nice.
> 
> I have a subsequent patch which makes DELL_SMBIOS_WMI|SMM "depend on"
> DELL_LAPTOP, which makes this look a lot nicer. These just appear under
> DELL_LAPTOP as bool options. The problem with this approach is DELL_WMI
> also requires DELL_SMBIOS and DELL_WMI doesn't currently depend on
> DELL_LAPTOP. One option is to make it depend on DELL_LAPTOP as it seems
> unlikely for them to ever be used separately.  I don't love this either.
> Thoughts?
> 
> Perhaps the compromise is leave the DELL_SMBIOS prompt. This doesn't
> have the negative consequences the kconfig-languages is trying to guard
> against, and it makes it obvious what the DELL_SMBIOS_* options are -
> and we can still default them to y. See below.
> 
> This seems like it might be the least bad option at the moment. I'll
> keep thinking on it a bit...
> 
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
> Date: Fri, 2 Mar 2018 17:40:32 -0800
> Subject: [PATCH] platform/x86: Allow for SMBIOS backend defaults
> 
> Avoid accidental configurations by setting default y for DELL_SMBIOS
> backends. Avoid this impacting the default build size, by making them
> dependent on DELL_SMBIOS, so they only appear when DELL_SMBIOS is
> manually selected, or by DELL_LAPTOP or DELL_WMI.
> 
> Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
> ---
>  drivers/platform/x86/Kconfig | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index e3f682c..022c837 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -115,9 +115,10 @@ config DELL_SMBIOS
> 
>  config DELL_SMBIOS_WMI
>  	bool "Dell SMBIOS driver WMI backend"
> +	default y
>  	depends on ACPI_WMI
>  	select DELL_WMI_DESCRIPTOR
> -	select DELL_SMBIOS
> +	depends on DELL_SMBIOS
>  	---help---
>  	This provides an implementation for the Dell SMBIOS calling interface
>  	communicated over ACPI-WMI.
> @@ -128,8 +129,9 @@ config DELL_SMBIOS_WMI
> 
>  config DELL_SMBIOS_SMM
>  	bool "Dell SMBIOS driver SMM backend"
> +	default y
>  	depends on DCDBAS
> -	select DELL_SMBIOS
> +	depends on DELL_SMBIOS
>  	---help---
>  	This provides an implementation for the Dell SMBIOS calling interface
>  	communicated over SMI/SMM.
> --
> 2.9.5
> 
> 

Ooph. I think the compromise of leaving the dell smbios prompt and sane defaults
behind extras is the best.
With it being a library now there shouldn't be a way for someone to break other
modules in unexpected ways.  Init will fail for dell-smbios and then nothing
else will be able to use it.

Re: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-* modules together

[Darren Hart]

On Sat, Mar 03, 2018 at 11:59:55AM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Darren Hart [mailto:dvhart@infradead.org]
> > Sent: Friday, March 2, 2018 8:11 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; pali.rohar@gmail.com; LKML
> > <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org
> > Subject: Re: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-*
> > modules together
> > 
> > On Tue, Feb 27, 2018 at 12:23:04PM -0600, Mario Limonciello wrote:
> > > Some race conditions were raised due to dell-smbios and its backends
> > > not being ready by the time that a consumer would call one of the
> > > exported methods.
> > >
> > > To avoid this problem, guarantee that all initialization has been
> > > done by linking them all together and running init for them all.
> > >
> > > As part of this change the Kconfig needs to be adjusted so that
> > > CONFIG_DELL_SMBIOS_SMM and CONFIG_DELL_SMBIOS_WMI are boolean
> > > rather than modules.
> > >
> > > CONFIG_DELL_SMBIOS is a visually selectable option again and both
> > > CONFIG_DELL_SMBIOS_WMI and CONFIG_DELL_SMBIOS_SMM are optional.
> > >
> > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > ---
> > > Changes from v2:
> > >  * Move rest of #ifdef out of source files
> > >  * Only compile units as necessary
> > >
> > >  drivers/platform/x86/Kconfig            | 11 ++++++++---
> > >  drivers/platform/x86/Makefile           |  4 ++--
> > >  drivers/platform/x86/dell-smbios-base.c | 21 ++++++++++++++++++++-
> > >  drivers/platform/x86/dell-smbios-smm.c  | 18 ++++--------------
> > >  drivers/platform/x86/dell-smbios-wmi.c  | 14 ++++----------
> > >  drivers/platform/x86/dell-smbios.h      | 27 ++++++++++++++++++++++++++-
> > >  6 files changed, 64 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > index 9a8f964..0c0897e 100644
> > > --- a/drivers/platform/x86/Kconfig
> > > +++ b/drivers/platform/x86/Kconfig
> > > @@ -106,10 +106,15 @@ config ASUS_LAPTOP
> > >  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
> > >
> > >  config DELL_SMBIOS
> > > -	tristate
> > > +	tristate "Dell SMBIOS driver"
> > > +	---help---
> > > +	This provides support for the Dell SMBIOS calling interface.
> > > +	If you have a Dell computer you should enable this option.
> > > +
> > > +	Be sure to select at least one backend for it to work properly.
> > >
> > >  config DELL_SMBIOS_WMI
> > > -	tristate "Dell SMBIOS calling interface (WMI implementation)"
> > > +	bool "Dell SMBIOS driver WMI support"
> > >  	depends on ACPI_WMI
> > >  	select DELL_WMI_DESCRIPTOR
> > >  	select DELL_SMBIOS
> > 
> > Blarg, this also violates the kconfig-language guidance for using select
> > only for non-visible symbols with no dependencies.
> > 
> > I have a patch that works, but I don't love it. We can remove the prompt
> > from DELL_SMBIOS, and make DELL_SMBIOS_WMI|SMM "depend on"
> > DELL_SMBIOS.
> > The options just appear sort of randomly when either DELL_LAPTOP or
> > DELL_WMI are selected - but they can default to y, which is nice.
> > 
> > I have a subsequent patch which makes DELL_SMBIOS_WMI|SMM "depend on"
> > DELL_LAPTOP, which makes this look a lot nicer. These just appear under
> > DELL_LAPTOP as bool options. The problem with this approach is DELL_WMI
> > also requires DELL_SMBIOS and DELL_WMI doesn't currently depend on
> > DELL_LAPTOP. One option is to make it depend on DELL_LAPTOP as it seems
> > unlikely for them to ever be used separately.  I don't love this either.
> > Thoughts?
> > 
> > Perhaps the compromise is leave the DELL_SMBIOS prompt. This doesn't
> > have the negative consequences the kconfig-languages is trying to guard
> > against, and it makes it obvious what the DELL_SMBIOS_* options are -
> > and we can still default them to y. See below.
> > 
> > This seems like it might be the least bad option at the moment. I'll
> > keep thinking on it a bit...
> > 
> > From: "Darren Hart (VMware)" <dvhart@infradead.org>
> > Date: Fri, 2 Mar 2018 17:40:32 -0800
> > Subject: [PATCH] platform/x86: Allow for SMBIOS backend defaults
> > 
> > Avoid accidental configurations by setting default y for DELL_SMBIOS
> > backends. Avoid this impacting the default build size, by making them
> > dependent on DELL_SMBIOS, so they only appear when DELL_SMBIOS is
> > manually selected, or by DELL_LAPTOP or DELL_WMI.
> > 
> > Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
> > ---
> >  drivers/platform/x86/Kconfig | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index e3f682c..022c837 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -115,9 +115,10 @@ config DELL_SMBIOS
> > 
> >  config DELL_SMBIOS_WMI
> >  	bool "Dell SMBIOS driver WMI backend"
> > +	default y
> >  	depends on ACPI_WMI
> >  	select DELL_WMI_DESCRIPTOR
> > -	select DELL_SMBIOS
> > +	depends on DELL_SMBIOS
> >  	---help---
> >  	This provides an implementation for the Dell SMBIOS calling interface
> >  	communicated over ACPI-WMI.
> > @@ -128,8 +129,9 @@ config DELL_SMBIOS_WMI
> > 
> >  config DELL_SMBIOS_SMM
> >  	bool "Dell SMBIOS driver SMM backend"
> > +	default y
> >  	depends on DCDBAS
> > -	select DELL_SMBIOS
> > +	depends on DELL_SMBIOS
> >  	---help---
> >  	This provides an implementation for the Dell SMBIOS calling interface
> >  	communicated over SMI/SMM.
> > --
> > 2.9.5
> > 
> > 
> 
> Ooph. I think the compromise of leaving the dell smbios prompt and sane defaults
> behind extras is the best.
> With it being a library now there shouldn't be a way for someone to break other
> modules in unexpected ways.  Init will fail for dell-smbios and then nothing
> else will be able to use it.
> 

OK, I'm going to go with the above patch with a little more context about the
visible and selectable DELL_SMBIOS. We can revisit if all this belongs under a
DELL_EXTRAS menuconfig as a separate exercise.

I am planning on pushing your 3 and my patch above to the 4.16 rc cycle. Holler
if that wasn't your expectation.

-- 
Darren Hart
VMware Open Source Technology Center

RE: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-* modules together

[Mario.Limonciello]

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86-
> owner@vger.kernel.org] On Behalf Of Darren Hart
> Sent: Tuesday, March 6, 2018 7:49 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: andy.shevchenko@gmail.com; pali.rohar@gmail.com; linux-
> kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org
> Subject: Re: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-*
> modules together
> 
> On Sat, Mar 03, 2018 at 11:59:55AM +0000, Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Darren Hart [mailto:dvhart@infradead.org]
> > > Sent: Friday, March 2, 2018 8:11 PM
> > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; pali.rohar@gmail.com;
> LKML
> > > <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org
> > > Subject: Re: [PATCH v3 3/3] platform/x86: dell-smbios: Link all dell-smbios-*
> > > modules together
> > >
> > > On Tue, Feb 27, 2018 at 12:23:04PM -0600, Mario Limonciello wrote:
> > > > Some race conditions were raised due to dell-smbios and its backends
> > > > not being ready by the time that a consumer would call one of the
> > > > exported methods.
> > > >
> > > > To avoid this problem, guarantee that all initialization has been
> > > > done by linking them all together and running init for them all.
> > > >
> > > > As part of this change the Kconfig needs to be adjusted so that
> > > > CONFIG_DELL_SMBIOS_SMM and CONFIG_DELL_SMBIOS_WMI are boolean
> > > > rather than modules.
> > > >
> > > > CONFIG_DELL_SMBIOS is a visually selectable option again and both
> > > > CONFIG_DELL_SMBIOS_WMI and CONFIG_DELL_SMBIOS_SMM are optional.
> > > >
> > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > > ---
> > > > Changes from v2:
> > > >  * Move rest of #ifdef out of source files
> > > >  * Only compile units as necessary
> > > >
> > > >  drivers/platform/x86/Kconfig            | 11 ++++++++---
> > > >  drivers/platform/x86/Makefile           |  4 ++--
> > > >  drivers/platform/x86/dell-smbios-base.c | 21 ++++++++++++++++++++-
> > > >  drivers/platform/x86/dell-smbios-smm.c  | 18 ++++--------------
> > > >  drivers/platform/x86/dell-smbios-wmi.c  | 14 ++++----------
> > > >  drivers/platform/x86/dell-smbios.h      | 27 ++++++++++++++++++++++++++-
> > > >  6 files changed, 64 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > > index 9a8f964..0c0897e 100644
> > > > --- a/drivers/platform/x86/Kconfig
> > > > +++ b/drivers/platform/x86/Kconfig
> > > > @@ -106,10 +106,15 @@ config ASUS_LAPTOP
> > > >  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
> > > >
> > > >  config DELL_SMBIOS
> > > > -	tristate
> > > > +	tristate "Dell SMBIOS driver"
> > > > +	---help---
> > > > +	This provides support for the Dell SMBIOS calling interface.
> > > > +	If you have a Dell computer you should enable this option.
> > > > +
> > > > +	Be sure to select at least one backend for it to work properly.
> > > >
> > > >  config DELL_SMBIOS_WMI
> > > > -	tristate "Dell SMBIOS calling interface (WMI implementation)"
> > > > +	bool "Dell SMBIOS driver WMI support"
> > > >  	depends on ACPI_WMI
> > > >  	select DELL_WMI_DESCRIPTOR
> > > >  	select DELL_SMBIOS
> > >
> > > Blarg, this also violates the kconfig-language guidance for using select
> > > only for non-visible symbols with no dependencies.
> > >
> > > I have a patch that works, but I don't love it. We can remove the prompt
> > > from DELL_SMBIOS, and make DELL_SMBIOS_WMI|SMM "depend on"
> > > DELL_SMBIOS.
> > > The options just appear sort of randomly when either DELL_LAPTOP or
> > > DELL_WMI are selected - but they can default to y, which is nice.
> > >
> > > I have a subsequent patch which makes DELL_SMBIOS_WMI|SMM "depend on"
> > > DELL_LAPTOP, which makes this look a lot nicer. These just appear under
> > > DELL_LAPTOP as bool options. The problem with this approach is DELL_WMI
> > > also requires DELL_SMBIOS and DELL_WMI doesn't currently depend on
> > > DELL_LAPTOP. One option is to make it depend on DELL_LAPTOP as it seems
> > > unlikely for them to ever be used separately.  I don't love this either.
> > > Thoughts?
> > >
> > > Perhaps the compromise is leave the DELL_SMBIOS prompt. This doesn't
> > > have the negative consequences the kconfig-languages is trying to guard
> > > against, and it makes it obvious what the DELL_SMBIOS_* options are -
> > > and we can still default them to y. See below.
> > >
> > > This seems like it might be the least bad option at the moment. I'll
> > > keep thinking on it a bit...
> > >
> > > From: "Darren Hart (VMware)" <dvhart@infradead.org>
> > > Date: Fri, 2 Mar 2018 17:40:32 -0800
> > > Subject: [PATCH] platform/x86: Allow for SMBIOS backend defaults
> > >
> > > Avoid accidental configurations by setting default y for DELL_SMBIOS
> > > backends. Avoid this impacting the default build size, by making them
> > > dependent on DELL_SMBIOS, so they only appear when DELL_SMBIOS is
> > > manually selected, or by DELL_LAPTOP or DELL_WMI.
> > >
> > > Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
> > > ---
> > >  drivers/platform/x86/Kconfig | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > index e3f682c..022c837 100644
> > > --- a/drivers/platform/x86/Kconfig
> > > +++ b/drivers/platform/x86/Kconfig
> > > @@ -115,9 +115,10 @@ config DELL_SMBIOS
> > >
> > >  config DELL_SMBIOS_WMI
> > >  	bool "Dell SMBIOS driver WMI backend"
> > > +	default y
> > >  	depends on ACPI_WMI
> > >  	select DELL_WMI_DESCRIPTOR
> > > -	select DELL_SMBIOS
> > > +	depends on DELL_SMBIOS
> > >  	---help---
> > >  	This provides an implementation for the Dell SMBIOS calling interface
> > >  	communicated over ACPI-WMI.
> > > @@ -128,8 +129,9 @@ config DELL_SMBIOS_WMI
> > >
> > >  config DELL_SMBIOS_SMM
> > >  	bool "Dell SMBIOS driver SMM backend"
> > > +	default y
> > >  	depends on DCDBAS
> > > -	select DELL_SMBIOS
> > > +	depends on DELL_SMBIOS
> > >  	---help---
> > >  	This provides an implementation for the Dell SMBIOS calling interface
> > >  	communicated over SMI/SMM.
> > > --
> > > 2.9.5
> > >
> > >
> >
> > Ooph. I think the compromise of leaving the dell smbios prompt and sane defaults
> > behind extras is the best.
> > With it being a library now there shouldn't be a way for someone to break other
> > modules in unexpected ways.  Init will fail for dell-smbios and then nothing
> > else will be able to use it.
> >
> 
> OK, I'm going to go with the above patch with a little more context about the
> visible and selectable DELL_SMBIOS. We can revisit if all this belongs under a
> DELL_EXTRAS menuconfig as a separate exercise.
> 
> I am planning on pushing your 3 and my patch above to the 4.16 rc cycle. Holler
> if that wasn't your expectation.
> 

OK Fine by me.