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

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

[Mattias Jacobsson]

The kernel provides the macro MODULE_DEVICE_TABLE() which can help
driver authors to generate the appropriate MODULE_ALIAS() output. The
WMI device type is currently not supported by MODULE_DEVICE_TABLE().

While using MODULE_DEVICE_TABLE() does increase the complexity as well
as spreading out the implementation across the kernel, it does come with
some benefits too;
* It makes different drivers look more similar; if you can specify the
  array of device_ids any device type specific input to MODULE_ALIAS()
  will automatically be generated for you.
* It helps each driver avoid keeping multiple versions of the same
  information in sync. That is, both the array of device_ids and the
  potential multitude of MODULE_ALIAS()'s.
* Other things eg. [2]

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()

Changes in v3:
* use UUID_STRING_LEN instead of self-defined
* change loop condition in wmi_dev_match() according to comments
* change the usage of snprintf return code in do_wmi_entry()

Changes in v2:
* add one Suggested-by and one Reviewed-by tag
* depend upon patch [1]
* reword commit message for [PATCH 2/3] and [PATCH 3/3] to document the
  reasoning behind the changes

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

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              | 12 +++++++++++
 include/linux/wmi.h                          |  5 +----
 scripts/mod/devicetable-offsets.c            |  3 +++
 scripts/mod/file2alias.c                     | 22 ++++++++++++++++++++
 11 files changed, 46 insertions(+), 13 deletions(-)

-- 
2.20.1

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

[Mattias Jacobsson]

The kernel provides the macro MODULE_DEVICE_TABLE() which can help
driver authors to generate the appropriate MODULE_ALIAS() output. The
WMI device type is currently not supported by MODULE_DEVICE_TABLE().

While using MODULE_DEVICE_TABLE() does increase the complexity as well
as spreading out the implementation across the kernel, it does come with
some benefits too;
* It makes different drivers look more similar; if you can specify the
  array of device_ids any device type specific input to MODULE_ALIAS()
  will automatically be generated for you.
* It helps each driver avoid keeping multiple versions of the same
  information in sync. That is, both the array of device_ids and the
  potential multitude of MODULE_ALIAS()'s.
* Other things eg. [2]

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()

Changes in v3:
* use UUID_STRING_LEN instead of self-defined
* change loop condition in wmi_dev_match() according to comments
* change the usage of snprintf return code in do_wmi_entry()

Changes in v2:
* add one Suggested-by and one Reviewed-by tag
* depend upon patch [1]
* reword commit message for [PATCH 2/3] and [PATCH 3/3] to document the
  reasoning behind the changes

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

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              | 12 +++++++++++
 include/linux/wmi.h                          |  5 +----
 scripts/mod/devicetable-offsets.c            |  3 +++
 scripts/mod/file2alias.c                     | 22 ++++++++++++++++++++
 11 files changed, 46 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH v3 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 | 12 ++++++++++++
 include/linux/wmi.h             |  5 +----
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index b0f3d8ecd898..7b26b6ccf1a0 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -771,7 +771,7 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
 	if (id == NULL)
 		return 0;
 
