[PATCH 0/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()

[PATCH 0/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()

[Mattias Jacobsson]

This patchset adds WMI support to MODULE_DEVICE_TABLE().

[PATCH 1/3]: prepare struct wmi_device_id
[PATCH 2/3]: add support
[PATCH 3/3]: update existing drivers to use MODULE_DEVICE_TABLE()

Mattias Jacobsson (3):
  platform/x86: wmi: move struct wmi_device_id to mod_devicetable.h
  platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()
  platform/x86: wmi: use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS()

 drivers/platform/x86/dell-smbios-wmi.c       |  2 +-
 drivers/platform/x86/dell-wmi-descriptor.c   |  2 +-
 drivers/platform/x86/dell-wmi.c              |  4 ++--
 drivers/platform/x86/huawei-wmi.c            |  3 +--
 drivers/platform/x86/intel-wmi-thunderbolt.c |  2 +-
 drivers/platform/x86/wmi-bmof.c              |  2 +-
 drivers/platform/x86/wmi.c                   |  2 +-
 include/linux/mod_devicetable.h              | 13 +++++++++++++
 include/linux/wmi.h                          |  5 +----
 scripts/mod/devicetable-offsets.c            |  3 +++
 scripts/mod/file2alias.c                     | 18 ++++++++++++++++++
 11 files changed, 43 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH 1/3] platform/x86: wmi: move struct wmi_device_id to mod_devicetable.h

[Mattias Jacobsson]

In preparation for adding WMI support to MODULE_DEVICE_TABLE() move the
definition of struct wmi_device_id to mod_devicetable.h and inline
guid_string in the struct.

Changing guid_string to an inline char array changes the loop conditions
when looping over an array of struct wmi_device_id. Therefore update
wmi_dev_match()'s loop to check for an empty guid_string instead of a
NULL pointer.

Signed-off-by: Mattias Jacobsson <2pi@mok.nu>
---
 drivers/platform/x86/wmi.c      |  2 +-
 include/linux/mod_devicetable.h | 13 +++++++++++++
 include/linux/wmi.h             |  5 +----
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index bea35be68706..d7a2f5c8b959 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -768,7 +768,7 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
 	struct wmi_block *wblock = dev_to_wblock(dev);
 	const struct wmi_device_id *id = wmi_driver->id_table;
 
-	while (id->guid_string) {
+	while (id->guid_string[0] != '\0') {
 		uuid_le driver_guid;
 
 		if (WARN_ON(uuid_le_to_bin(id->guid_string, &driver_guid)))
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index f9bd2f34b99f..ccc9bd4f32d2 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -779,4 +779,17 @@ struct typec_device_id {
 	kernel_ulong_t driver_data;
 };
 
+/* WMI */
+
+#define WMI_MODULE_PREFIX	"wmi:"
+#define WMI_GUID_STRING_LEN	36
+
+/**
+ * struct wmi_device_id - WMI device identifier
+ * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
+ */
+struct wmi_device_id {
+	const char guid_string[WMI_GUID_STRING_LEN+1];
+};
+
 #endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 4757cb5077e5..592f81afecbb 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -18,6 +18,7 @@
 
 #include <linux/device.h>
 #include <linux/acpi.h>
+#include <linux/mod_devicetable.h>
 #include <uapi/linux/wmi.h>
 
 struct wmi_device {
@@ -39,10 +40,6 @@ extern union acpi_object *wmidev_block_query(struct wmi_device *wdev,
 
 extern int set_required_buffer_size(struct wmi_device *wdev, u64 length);
 
-struct wmi_device_id {
-	const char *guid_string;
-};
-
 struct wmi_driver {
 	struct device_driver driver;
 	const struct wmi_device_id *id_table;
-- 
2.20.1

[PATCH 2/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()

[Mattias Jacobsson]

Add WMI support to MODULE_DEVICE_TABLE() by adding info about struct
wmi_device_id in devicetable-offsets.c and add a WMI entry point in
file2alias.c.

The type argument for MODULE_DEVICE_TABLE(type, name) is wmi.

Signed-off-by: Mattias Jacobsson <2pi@mok.nu>
---

The idea of adding wmi support to MODULE_DEVICE_TABLE() originates from
a suggestion at [1]. However [2] states: "Please note that this tag
should not be added without the reporter's permission, especially if
the idea was not posted in a public forum." about the "Suggested-by:"
tag.
Pali Rohár: May I add a "Suggested-by:" tag?

[1]: https://lore.kernel.org/patchwork/patch/795892/#989423
[2]: Documentation/process/submitting-patches.rst

---
 scripts/mod/devicetable-offsets.c |  3 +++
 scripts/mod/file2alias.c          | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index 293004499b4d..99276a422e77 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -225,5 +225,8 @@ int main(void)
 	DEVID_FIELD(typec_device_id, svid);
 	DEVID_FIELD(typec_device_id, mode);
 
+	DEVID(wmi_device_id);
+	DEVID_FIELD(wmi_device_id, guid_string);
+
 	return 0;
 }
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index a37af7d71973..f014a2466ff7 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1287,6 +1287,23 @@ static int do_typec_entry(const char *filename, void *symval, char *alias)
 	return 1;
 }
 
+/* Looks like: wmi:guid */
+static int do_wmi_entry(const char *filename, void *symval, char *alias)
+{
+	DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
+	if (strlen(*guid_string) != WMI_GUID_STRING_LEN) {
+		warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
+				*guid_string, filename);
+		return 0;
+	}
+	if (snprintf(alias, 500, WMI_MODULE_PREFIX "%s", *guid_string) < 0) {
+		warn("Could not generate all MODULE_ALIAS's in '%s'\n",
+				filename);
+		return 0;
+	}
+	return 1;
+}
+
 /* Does namelen bytes of name exactly match the symbol? */
 static bool sym_is(const char *name, unsigned namelen, const char *symbol)
 {
@@ -1357,6 +1374,7 @@ static const struct devtable devtable[] = {
 	{"fslmc", SIZE_fsl_mc_device_id, do_fsl_mc_entry},
 	{"tbsvc", SIZE_tb_service_id, do_tbsvc_entry},
 	{"typec", SIZE_typec_device_id, do_typec_entry},
+	{"wmi", SIZE_wmi_device_id, do_wmi_entry},
 };
 
 /* Create MODULE_ALIAS() statements.
-- 
2.20.1

[PATCH 3/3] platform/x86: wmi: use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS()

[Mattias Jacobsson]

WMI drivers can if they have specified an array of struct wmi_device_id
use the MODULE_DEVICE_TABLE() macro to automatically generate the
appropriate MODULE_ALIAS() output.

Change all driver that have specified an array of struct wmi_device_id
to use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS().

Signed-off-by: Mattias Jacobsson <2pi@mok.nu>
---
 drivers/platform/x86/dell-smbios-wmi.c       | 2 +-
 drivers/platform/x86/dell-wmi-descriptor.c   | 2 +-
 drivers/platform/x86/dell-wmi.c              | 4 ++--
 drivers/platform/x86/huawei-wmi.c            | 3 +--
 drivers/platform/x86/intel-wmi-thunderbolt.c | 2 +-
 drivers/platform/x86/wmi-bmof.c              | 2 +-
 6 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-smbios-wmi.c
index cf2229ece9ff..c3ed3c8c17b9 100644
--- a/drivers/platform/x86/dell-smbios-wmi.c
+++ b/drivers/platform/x86/dell-smbios-wmi.c
@@ -277,4 +277,4 @@ void exit_dell_smbios_wmi(void)
 	wmi_driver_unregister(&dell_smbios_wmi_driver);
 }
 
-MODULE_ALIAS("wmi:" DELL_WMI_SMBIOS_GUID);
+MODULE_DEVICE_TABLE(wmi, dell_smbios_wmi_id_table);
diff --git a/drivers/platform/x86/dell-wmi-descriptor.c b/drivers/platform/x86/dell-wmi-descriptor.c
index 072821aa47fc..14ab250b7d5a 100644
--- a/drivers/platform/x86/dell-wmi-descriptor.c
+++ b/drivers/platform/x86/dell-wmi-descriptor.c
@@ -207,7 +207,7 @@ static struct wmi_driver dell_wmi_descriptor_driver = {
 
 module_wmi_driver(dell_wmi_descriptor_driver);
 
-MODULE_ALIAS("wmi:" DELL_WMI_DESCRIPTOR_GUID);
+MODULE_DEVICE_TABLE(wmi, dell_wmi_descriptor_id_table);
 MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
 MODULE_DESCRIPTION("Dell WMI descriptor driver");
 MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 16c7f3d9a335..0602aba62b3f 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -50,8 +50,6 @@ MODULE_LICENSE("GPL");
 
 static bool wmi_requires_smbios_request;
 
-MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
-
 struct dell_wmi_priv {
 	struct input_dev *input_dev;
 	u32 interface_version;
@@ -738,3 +736,5 @@ static void __exit dell_wmi_exit(void)
 	wmi_driver_unregister(&dell_wmi_driver);
 }
 module_exit(dell_wmi_exit);
+
+MODULE_DEVICE_TABLE(wmi, dell_wmi_id_table);
diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
index 59872f87b741..52fcac5b393a 100644
--- a/drivers/platform/x86/huawei-wmi.c
+++ b/drivers/platform/x86/huawei-wmi.c
@@ -201,8 +201,7 @@ static struct wmi_driver huawei_wmi_driver = {
 
 module_wmi_driver(huawei_wmi_driver);
 
-MODULE_ALIAS("wmi:"WMI0_EVENT_GUID);
-MODULE_ALIAS("wmi:"AMW0_EVENT_GUID);
+MODULE_DEVICE_TABLE(wmi, huawei_wmi_id_table);
 MODULE_AUTHOR("Ayman Bagabas <ayman.bagabas@gmail.com>");
 MODULE_DESCRIPTION("Huawei WMI hotkeys");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/x86/intel-wmi-thunderbolt.c b/drivers/platform/x86/intel-wmi-thunderbolt.c
index 9ded8e2af312..4dfa61434a76 100644
--- a/drivers/platform/x86/intel-wmi-thunderbolt.c
+++ b/drivers/platform/x86/intel-wmi-thunderbolt.c
@@ -88,7 +88,7 @@ static struct wmi_driver intel_wmi_thunderbolt_driver = {
 
 module_wmi_driver(intel_wmi_thunderbolt_driver);
 
-MODULE_ALIAS("wmi:" INTEL_WMI_THUNDERBOLT_GUID);
+MODULE_DEVICE_TABLE(wmi, intel_wmi_thunderbolt_id_table);
 MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
 MODULE_DESCRIPTION("Intel WMI Thunderbolt force power driver");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c
index c4530ba715e8..8751a13134be 100644
--- a/drivers/platform/x86/wmi-bmof.c
+++ b/drivers/platform/x86/wmi-bmof.c
@@ -119,7 +119,7 @@ static struct wmi_driver wmi_bmof_driver = {
 
 module_wmi_driver(wmi_bmof_driver);
 
-MODULE_ALIAS("wmi:" WMI_BMOF_GUID);
+MODULE_DEVICE_TABLE(wmi, wmi_bmof_id_table);
 MODULE_AUTHOR("Andrew Lutomirski <luto@kernel.org>");
 MODULE_DESCRIPTION("WMI embedded Binary MOF driver");
 MODULE_LICENSE("GPL");
-- 
2.20.1

Re: [PATCH 2/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 2846 bytes --]

On Saturday 19 January 2019 12:55:54 Mattias Jacobsson wrote:
> Add WMI support to MODULE_DEVICE_TABLE() by adding info about struct
> wmi_device_id in devicetable-offsets.c and add a WMI entry point in
> file2alias.c.
> 
> The type argument for MODULE_DEVICE_TABLE(type, name) is wmi.
> 
> Signed-off-by: Mattias Jacobsson <2pi@mok.nu>
> ---
> 
> The idea of adding wmi support to MODULE_DEVICE_TABLE() originates from
> a suggestion at [1].

Thanks for implementation!

> However [2] states: "Please note that this tag
> should not be added without the reporter's permission, especially if
> the idea was not posted in a public forum." about the "Suggested-by:"
> tag.
> Pali Rohár: May I add a "Suggested-by:" tag?

Yes, you can!

> [1]: https://lore.kernel.org/patchwork/patch/795892/#989423
> [2]: Documentation/process/submitting-patches.rst
> 
> ---
>  scripts/mod/devicetable-offsets.c |  3 +++
>  scripts/mod/file2alias.c          | 18 ++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
> index 293004499b4d..99276a422e77 100644
> --- a/scripts/mod/devicetable-offsets.c
> +++ b/scripts/mod/devicetable-offsets.c
> @@ -225,5 +225,8 @@ int main(void)
>  	DEVID_FIELD(typec_device_id, svid);
>  	DEVID_FIELD(typec_device_id, mode);
>  
> +	DEVID(wmi_device_id);
> +	DEVID_FIELD(wmi_device_id, guid_string);
> +
>  	return 0;
>  }
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index a37af7d71973..f014a2466ff7 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1287,6 +1287,23 @@ static int do_typec_entry(const char *filename, void *symval, char *alias)
>  	return 1;
>  }
>  
> +/* Looks like: wmi:guid */
> +static int do_wmi_entry(const char *filename, void *symval, char *alias)
> +{
> +	DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
> +	if (strlen(*guid_string) != WMI_GUID_STRING_LEN) {
> +		warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
> +				*guid_string, filename);
> +		return 0;
> +	}
> +	if (snprintf(alias, 500, WMI_MODULE_PREFIX "%s", *guid_string) < 0) {
> +		warn("Could not generate all MODULE_ALIAS's in '%s'\n",
> +				filename);
> +		return 0;
> +	}
> +	return 1;
> +}
> +
>  /* Does namelen bytes of name exactly match the symbol? */
>  static bool sym_is(const char *name, unsigned namelen, const char *symbol)
>  {
> @@ -1357,6 +1374,7 @@ static const struct devtable devtable[] = {
>  	{"fslmc", SIZE_fsl_mc_device_id, do_fsl_mc_entry},
>  	{"tbsvc", SIZE_tb_service_id, do_tbsvc_entry},
>  	{"typec", SIZE_typec_device_id, do_typec_entry},
> +	{"wmi", SIZE_wmi_device_id, do_wmi_entry},
>  };
>  
>  /* Create MODULE_ALIAS() statements.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

RE: [PATCH 3/3] platform/x86: wmi: use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS()

[Mario.Limonciello]

> -----Original Message-----
> From: Mattias Jacobsson <2pi@mok.nu>
> Sent: Saturday, January 19, 2019 5:56 AM
> To: Limonciello, Mario; dvhart@infradead.org; andy@infradead.org;
> mjg59@srcf.ucam.org; pali.rohar@gmail.com
> Cc: 2pi@mok.nu; platform-driver-x86@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH 3/3] platform/x86: wmi: use MODULE_DEVICE_TABLE() instead of
> MODULE_ALIAS()
> 
> 
> [EXTERNAL EMAIL]
> 
> WMI drivers can if they have specified an array of struct wmi_device_id
> use the MODULE_DEVICE_TABLE() macro to automatically generate the
> appropriate MODULE_ALIAS() output.
> 
> Change all driver that have specified an array of struct wmi_device_id
> to use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS().
> 
> Signed-off-by: Mattias Jacobsson <2pi@mok.nu>

No concerns to the Dell pieces I'm on assuming other patches in series accepted.

Reviewed-by: Mario Limonciello <mario.limonciello@dell.com>

> ---
>  drivers/platform/x86/dell-smbios-wmi.c       | 2 +-
>  drivers/platform/x86/dell-wmi-descriptor.c   | 2 +-
>  drivers/platform/x86/dell-wmi.c              | 4 ++--
>  drivers/platform/x86/huawei-wmi.c            | 3 +--
>  drivers/platform/x86/intel-wmi-thunderbolt.c | 2 +-
>  drivers/platform/x86/wmi-bmof.c              | 2 +-
>  6 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-
> smbios-wmi.c
> index cf2229ece9ff..c3ed3c8c17b9 100644
> --- a/drivers/platform/x86/dell-smbios-wmi.c
> +++ b/drivers/platform/x86/dell-smbios-wmi.c
> @@ -277,4 +277,4 @@ void exit_dell_smbios_wmi(void)
>  	wmi_driver_unregister(&dell_smbios_wmi_driver);
>  }
> 
> -MODULE_ALIAS("wmi:" DELL_WMI_SMBIOS_GUID);
> +MODULE_DEVICE_TABLE(wmi, dell_smbios_wmi_id_table);
> diff --git a/drivers/platform/x86/dell-wmi-descriptor.c
> b/drivers/platform/x86/dell-wmi-descriptor.c
> index 072821aa47fc..14ab250b7d5a 100644
> --- a/drivers/platform/x86/dell-wmi-descriptor.c
> +++ b/drivers/platform/x86/dell-wmi-descriptor.c
> @@ -207,7 +207,7 @@ static struct wmi_driver dell_wmi_descriptor_driver = {
> 
>  module_wmi_driver(dell_wmi_descriptor_driver);
> 
> -MODULE_ALIAS("wmi:" DELL_WMI_DESCRIPTOR_GUID);
> +MODULE_DEVICE_TABLE(wmi, dell_wmi_descriptor_id_table);
>  MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
>  MODULE_DESCRIPTION("Dell WMI descriptor driver");
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 16c7f3d9a335..0602aba62b3f 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -50,8 +50,6 @@ MODULE_LICENSE("GPL");
> 
>  static bool wmi_requires_smbios_request;
> 
> -MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
> -
>  struct dell_wmi_priv {
>  	struct input_dev *input_dev;
>  	u32 interface_version;
> @@ -738,3 +736,5 @@ static void __exit dell_wmi_exit(void)
>  	wmi_driver_unregister(&dell_wmi_driver);
>  }
>  module_exit(dell_wmi_exit);
> +
> +MODULE_DEVICE_TABLE(wmi, dell_wmi_id_table);
> diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-
> wmi.c
> index 59872f87b741..52fcac5b393a 100644
> --- a/drivers/platform/x86/huawei-wmi.c
> +++ b/drivers/platform/x86/huawei-wmi.c
> @@ -201,8 +201,7 @@ static struct wmi_driver huawei_wmi_driver = {
> 
>  module_wmi_driver(huawei_wmi_driver);
> 
> -MODULE_ALIAS("wmi:"WMI0_EVENT_GUID);
> -MODULE_ALIAS("wmi:"AMW0_EVENT_GUID);
> +MODULE_DEVICE_TABLE(wmi, huawei_wmi_id_table);
>  MODULE_AUTHOR("Ayman Bagabas <ayman.bagabas@gmail.com>");
>  MODULE_DESCRIPTION("Huawei WMI hotkeys");
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/platform/x86/intel-wmi-thunderbolt.c
> b/drivers/platform/x86/intel-wmi-thunderbolt.c
> index 9ded8e2af312..4dfa61434a76 100644
> --- a/drivers/platform/x86/intel-wmi-thunderbolt.c
> +++ b/drivers/platform/x86/intel-wmi-thunderbolt.c
> @@ -88,7 +88,7 @@ static struct wmi_driver intel_wmi_thunderbolt_driver = {
> 
>  module_wmi_driver(intel_wmi_thunderbolt_driver);
> 
> -MODULE_ALIAS("wmi:" INTEL_WMI_THUNDERBOLT_GUID);
> +MODULE_DEVICE_TABLE(wmi, intel_wmi_thunderbolt_id_table);
>  MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
>  MODULE_DESCRIPTION("Intel WMI Thunderbolt force power driver");
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c
> index c4530ba715e8..8751a13134be 100644
> --- a/drivers/platform/x86/wmi-bmof.c
> +++ b/drivers/platform/x86/wmi-bmof.c
> @@ -119,7 +119,7 @@ static struct wmi_driver wmi_bmof_driver = {
> 
>  module_wmi_driver(wmi_bmof_driver);
> 
> -MODULE_ALIAS("wmi:" WMI_BMOF_GUID);
> +MODULE_DEVICE_TABLE(wmi, wmi_bmof_id_table);
>  MODULE_AUTHOR("Andrew Lutomirski <luto@kernel.org>");
>  MODULE_DESCRIPTION("WMI embedded Binary MOF driver");
>  MODULE_LICENSE("GPL");
> --
> 2.20.1

Re: [PATCH 1/3] platform/x86: wmi: move struct wmi_device_id to mod_devicetable.h

[Mattias Jacobsson]

On 2019-01-19, Mattias Jacobsson wrote:
> In preparation for adding WMI support to MODULE_DEVICE_TABLE() move the
> definition of struct wmi_device_id to mod_devicetable.h and inline
> guid_string in the struct.
> 
> Changing guid_string to an inline char array changes the loop conditions
> when looping over an array of struct wmi_device_id. Therefore update
> wmi_dev_match()'s loop to check for an empty guid_string instead of a
> NULL pointer.
> 
> Signed-off-by: Mattias Jacobsson <2pi@mok.nu>
> ---
>  drivers/platform/x86/wmi.c      |  2 +-
>  include/linux/mod_devicetable.h | 13 +++++++++++++
>  include/linux/wmi.h             |  5 +----
>  3 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index bea35be68706..d7a2f5c8b959 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -768,7 +768,7 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
>  	struct wmi_block *wblock = dev_to_wblock(dev);
>  	const struct wmi_device_id *id = wmi_driver->id_table;
>  
> -	while (id->guid_string) {
> +	while (id->guid_string[0] != '\0') {

While looking through my patchset again I realized that I've inherited a potential
null pointer dereference situation. This is relevant regardless of this patchset
and I've therefore submitted a separate patch for that [1]. Further versions of
this patchset will explicitly depend on [1], as this doesn't require any actual
changes to this patchset, I'll wait for more comments before sending it.

[1]: https://lkml.kernel.org/r/20190122200302.19861-1-2pi@mok.nu

>  		uuid_le driver_guid;
>  
>  		if (WARN_ON(uuid_le_to_bin(id->guid_string, &driver_guid)))
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index f9bd2f34b99f..ccc9bd4f32d2 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -779,4 +779,17 @@ struct typec_device_id {
>  	kernel_ulong_t driver_data;
>  };
>  
> +/* WMI */
> +
> +#define WMI_MODULE_PREFIX	"wmi:"
> +#define WMI_GUID_STRING_LEN	36
> +
> +/**
> + * struct wmi_device_id - WMI device identifier
> + * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
> + */
> +struct wmi_device_id {
> +	const char guid_string[WMI_GUID_STRING_LEN+1];
> +};
> +
>  #endif /* LINUX_MOD_DEVICETABLE_H */
> diff --git a/include/linux/wmi.h b/include/linux/wmi.h
> index 4757cb5077e5..592f81afecbb 100644
> --- a/include/linux/wmi.h
> +++ b/include/linux/wmi.h
> @@ -18,6 +18,7 @@
>  
>  #include <linux/device.h>
>  #include <linux/acpi.h>
> +#include <linux/mod_devicetable.h>
>  #include <uapi/linux/wmi.h>
>  
>  struct wmi_device {
> @@ -39,10 +40,6 @@ extern union acpi_object *wmidev_block_query(struct wmi_device *wdev,
>  
>  extern int set_required_buffer_size(struct wmi_device *wdev, u64 length);
>  
> -struct wmi_device_id {
> -	const char *guid_string;
> -};
> -
>  struct wmi_driver {
>  	struct device_driver driver;
>  	const struct wmi_device_id *id_table;
> -- 
> 2.20.1
> 

Thanks,
Mattias

Re: [PATCH 0/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()

[Darren Hart]

On Sat, Jan 19, 2019 at 12:55:52PM +0100, Mattias Jacobsson wrote:
> This patchset adds WMI support to MODULE_DEVICE_TABLE().

Hi Mattias,

Thanks for the patch series. I've reviewed and found no major issues - but what
I'm missing from this cover letter and each commit message is the why. What is
the problem this series is intended to address? This should be clear in the
commit messages so developers reading the git history have the necessary context
to understand why the change was made and the intent behind them were.

The only advantage that I see by the end of the series is removing the need for
driver authors to prefix the modules alias  with "wmi:" - which is an advantage
and avoids careless errors. Is that the only goal? It adds some complexity by
spreading the implementation across more files and more directories, so we need
to document the justification.

Thanks,

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 0/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()

[Mattias Jacobsson]

Hi Darren,

On 2019-01-26, Darren Hart wrote:
> On Sat, Jan 19, 2019 at 12:55:52PM +0100, Mattias Jacobsson wrote:
> > This patchset adds WMI support to MODULE_DEVICE_TABLE().
> 
> Hi Mattias,
> 
> Thanks for the patch series. I've reviewed and found no major issues - but what
> I'm missing from this cover letter and each commit message is the why. What is
> the problem this series is intended to address? This should be clear in the
> commit messages so developers reading the git history have the necessary context
> to understand why the change was made and the intent behind them were.

Thanks for reviewing the patchset!

> 
> The only advantage that I see by the end of the series is removing the need for
> driver authors to prefix the modules alias  with "wmi:" - which is an advantage
> and avoids careless errors. Is that the only goal? It adds some complexity by
> spreading the implementation across more files and more directories, so we need
> to document the justification.

Now that you point it out I can definitely see that the reasoning isn't
explicitly stated. Sorry about that!

I've updated the commit messages for [PATCH 2/3] and [PATCH 3/3] in v2
of this patchset to clarify the "why".

> 
> Thanks,
> 
> -- 
> Darren Hart
> VMware Open Source Technology Center

Thanks,
Mattias