platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()

PATCH Superseded Mattias Jacobsson 3 participants 11 messages Jan 30, 2019 – Feb 7, 2019

Subsystems (3)

Kbuild x86 platform drivers WMI binary mof driver

lore

Share & tools

lore

Mattias Jacobsson

author

2019-01-30 15:14 UTC #0 lore raw

v3 cover platform/x86: wmi: add WMI support to MODULE_DEVICE_TABLE()

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/[email protected]
[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(-)

Signature · 1 line

2.20.1

Mattias Jacobsson

author

2019-01-30 15:14 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/platform/x86/wmi.c      |  2 +-
 include/linux/mod_devicetable.h | 12 ++++++++++++
 include/linux/wmi.h             |  5 +----
 3 files changed, 14 insertions(+), 5 deletions(-)

drivers/platform/x86/wmi.c +1 -1

index b0f3d8ecd898..7b26b6ccf1a0 100644
@@ -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)))

include/linux/mod_devicetable.h +12 -0

index f9bd2f34b99f..e44b90fa0aef 100644
@@ -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 */

include/linux/wmi.h +1 -4

index 4757cb5077e5..592f81afecbb 100644
@@ -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;

Signature · 1 line

2.20.1

Mattias Jacobsson

author

2019-01-30 15:14 UTC #2 lore raw

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

In reply to 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.

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 <[email protected]>

---

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

scripts/mod/devicetable-offsets.c +3 -0

index 293004499b4d..99276a422e77 100644
@@ -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;
 }

scripts/mod/file2alias.c +22 -0

index a37af7d71973..abaa6c090564 100644
@@ -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.

Signature · 1 line

2.20.1

Andy Shevchenko

2019-01-30 16:01 UTC #3 lore raw

Quoted 25 lines · 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 <[email protected]>
Signed-off-by Mattias Jacobsson <[email protected]>

Quoted 7 lines

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

scripts/mod/file2alias.c static int do_typec_entry(const char *filename, void *symval, char *alias) · line 1301

Quoted 12 lines

scripts/mod/file2alias.c +11 -0

