Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1335481 > unrolled thread

[PATCH v3 0/5] Process Dell Instant Launch hotkey on Vostro V131 and Inspiron M5110

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 0/5] Process Dell Instant Launch hotkey on Vostro V131 and Inspiron M5110 Michał Kępień <kernel@kempniu.pl> - 2016-02-16 16:00 +0100
    [PATCH v3 2/5] dell-smbios: rename dell_smi_error() to dell_smbios_error() Michał Kępień <kernel@kempniu.pl> - 2016-02-16 16:00 +0100
    [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131 Michał Kępień <kernel@kempniu.pl> - 2016-02-16 16:00 +0100
      Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell  Vostro V131 Pali Rohár <pali.rohar@gmail.com> - 2016-02-16 16:20 +0100
        Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell  Vostro V131 Michał Kępień <kernel@kempniu.pl> - 2016-02-16 23:00 +0100
      Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell  Vostro V131 Darren Hart <dvhart@infradead.org> - 2016-02-20 02:30 +0100
        Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell  Vostro V131 Michał Kępień <kernel@kempniu.pl> - 2016-02-22 10:00 +0100
          Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell  Vostro V131 Pali Rohár <pali.rohar@gmail.com> - 2016-02-22 10:10 +0100
            Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell  Vostro V131 Michał Kępień <kernel@kempniu.pl> - 2016-02-22 10:40 +0100
          Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell  Vostro V131 Darren Hart <dvhart@infradead.org> - 2016-02-22 22:20 +0100
    [PATCH v3 1/5] dell-laptop: move dell_smi_error() to dell-smbios Michał Kępień <kernel@kempniu.pl> - 2016-02-16 16:00 +0100

#1335481 — [PATCH v3 0/5] Process Dell Instant Launch hotkey on Vostro V131 and Inspiron M5110

Changes from v2:

  - Use a static variable instead of a quirk structure

  - Use the API exported by dell-smbios to issue the SMBIOS request
    required for generating WMI events, returning with error from
    dell_wmi_init() if it fails

  - Move dell_smi_error() from dell-laptop to dell-smbios and use it to
    determine the error code for returning from dell_wmi_init() when
    enabling WMI fails

  - Support Dell Inspiron M5110

Changes from v1:

  - Use DMI matching instead of a module parameter
  - Change flag name to improve readability

This patch series makes use of the API exported by dell-smbios, so it
should be applied to either testing or dell-smbios.

This patch series was tested on a Dell Inspiron M5110 by Darek Stojaczyk
(CC'd), who reported that it had similar issues as Dell Vostro V131 back
in July 2015 [1].

Pali, returning to our debate whether to place the WMI-enabling SMBIOS
request in dell-laptop or in dell-wmi, I still strongly recommend the
latter.  Apart from the reasons I had discussed before [2], I came up
with another one.  The SMBIOS request in question registers/unregisters
an event listener (dell-wmi).  If we only register for WMI events in
dell-laptop and dell-wmi gets unloaded at some point further on,
brightness keys will not be properly processed any more as they will
only be reported using WMI.  If we properly unregister in
dell_wmi_exit(), brightness keys will be properly reported by ACPI as if
dell-wmi was never loaded.

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1205791/comments/12
[2] http://www.spinics.net/lists/platform-driver-x86/msg08289.html

 drivers/platform/x86/Kconfig       |    1 +
 drivers/platform/x86/dell-laptop.c |   30 +++++------------
 drivers/platform/x86/dell-smbios.c |   16 +++++++++
 drivers/platform/x86/dell-smbios.h |    2 ++
 drivers/platform/x86/dell-wmi.c    |   63 +++++++++++++++++++++++++++++++++++-
 5 files changed, 89 insertions(+), 23 deletions(-)

-- 
1.7.10.4

[toc] | [next] | [standalone]

#1335487 — [PATCH v3 2/5] dell-smbios: rename dell_smi_error() to dell_smbios_error()

As dell_smi_error() is exported by dell-smbios, its prefix should be
consistent with other exported symbols, so change function name to
dell_smbios_error().

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/dell-laptop.c |   16 ++++++++--------
 drivers/platform/x86/dell-smbios.c |    4 ++--
 drivers/platform/x86/dell-smbios.h |    2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index cbafb95..2c2f02b 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -433,7 +433,7 @@ static int dell_rfkill_set(void *data, bool blocked)
 
  out:
 	dell_smbios_release_buffer();
-	return dell_smi_error(ret);
+	return dell_smbios_error(ret);
 }
 
 /* Must be called with the buffer held */
@@ -876,7 +876,7 @@ static int dell_send_intensity(struct backlight_device *bd)
 	else
 		dell_smbios_send_request(1, 1);
 
-	ret = dell_smi_error(buffer->output[0]);
+	ret = dell_smbios_error(buffer->output[0]);
 
 	dell_smbios_release_buffer();
 	return ret;
@@ -901,7 +901,7 @@ static int dell_get_intensity(struct backlight_device *bd)
 		dell_smbios_send_request(0, 1);
 
 	if (buffer->output[0])
-		ret = dell_smi_error(buffer->output[0]);
+		ret = dell_smbios_error(buffer->output[0]);
 	else
 		ret = buffer->output[1];
 
@@ -1160,7 +1160,7 @@ static int kbd_get_info(struct kbd_info *info)
 	ret = buffer->output[0];
 
 	if (ret) {
-		ret = dell_smi_error(ret);
+		ret = dell_smbios_error(ret);
 		goto out;
 	}
 
@@ -1249,7 +1249,7 @@ static int kbd_get_state(struct kbd_state *state)
 	ret = buffer->output[0];
 
 	if (ret) {
-		ret = dell_smi_error(ret);
+		ret = dell_smbios_error(ret);
 		goto out;
 	}
 
@@ -1286,7 +1286,7 @@ static int kbd_set_state(struct kbd_state *state)
 	ret = buffer->output[0];
 	dell_smbios_release_buffer();
 
-	return dell_smi_error(ret);
+	return dell_smbios_error(ret);
 }
 
 static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
