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

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

-- 
2.20.1

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

[Mattias Jacobsson]

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

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

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

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

[PATCH v2 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>
---
 scripts/mod/devicetable-offsets.c |  3 +++
 scripts/mod/file2alias.c          | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+)

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

[PATCH v2 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 v2 1/3] platform/x86: wmi: move struct wmi_device_id to mod_devicetable.h

[Andy Shevchenko]

On Sun, Jan 27, 2019 at 9:04 PM Mattias Jacobsson <2pi@mok.nu> wrote:
>
> In preparation for adding WMI support to MODULE_DEVICE_TABLE() move the
> definition of struct wmi_device_id to mod_devicetable.h and inline
> guid_string in the struct.
>
> Changing guid_string to an inline char array changes the loop conditions
> when looping over an array of struct wmi_device_id. Therefore update
> wmi_dev_match()'s loop to check for an empty guid_string instead of a
> NULL pointer.

Below some minor comments.

> -       while (id->guid_string) {
> +       while (id->guid_string[0] != '\0') {

Hmm.. I would rather put it as

while (*id->guid_string) {

> +#define WMI_MODULE_PREFIX      "wmi:"

> +#define WMI_GUID_STRING_LEN    36

Isn't this already defined in UUID namespace?
(include/linux/uuid.h IIRC)

>  #include <linux/device.h>
>  #include <linux/acpi.h>

> +#include <linux/mod_devicetable.h>

Not sure it's needed since acpi.h includes that.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Jan 27, 2019 at 9:04 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.

> +/* Looks like: wmi:guid */
> +static int do_wmi_entry(const char *filename, void *symval, char *alias)
> +{
> +       DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
> +       if (strlen(*guid_string) != WMI_GUID_STRING_LEN) {
> +               warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
> +                               *guid_string, filename);
> +               return 0;
> +       }

> +       if (snprintf(alias, 500, WMI_MODULE_PREFIX "%s", *guid_string) < 0) {

What the point to use snprintf here with arbitrary buffer size if we
exactly know 2 facts:
1. UUID string is  36 characters
2. buffer is long enough

?

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

-- 
With Best Regards,
Andy Shevchenko

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

[Mattias Jacobsson]

Hi Andy,

Thanks for reviewing!

On 2019-01-27, Andy Shevchenko wrote:
> On Sun, Jan 27, 2019 at 9:04 PM Mattias Jacobsson <2pi@mok.nu> wrote:
> >
> > In preparation for adding WMI support to MODULE_DEVICE_TABLE() move the
> > definition of struct wmi_device_id to mod_devicetable.h and inline
> > guid_string in the struct.
> >
> > Changing guid_string to an inline char array changes the loop conditions
> > when looping over an array of struct wmi_device_id. Therefore update
> > wmi_dev_match()'s loop to check for an empty guid_string instead of a
> > NULL pointer.
> 
> Below some minor comments.
> 
> > -       while (id->guid_string) {
> > +       while (id->guid_string[0] != '\0') {
> 
> Hmm.. I would rather put it as
> 
> while (*id->guid_string) {

Sure.

> 
> > +#define WMI_MODULE_PREFIX      "wmi:"
> 
> > +#define WMI_GUID_STRING_LEN    36
> 
> Isn't this already defined in UUID namespace?
> (include/linux/uuid.h IIRC)  

Kind of, UUID_STRING_LEN is defined in uuid.h. It is included behind a
#ifdef __KERNEL__, but others seam to use things included through it so
I guess it is alright...

Let me know how you want it.

> 
> >  #include <linux/device.h>
> >  #include <linux/acpi.h>
> 
> > +#include <linux/mod_devicetable.h>
> 
> Not sure it's needed since acpi.h includes that.

It is included in acpi.h(behind CONFIG_ACPI), I thought it was cleaner
with it included explicitly. Plus that we aren't relying on others to
include it.

Let me know how you want it.

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

Thanks,
Mattias

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

[Mattias Jacobsson]

Hi,

On 2019-01-27, Andy Shevchenko wrote:
> On Sun, Jan 27, 2019 at 9:04 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.
> 
> > +/* Looks like: wmi:guid */
> > +static int do_wmi_entry(const char *filename, void *symval, char *alias)
> > +{
> > +       DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
> > +       if (strlen(*guid_string) != WMI_GUID_STRING_LEN) {
> > +               warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
> > +                               *guid_string, filename);
> > +               return 0;
> > +       }
> 
> > +       if (snprintf(alias, 500, WMI_MODULE_PREFIX "%s", *guid_string) < 0) {
> 
> What the point to use snprintf here with arbitrary buffer size if we
> exactly know 2 facts:
> 1. UUID string is  36 characters
> 2. buffer is long enough
> 
> ?

As long as no one changes the code, not much.

> 
> > +               warn("Could not generate all MODULE_ALIAS's in '%s'\n",
> > +                               filename);
> > +               return 0;
> > +       }
> > +       return 1;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Thanks,
Mattias

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

[Pali Rohár]

On Monday 28 January 2019 15:09:11 Mattias Jacobsson wrote:
> Hi,
> 
> On 2019-01-27, Andy Shevchenko wrote:
> > On Sun, Jan 27, 2019 at 9:04 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.
> > 
> > > +/* Looks like: wmi:guid */
> > > +static int do_wmi_entry(const char *filename, void *symval, char *alias)
> > > +{
> > > +       DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
> > > +       if (strlen(*guid_string) != WMI_GUID_STRING_LEN) {
> > > +               warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
> > > +                               *guid_string, filename);
> > > +               return 0;
> > > +       }
> > 
> > > +       if (snprintf(alias, 500, WMI_MODULE_PREFIX "%s", *guid_string) < 0) {
> > 
> > What the point to use snprintf here with arbitrary buffer size if we
> > exactly know 2 facts:
> > 1. UUID string is  36 characters
> > 2. buffer is long enough
> > 
> > ?
> 
> As long as no one changes the code, not much.

At least instead of hardcoded number 500, you should use pass size of alias:

  static int do_wmi_entry(const char *filename, void *symval, char *alias, size_t alias_size)

  if (snprintf(alias, alias_size, WMI_MODULE_PREFIX "%s", *guid_string) < 0) {

Or pass buffer of constant size and then you do not need to use snprintf:

  #define ALIAS_SIZE (sizeof(WMI_MODULE_PREFIX)+WMI_GUID_STRING_LEN)

  static int do_wmi_entry(const char *filename, void *symval, char alias[ALIAS_SIZE])

This should not break even when code around changes.

> > 
> > > +               warn("Could not generate all MODULE_ALIAS's in '%s'\n",
> > > +                               filename);
> > > +               return 0;
> > > +       }
> > > +       return 1;
> > > +}
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> 
> Thanks,
> Mattias

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

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

[Mattias Jacobsson]

On 2019-01-28, Pali Rohár wrote:
> On Monday 28 January 2019 15:09:11 Mattias Jacobsson wrote:
> > Hi,
> > 
> > On 2019-01-27, Andy Shevchenko wrote:
> > > On Sun, Jan 27, 2019 at 9:04 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.
> > > 
> > > > +/* Looks like: wmi:guid */
> > > > +static int do_wmi_entry(const char *filename, void *symval, char *alias)
> > > > +{
> > > > +       DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
> > > > +       if (strlen(*guid_string) != WMI_GUID_STRING_LEN) {
> > > > +               warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
> > > > +                               *guid_string, filename);
> > > > +               return 0;
> > > > +       }
> > > 
> > > > +       if (snprintf(alias, 500, WMI_MODULE_PREFIX "%s", *guid_string) < 0) {
> > > 
> > > What the point to use snprintf here with arbitrary buffer size if we
> > > exactly know 2 facts:
> > > 1. UUID string is  36 characters
> > > 2. buffer is long enough
> > > 
> > > ?
> > 
> > As long as no one changes the code, not much.
> 
> At least instead of hardcoded number 500, you should use pass size of alias:

Just a note; 500 comes from a few lines below in the do_table()
function. It is the actual size of alias we get in do_wmi_entry().

> 
>   static int do_wmi_entry(const char *filename, void *symval, char *alias, size_t alias_size)
> 
>   if (snprintf(alias, alias_size, WMI_MODULE_PREFIX "%s", *guid_string) < 0) {

That is a good idea, but requires changing all other entry points.
I was thinking of defining a DO_ENTRY_ALIAS_SIZE macro to replace
all/any 500 in file2alias.c. However that is a separate patch.

> 
> Or pass buffer of constant size and then you do not need to use snprintf:
> 
>   #define ALIAS_SIZE (sizeof(WMI_MODULE_PREFIX)+WMI_GUID_STRING_LEN)

I could use ALIAS_SIZE and add that to snprintf() instead of 500.
While I guess it is unlikely that alias ever will be changed to be too
short for us, using ALIAS_SIZE doesn't "guarantee" that we are within
the bounds of alias if anything changes.

> 
>   static int do_wmi_entry(const char *filename, void *symval, char alias[ALIAS_SIZE])
> 
> This should not break even when code around changes.
> 
> > > 
> > > > +               warn("Could not generate all MODULE_ALIAS's in '%s'\n",
> > > > +                               filename);
> > > > +               return 0;
> > > > +       }
> > > > +       return 1;
> > > > +}
> > > 
> > > -- 
> > > With Best Regards,
> > > Andy Shevchenko
> > 
> > Thanks,
> > Mattias
> 
> -- 
> Pali Rohár
> pali.rohar@gmail.com

Thanks,
Mattias

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

[Andy Shevchenko]

On Mon, Jan 28, 2019 at 3:55 PM Mattias Jacobsson <2pi@mok.nu> wrote:
> On 2019-01-27, Andy Shevchenko wrote:
> > On Sun, Jan 27, 2019 at 9:04 PM Mattias Jacobsson <2pi@mok.nu> wrote:

> > > +#define WMI_GUID_STRING_LEN    36
> >
> > Isn't this already defined in UUID namespace?
> > (include/linux/uuid.h IIRC)
>
> Kind of, UUID_STRING_LEN is defined in uuid.h. It is included behind a
> #ifdef __KERNEL__, but others seam to use things included through it so
> I guess it is alright...
>
> Let me know how you want it.

On one hand I think too many places with the same information is not
good. On the other hand WMI might change this limit in the future and
on top of this it seems you are sharing it with user space.
However, file2alias.c includes kernel header directly and uses a
duplicate definition for cases !__KERNEL__.

So, I would do that way.
Use in mod_devicetable.h the definition from uuid.h (which is already
included there), and duplicate the same definition in file2alias.c in
the way it's done for the rest UUID stuff there.

> > >  #include <linux/acpi.h>
> >
> > > +#include <linux/mod_devicetable.h>
> >
> > Not sure it's needed since acpi.h includes that.
>
> It is included in acpi.h(behind CONFIG_ACPI), I thought it was cleaner
> with it included explicitly. Plus that we aren't relying on others to
> include it.
>
> Let me know how you want it.

Since it's minor, it's up to you. Ideally we would better to split out
that inclusion from acpi.h and explicitly do this in all similar
cases, but it's not ours call here.
That said, your variant is okay.

-- 
With Best Regards,
Andy Shevchenko