-	while (id->guid_string) {
+	while (*id->guid_string) {
 		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..e44b90fa0aef 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -779,4 +779,16 @@ struct typec_device_id {
 	kernel_ulong_t driver_data;
 };
 
+/* WMI */
+
+#define WMI_MODULE_PREFIX	"wmi:"
+
+/**
+ * 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[UUID_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 v3 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 | 12 ++++++++++++
 include/linux/wmi.h             |  5 +----
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index b0f3d8ecd898..7b26b6ccf1a0 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -771,7 +771,7 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
 	if (id == NULL)
 		return 0;
 
-	while (id->guid_string) {
+	while (*id->guid_string) {
 		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..e44b90fa0aef 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -779,4 +779,16 @@ struct typec_device_id {
 	kernel_ulong_t driver_data;
 };
 
+/* WMI */
+
+#define WMI_MODULE_PREFIX	"wmi:"
+
+/**
+ * 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[UUID_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 v3 2/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()

[Mattias Jacobsson]

The kernel provides the macro MODULE_DEVICE_TABLE() where driver authors
can specify their device type and their array of device_ids and thereby
trigger the generation of the appropriate MODULE_ALIAS() output. This is
opposed to having to specify one MODULE_ALIAS() for each device. The WMI
device type is currently not supported.

While using MODULE_DEVICE_TABLE() does increase the complexity as well
as spreading out the implementation across the kernel, it does come with
some benefits too;
* It makes different drivers look more similar; if you can specify the
  array of device_ids any device type specific input to MODULE_ALIAS()
  will automatically be generated for you.
* It helps each driver avoid keeping multiple versions of the same
  information in sync. That is, both the array of device_ids and the
  potential multitude of MODULE_ALIAS()'s.

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.

Suggested-by: Pali Rohár <pali.rohar@gmail.com>
Signed-off-by: Mattias Jacobsson <2pi@mok.nu>
---

What do you think about this usage of snprintf()? Now we check if there
is an error or if the printed string tried to exceeded the buffer.
Ideally 500 should be a macro or a parameter, but there isn't one
available. The number 500 comes from a few lines below in the function
do_table().

 scripts/mod/devicetable-offsets.c |  3 +++
 scripts/mod/file2alias.c          | 22 ++++++++++++++++++++++
 2 files changed, 25 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..abaa6c090564 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -37,6 +37,7 @@ typedef unsigned char	__u8;
 typedef struct {
 	__u8 b[16];
 } uuid_le;
+#define	UUID_STRING_LEN		36
 
 /* Big exception to the "don't include kernel headers into userspace, which
  * even potentially has different endianness and word sizes, since
@@ -1287,6 +1288,26 @@ 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) != UUID_STRING_LEN) {
+		warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
+				*guid_string, filename);
+		return 0;
+	}
+
+	int len = snprintf(alias, 500, WMI_MODULE_PREFIX "%s", *guid_string);
+
+	if (len < 0 || len >= 500) {
+		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 +1378,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 v3 2/3] platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()

[Mattias Jacobsson]

The kernel provides the macro MODULE_DEVICE_TABLE() where driver authors
can specify their device type and their array of device_ids and thereby
trigger the generation of the appropriate MODULE_ALIAS() output. This is
opposed to having to specify one MODULE_ALIAS() for each device. The WMI
device type is currently not supported.

While using MODULE_DEVICE_TABLE() does increase the complexity as well
as spreading out the implementation across the kernel, it does come with
some benefits too;
* It makes different drivers look more similar; if you can specify the
  array of device_ids any device type specific input to MODULE_ALIAS()
  will automatically be generated for you.
* It helps each driver avoid keeping multiple versions of the same
  information in sync. That is, both the array of device_ids and the
  potential multitude of MODULE_ALIAS()'s.

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.

Suggested-by: Pali Rohár <pali.rohar@gmail.com>
Signed-off-by: Mattias Jacobsson <2pi@mok.nu>
---

What do you think about this usage of snprintf()? Now we check if there
is an error or if the printed string tried to exceeded the buffer.
Ideally 500 should be a macro or a parameter, but there isn't one
available. The number 500 comes from a few lines below in the function
do_table().

 scripts/mod/devicetable-offsets.c |  3 +++
 scripts/mod/file2alias.c          | 22 ++++++++++++++++++++++
 2 files changed, 25 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..abaa6c090564 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -37,6 +37,7 @@ typedef unsigned char	__u8;
 typedef struct {
 	__u8 b[16];
 } uuid_le;
+#define	UUID_STRING_LEN		36
 
 /* Big exception to the "don't include kernel headers into userspace, which
  * even potentially has different endianness and word sizes, since
@@ -1287,6 +1288,26 @@ 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) != UUID_STRING_LEN) {
+		warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
+				*guid_string, filename);
+		return 0;
+	}
+
+	int len = snprintf(alias, 500, WMI_MODULE_PREFIX "%s", *guid_string);
+
+	if (len < 0 || len >= 500) {
+		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 +1378,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 v3 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. Thus avoiding to keep both the array
of struct wmi_device_id and the MODULE_ALIAS() declaration(s) in sync.

Change all drivers 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>
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

[PATCH v3 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. Thus avoiding to keep both the array
of struct wmi_device_id and the MODULE_ALIAS() declaration(s) in sync.

Change all drivers 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>
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 v3 3/3] platform/x86: wmi: use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS()

[Andy Shevchenko]

On Wed, Jan 30, 2019 at 5:15 PM Mattias Jacobsson <2pi@mok.nu> wrote:
>
> 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. Thus avoiding to keep both the array
> of struct wmi_device_id and the MODULE_ALIAS() declaration(s) in sync.
>
> Change all drivers that have specified an array of struct wmi_device_id
> to use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS().

Please, split this per driver basis. It would help in maintenance phase...

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

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

...and AFAIU Mario gave his tag only for Dell related stuff.

> ---
>  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
>


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Jan 30, 2019 at 5:15 PM Mattias Jacobsson <2pi@mok.nu> wrote:
>
> The kernel provides the macro MODULE_DEVICE_TABLE() where driver authors
> can specify their device type and their array of device_ids and thereby
> trigger the generation of the appropriate MODULE_ALIAS() output. This is
> opposed to having to specify one MODULE_ALIAS() for each device. The WMI
> device type is currently not supported.
>
> While using MODULE_DEVICE_TABLE() does increase the complexity as well
> as spreading out the implementation across the kernel, it does come with
> some benefits too;
> * It makes different drivers look more similar; if you can specify the
>   array of device_ids any device type specific input to MODULE_ALIAS()
>   will automatically be generated for you.
> * It helps each driver avoid keeping multiple versions of the same
>   information in sync. That is, both the array of device_ids and the
>   potential multitude of MODULE_ALIAS()'s.
>
> 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.
>
> Suggested-by: Pali Rohár <pali.rohar@gmail.com>
> Signed-off-by: Mattias Jacobsson <2pi@mok.nu>
> ---
>
> What do you think about this usage of snprintf()? Now we check if there
> is an error or if the printed string tried to exceeded the buffer.
> Ideally 500 should be a macro or a parameter, but there isn't one
> available. The number 500 comes from a few lines below in the function
> do_table().

This looks better, though minor comments.

Indeed, 500 would be nicer to be defined as a constant (via preprocessor macro).

> +/* 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) != UUID_STRING_LEN) {
> +               warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
> +                               *guid_string, filename);
> +               return 0;
> +       }
> +

> +       int len = snprintf(alias, 500, WMI_MODULE_PREFIX "%s", *guid_string);

Please, split declaration and assignment...

> +

...and drop this line.

> +       if (len < 0 || len >= 500) {

Would it even possible to get a negative number here?
Same for any other number than slightly bigger than 36.

You have above a check and here is the matter of either sudden
replacement of the string during the operation or how snprintf is
broken itself.
Do you have a case in mind which can bring to the above conditions?

> +               warn("Could not generate all MODULE_ALIAS's in '%s'\n",
> +                               filename);
> +               return 0;
> +       }

On top of that you have an ordinary case here and in similar ones we
don't care about buffer size at all (perhaps BUILD_BUG_ON() what is
needed here).

So, what about simple

{
 DEF_FIELD_ADDR(...);
 size_t len;

 len = strlen(*guid_string);
 if (len != ...) {
  ...
 }
sprintf(...);
return 1;
}

?

> +       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 +1378,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
>


-- 
With Best Regards,
Andy Shevchenko

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

[Mattias Jacobsson]

On 2019-01-30, Andy Shevchenko wrote:
> On Wed, Jan 30, 2019 at 5:15 PM Mattias Jacobsson <2pi@mok.nu> wrote:
> >
> > 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. Thus avoiding to keep both the array
> > of struct wmi_device_id and the MODULE_ALIAS() declaration(s) in sync.
> >
> > Change all drivers that have specified an array of struct wmi_device_id
> > to use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS().
> 
> Please, split this per driver basis. It would help in maintenance phase...

Sure.

> 
> > Signed-off-by: Mattias Jacobsson <2pi@mok.nu>
> 
> > Reviewed-by: Mario Limonciello <mario.limonciello@dell.com>
> 
> ...and AFAIU Mario gave his tag only for Dell related stuff.

Yes, that is correct. I applied the same reasoning as for the Acked-by:
tag in [1]: "Acked-by: does not necessarily indicate acknowledgement of
the entire patch.". Was that incorrect?

Anyway, now that we are splitting the patch up;

Mario: which files did you give the tag for? all involved *dell* files?

> 
> > ---
> >  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
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Thanks,
Mattias

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

[Mario.Limonciello]

> -----Original Message-----
> From: Mattias Jacobsson <2pi@mok.nu>
> Sent: Thursday, January 31, 2019 9:24 AM
> To: Andy Shevchenko
> Cc: Limonciello, Mario; Darren Hart; Andy Shevchenko; Matthew Garrett; Pali
> Rohár; Platform Driver; Linux Kernel Mailing List; 2pi@mok.nu
> Subject: Re: [PATCH v3 3/3] platform/x86: wmi: use MODULE_DEVICE_TABLE()
> instead of MODULE_ALIAS()
> 
> 
> [EXTERNAL EMAIL]
> 
> On 2019-01-30, Andy Shevchenko wrote:
> > On Wed, Jan 30, 2019 at 5:15 PM Mattias Jacobsson <2pi@mok.nu> wrote:
> > >
> > > 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. Thus avoiding to keep both the array
> > > of struct wmi_device_id and the MODULE_ALIAS() declaration(s) in sync.
> > >
> > > Change all drivers that have specified an array of struct wmi_device_id
> > > to use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS().
> >
> > Please, split this per driver basis. It would help in maintenance phase...
> 
> Sure.
> 
> >
> > > Signed-off-by: Mattias Jacobsson <2pi@mok.nu>
> >
> > > Reviewed-by: Mario Limonciello <mario.limonciello@dell.com>
> >
> > ...and AFAIU Mario gave his tag only for Dell related stuff.
> 
> Yes, that is correct. I applied the same reasoning as for the Acked-by:
> tag in [1]: "Acked-by: does not necessarily indicate acknowledgement of
> the entire patch.". Was that incorrect?
> 
> Anyway, now that we are splitting the patch up;
> 
> Mario: which files did you give the tag for? all involved *dell* files?

Yes.

> 
> >
> > > ---
> > >  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
> > >
> >
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> 
> Thanks,
> Mattias

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

[Mario.Limonciello]

> -----Original Message-----
> From: Mattias Jacobsson <2pi@mok.nu>
> Sent: Thursday, January 31, 2019 9:24 AM
> To: Andy Shevchenko
> Cc: Limonciello, Mario; Darren Hart; Andy Shevchenko; Matthew Garrett; Pali
> Rohár; Platform Driver; Linux Kernel Mailing List; 2pi@mok.nu
> Subject: Re: [PATCH v3 3/3] platform/x86: wmi: use MODULE_DEVICE_TABLE()
> instead of MODULE_ALIAS()
> 
> 
> [EXTERNAL EMAIL]
> 
> On 2019-01-30, Andy Shevchenko wrote:
> > On Wed, Jan 30, 2019 at 5:15 PM Mattias Jacobsson <2pi@mok.nu> wrote:
> > >
> > > 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. Thus avoiding to keep both the array
> > > of struct wmi_device_id and the MODULE_ALIAS() declaration(s) in sync.
> > >
> > > Change all drivers that have specified an array of struct wmi_device_id
> > > to use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS().
> >
> > Please, split this per driver basis. It would help in maintenance phase...
> 
> Sure.
> 
> >
> > > Signed-off-by: Mattias Jacobsson <2pi@mok.nu>
> >
> > > Reviewed-by: Mario Limonciello <mario.limonciello@dell.com>
> >
> > ...and AFAIU Mario gave his tag only for Dell related stuff.
> 
> Yes, that is correct. I applied the same reasoning as for the Acked-by:
> tag in [1]: "Acked-by: does not necessarily indicate acknowledgement of
> the entire patch.". Was that incorrect?
> 
> Anyway, now that we are splitting the patch up;
> 
> Mario: which files did you give the tag for? all involved *dell* files?

Yes.

> 
> >
> > > ---
> > >  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
> > >
> >
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> 
> Thanks,
> Mattias

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

[Mattias Jacobsson]

On 2019-01-30, Andy Shevchenko wrote:
> On Wed, Jan 30, 2019 at 5:15 PM Mattias Jacobsson <2pi@mok.nu> wrote:
> >
> > The kernel provides the macro MODULE_DEVICE_TABLE() where driver authors
> > can specify their device type and their array of device_ids and thereby
> > trigger the generation of the appropriate MODULE_ALIAS() output. This is
> > opposed to having to specify one MODULE_ALIAS() for each device. The WMI
> > device type is currently not supported.
> >
> > While using MODULE_DEVICE_TABLE() does increase the complexity as well
> > as spreading out the implementation across the kernel, it does come with
> > some benefits too;
> > * It makes different drivers look more similar; if you can specify the
> >   array of device_ids any device type specific input to MODULE_ALIAS()
> >   will automatically be generated for you.
> > * It helps each driver avoid keeping multiple versions of the same
> >   information in sync. That is, both the array of device_ids and the
> >   potential multitude of MODULE_ALIAS()'s.
> >
> > 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.
> >
> > Suggested-by: Pali Rohár <pali.rohar@gmail.com>
> > Signed-off-by: Mattias Jacobsson <2pi@mok.nu>
> > ---
> >
> > What do you think about this usage of snprintf()? Now we check if there
> > is an error or if the printed string tried to exceeded the buffer.
> > Ideally 500 should be a macro or a parameter, but there isn't one
> > available. The number 500 comes from a few lines below in the function
> > do_table().
> 
> This looks better, though minor comments.
> 
> Indeed, 500 would be nicer to be defined as a constant (via preprocessor macro).
> 
> > +/* 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) != UUID_STRING_LEN) {
> > +               warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
> > +                               *guid_string, filename);
> > +               return 0;
> > +       }
> > +
> 
> > +       int len = snprintf(alias, 500, WMI_MODULE_PREFIX "%s", *guid_string);
> 
> Please, split declaration and assignment...

Done.

> 
> > +
> 
> ...and drop this line.

Done.

> 
> > +       if (len < 0 || len >= 500) {
> 
> Would it even possible to get a negative number here?
> Same for any other number than slightly bigger than 36.

snprintf returns a negative number on error. BTW AFAIU the code from
file2alias.c gets dynamically linked against a libc.

> 
> You have above a check and here is the matter of either sudden
> replacement of the string during the operation or how snprintf is
> broken itself.
> Do you have a case in mind which can bring to the above conditions?
> 
> > +               warn("Could not generate all MODULE_ALIAS's in '%s'\n",
> > +                               filename);
> > +               return 0;
> > +       }
> 
> On top of that you have an ordinary case here and in similar ones we
> don't care about buffer size at all (perhaps BUILD_BUG_ON() what is
> needed here).

I used warn() as it is what is being used in similar conditions
elsewhere in the file.

> 
> So, what about simple
> 
> {
>  DEF_FIELD_ADDR(...);
>  size_t len;
> 
>  len = strlen(*guid_string);
>  if (len != ...) {
>   ...
>  }
> sprintf(...);
> return 1;
> }
> 
> ?

Then we are missing the check that we are within the bounds of alias as
well as the negative code from s*printf(). snprintf() does this nicely
for us.

> 
> > +       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 +1378,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
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Thanks,
Mattias

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

[Andy Shevchenko]

On Sun, Feb 3, 2019 at 9:04 PM Mattias Jacobsson <2pi@mok.nu> wrote:
> On 2019-01-30, Andy Shevchenko wrote:
> > On Wed, Jan 30, 2019 at 5:15 PM Mattias Jacobsson <2pi@mok.nu> wrote:

> > > +       if (len < 0 || len >= 500) {
> >
> > Would it even possible to get a negative number here?
> > Same for any other number than slightly bigger than 36.
>
> snprintf returns a negative number on error. BTW AFAIU the code from
> file2alias.c gets dynamically linked against a libc.

OK.

> > So, what about simple
> >
> > {
> >  DEF_FIELD_ADDR(...);
> >  size_t len;
> >
> >  len = strlen(*guid_string);
> >  if (len != ...) {
> >   ...
> >  }
> > sprintf(...);
> > return 1;
> > }
> >
> > ?
>
> Then we are missing the check that we are within the bounds of alias

I don't see how. By checking a length of string we be sure, that the
result would have a non-arbitrary length.

> as well as the negative code from s*printf(). snprintf() does this nicely
> for us.

This one I agree with, means in the above example we may do

return sprintf(...);

if callers recognize just a sign, or

len = sprintf(...);
if (len < 0)
 return len; // -1? 0?

return 1;

otherwise.

-- 
With Best Regards,
Andy Shevchenko

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

[Mattias Jacobsson]

On 2019-02-05, Andy Shevchenko wrote:
> On Sun, Feb 3, 2019 at 9:04 PM Mattias Jacobsson <2pi@mok.nu> wrote:
> > On 2019-01-30, Andy Shevchenko wrote:
> > > On Wed, Jan 30, 2019 at 5:15 PM Mattias Jacobsson <2pi@mok.nu> wrote:
> 
> > > > +       if (len < 0 || len >= 500) {
> > >
> > > Would it even possible to get a negative number here?
> > > Same for any other number than slightly bigger than 36.
> >
> > snprintf returns a negative number on error. BTW AFAIU the code from
> > file2alias.c gets dynamically linked against a libc.
> 
> OK.
> 
> > > So, what about simple
> > >
> > > {
> > >  DEF_FIELD_ADDR(...);
> > >  size_t len;
> > >
> > >  len = strlen(*guid_string);
> > >  if (len != ...) {
> > >   ...
> > >  }
> > > sprintf(...);
> > > return 1;
> > > }
> > >
> > > ?
> >
> > Then we are missing the check that we are within the bounds of alias
> 
> I don't see how. By checking a length of string we be sure, that the
> result would have a non-arbitrary length.

If you do s/500/ALIAS_SIZE/ on the patch? My code is written with that
in mind, I guess that wasn't totally clear.

BTW I've posted [1] to introduce the ALIAS_SIZE macro.

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

> 
> > as well as the negative code from s*printf(). snprintf() does this nicely
> > for us.
> 
> This one I agree with, means in the above example we may do
> 
> return sprintf(...);
> 
> if callers recognize just a sign, or
> 
> len = sprintf(...);
> if (len < 0)
>  return len; // -1? 0?
> 
> return 1;
> 
> otherwise.

Great

> 
> -- 
> With Best Regards,
> Andy Shevchenko

Thanks,
Mattias