@@ -1329,7 +1329,7 @@ static int kbd_set_token_bit(u8 bit)
 	ret = buffer->output[0];
 	dell_smbios_release_buffer();
 
-	return dell_smi_error(ret);
+	return dell_smbios_error(ret);
 }
 
 static int kbd_get_token_bit(u8 bit)
@@ -1354,7 +1354,7 @@ static int kbd_get_token_bit(u8 bit)
 	dell_smbios_release_buffer();
 
 	if (ret)
-		return dell_smi_error(ret);
+		return dell_smbios_error(ret);
 
 	return (val == token->value);
 }
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 942572f..d2412ab 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -40,7 +40,7 @@ static int da_command_code;
 static int da_num_tokens;
 static struct calling_interface_token *da_tokens;
 
-int dell_smi_error(int value)
+int dell_smbios_error(int value)
 {
 	switch (value) {
 	case 0: /* Completed successfully */
@@ -53,7 +53,7 @@ int dell_smi_error(int value)
 		return -EINVAL;
 	}
 }
-EXPORT_SYMBOL_GPL(dell_smi_error);
+EXPORT_SYMBOL_GPL(dell_smbios_error);
 
 struct calling_interface_buffer *dell_smbios_get_buffer(void)
 {
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 52febe6..ec7d40a 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -35,7 +35,7 @@ struct calling_interface_token {
 	};
 };
 
-int dell_smi_error(int value);
+int dell_smbios_error(int value);
 
 struct calling_interface_buffer *dell_smbios_get_buffer(void);
 void dell_smbios_clear_buffer(void);
-- 
1.7.10.4

[toc] | [prev] | [next] | [standalone]

#1335489 — [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

On some laptop models (e.g. Dell Vostro V131), WMI events are not
generated until a specific SMBIOS request is issued to register an event
listener [1].  As there seems to be no ACPI method or SMBIOS request to
determine without possible side effects whether a given machine needs to
issue this SMBIOS request in order to receive WMI events, DMI matching
is used to whitelist the models which need it.

[1] https://lists.us.dell.com/pipermail/libsmbios-devel/2015-July/000612.html

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/Kconfig    |    1 +
 drivers/platform/x86/dell-wmi.c |   49 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 3e4d9c3..5ceb53a 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -122,6 +122,7 @@ config DELL_WMI
 	depends on ACPI_WMI
 	depends on INPUT
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
+	depends on DELL_SMBIOS
 	select INPUT_SPARSEKMAP
 	---help---
 	  Say Y here if you want to support WMI-based hotkeys on Dell laptops.
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 368e193..ca8233a 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -37,6 +37,7 @@
 #include <linux/string.h>
 #include <linux/dmi.h>
 #include <acpi/video.h>