+/* 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...

Quoted 1 line

+

...and drop this line.

Quoted 1 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?

scripts/mod/file2alias.c static int do_typec_entry(const char *filename, void *symval, char *alias) · line 1307

Quoted 4 lines

scripts/mod/file2alias.c +4 -0

+               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;
}

Quoted 17 lines

scripts/mod/file2alias.c +4 -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

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Mattias Jacobsson

author

2019-02-03 19:04 UTC #4 lore raw

Quoted 49 lines · Andy Shevchenko

Quoted 25 lines · 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 <[email protected]>
Signed-off-by Mattias Jacobsson <[email protected]>
Quoted 7 lines
---
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).
Quoted 10 lines
scripts/mod/file2alias.c +10 -0
+/* 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;
+       }
+

Quoted 3 lines

Quoted 1 line
+ int len = snprintf(alias, 500, WMI_MODULE_PREFIX "%s", *guid_string);
Please, split declaration and assignment...

Done.

Quoted 4 lines

Quoted 2 lines
+
...and drop this line.

Done.

Quoted 5 lines

Quoted 2 lines
+ 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.

Quoted 6 lines

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?

Quoted 8 lines

scripts/mod/file2alias.c static int do_typec_entry(const char *filename, void *symval, char *alias) · line 1307
Quoted 4 lines
scripts/mod/file2alias.c +4 -0
+               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.

Quoted 15 lines

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

Quoted 1 line

?

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.

Quoted 14 lines

scripts/mod/file2alias.c +4 -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},
 };

Quoted 9 lines

Quoted 4 lines
(quoted diff) +0 -1
 /* Create MODULE_ALIAS() statements.
--
2.20.1
Signature · 2 lines
With Best Regards,
Andy Shevchenko

Thanks,
Mattias

Andy Shevchenko

2019-02-05 15:45 UTC #5 lore raw

Quoted 10 lines · Mattias Jacobsson

Quoted 6 lines · Andy Shevchenko
Quoted 1 line · Mattias Jacobsson
+ 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.

Quoted 14 lines

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

Quoted 3 lines

Quoted 1 line
?
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.

Quoted 2 lines

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.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Mattias Jacobsson

author

2019-02-07 12:39 UTC #6 lore raw

Quoted 28 lines · Andy Shevchenko

Quoted 10 lines · Mattias Jacobsson
Quoted 6 lines · Andy Shevchenko
Quoted 1 line · Mattias Jacobsson
+ 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.
Quoted 14 lines
Quoted 14 lines
So, what about simple
{
DEF_FIELD_ADDR(...);
size_t len;
len = strlen(*guid_string);
if (len != ...) {
...
}
sprintf(...);
return 1;
}

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
?
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/[email protected]/

Quoted 10 lines

Quoted 3 lines
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

Quoted 7 lines

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

Great

Quoted 4 lines

Signature · 3 lines
-- 
With Best Regards,
Andy Shevchenko

Thanks,
Mattias

Mattias Jacobsson

author

2019-01-30 15:14 UTC #7 lore raw

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

In reply to 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 <[email protected]>

Reviewed-by Mario Limonciello <[email protected]>

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

drivers/platform/x86/dell-smbios-wmi.c +1 -1

index cf2229ece9ff..c3ed3c8c17b9 100644
@@ -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);

drivers/platform/x86/dell-wmi-descriptor.c +1 -1

index 072821aa47fc..14ab250b7d5a 100644
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("Dell WMI descriptor driver");
 MODULE_LICENSE("GPL");

drivers/platform/x86/dell-wmi.c +2 -2

index 16c7f3d9a335..0602aba62b3f 100644
@@ -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);

drivers/platform/x86/huawei-wmi.c +1 -2

index 59872f87b741..52fcac5b393a 100644
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("Huawei WMI hotkeys");
 MODULE_LICENSE("GPL v2");

drivers/platform/x86/intel-wmi-thunderbolt.c +1 -1

index 9ded8e2af312..4dfa61434a76 100644
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("Intel WMI Thunderbolt force power driver");
 MODULE_LICENSE("GPL v2");

drivers/platform/x86/wmi-bmof.c +1 -1

index c4530ba715e8..8751a13134be 100644
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("WMI embedded Binary MOF driver");
 MODULE_LICENSE("GPL");

Signature · 1 line

2.20.1

Andy Shevchenko

2019-01-30 15:48 UTC #8 lore raw

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

Quoted 8 lines · 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().

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

Quoted 3 lines

Signed-off-by Mattias Jacobsson <[email protected]>
Reviewed-by Mario Limonciello <[email protected]>

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

Quoted 94 lines

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

drivers/platform/x86/dell-smbios-wmi.c +1 -1

index cf2229ece9ff..c3ed3c8c17b9 100644
@@ -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);

drivers/platform/x86/dell-wmi-descriptor.c +1 -1

index 072821aa47fc..14ab250b7d5a 100644
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("Dell WMI descriptor driver");
 MODULE_LICENSE("GPL");

drivers/platform/x86/dell-wmi.c +2 -2

index 16c7f3d9a335..0602aba62b3f 100644
@@ -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);

drivers/platform/x86/huawei-wmi.c +1 -2

index 59872f87b741..52fcac5b393a 100644
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("Huawei WMI hotkeys");
 MODULE_LICENSE("GPL v2");

drivers/platform/x86/intel-wmi-thunderbolt.c +1 -1

index 9ded8e2af312..4dfa61434a76 100644
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("Intel WMI Thunderbolt force power driver");
 MODULE_LICENSE("GPL v2");

drivers/platform/x86/wmi-bmof.c +1 -2

index c4530ba715e8..8751a13134be 100644
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("WMI embedded Binary MOF driver");
 MODULE_LICENSE("GPL");
--

2.20.1

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Mattias Jacobsson

author

2019-01-31 15:24 UTC #9 lore raw

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

Quoted 11 lines · Andy Shevchenko

Quoted 8 lines · 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().
Please, split this per driver basis. It would help in maintenance phase...

Sure.

Quoted 6 lines

Quoted 4 lines
Signed-off-by Mattias Jacobsson <[email protected]>
Reviewed-by Mario Limonciello <[email protected]>
...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?

Quoted 87 lines

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

drivers/platform/x86/dell-smbios-wmi.c +1 -1

index cf2229ece9ff..c3ed3c8c17b9 100644
@@ -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);

drivers/platform/x86/dell-wmi-descriptor.c +1 -1

index 072821aa47fc..14ab250b7d5a 100644
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("Dell WMI descriptor driver");
 MODULE_LICENSE("GPL");

drivers/platform/x86/dell-wmi.c +2 -2

index 16c7f3d9a335..0602aba62b3f 100644
@@ -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);

drivers/platform/x86/huawei-wmi.c +1 -2

index 59872f87b741..52fcac5b393a 100644
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("Huawei WMI hotkeys");
 MODULE_LICENSE("GPL v2");

drivers/platform/x86/intel-wmi-thunderbolt.c +1 -1

index 9ded8e2af312..4dfa61434a76 100644
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("Intel WMI Thunderbolt force power driver");
 MODULE_LICENSE("GPL v2");

drivers/platform/x86/wmi-bmof.c +0 -0

index c4530ba715e8..8751a13134be 100644
@@ -119,7 +119,7 @@ static struct wmi_driver wmi_bmof_driver = {
 
 module_wmi_driver(wmi_bmof_driver);

Quoted 13 lines

Quoted 8 lines

drivers/platform/x86/wmi-bmof.c +1 -2

-MODULE_ALIAS("wmi:" WMI_BMOF_GUID);
+MODULE_DEVICE_TABLE(wmi, wmi_bmof_id_table);
 MODULE_AUTHOR("Andrew Lutomirski <[email protected]>");
 MODULE_DESCRIPTION("WMI embedded Binary MOF driver");
 MODULE_LICENSE("GPL");
--

2.20.1

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Thanks,
Mattias

[email protected]

2019-01-31 15:38 UTC #10 lore raw

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

Quoted 36 lines

-----Original Message-----
From: Mattias Jacobsson <[email protected]>
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; [email protected]
Subject: Re: [PATCH v3 3/3] platform/x86: wmi: use MODULE_DEVICE_TABLE() instead of MODULE_ALIAS()
[EXTERNAL EMAIL]
Quoted 11 lines · Andy Shevchenko
Quoted 8 lines · 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().
Please, split this per driver basis. It would help in maintenance phase...
Sure.
Quoted 6 lines
Quoted 4 lines
Signed-off-by Mattias Jacobsson <[email protected]>
Reviewed-by Mario Limonciello <[email protected]>
...and AFAIU Mario gave his tag only for Dell related stuff.

Quoted 7 lines

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.

Quoted 109 lines

Quoted 11 lines
Quoted 11 lines
---
 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(-)
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/platform/x86/dell-smbios-wmi.c
Quoted 10 lines
Review
Quoted 10 lines
index cf2229ece9ff..c3ed3c8c17b9 100644
drivers/platform/x86/dell-smbios-wmi.c +1 -1
@@ -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);
(quoted diff) +0 -0
b/drivers/platform/x86/dell-wmi-descriptor.c
Quoted 20 lines
Quoted 20 lines
index 072821aa47fc..14ab250b7d5a 100644
drivers/platform/x86/dell-wmi-descriptor.c +1 -1
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("Dell WMI descriptor driver");
 MODULE_LICENSE("GPL");
drivers/platform/x86/dell-wmi.c +0 -0
index 16c7f3d9a335..0602aba62b3f 100644
@@ -50,8 +50,6 @@ MODULE_LICENSE("GPL");
 
 static bool wmi_requires_smbios_request;
 
Quoted 12 lines
Review
Quoted 12 lines
drivers/platform/x86/dell-wmi.c +2 -2
-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);
(quoted diff) +0 -0
b/drivers/platform/x86/huawei-wmi.c
Quoted 7 lines
Quoted 7 lines
index 59872f87b741..52fcac5b393a 100644
drivers/platform/x86/huawei-wmi.c +0 -0
@@ -201,8 +201,7 @@ static struct wmi_driver huawei_wmi_driver = {
 
 module_wmi_driver(huawei_wmi_driver);
 
Quoted 7 lines
Review
Quoted 7 lines
drivers/platform/x86/huawei-wmi.c +1 -2
-MODULE_ALIAS("wmi:"WMI0_EVENT_GUID);
-MODULE_ALIAS("wmi:"AMW0_EVENT_GUID);
+MODULE_DEVICE_TABLE(wmi, huawei_wmi_id_table);
 MODULE_AUTHOR("Ayman Bagabas <[email protected]>");
 MODULE_DESCRIPTION("Huawei WMI hotkeys");
 MODULE_LICENSE("GPL v2");
(quoted diff) +0 -0
b/drivers/platform/x86/intel-wmi-thunderbolt.c
Quoted 5 lines
Quoted 5 lines
index 9ded8e2af312..4dfa61434a76 100644
drivers/platform/x86/intel-wmi-thunderbolt.c +0 -0
@@ -88,7 +88,7 @@ static struct wmi_driver intel_wmi_thunderbolt_driver = {
 
Quoted 8 lines
drivers/platform/x86/wmi-
Quoted 8 lines
drivers/platform/x86/intel-wmi-thunderbolt.c +1 -1
 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 <[email protected]>");
 MODULE_DESCRIPTION("Intel WMI Thunderbolt force power driver");
 MODULE_LICENSE("GPL v2");
drivers/platform/x86/wmi- renamed from drivers/platform/x86/wmi-bmof.c +0 -0
bmof.c
Quoted 7 lines
Quoted 7 lines
index c4530ba715e8..8751a13134be 100644
drivers/platform/x86/wmi-bmof.c +0 -0
@@ -119,7 +119,7 @@ static struct wmi_driver wmi_bmof_driver = {
 
 module_wmi_driver(wmi_bmof_driver);
 
Quoted 13 lines
Quoted 8 lines
drivers/platform/x86/wmi-bmof.c +1 -2
-MODULE_ALIAS("wmi:" WMI_BMOF_GUID);
+MODULE_DEVICE_TABLE(wmi, wmi_bmof_id_table);
 MODULE_AUTHOR("Andrew Lutomirski <[email protected]>");
 MODULE_DESCRIPTION("WMI embedded Binary MOF driver");
 MODULE_LICENSE("GPL");
--
2.20.1
--
With Best Regards,
Andy Shevchenko
Thanks,
Mattias