+#include "dell-smbios.h"
 
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
 MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
@@ -47,10 +48,29 @@ MODULE_LICENSE("GPL");
 #define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
 
 static u32 dell_wmi_interface_version;
+static bool wmi_requires_smbios_request;
 
 MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
 MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
 
+static int __init dmi_matched(const struct dmi_system_id *dmi)
+{
+	wmi_requires_smbios_request = 1;
+	return 1;
+}
+
+static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
+	{
+		.callback = dmi_matched,
+		.ident = "Dell Vostro V131",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
+		},
+	},
+	{ }
+};
+
 /*
  * Certain keys are flagged as KE_IGNORE. All of these are either
  * notifications (rather than requests for change) or are also sent
@@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
 {
 	int err;
 	acpi_status status;
+	struct calling_interface_buffer *buffer;
 
 	if (!wmi_has_guid(DELL_EVENT_GUID) ||
 	    !wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
@@ -538,12 +559,40 @@ static int __init dell_wmi_init(void)
 		return -ENODEV;
 	}
 
+	dmi_check_system(dell_wmi_smbios_list);
+
+	if (wmi_requires_smbios_request) {
+		buffer = dell_smbios_get_buffer();
+		buffer->input[0] = 0x10000;
+		buffer->input[1] = 0x51534554;
+		buffer->input[3] = 0x1;
+		dell_smbios_send_request(17, 3);
+		err = buffer->output[0];
+		dell_smbios_release_buffer();
+		if (err) {
+			pr_err("Failed to enable WMI (error %d)\n", err);
+			wmi_remove_notify_handler(DELL_EVENT_GUID);
+			dell_wmi_input_destroy();
+			return dell_smbios_error(err);
+		}
+	}
+
 	return 0;
 }
 module_init(dell_wmi_init);
 
 static void __exit dell_wmi_exit(void)
 {
+	struct calling_interface_buffer *buffer;
+
+	if (wmi_requires_smbios_request) {
+		buffer = dell_smbios_get_buffer();
+		buffer->input[0] = 0x10000;
+		buffer->input[1] = 0x51534554;
+		dell_smbios_send_request(17, 3);
+		dell_smbios_release_buffer();
+	}
+
 	wmi_remove_notify_handler(DELL_EVENT_GUID);
 	dell_wmi_input_destroy();
 }
-- 
1.7.10.4

[toc] | [prev] | [next] | [standalone]

#1335513 — Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

On Tuesday 16 February 2016 15:50:28 Michał Kępień wrote:
> +	if (wmi_requires_smbios_request) {
> +		buffer = dell_smbios_get_buffer();
> +		buffer->input[0] = 0x10000;
> +		buffer->input[1] = 0x51534554;
> +		buffer->input[3] = 0x1;
> +		dell_smbios_send_request(17, 3);
> +		err = buffer->output[0];
> +		dell_smbios_release_buffer();
> +		if (err) {
> +			pr_err("Failed to enable WMI (error %d)\n", err);
> +			wmi_remove_notify_handler(DELL_EVENT_GUID);
> +			dell_wmi_input_destroy();
> +			return dell_smbios_error(err);
> +		}
> +	}
> +
>  	return 0;
>  }
>  module_init(dell_wmi_init);
>  
>  static void __exit dell_wmi_exit(void)
>  {
> +	struct calling_interface_buffer *buffer;
> +
> +	if (wmi_requires_smbios_request) {
> +		buffer = dell_smbios_get_buffer();
> +		buffer->input[0] = 0x10000;
> +		buffer->input[1] = 0x51534554;
> +		dell_smbios_send_request(17, 3);
> +		dell_smbios_release_buffer();
> +	}
> +
>  	wmi_remove_notify_handler(DELL_EVENT_GUID);
>  	dell_wmi_input_destroy();
>  }

Hi! I would propose moving this get_buffer, send, release code into own
function with boolean argument on/off. This de-duplicate same code plus
decrease level of indentation in _init function. And maybe adding ascii
string comment representation for that 0x5153... number can be useful.

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

[toc] | [prev] | [next] | [standalone]

#1335882 — Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

> >  static void __exit dell_wmi_exit(void)
> >  {
> > +	struct calling_interface_buffer *buffer;
> > +
> > +	if (wmi_requires_smbios_request) {
> > +		buffer = dell_smbios_get_buffer();
> > +		buffer->input[0] = 0x10000;
> > +		buffer->input[1] = 0x51534554;
> > +		dell_smbios_send_request(17, 3);
> > +		dell_smbios_release_buffer();
> > +	}
> > +
> >  	wmi_remove_notify_handler(DELL_EVENT_GUID);
> >  	dell_wmi_input_destroy();
> >  }
> 
> Hi! I would propose moving this get_buffer, send, release code into own
> function with boolean argument on/off. This de-duplicate same code plus
> decrease level of indentation in _init function. And maybe adding ascii
> string comment representation for that 0x5153... number can be useful.

Ah, yes, sure.  I will do that in v4.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1338607 — Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

On Tue, Feb 16, 2016 at 03:50:28PM +0100, Michał Kępień wrote:
> On some laptop models (e.g. Dell Vostro V131), WMI events are not
> generated until a specific SMBIOS request is issued to register an event
> listener [1].  As there seems to be no ACPI method or SMBIOS request to
> determine without possible side effects whether a given machine needs to
> issue this SMBIOS request in order to receive WMI events, DMI matching
> is used to whitelist the models which need it.
> 
> [1] https://lists.us.dell.com/pipermail/libsmbios-devel/2015-July/000612.html
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/Kconfig    |    1 +
>  drivers/platform/x86/dell-wmi.c |   49 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 3e4d9c3..5ceb53a 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -122,6 +122,7 @@ config DELL_WMI
>  	depends on ACPI_WMI
>  	depends on INPUT
>  	depends on ACPI_VIDEO || ACPI_VIDEO = n
> +	depends on DELL_SMBIOS
>  	select INPUT_SPARSEKMAP
>  	---help---
>  	  Say Y here if you want to support WMI-based hotkeys on Dell laptops.
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 368e193..ca8233a 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -37,6 +37,7 @@
>  #include <linux/string.h>
>  #include <linux/dmi.h>
>  #include <acpi/video.h>
> +#include "dell-smbios.h"
>  
>  MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
>  MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
> @@ -47,10 +48,29 @@ MODULE_LICENSE("GPL");
>  #define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
>  
>  static u32 dell_wmi_interface_version;
> +static bool wmi_requires_smbios_request;
>  
>  MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
>  MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
>  
> +static int __init dmi_matched(const struct dmi_system_id *dmi)
> +{
> +	wmi_requires_smbios_request = 1;
> +	return 1;
> +}
> +
> +static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
> +	{
> +		.callback = dmi_matched,
> +		.ident = "Dell Vostro V131",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> +		},
> +	},
> +	{ }
> +};
> +
>  /*
>   * Certain keys are flagged as KE_IGNORE. All of these are either
>   * notifications (rather than requests for change) or are also sent
> @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
>  {
>  	int err;
>  	acpi_status status;
> +	struct calling_interface_buffer *buffer;

Please place the longest line first, and move int err to the last declaration.
When changing declarations of local variables, please use "Reverse Christmas
Tree" order (longest line to shortest line) wherever possible.

>  
>  	if (!wmi_has_guid(DELL_EVENT_GUID) ||
>  	    !wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
> @@ -538,12 +559,40 @@ static int __init dell_wmi_init(void)
>  		return -ENODEV;
>  	}
>  
> +	dmi_check_system(dell_wmi_smbios_list);
> +
> +	if (wmi_requires_smbios_request) {
> +		buffer = dell_smbios_get_buffer();
> +		buffer->input[0] = 0x10000;
> +		buffer->input[1] = 0x51534554;
> +		buffer->input[3] = 0x1;
> +		dell_smbios_send_request(17, 3);
> +		err = buffer->output[0];
> +		dell_smbios_release_buffer();
> +		if (err) {
> +			pr_err("Failed to enable WMI (error %d)\n", err);
> +			wmi_remove_notify_handler(DELL_EVENT_GUID);
> +			dell_wmi_input_destroy();
> +			return dell_smbios_error(err);
> +		}
> +	}
> +
>  	return 0;
>  }
>  module_init(dell_wmi_init);
>  
>  static void __exit dell_wmi_exit(void)
>  {
> +	struct calling_interface_buffer *buffer;
> +
> +	if (wmi_requires_smbios_request) {
> +		buffer = dell_smbios_get_buffer();
> +		buffer->input[0] = 0x10000;
> +		buffer->input[1] = 0x51534554;
> +		dell_smbios_send_request(17, 3);
> +		dell_smbios_release_buffer();
> +	}
> +

Pali's point about documenting the hardcoded values and eliminating the code
duplication with a function (inline) is a good one.

Otherwise, this series looks good to me. Looking forward to merging v4.

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1339226 — Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

> >  /*
> >   * Certain keys are flagged as KE_IGNORE. All of these are either
> >   * notifications (rather than requests for change) or are also sent
> > @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
> >  {
> >  	int err;
> >  	acpi_status status;
> > +	struct calling_interface_buffer *buffer;
> 
> Please place the longest line first, and move int err to the last declaration.
> When changing declarations of local variables, please use "Reverse Christmas
> Tree" order (longest line to shortest line) wherever possible.

Thanks, I'll keep that in mind for the future, though putting the
WMI-enabling SMBIOS request in a separate function renders the need for
the buffer variable in dell_wmi_init() void, so v4 won't touch this area
any more.

> Pali's point about documenting the hardcoded values and eliminating the code
> duplication with a function (inline) is a good one.

I plan to only put a comment next to 0x51534554 as 0x10000 is apparently
just something pulled out of a hat (as the link provided in the commit
message proves) and input[3] should be self-explanatory due to the name
of the variable whose value is put into it.

By the way, is there any kernel-wide or subsystem-wide policy for
marking a function inline?  I mean, this is hardly time-critical code,
so is your suggestion to make it inline just a preference or am I
unaware of some rule?

> Otherwise, this series looks good to me. Looking forward to merging v4.

I'll try to post a v4 within the next couple of days.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1339234 — Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

On Monday 22 February 2016 09:56:50 Michał Kępień wrote:
> > >  /*
> > >   * Certain keys are flagged as KE_IGNORE. All of these are either
> > >   * notifications (rather than requests for change) or are also sent
> > > @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
> > >  {
> > >  	int err;
> > >  	acpi_status status;
> > > +	struct calling_interface_buffer *buffer;
> > 
> > Please place the longest line first, and move int err to the last declaration.
> > When changing declarations of local variables, please use "Reverse Christmas
> > Tree" order (longest line to shortest line) wherever possible.
> 
> Thanks, I'll keep that in mind for the future, though putting the
> WMI-enabling SMBIOS request in a separate function renders the need for
> the buffer variable in dell_wmi_init() void, so v4 won't touch this area
> any more.
> 
> > Pali's point about documenting the hardcoded values and eliminating the code
> > duplication with a function (inline) is a good one.
> 
> I plan to only put a comment next to 0x51534554 as 0x10000 is apparently
> just something pulled out of a hat (as the link provided in the commit
> message proves) and input[3] should be self-explanatory due to the name
> of the variable whose value is put into it.

Maybe you can add documentation which we got from Dell on some ML about
this SMI call. Similarly what I added in dell-laptop.c...

> By the way, is there any kernel-wide or subsystem-wide policy for
> marking a function inline?  I mean, this is hardly time-critical code,
> so is your suggestion to make it inline just a preference or am I
> unaware of some rule?

IIRC recent versions of gcc ignores "inline" keyword and inline
functions as needed when doing optimizations.

If there is some functions which must be inlined you need to to use gcc
attrbiute always_inline.

But if there is policy? I do not know, maybe somebody else should
comment it.

> > Otherwise, this series looks good to me. Looking forward to merging v4.
> 
> I'll try to post a v4 within the next couple of days.

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

[toc] | [prev] | [next] | [standalone]

#1339291 — Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

> > > Pali's point about documenting the hardcoded values and eliminating the code
> > > duplication with a function (inline) is a good one.
> > 
> > I plan to only put a comment next to 0x51534554 as 0x10000 is apparently
> > just something pulled out of a hat (as the link provided in the commit
> > message proves) and input[3] should be self-explanatory due to the name
> > of the variable whose value is put into it.
> 
> Maybe you can add documentation which we got from Dell on some ML about
> this SMI call. Similarly what I added in dell-laptop.c...

Sure, I can do that.

> > By the way, is there any kernel-wide or subsystem-wide policy for
> > marking a function inline?  I mean, this is hardly time-critical code,
> > so is your suggestion to make it inline just a preference or am I
> > unaware of some rule?
> 
> IIRC recent versions of gcc ignores "inline" keyword and inline
> functions as needed when doing optimizations.

This was my hunch as well, but I couldn't find any proof immediately,
hence the question.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1339931 — Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

On Mon, Feb 22, 2016 at 09:56:50AM +0100, Michał Kępień wrote:
> > >  /*
> > >   * Certain keys are flagged as KE_IGNORE. All of these are either
> > >   * notifications (rather than requests for change) or are also sent
> > > @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
> > >  {
> > >  	int err;
> > >  	acpi_status status;
> > > +	struct calling_interface_buffer *buffer;
> > 
> > Please place the longest line first, and move int err to the last declaration.
> > When changing declarations of local variables, please use "Reverse Christmas
> > Tree" order (longest line to shortest line) wherever possible.
> 
> Thanks, I'll keep that in mind for the future, though putting the
> WMI-enabling SMBIOS request in a separate function renders the need for
> the buffer variable in dell_wmi_init() void, so v4 won't touch this area
> any more.
> 
> > Pali's point about documenting the hardcoded values and eliminating the code
> > duplication with a function (inline) is a good one.
> 
> I plan to only put a comment next to 0x51534554 as 0x10000 is apparently
> just something pulled out of a hat (as the link provided in the commit
> message proves) and input[3] should be self-explanatory due to the name
> of the variable whose value is put into it.
> 
> By the way, is there any kernel-wide or subsystem-wide policy for
> marking a function inline?  I mean, this is hardly time-critical code,
> so is your suggestion to make it inline just a preference or am I
> unaware of some rule?

I suggested inline because the code was inline before and there would be no size
overhead to continue to make it inline. That said, the best source of guidance
on this is in CodingStyle, Chapter 15. While this function is quite small and
static to the file, it is not performance critical as you say. So, upon closer
inspection, there is no real need to be inline. And in fact, as there is no need
for it, it perhaps should not be. Thanks for raising the question.

> 
> > Otherwise, this series looks good to me. Looking forward to merging v4.
> 
> I'll try to post a v4 within the next couple of days.

Great, thank you.

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1335491 — [PATCH v3 1/5] dell-laptop: move dell_smi_error() to dell-smbios

The dell_smi_error() method could be used by modules other than
dell-laptop for convenient translation of SMBIOS request errors into
errno values.  Thus, move it to dell-smbios.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/dell-laptop.c |   14 --------------
 drivers/platform/x86/dell-smbios.c |   16 ++++++++++++++++
 drivers/platform/x86/dell-smbios.h |    2 ++
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 76064c8..cbafb95 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -273,20 +273,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 	{ }
 };
 
-static inline int dell_smi_error(int value)
-{
-	switch (value) {
-	case 0: /* Completed successfully */
-		return 0;
-	case -1: /* Completed with error */
-		return -EIO;
-	case -2: /* Function not supported */
-		return -ENXIO;
-	default: /* Unknown error */
-		return -EINVAL;
-	}
-}
-
 /*
  * Derived from information in smbios-wireless-ctl:
  *
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 2a4992a..942572f 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/dmi.h>
+#include <linux/err.h>
 #include <linux/gfp.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
@@ -39,6 +40,21 @@ static int da_command_code;
 static int da_num_tokens;
 static struct calling_interface_token *da_tokens;
 
+int dell_smi_error(int value)
+{
+	switch (value) {
+	case 0: /* Completed successfully */
+		return 0;
+	case -1: /* Completed with error */
+		return -EIO;
+	case -2: /* Function not supported */
+		return -ENXIO;
+	default: /* Unknown error */
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL_GPL(dell_smi_error);
+
 struct calling_interface_buffer *dell_smbios_get_buffer(void)
 {
 	mutex_lock(&buffer_mutex);
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 4f69b16..52febe6 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -35,6 +35,8 @@ struct calling_interface_token {
 	};
 };
 
+int dell_smi_error(int value);
+
 struct calling_interface_buffer *dell_smbios_get_buffer(void);
 void dell_smbios_clear_buffer(void);
 void dell_smbios_release_buffer(void);
-- 
1.7.10.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web