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

Groups > linux.kernel > #1307447 > unrolled thread

[PATCH 00/14] Common Dell SMBIOS API

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/14] Common Dell SMBIOS API Michał Kępień <kernel@kempniu.pl> - 2016-01-12 15:10 +0100
    [PATCH 13/14] dell-led: use dell_smbios_find_token() for finding mic DMI tokens Michał Kępień <kernel@kempniu.pl> - 2016-01-12 15:10 +0100
      Re: [PATCH 13/14] dell-led: use dell_smbios_find_token() for finding  mic DMI tokens Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-01-21 12:00 +0100
        Re: [PATCH 13/14] dell-led: use dell_smbios_find_token() for finding  mic DMI tokens Michał Kępień <kernel@kempniu.pl> - 2016-01-21 16:10 +0100
          Re: [PATCH 13/14] dell-led: use dell_smbios_find_token() for finding  mic DMI tokens Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-01-21 16:50 +0100
    [PATCH 05/14] dell-smbios: rename get_buffer() to dell_smbios_get_buffer() Michał Kępień <kernel@kempniu.pl> - 2016-01-12 15:20 +0100
    [PATCH 02/14] dell-smbios: don't pass a buffer to dell_send_request() Michał Kępień <kernel@kempniu.pl> - 2016-01-12 15:20 +0100
    [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module Michał Kępień <kernel@kempniu.pl> - 2016-01-12 15:20 +0100
      Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Pali Rohár <pali.rohar@gmail.com> - 2016-01-16 16:30 +0100
        Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Michał Kępień <kernel@kempniu.pl> - 2016-01-18 11:40 +0100
        Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Michał Kępień <kernel@kempniu.pl> - 2016-01-20 10:30 +0100
          Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Pali Rohár <pali.rohar@gmail.com> - 2016-01-21 09:40 +0100
            Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Michał Kępień <kernel@kempniu.pl> - 2016-01-21 14:10 +0100
              Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Pali Rohár <pali.rohar@gmail.com> - 2016-01-21 14:20 +0100
                Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a  separate module Michał Kępień <kernel@kempniu.pl> - 2016-01-21 14:40 +0100
    [PATCH 03/14] dell-smbios: rename buffer to dell_smbios_buffer Michał Kępień <kernel@kempniu.pl> - 2016-01-12 15:20 +0100
    Re: [PATCH 00/14] Common Dell SMBIOS API Darren Hart <dvhart@infradead.org> - 2016-01-14 23:50 +0100

#1307447 — [PATCH 00/14] Common Dell SMBIOS API

The Linux kernel tree currently contains two Dell laptop-related drivers
issuing SMBIOS requests in different ways (dell-laptop in
drivers/platform/x86 and dell-led in drivers/led).  As an upcoming patch
series for the dell-wmi driver (also in drivers/platform/x86) will
change it so that it also performs SMBIOS requests, I took the
opportunity to unify the API used for issuing Dell SMBIOS requests
throughout the kernel before any further code duplication happens.
Credit for suggesting this goes to Pali Rohár.

This patch series is primarily intended for the platform-x86 subsystem,
with only 2 final patches touching the LED subsystem.  I decided to send
the whole series to everyone involved to provide context - my apologies
if this is frowned upon.

As for making dell-led dependent on a driver in drivers/platform/x86,
let me just hint that Pali and I think it could be possible to
eventually move all of dell-led's code to drivers/platform/x86.  But
first things first.

The first patch generates a lot of checkpatch warnings, but these are
also raised for the original code and I decided that not changing the
code while moving around large quantities of it is critical for
reviewability.

Alex, as I don't have the hardware to test the changes in dell-led
(beyond compilation) and you contributed the parts of it which this
patch series changes, is there any way you might test it on relevant
hardware?

 drivers/leds/Kconfig               |    1 +
 drivers/leds/dell-led.c            |  125 ++--------
 drivers/platform/x86/Kconfig       |   12 +-
 drivers/platform/x86/Makefile      |    1 +
 drivers/platform/x86/dell-laptop.c |  444 ++++++++++++------------------------
 drivers/platform/x86/dell-smbios.c |  179 +++++++++++++++
 drivers/platform/x86/dell-smbios.h |   48 ++++
 7 files changed, 395 insertions(+), 415 deletions(-)
 create mode 100644 drivers/platform/x86/dell-smbios.c
 create mode 100644 drivers/platform/x86/dell-smbios.h

-- 
1.7.10.4

[toc] | [next] | [standalone]

#1307450 — [PATCH 13/14] dell-led: use dell_smbios_find_token() for finding mic DMI tokens

With the advent of dell_smbios_find_token(), dell-led does not need to
perform any DMI walking on its own, but it can rather ask dell-smbios to
look up the DMI tokens it needs for changing the state of the microphone
LED.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/leds/Kconfig    |    1 +
 drivers/leds/dell-led.c |   63 +++++++----------------------------------------
 2 files changed, 10 insertions(+), 54 deletions(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b1ab8bd..171810c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -441,6 +441,7 @@ config LEDS_DELL_NETBOOKS
 	tristate "External LED on Dell Business Netbooks"
 	depends on LEDS_CLASS
 	depends on X86 && ACPI_WMI
+	depends on DELL_SMBIOS
 	help
 	  This adds support for the Latitude 2100 and similar
 	  notebooks that have an external LED.
diff --git a/drivers/leds/dell-led.c b/drivers/leds/dell-led.c
index c36acaf..bfa7511 100644
--- a/drivers/leds/dell-led.c
+++ b/drivers/leds/dell-led.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/dmi.h>
 #include <linux/dell-led.h>
+#include "../platform/x86/dell-smbios.h"
 
 MODULE_AUTHOR("Louis Davis/Jim Dailey");
 MODULE_DESCRIPTION("Dell LED Control Driver");
@@ -59,22 +60,6 @@ struct app_wmi_args {
 #define GLOBAL_MIC_MUTE_ENABLE	0x364
 #define GLOBAL_MIC_MUTE_DISABLE	0x365
 
-struct dell_bios_data_token {
-	u16 tokenid;
-	u16 location;
-	u16 value;
-};
-
-struct __attribute__ ((__packed__)) dell_bios_calling_interface {
-	struct	dmi_header header;
-	u16	cmd_io_addr;
-	u8	cmd_io_code;
-	u32	supported_cmds;
-	struct	dell_bios_data_token damap[];
-};
-
-static struct dell_bios_data_token dell_mic_tokens[2];
-
 static int dell_wmi_perform_query(struct app_wmi_args *args)
 {
 	struct app_wmi_args *bios_return;
@@ -112,43 +97,24 @@ static int dell_wmi_perform_query(struct app_wmi_args *args)
 	return rc;
 }
 
-static void __init find_micmute_tokens(const struct dmi_header *dm, void *dummy)
-{
-	struct dell_bios_calling_interface *calling_interface;
-	struct dell_bios_data_token *token;
-	int token_size = sizeof(struct dell_bios_data_token);
-	int i = 0;
-
-	if (dm->type == 0xda && dm->length > 17) {
-		calling_interface = container_of(dm,
-				struct dell_bios_calling_interface, header);
-
-		token = &calling_interface->damap[i];
-		while (token->tokenid != 0xffff) {
-			if (token->tokenid == GLOBAL_MIC_MUTE_DISABLE)
-				memcpy(&dell_mic_tokens[0], token, token_size);
-			else if (token->tokenid == GLOBAL_MIC_MUTE_ENABLE)
-				memcpy(&dell_mic_tokens[1], token, token_size);
-
-			i++;
-			token = &calling_interface->damap[i];
-		}
-	}
-}
-
 static int dell_micmute_led_set(int state)
 {
+	struct calling_interface_token *token;
 	struct app_wmi_args args;
-	struct dell_bios_data_token *token;
 
 	if (!wmi_has_guid(DELL_APP_GUID))
 		return -ENODEV;
 
-	if (state == 0 || state == 1)
-		token = &dell_mic_tokens[state];
+	if (state == 0)
+		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE);
+	else if (state == 1)
+		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE);
 	else
 		return -EINVAL;
 
+	if (!token)
+		return -ENODEV;
+
 	memset(&args, 0, sizeof(struct app_wmi_args));
 
 	args.class = 1;
@@ -177,14 +143,6 @@ int dell_app_wmi_led_set(int whichled, int on)
 }
 EXPORT_SYMBOL_GPL(dell_app_wmi_led_set);
 
-static int __init dell_micmute_led_init(void)
-{
-	memset(dell_mic_tokens, 0, sizeof(struct dell_bios_data_token) * 2);
-	dmi_walk(find_micmute_tokens, NULL);
-
-	return 0;
-}
-
 struct bios_args {
 	unsigned char length;
 	unsigned char result_code;
@@ -330,9 +288,6 @@ static int __init dell_led_init(void)
 	if (!wmi_has_guid(DELL_LED_BIOS_GUID) && !wmi_has_guid(DELL_APP_GUID))
 		return -ENODEV;
 
-	if (wmi_has_guid(DELL_APP_GUID))
-		error = dell_micmute_led_init();
-
 	if (wmi_has_guid(DELL_LED_BIOS_GUID)) {
 		error = led_off();
 		if (error != 0)
-- 
1.7.10.4

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

#1314097 — Re: [PATCH 13/14] dell-led: use dell_smbios_find_token() for finding mic DMI tokens

Hi Michał,

Thanks for the patches.
They should probably be merged through linux-platform-drivers-x86.git.

Feel free to add my

Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>

to 13/14 and 14/14.

Best Regards,
Jacek Anaszewski

On 01/12/2016 03:02 PM, Michał Kępień wrote:
> With the advent of dell_smbios_find_token(), dell-led does not need to
> perform any DMI walking on its own, but it can rather ask dell-smbios to
> look up the DMI tokens it needs for changing the state of the microphone
> LED.
>
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
>   drivers/leds/Kconfig    |    1 +
>   drivers/leds/dell-led.c |   63 +++++++----------------------------------------
>   2 files changed, 10 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index b1ab8bd..171810c 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -441,6 +441,7 @@ config LEDS_DELL_NETBOOKS
>   	tristate "External LED on Dell Business Netbooks"
>   	depends on LEDS_CLASS
>   	depends on X86 && ACPI_WMI
> +	depends on DELL_SMBIOS
>   	help
>   	  This adds support for the Latitude 2100 and similar
>   	  notebooks that have an external LED.
> diff --git a/drivers/leds/dell-led.c b/drivers/leds/dell-led.c
> index c36acaf..bfa7511 100644
> --- a/drivers/leds/dell-led.c
> +++ b/drivers/leds/dell-led.c
> @@ -17,6 +17,7 @@
>   #include <linux/module.h>
>   #include <linux/dmi.h>
>   #include <linux/dell-led.h>
> +#include "../platform/x86/dell-smbios.h"
>
>   MODULE_AUTHOR("Louis Davis/Jim Dailey");
>   MODULE_DESCRIPTION("Dell LED Control Driver");
> @@ -59,22 +60,6 @@ struct app_wmi_args {
>   #define GLOBAL_MIC_MUTE_ENABLE	0x364
>   #define GLOBAL_MIC_MUTE_DISABLE	0x365
>
> -struct dell_bios_data_token {
> -	u16 tokenid;
> -	u16 location;
> -	u16 value;
> -};
> -
> -struct __attribute__ ((__packed__)) dell_bios_calling_interface {
> -	struct	dmi_header header;
> -	u16	cmd_io_addr;
> -	u8	cmd_io_code;
> -	u32	supported_cmds;
> -	struct	dell_bios_data_token damap[];
> -};
> -
> -static struct dell_bios_data_token dell_mic_tokens[2];
> -
>   static int dell_wmi_perform_query(struct app_wmi_args *args)
>   {
>   	struct app_wmi_args *bios_return;
> @@ -112,43 +97,24 @@ static int dell_wmi_perform_query(struct app_wmi_args *args)
>   	return rc;
>   }
>
> -static void __init find_micmute_tokens(const struct dmi_header *dm, void *dummy)
> -{
> -	struct dell_bios_calling_interface *calling_interface;
> -	struct dell_bios_data_token *token;
> -	int token_size = sizeof(struct dell_bios_data_token);
> -	int i = 0;
> -
> -	if (dm->type == 0xda && dm->length > 17) {
> -		calling_interface = container_of(dm,
> -				struct dell_bios_calling_interface, header);
> -
> -		token = &calling_interface->damap[i];
> -		while (token->tokenid != 0xffff) {
> -			if (token->tokenid == GLOBAL_MIC_MUTE_DISABLE)
> -				memcpy(&dell_mic_tokens[0], token, token_size);
> -			else if (token->tokenid == GLOBAL_MIC_MUTE_ENABLE)
> -				memcpy(&dell_mic_tokens[1], token, token_size);
> -
> -			i++;
> -			token = &calling_interface->damap[i];
> -		}
> -	}
> -}
> -
>   static int dell_micmute_led_set(int state)
>   {
> +	struct calling_interface_token *token;
>   	struct app_wmi_args args;
> -	struct dell_bios_data_token *token;
>
>   	if (!wmi_has_guid(DELL_APP_GUID))
>   		return -ENODEV;
>
> -	if (state == 0 || state == 1)
> -		token = &dell_mic_tokens[state];
> +	if (state == 0)
> +		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE);
> +	else if (state == 1)
> +		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE);
>   	else
>   		return -EINVAL;
>
> +	if (!token)
> +		return -ENODEV;
> +
>   	memset(&args, 0, sizeof(struct app_wmi_args));
>
>   	args.class = 1;
> @@ -177,14 +143,6 @@ int dell_app_wmi_led_set(int whichled, int on)
>   }
>   EXPORT_SYMBOL_GPL(dell_app_wmi_led_set);
>
> -static int __init dell_micmute_led_init(void)
> -{
> -	memset(dell_mic_tokens, 0, sizeof(struct dell_bios_data_token) * 2);
> -	dmi_walk(find_micmute_tokens, NULL);
> -
> -	return 0;
> -}
> -
>   struct bios_args {
>   	unsigned char length;
>   	unsigned char result_code;
> @@ -330,9 +288,6 @@ static int __init dell_led_init(void)
>   	if (!wmi_has_guid(DELL_LED_BIOS_GUID) && !wmi_has_guid(DELL_APP_GUID))
>   		return -ENODEV;
>
> -	if (wmi_has_guid(DELL_APP_GUID))
> -		error = dell_micmute_led_init();
> -
>   	if (wmi_has_guid(DELL_LED_BIOS_GUID)) {
>   		error = led_off();
>   		if (error != 0)
>

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

#1314258 — Re: [PATCH 13/14] dell-led: use dell_smbios_find_token() for finding mic DMI tokens

> Thanks for the patches.
> They should probably be merged through linux-platform-drivers-x86.git.
> 
> Feel free to add my
> 
> Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> 
> to 13/14 and 14/14.

Thanks for reviewing.  I will soon post a v2 of this series.  Given that
the only difference between v1 and v2 for dell-led will be a subtle
difference in API use, I'll allow myself to use your Acked-by for the
last two patches when I post v2.  Please let me know if that's
inappropriate.

-- 
Best regards,
Michał Kępień

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

#1314285 — Re: [PATCH 13/14] dell-led: use dell_smbios_find_token() for finding mic DMI tokens

On 01/21/2016 04:00 PM, Michał Kępień wrote:
>> Thanks for the patches.
>> They should probably be merged through linux-platform-drivers-x86.git.
>>
>> Feel free to add my
>>
>> Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>>
>> to 13/14 and 14/14.
>
> Thanks for reviewing.  I will soon post a v2 of this series.  Given that
> the only difference between v1 and v2 for dell-led will be a subtle
> difference in API use, I'll allow myself to use your Acked-by for the
> last two patches when I post v2.  Please let me know if that's
> inappropriate.
>

Yes, feel free to carry my acks.

-- 
Best Regards,
Jacek Anaszewski

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

#1307455 — [PATCH 05/14] dell-smbios: rename get_buffer() to dell_smbios_get_buffer()

As get_buffer() is exported from the module, it has to be renamed to
something less generic, so add a "dell_smbios_" prefix to the function
name.

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

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 1bfc95e..5df30a9 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -416,7 +416,7 @@ static int dell_rfkill_set(void *data, bool blocked)
 	int status;
 	int ret;
 
-	get_buffer();
+	dell_smbios_get_buffer();
 
 	dell_send_request(17, 11);
 	ret = dell_smbios_buffer->output[0];
@@ -480,7 +480,7 @@ static void dell_rfkill_query(struct rfkill *rfkill, void *data)
 	int status;
 	int ret;
 
-	get_buffer();
+	dell_smbios_get_buffer();
 
 	dell_send_request(17, 11);
 	ret = dell_smbios_buffer->output[0];
@@ -520,7 +520,7 @@ static int dell_debugfs_show(struct seq_file *s, void *data)
 	int status;
 	int ret;
 
-	get_buffer();
+	dell_smbios_get_buffer();
 
 	dell_send_request(17, 11);
 	ret = dell_smbios_buffer->output[0];
@@ -618,7 +618,7 @@ static void dell_update_rfkill(struct work_struct *ignored)
 	int status;
 	int ret;
 
-	get_buffer();
+	dell_smbios_get_buffer();
 
 	dell_send_request(17, 11);
 	ret = dell_smbios_buffer->output[0];
@@ -710,7 +710,7 @@ static int __init dell_setup_rfkill(void)
 	if (!force_rfkill && !whitelisted)
 		return 0;
 
-	get_buffer();
+	dell_smbios_get_buffer();
 	dell_send_request(17, 11);
 	ret = dell_smbios_buffer->output[0];
 	status = dell_smbios_buffer->output[1];
@@ -874,7 +874,7 @@ static int dell_send_intensity(struct backlight_device *bd)
 	if (token == -1)
 		return -ENODEV;
 
-	get_buffer();
+	dell_smbios_get_buffer();
 	dell_smbios_buffer->input[0] = token;
 	dell_smbios_buffer->input[1] = bd->props.brightness;
 
@@ -898,7 +898,7 @@ static int dell_get_intensity(struct backlight_device *bd)
 	if (token == -1)
 		return -ENODEV;
 
-	get_buffer();
+	dell_smbios_get_buffer();
 	dell_smbios_buffer->input[0] = token;
 
 	if (power_supply_is_system_supplied() > 0)
@@ -1158,7 +1158,7 @@ static int kbd_get_info(struct kbd_info *info)
 	u8 units;
 	int ret;
 
-	get_buffer();
+	dell_smbios_get_buffer();
 
 	dell_smbios_buffer->input[0] = 0x0;
 	dell_send_request(4, 11);
@@ -1246,7 +1246,7 @@ static int kbd_get_state(struct kbd_state *state)
 {
 	int ret;
 
-	get_buffer();
+	dell_smbios_get_buffer();
 
 	dell_smbios_buffer->input[0] = 0x1;
 	dell_send_request(4, 11);
@@ -1277,7 +1277,7 @@ static int kbd_set_state(struct kbd_state *state)
 {
 	int ret;
 
-	get_buffer();
+	dell_smbios_get_buffer();
 	dell_smbios_buffer->input[0] = 0x2;
 	dell_smbios_buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
 	dell_smbios_buffer->input[1] |= (state->triggers & 0xFF) << 16;
@@ -1324,7 +1324,7 @@ static int kbd_set_token_bit(u8 bit)
 	if (id == -1)
 		return -EINVAL;
 
-	get_buffer();
+	dell_smbios_get_buffer();
 	dell_smbios_buffer->input[0] = da_tokens[id].location;
 	dell_smbios_buffer->input[1] = da_tokens[id].value;
 	dell_send_request(1, 0);
@@ -1347,7 +1347,7 @@ static int kbd_get_token_bit(u8 bit)
 	if (id == -1)
 		return -EINVAL;
 
-	get_buffer();
+	dell_smbios_get_buffer();
 	dell_smbios_buffer->input[0] = da_tokens[id].location;
 	dell_send_request(0, 0);
 	ret = dell_smbios_buffer->output[0];
@@ -2018,7 +2018,7 @@ static int __init dell_init(void)
 
 	token = find_token_location(BRIGHTNESS_TOKEN);
 	if (token != -1) {
-		get_buffer();
+		dell_smbios_get_buffer();
 		dell_smbios_buffer->input[0] = token;
 		dell_send_request(0, 2);
 		if (dell_smbios_buffer->output[0] == 0)
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 8fa740c..27dbfb9 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -46,12 +46,12 @@ void dell_smbios_clear_buffer(void)
 }
 EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);
 
-void get_buffer(void)
+void dell_smbios_get_buffer(void)
 {
 	mutex_lock(&buffer_mutex);
 	dell_smbios_clear_buffer();
 }
-EXPORT_SYMBOL_GPL(get_buffer);
+EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);
 
 void release_buffer(void)
 {
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index ba0bc22..e3c2014 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -39,7 +39,7 @@ extern struct calling_interface_buffer *dell_smbios_buffer;
 extern struct calling_interface_token *da_tokens;
 
 void dell_smbios_clear_buffer(void);
-void get_buffer(void);
+void dell_smbios_get_buffer(void);
 void release_buffer(void);
 
 int find_token_id(int tokenid);
-- 
1.7.10.4

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

#1307456 — [PATCH 02/14] dell-smbios: don't pass a buffer to dell_send_request()

Passing a struct calling_interface_buffer pointer to dell_send_request()
is redundant as it should always operate on the buffer exported from the
module.

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

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index d45d356..572bdca 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -418,7 +418,7 @@ static int dell_rfkill_set(void *data, bool blocked)
 
 	get_buffer();
 
-	dell_send_request(buffer, 17, 11);
+	dell_send_request(17, 11);
 	ret = buffer->output[0];
 	status = buffer->output[1];
 
@@ -428,7 +428,7 @@ static int dell_rfkill_set(void *data, bool blocked)
 	clear_buffer();
 
 	buffer->input[0] = 0x2;
-	dell_send_request(buffer, 17, 11);
+	dell_send_request(17, 11);
 	ret = buffer->output[0];
 	hwswitch = buffer->output[1];
 
@@ -441,7 +441,7 @@ static int dell_rfkill_set(void *data, bool blocked)
 	clear_buffer();
 
 	buffer->input[0] = (1 | (radio<<8) | (disable << 16));
-	dell_send_request(buffer, 17, 11);
+	dell_send_request(17, 11);
 	ret = buffer->output[0];
 
  out:
@@ -458,7 +458,7 @@ static void dell_rfkill_update_sw_state(struct rfkill *rfkill, int radio,
 		int block = rfkill_blocked(rfkill);
 		clear_buffer();
 		buffer->input[0] = (1 | (radio << 8) | (block << 16));
-		dell_send_request(buffer, 17, 11);
+		dell_send_request(17, 11);
 	} else {
 		/* No hw-switch, sync BIOS state to sw_state */
 		rfkill_set_sw_state(rfkill, !!(status & BIT(radio + 16)));
@@ -481,7 +481,7 @@ static void dell_rfkill_query(struct rfkill *rfkill, void *data)
 
 	get_buffer();
 
-	dell_send_request(buffer, 17, 11);
+	dell_send_request(17, 11);
 	ret = buffer->output[0];
 	status = buffer->output[1];
 
@@ -493,7 +493,7 @@ static void dell_rfkill_query(struct rfkill *rfkill, void *data)
 	clear_buffer();
 
 	buffer->input[0] = 0x2;
-	dell_send_request(buffer, 17, 11);
+	dell_send_request(17, 11);
 	ret = buffer->output[0];
 	hwswitch = buffer->output[1];
 
@@ -521,14 +521,14 @@ static int dell_debugfs_show(struct seq_file *s, void *data)
 
 	get_buffer();
 
-	dell_send_request(buffer, 17, 11);
+	dell_send_request(17, 11);
 	ret = buffer->output[0];
 	status = buffer->output[1];
 
 	clear_buffer();
 
 	buffer->input[0] = 0x2;
-	dell_send_request(buffer, 17, 11);
+	dell_send_request(17, 11);
 	hwswitch_ret = buffer->output[0];
 	hwswitch_state = buffer->output[1];
 
@@ -619,7 +619,7 @@ static void dell_update_rfkill(struct work_struct *ignored)
 
 	get_buffer();
 
-	dell_send_request(buffer, 17, 11);
+	dell_send_request(17, 11);
 	ret = buffer->output[0];
 	status = buffer->output[1];
 
@@ -629,7 +629,7 @@ static void dell_update_rfkill(struct work_struct *ignored)
 	clear_buffer();
 
 	buffer->input[0] = 0x2;
-	dell_send_request(buffer, 17, 11);
+	dell_send_request(17, 11);
 	ret = buffer->output[0];
 
 	if (ret == 0 && (status & BIT(0)))
@@ -710,7 +710,7 @@ static int __init dell_setup_rfkill(void)
 		return 0;
 
 	get_buffer();
-	dell_send_request(buffer, 17, 11);
+	dell_send_request(17, 11);
 	ret = buffer->output[0];
 	status = buffer->output[1];
 	release_buffer();
@@ -878,9 +878,9 @@ static int dell_send_intensity(struct backlight_device *bd)
 	buffer->input[1] = bd->props.brightness;
 
 	if (power_supply_is_system_supplied() > 0)
-		dell_send_request(buffer, 1, 2);
+		dell_send_request(1, 2);
 	else
-		dell_send_request(buffer, 1, 1);
+		dell_send_request(1, 1);
 
 	ret = dell_smi_error(buffer->output[0]);
 
@@ -901,9 +901,9 @@ static int dell_get_intensity(struct backlight_device *bd)
 	buffer->input[0] = token;
 
 	if (power_supply_is_system_supplied() > 0)
-		dell_send_request(buffer, 0, 2);
+		dell_send_request(0, 2);
 	else
-		dell_send_request(buffer, 0, 1);
+		dell_send_request(0, 1);
 
 	if (buffer->output[0])
 		ret = dell_smi_error(buffer->output[0]);
@@ -1160,7 +1160,7 @@ static int kbd_get_info(struct kbd_info *info)
 	get_buffer();
 
 	buffer->input[0] = 0x0;
-	dell_send_request(buffer, 4, 11);
+	dell_send_request(4, 11);
 	ret = buffer->output[0];
 
 	if (ret) {
@@ -1248,7 +1248,7 @@ static int kbd_get_state(struct kbd_state *state)
 	get_buffer();
 
 	buffer->input[0] = 0x1;
-	dell_send_request(buffer, 4, 11);
+	dell_send_request(4, 11);
 	ret = buffer->output[0];
 
 	if (ret) {
@@ -1284,7 +1284,7 @@ static int kbd_set_state(struct kbd_state *state)
 	buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
 	buffer->input[2] = state->als_setting & 0xFF;
 	buffer->input[2] |= (state->level & 0xFF) << 16;
-	dell_send_request(buffer, 4, 11);
+	dell_send_request(4, 11);
 	ret = buffer->output[0];
 	release_buffer();
 
@@ -1326,7 +1326,7 @@ static int kbd_set_token_bit(u8 bit)
 	get_buffer();
 	buffer->input[0] = da_tokens[id].location;
 	buffer->input[1] = da_tokens[id].value;
-	dell_send_request(buffer, 1, 0);
+	dell_send_request(1, 0);
 	ret = buffer->output[0];
 	release_buffer();
 
@@ -1348,7 +1348,7 @@ static int kbd_get_token_bit(u8 bit)
 
 	get_buffer();
 	buffer->input[0] = da_tokens[id].location;
-	dell_send_request(buffer, 0, 0);
+	dell_send_request(0, 0);
 	ret = buffer->output[0];
 	val = buffer->output[1];
 	release_buffer();
@@ -2019,7 +2019,7 @@ static int __init dell_init(void)
 	if (token != -1) {
 		get_buffer();
 		buffer->input[0] = token;
-		dell_send_request(buffer, 0, 2);
+		dell_send_request(0, 2);
 		if (buffer->output[0] == 0)
 			max_intensity = buffer->output[3];
 		release_buffer();
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 260a32a..758680f 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -84,9 +84,7 @@ int find_token_location(int tokenid)
 }
 EXPORT_SYMBOL_GPL(find_token_location);
 
-struct calling_interface_buffer *
-dell_send_request(struct calling_interface_buffer *buffer, int class,
-		  int select)
+struct calling_interface_buffer *dell_send_request(int class, int select)
 {
 	struct smi_cmd command;
 
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 00e03b2..0f58ce8 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -45,7 +45,5 @@ void release_buffer(void);
 int find_token_id(int tokenid);
 int find_token_location(int tokenid);
 
-struct calling_interface_buffer *
-dell_send_request(struct calling_interface_buffer *buffer, int class,
-		  int select);
+struct calling_interface_buffer *dell_send_request(int class, int select);
 #endif
-- 
1.7.10.4

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

#1307457 — [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module

Extract SMBIOS-related code from dell-laptop to a new kernel module,
dell-smbios.  The static specifier was removed from exported symbols,
otherwise code is just moved around.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/Kconfig       |   12 ++-
 drivers/platform/x86/Makefile      |    1 +
 drivers/platform/x86/dell-laptop.c |  163 +-----------------------------
 drivers/platform/x86/dell-smbios.c |  193 ++++++++++++++++++++++++++++++++++++
 drivers/platform/x86/dell-smbios.h |   51 ++++++++++
 5 files changed, 257 insertions(+), 163 deletions(-)
 create mode 100644 drivers/platform/x86/dell-smbios.c
 create mode 100644 drivers/platform/x86/dell-smbios.h

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index f37821f..177a794 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -91,10 +91,20 @@ config ASUS_LAPTOP
 
 	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
 
+config DELL_SMBIOS
+	tristate "Dell SMBIOS Support"
+	depends on DCDBAS
+	default n
+	---help---
+	This module provides common functions for kernel modules using
+	Dell SMBIOS.
+
+	If you have a Dell laptop, say Y or M here.
+
 config DELL_LAPTOP
 	tristate "Dell Laptop Extras"
 	depends on X86
-	depends on DCDBAS
+	depends on DELL_SMBIOS
 	depends on BACKLIGHT_CLASS_DEVICE
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
 	depends on RFKILL || RFKILL = n
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 8b8df29..1128595 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_EEEPC_WMI)		+= eeepc-wmi.o
 obj-$(CONFIG_MSI_LAPTOP)	+= msi-laptop.o
 obj-$(CONFIG_ACPI_CMPC)		+= classmate-laptop.o
 obj-$(CONFIG_COMPAL_LAPTOP)	+= compal-laptop.o
+obj-$(CONFIG_DELL_SMBIOS)	+= dell-smbios.o
 obj-$(CONFIG_DELL_LAPTOP)	+= dell-laptop.o
 obj-$(CONFIG_DELL_WMI)		+= dell-wmi.o
 obj-$(CONFIG_DELL_WMI_AIO)	+= dell-wmi-aio.o
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index aaeeae8..d45d356 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -28,12 +28,11 @@
 #include <linux/acpi.h>
 #include <linux/mm.h>
 #include <linux/i8042.h>
-#include <linux/slab.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
 #include <acpi/video.h>
-#include "../../firmware/dcdbas.h"
 #include "dell-rbtn.h"
+#include "dell-smbios.h"
 
 #define BRIGHTNESS_TOKEN 0x7d
 #define KBD_LED_OFF_TOKEN 0x01E1
@@ -44,33 +43,6 @@
 #define KBD_LED_AUTO_75_TOKEN 0x02EC
 #define KBD_LED_AUTO_100_TOKEN 0x02F6
 
-/* This structure will be modified by the firmware when we enter
- * system management mode, hence the volatiles */
-
-struct calling_interface_buffer {
-	u16 class;
-	u16 select;
-	volatile u32 input[4];
-	volatile u32 output[4];
-} __packed;
-
-struct calling_interface_token {
-	u16 tokenID;
-	u16 location;
-	union {
-		u16 value;
-		u16 stringlength;
-	};
-};
-
-struct calling_interface_structure {
-	struct dmi_header header;
-	u16 cmdIOAddress;
-	u8 cmdIOCode;
-	u32 supportedCmds;
-	struct calling_interface_token tokens[];
-} __packed;
-
 struct quirk_entry {
 	u8 touchpad_led;
 
@@ -103,11 +75,6 @@ static struct quirk_entry quirk_dell_xps13_9333 = {
 	.kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 },
 };
 
-static int da_command_address;
-static int da_command_code;
-static int da_num_tokens;
-static struct calling_interface_token *da_tokens;
-
 static struct platform_driver platform_driver = {
 	.driver = {
 		.name = "dell-laptop",
@@ -306,112 +273,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 	{ }
 };
 
-static struct calling_interface_buffer *buffer;
-static DEFINE_MUTEX(buffer_mutex);
-
-static void clear_buffer(void)
-{
-	memset(buffer, 0, sizeof(struct calling_interface_buffer));
-}
-
-static void get_buffer(void)
-{
-	mutex_lock(&buffer_mutex);
-	clear_buffer();
-}
-
-static void release_buffer(void)
-{
-	mutex_unlock(&buffer_mutex);
-}
-
-static void __init parse_da_table(const struct dmi_header *dm)
-{
-	/* Final token is a terminator, so we don't want to copy it */
-	int tokens = (dm->length-11)/sizeof(struct calling_interface_token)-1;
-	struct calling_interface_token *new_da_tokens;
-	struct calling_interface_structure *table =
-		container_of(dm, struct calling_interface_structure, header);
-
-	/* 4 bytes of table header, plus 7 bytes of Dell header, plus at least
-	   6 bytes of entry */
-
-	if (dm->length < 17)
-		return;
-
-	da_command_address = table->cmdIOAddress;
-	da_command_code = table->cmdIOCode;
-
-	new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
-				 sizeof(struct calling_interface_token),
-				 GFP_KERNEL);
-
-	if (!new_da_tokens)
-		return;
-	da_tokens = new_da_tokens;
-
-	memcpy(da_tokens+da_num_tokens, table->tokens,
-	       sizeof(struct calling_interface_token) * tokens);
-
-	da_num_tokens += tokens;
-}
-
-static void __init find_tokens(const struct dmi_header *dm, void *dummy)
-{
-	switch (dm->type) {
-	case 0xd4: /* Indexed IO */
-	case 0xd5: /* Protected Area Type 1 */
-	case 0xd6: /* Protected Area Type 2 */
-		break;
-	case 0xda: /* Calling interface */
-		parse_da_table(dm);
-		break;
-	}
-}
-
-static int find_token_id(int tokenid)
-{
-	int i;
-
-	for (i = 0; i < da_num_tokens; i++) {
-		if (da_tokens[i].tokenID == tokenid)
-			return i;
-	}
-
-	return -1;
-}
-
-static int find_token_location(int tokenid)
-{
-	int id;
-
-	id = find_token_id(tokenid);
-	if (id == -1)
-		return -1;
-
-	return da_tokens[id].location;
-}
-
-static struct calling_interface_buffer *
-dell_send_request(struct calling_interface_buffer *buffer, int class,
-		  int select)
-{
-	struct smi_cmd command;
-
-	command.magic = SMI_CMD_MAGIC;
-	command.command_address = da_command_address;
-	command.command_code = da_command_code;
-	command.ebx = virt_to_phys(buffer);
-	command.ecx = 0x42534931;
-
-	buffer->class = class;
-	buffer->select = select;
-
-	dcdbas_smi_request(&command);
-
-	return buffer;
-}
-
 static inline int dell_smi_error(int value)
 {
 	switch (value) {
@@ -2122,13 +1983,6 @@ static int __init dell_init(void)
 	/* find if this machine support other functions */
 	dmi_check_system(dell_quirks);
 
-	dmi_walk(find_tokens, NULL);
-
-	if (!da_tokens)  {
-		pr_info("Unable to find dmi tokens\n");
-		return -ENODEV;
-	}
-
 	ret = platform_driver_register(&platform_driver);
 	if (ret)
 		goto fail_platform_driver;
@@ -2141,16 +1995,6 @@ static int __init dell_init(void)
 	if (ret)
 		goto fail_platform_device2;
 
-	/*
-	 * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
-	 * is passed to SMI handler.
-	 */
-	buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
-	if (!buffer) {
-		ret = -ENOMEM;
-		goto fail_buffer;
-	}
-
 	ret = dell_setup_rfkill();
 
 	if (ret) {
@@ -2208,15 +2052,12 @@ static int __init dell_init(void)
 fail_backlight:
 	dell_cleanup_rfkill();
 fail_rfkill:
-	free_page((unsigned long)buffer);
-fail_buffer:
 	platform_device_del(platform_device);
 fail_platform_device2:
 	platform_device_put(platform_device);
 fail_platform_device1:
 	platform_driver_unregister(&platform_driver);
 fail_platform_driver:
-	kfree(da_tokens);
 	return ret;
 }
 
@@ -2232,8 +2073,6 @@ static void __exit dell_exit(void)
 		platform_device_unregister(platform_device);
 		platform_driver_unregister(&platform_driver);
 	}
-	kfree(da_tokens);
-	free_page((unsigned long)buffer);
 }
 
 /* dell-rbtn.c driver export functions which will not work correctly (and could
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
new file mode 100644
index 0000000..260a32a
--- /dev/null
+++ b/drivers/platform/x86/dell-smbios.c
@@ -0,0 +1,193 @@
+/*
+ *  Common functions for kernel modules using Dell SMBIOS
+ *
+ *  Copyright (c) Red Hat <mjg@redhat.com>
+ *  Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com>
+ *  Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com>
+ *
+ *  Based on documentation in the libsmbios package:
+ *  Copyright (C) 2005-2014 Dell Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/dmi.h>
+#include <linux/gfp.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include "../../firmware/dcdbas.h"
+#include "dell-smbios.h"
+
+struct calling_interface_structure {
+	struct dmi_header header;
+	u16 cmdIOAddress;
+	u8 cmdIOCode;
+	u32 supportedCmds;
+	struct calling_interface_token tokens[];
+} __packed;
+
+static DEFINE_MUTEX(buffer_mutex);
+struct calling_interface_buffer *buffer;
+EXPORT_SYMBOL_GPL(buffer);
+
+static int da_command_address;
+static int da_command_code;
+static int da_num_tokens;
+struct calling_interface_token *da_tokens;
+EXPORT_SYMBOL_GPL(da_tokens);
+
+void clear_buffer(void)
+{
+	memset(buffer, 0, sizeof(struct calling_interface_buffer));
+}
+EXPORT_SYMBOL_GPL(clear_buffer);
+
+void get_buffer(void)
+{
+	mutex_lock(&buffer_mutex);
+	clear_buffer();
+}
+EXPORT_SYMBOL_GPL(get_buffer);
+
+void release_buffer(void)
+{
+	mutex_unlock(&buffer_mutex);
+}
+EXPORT_SYMBOL_GPL(release_buffer);
+
+int find_token_id(int tokenid)
+{
+	int i;
+
+	for (i = 0; i < da_num_tokens; i++) {
+		if (da_tokens[i].tokenID == tokenid)
+			return i;
+	}
+
+	return -1;
+}
+EXPORT_SYMBOL_GPL(find_token_id);
+
+int find_token_location(int tokenid)
+{
+	int id;
+
+	id = find_token_id(tokenid);
+	if (id == -1)
+		return -1;
+
+	return da_tokens[id].location;
+}
+EXPORT_SYMBOL_GPL(find_token_location);
+
+struct calling_interface_buffer *
+dell_send_request(struct calling_interface_buffer *buffer, int class,
+		  int select)
+{
+	struct smi_cmd command;
+
+	command.magic = SMI_CMD_MAGIC;
+	command.command_address = da_command_address;
+	command.command_code = da_command_code;
+	command.ebx = virt_to_phys(buffer);
+	command.ecx = 0x42534931;
+
+	buffer->class = class;
+	buffer->select = select;
+
+	dcdbas_smi_request(&command);
+
+	return buffer;
+}
+EXPORT_SYMBOL_GPL(dell_send_request);
+
+static void __init parse_da_table(const struct dmi_header *dm)
+{
+	/* Final token is a terminator, so we don't want to copy it */
+	int tokens = (dm->length-11)/sizeof(struct calling_interface_token)-1;
+	struct calling_interface_token *new_da_tokens;
+	struct calling_interface_structure *table =
+		container_of(dm, struct calling_interface_structure, header);
+
+	/* 4 bytes of table header, plus 7 bytes of Dell header, plus at least
+	   6 bytes of entry */
+
+	if (dm->length < 17)
+		return;
+
+	da_command_address = table->cmdIOAddress;
+	da_command_code = table->cmdIOCode;
+
+	new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
+				 sizeof(struct calling_interface_token),
+				 GFP_KERNEL);
+
+	if (!new_da_tokens)
+		return;
+	da_tokens = new_da_tokens;
+
+	memcpy(da_tokens+da_num_tokens, table->tokens,
+	       sizeof(struct calling_interface_token) * tokens);
+
+	da_num_tokens += tokens;
+}
+
+static void __init find_tokens(const struct dmi_header *dm, void *dummy)
+{
+	switch (dm->type) {
+	case 0xd4: /* Indexed IO */
+	case 0xd5: /* Protected Area Type 1 */
+	case 0xd6: /* Protected Area Type 2 */
+		break;
+	case 0xda: /* Calling interface */
+		parse_da_table(dm);
+		break;
+	}
+}
+
+static int __init dell_smbios_init(void)
+{
+	int ret;
+
+	dmi_walk(find_tokens, NULL);
+
+	if (!da_tokens)  {
+		pr_info("Unable to find dmi tokens\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
+	 * is passed to SMI handler.
+	 */
+	buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
+	if (!buffer) {
+		ret = -ENOMEM;
+		goto fail_buffer;
+	}
+
+	return 0;
+
+fail_buffer:
+	kfree(da_tokens);
+	return ret;
+}
+
+static void __exit dell_smbios_exit(void)
+{
+	kfree(da_tokens);
+	free_page((unsigned long)buffer);
+}
+
+subsys_initcall(dell_smbios_init);
+module_exit(dell_smbios_exit);
+
+MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
+MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>");
+MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
+MODULE_DESCRIPTION("Common functions for kernel modules using Dell SMBIOS");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
new file mode 100644
index 0000000..00e03b2
--- /dev/null
+++ b/drivers/platform/x86/dell-smbios.h
@@ -0,0 +1,51 @@
+/*
+ *  Common functions for kernel modules using Dell SMBIOS
+ *
+ *  Copyright (c) Red Hat <mjg@redhat.com>
+ *  Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com>
+ *  Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com>
+ *
+ *  Based on documentation in the libsmbios package:
+ *  Copyright (C) 2005-2014 Dell Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ */
+
+#ifndef _DELL_SMBIOS_H_
+#define _DELL_SMBIOS_H_
+
+/* This structure will be modified by the firmware when we enter
+ * system management mode, hence the volatiles */
+
+struct calling_interface_buffer {
+	u16 class;
+	u16 select;
+	volatile u32 input[4];
+	volatile u32 output[4];
+} __packed;
+
+struct calling_interface_token {
+	u16 tokenID;
+	u16 location;
+	union {
+		u16 value;
+		u16 stringlength;
+	};
+};
+
+extern struct calling_interface_buffer *buffer;
+extern struct calling_interface_token *da_tokens;
+
+void clear_buffer(void);
+void get_buffer(void);
+void release_buffer(void);
+
+int find_token_id(int tokenid);
+int find_token_location(int tokenid);
+
+struct calling_interface_buffer *
+dell_send_request(struct calling_interface_buffer *buffer, int class,
+		  int select);
+#endif
-- 
1.7.10.4

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

#1310966 — Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module

On Tuesday 12 January 2016 15:02:47 Michał Kępień wrote:
> Extract SMBIOS-related code from dell-laptop to a new kernel module,
> dell-smbios.  The static specifier was removed from exported symbols,
> otherwise code is just moved around.
> 
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
>  drivers/platform/x86/Kconfig       |   12 ++-
>  drivers/platform/x86/Makefile      |    1 +
>  drivers/platform/x86/dell-laptop.c |  163 +-----------------------------
>  drivers/platform/x86/dell-smbios.c |  193 ++++++++++++++++++++++++++++++++++++
>  drivers/platform/x86/dell-smbios.h |   51 ++++++++++
>  5 files changed, 257 insertions(+), 163 deletions(-)
>  create mode 100644 drivers/platform/x86/dell-smbios.c
>  create mode 100644 drivers/platform/x86/dell-smbios.h
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index f37821f..177a794 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -91,10 +91,20 @@ config ASUS_LAPTOP
>  
>  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
>  
> +config DELL_SMBIOS
> +	tristate "Dell SMBIOS Support"
> +	depends on DCDBAS
> +	default n
> +	---help---
> +	This module provides common functions for kernel modules using
> +	Dell SMBIOS.
> +
> +	If you have a Dell laptop, say Y or M here.
> +
>  config DELL_LAPTOP
>  	tristate "Dell Laptop Extras"
>  	depends on X86
> -	depends on DCDBAS
> +	depends on DELL_SMBIOS
>  	depends on BACKLIGHT_CLASS_DEVICE
>  	depends on ACPI_VIDEO || ACPI_VIDEO = n
>  	depends on RFKILL || RFKILL = n
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 8b8df29..1128595 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_EEEPC_WMI)		+= eeepc-wmi.o
>  obj-$(CONFIG_MSI_LAPTOP)	+= msi-laptop.o
>  obj-$(CONFIG_ACPI_CMPC)		+= classmate-laptop.o
>  obj-$(CONFIG_COMPAL_LAPTOP)	+= compal-laptop.o
> +obj-$(CONFIG_DELL_SMBIOS)	+= dell-smbios.o
>  obj-$(CONFIG_DELL_LAPTOP)	+= dell-laptop.o
>  obj-$(CONFIG_DELL_WMI)		+= dell-wmi.o
>  obj-$(CONFIG_DELL_WMI_AIO)	+= dell-wmi-aio.o
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index aaeeae8..d45d356 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -28,12 +28,11 @@
>  #include <linux/acpi.h>
>  #include <linux/mm.h>
>  #include <linux/i8042.h>
> -#include <linux/slab.h>
>  #include <linux/debugfs.h>
>  #include <linux/seq_file.h>
>  #include <acpi/video.h>
> -#include "../../firmware/dcdbas.h"
>  #include "dell-rbtn.h"
> +#include "dell-smbios.h"
>  
>  #define BRIGHTNESS_TOKEN 0x7d
>  #define KBD_LED_OFF_TOKEN 0x01E1
> @@ -44,33 +43,6 @@
>  #define KBD_LED_AUTO_75_TOKEN 0x02EC
>  #define KBD_LED_AUTO_100_TOKEN 0x02F6
>  
> -/* This structure will be modified by the firmware when we enter
> - * system management mode, hence the volatiles */
> -
> -struct calling_interface_buffer {
> -	u16 class;
> -	u16 select;
> -	volatile u32 input[4];
> -	volatile u32 output[4];
> -} __packed;
> -
> -struct calling_interface_token {
> -	u16 tokenID;
> -	u16 location;
> -	union {
> -		u16 value;
> -		u16 stringlength;
> -	};
> -};
> -
> -struct calling_interface_structure {
> -	struct dmi_header header;
> -	u16 cmdIOAddress;
> -	u8 cmdIOCode;
> -	u32 supportedCmds;
> -	struct calling_interface_token tokens[];
> -} __packed;
> -
>  struct quirk_entry {
>  	u8 touchpad_led;
>  
> @@ -103,11 +75,6 @@ static struct quirk_entry quirk_dell_xps13_9333 = {
>  	.kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 },
>  };
>  
> -static int da_command_address;
> -static int da_command_code;
> -static int da_num_tokens;
> -static struct calling_interface_token *da_tokens;
> -
>  static struct platform_driver platform_driver = {
>  	.driver = {
>  		.name = "dell-laptop",
> @@ -306,112 +273,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
>  	{ }
>  };
>  
> -static struct calling_interface_buffer *buffer;
> -static DEFINE_MUTEX(buffer_mutex);
> -
> -static void clear_buffer(void)
> -{
> -	memset(buffer, 0, sizeof(struct calling_interface_buffer));
> -}
> -
> -static void get_buffer(void)
> -{
> -	mutex_lock(&buffer_mutex);
> -	clear_buffer();
> -}
> -
> -static void release_buffer(void)
> -{
> -	mutex_unlock(&buffer_mutex);
> -}
> -
> -static void __init parse_da_table(const struct dmi_header *dm)
> -{
> -	/* Final token is a terminator, so we don't want to copy it */
> -	int tokens = (dm->length-11)/sizeof(struct calling_interface_token)-1;
> -	struct calling_interface_token *new_da_tokens;
> -	struct calling_interface_structure *table =
> -		container_of(dm, struct calling_interface_structure, header);
> -
> -	/* 4 bytes of table header, plus 7 bytes of Dell header, plus at least
> -	   6 bytes of entry */
> -
> -	if (dm->length < 17)
> -		return;
> -
> -	da_command_address = table->cmdIOAddress;
> -	da_command_code = table->cmdIOCode;
> -
> -	new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
> -				 sizeof(struct calling_interface_token),
> -				 GFP_KERNEL);
> -
> -	if (!new_da_tokens)
> -		return;
> -	da_tokens = new_da_tokens;
> -
> -	memcpy(da_tokens+da_num_tokens, table->tokens,
> -	       sizeof(struct calling_interface_token) * tokens);
> -
> -	da_num_tokens += tokens;
> -}
> -
> -static void __init find_tokens(const struct dmi_header *dm, void *dummy)
> -{
> -	switch (dm->type) {
> -	case 0xd4: /* Indexed IO */
> -	case 0xd5: /* Protected Area Type 1 */
> -	case 0xd6: /* Protected Area Type 2 */
> -		break;
> -	case 0xda: /* Calling interface */
> -		parse_da_table(dm);
> -		break;
> -	}
> -}
> -
> -static int find_token_id(int tokenid)
> -{
> -	int i;
> -
> -	for (i = 0; i < da_num_tokens; i++) {
> -		if (da_tokens[i].tokenID == tokenid)
> -			return i;
> -	}
> -
> -	return -1;
> -}
> -
> -static int find_token_location(int tokenid)
> -{
> -	int id;
> -
> -	id = find_token_id(tokenid);
> -	if (id == -1)
> -		return -1;
> -
> -	return da_tokens[id].location;
> -}
> -
> -static struct calling_interface_buffer *
> -dell_send_request(struct calling_interface_buffer *buffer, int class,
> -		  int select)
> -{
> -	struct smi_cmd command;
> -
> -	command.magic = SMI_CMD_MAGIC;
> -	command.command_address = da_command_address;
> -	command.command_code = da_command_code;
> -	command.ebx = virt_to_phys(buffer);
> -	command.ecx = 0x42534931;
> -
> -	buffer->class = class;
> -	buffer->select = select;
> -
> -	dcdbas_smi_request(&command);
> -
> -	return buffer;
> -}
> -
>  static inline int dell_smi_error(int value)
>  {
>  	switch (value) {
> @@ -2122,13 +1983,6 @@ static int __init dell_init(void)
>  	/* find if this machine support other functions */
>  	dmi_check_system(dell_quirks);
>  
> -	dmi_walk(find_tokens, NULL);
> -
> -	if (!da_tokens)  {
> -		pr_info("Unable to find dmi tokens\n");
> -		return -ENODEV;
> -	}
> -
>  	ret = platform_driver_register(&platform_driver);
>  	if (ret)
>  		goto fail_platform_driver;
> @@ -2141,16 +1995,6 @@ static int __init dell_init(void)
>  	if (ret)
>  		goto fail_platform_device2;
>  
> -	/*
> -	 * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
> -	 * is passed to SMI handler.
> -	 */
> -	buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
> -	if (!buffer) {
> -		ret = -ENOMEM;
> -		goto fail_buffer;
> -	}
> -
>  	ret = dell_setup_rfkill();
>  
>  	if (ret) {
> @@ -2208,15 +2052,12 @@ static int __init dell_init(void)
>  fail_backlight:
>  	dell_cleanup_rfkill();
>  fail_rfkill:
> -	free_page((unsigned long)buffer);
> -fail_buffer:
>  	platform_device_del(platform_device);
>  fail_platform_device2:
>  	platform_device_put(platform_device);
>  fail_platform_device1:
>  	platform_driver_unregister(&platform_driver);
>  fail_platform_driver:
> -	kfree(da_tokens);
>  	return ret;
>  }
>  
> @@ -2232,8 +2073,6 @@ static void __exit dell_exit(void)
>  		platform_device_unregister(platform_device);
>  		platform_driver_unregister(&platform_driver);
>  	}
> -	kfree(da_tokens);
> -	free_page((unsigned long)buffer);
>  }
>  
>  /* dell-rbtn.c driver export functions which will not work correctly (and could
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> new file mode 100644
> index 0000000..260a32a
> --- /dev/null
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -0,0 +1,193 @@
> +/*
> + *  Common functions for kernel modules using Dell SMBIOS
> + *
> + *  Copyright (c) Red Hat <mjg@redhat.com>
> + *  Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com>
> + *  Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com>
> + *
> + *  Based on documentation in the libsmbios package:
> + *  Copyright (C) 2005-2014 Dell Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/dmi.h>
> +#include <linux/gfp.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include "../../firmware/dcdbas.h"
> +#include "dell-smbios.h"
> +
> +struct calling_interface_structure {
> +	struct dmi_header header;
> +	u16 cmdIOAddress;
> +	u8 cmdIOCode;
> +	u32 supportedCmds;
> +	struct calling_interface_token tokens[];
> +} __packed;
> +
> +static DEFINE_MUTEX(buffer_mutex);
> +struct calling_interface_buffer *buffer;
> +EXPORT_SYMBOL_GPL(buffer);
> +
> +static int da_command_address;
> +static int da_command_code;
> +static int da_num_tokens;
> +struct calling_interface_token *da_tokens;
> +EXPORT_SYMBOL_GPL(da_tokens);
> +
> +void clear_buffer(void)
> +{
> +	memset(buffer, 0, sizeof(struct calling_interface_buffer));
> +}
> +EXPORT_SYMBOL_GPL(clear_buffer);
> +
> +void get_buffer(void)
> +{
> +	mutex_lock(&buffer_mutex);
> +	clear_buffer();
> +}
> +EXPORT_SYMBOL_GPL(get_buffer);
> +
> +void release_buffer(void)
> +{
> +	mutex_unlock(&buffer_mutex);
> +}
> +EXPORT_SYMBOL_GPL(release_buffer);
> +
> +int find_token_id(int tokenid)
> +{
> +	int i;
> +
> +	for (i = 0; i < da_num_tokens; i++) {
> +		if (da_tokens[i].tokenID == tokenid)
> +			return i;
> +	}
> +
> +	return -1;
> +}
> +EXPORT_SYMBOL_GPL(find_token_id);
> +
> +int find_token_location(int tokenid)
> +{
> +	int id;
> +
> +	id = find_token_id(tokenid);
> +	if (id == -1)
> +		return -1;
> +
> +	return da_tokens[id].location;
> +}
> +EXPORT_SYMBOL_GPL(find_token_location);
> +
> +struct calling_interface_buffer *
> +dell_send_request(struct calling_interface_buffer *buffer, int class,
> +		  int select)
> +{
> +	struct smi_cmd command;
> +
> +	command.magic = SMI_CMD_MAGIC;
> +	command.command_address = da_command_address;
> +	command.command_code = da_command_code;
> +	command.ebx = virt_to_phys(buffer);
> +	command.ecx = 0x42534931;
> +
> +	buffer->class = class;
> +	buffer->select = select;
> +
> +	dcdbas_smi_request(&command);
> +
> +	return buffer;
> +}
> +EXPORT_SYMBOL_GPL(dell_send_request);
> +
> +static void __init parse_da_table(const struct dmi_header *dm)
> +{
> +	/* Final token is a terminator, so we don't want to copy it */
> +	int tokens = (dm->length-11)/sizeof(struct calling_interface_token)-1;
> +	struct calling_interface_token *new_da_tokens;
> +	struct calling_interface_structure *table =
> +		container_of(dm, struct calling_interface_structure, header);
> +
> +	/* 4 bytes of table header, plus 7 bytes of Dell header, plus at least
> +	   6 bytes of entry */
> +
> +	if (dm->length < 17)
> +		return;
> +
> +	da_command_address = table->cmdIOAddress;
> +	da_command_code = table->cmdIOCode;
> +
> +	new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
> +				 sizeof(struct calling_interface_token),
> +				 GFP_KERNEL);
> +
> +	if (!new_da_tokens)
> +		return;
> +	da_tokens = new_da_tokens;
> +
> +	memcpy(da_tokens+da_num_tokens, table->tokens,
> +	       sizeof(struct calling_interface_token) * tokens);
> +
> +	da_num_tokens += tokens;
> +}
> +
> +static void __init find_tokens(const struct dmi_header *dm, void *dummy)
> +{
> +	switch (dm->type) {
> +	case 0xd4: /* Indexed IO */
> +	case 0xd5: /* Protected Area Type 1 */
> +	case 0xd6: /* Protected Area Type 2 */
> +		break;
> +	case 0xda: /* Calling interface */
> +		parse_da_table(dm);
> +		break;
> +	}
> +}
> +
> +static int __init dell_smbios_init(void)
> +{
> +	int ret;
> +
> +	dmi_walk(find_tokens, NULL);
> +
> +	if (!da_tokens)  {
> +		pr_info("Unable to find dmi tokens\n");
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
> +	 * is passed to SMI handler.
> +	 */
> +	buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
> +	if (!buffer) {
> +		ret = -ENOMEM;
> +		goto fail_buffer;
> +	}
> +
> +	return 0;
> +
> +fail_buffer:
> +	kfree(da_tokens);
> +	return ret;
> +}
> +
> +static void __exit dell_smbios_exit(void)
> +{
> +	kfree(da_tokens);
> +	free_page((unsigned long)buffer);
> +}
> +
> +subsys_initcall(dell_smbios_init);
> +module_exit(dell_smbios_exit);
> +
> +MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
> +MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>");
> +MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
> +MODULE_DESCRIPTION("Common functions for kernel modules using Dell SMBIOS");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
> new file mode 100644
> index 0000000..00e03b2
> --- /dev/null
> +++ b/drivers/platform/x86/dell-smbios.h
> @@ -0,0 +1,51 @@
> +/*
> + *  Common functions for kernel modules using Dell SMBIOS
> + *
> + *  Copyright (c) Red Hat <mjg@redhat.com>
> + *  Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com>
> + *  Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com>
> + *
> + *  Based on documentation in the libsmbios package:
> + *  Copyright (C) 2005-2014 Dell Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + */
> +
> +#ifndef _DELL_SMBIOS_H_
> +#define _DELL_SMBIOS_H_
> +
> +/* This structure will be modified by the firmware when we enter
> + * system management mode, hence the volatiles */
> +
> +struct calling_interface_buffer {
> +	u16 class;
> +	u16 select;
> +	volatile u32 input[4];
> +	volatile u32 output[4];
> +} __packed;
> +
> +struct calling_interface_token {
> +	u16 tokenID;
> +	u16 location;
> +	union {
> +		u16 value;
> +		u16 stringlength;
> +	};
> +};

After patch 12/14 you do not need to define this struct in header file.

> +extern struct calling_interface_buffer *buffer;
> +extern struct calling_interface_token *da_tokens;

Better hide this variable in dell-smbios.c code ...

> +void clear_buffer(void);
> +void get_buffer(void);
> +void release_buffer(void);

... and let those functions to get parameter to buffer.

E.g. get_buffer will return buffer and other two functions will take
buffer parameter.

> +int find_token_id(int tokenid);
> +int find_token_location(int tokenid);
> +
> +struct calling_interface_buffer *
> +dell_send_request(struct calling_interface_buffer *buffer, int class,
> +		  int select);
> +#endif

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

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

#1311463 — Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module

Hi Pali,

Thanks for taking a look.

> > +struct calling_interface_token {
> > +	u16 tokenID;
> > +	u16 location;
> > +	union {
> > +		u16 value;
> > +		u16 stringlength;
> > +	};
> > +};
> 
> After patch 12/14 you do not need to define this struct in header file.

dell_smbios_find_token() returns a struct calling_interface_token *,
which can then be dereferenced by the caller.  See patch 13.

> > +extern struct calling_interface_buffer *buffer;
> > +extern struct calling_interface_token *da_tokens;
> 
> Better hide this variable in dell-smbios.c code ...

Patch 12 makes da_tokens static, but I believe you were referring to
buffer.

> > +void clear_buffer(void);
> > +void get_buffer(void);
> > +void release_buffer(void);
> 
> ... and let those functions to get parameter to buffer.
> 
> E.g. get_buffer will return buffer and other two functions will take
> buffer parameter.

You're right.  My original approach looked tempting because it reduces
the amount of changes required in dell-laptop, but on second thought, it
_assumes_ that users of this API would play nicely with the SMBIOS
buffer while your way _enforces_ it.

I will prepare a v2 including this suggestion within the next couple of
days.

-- 
Best regards,
Michał Kępień

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

#1313027 — Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module

> > +extern struct calling_interface_buffer *buffer;
> > +extern struct calling_interface_token *da_tokens;
> 
> Better hide this variable in dell-smbios.c code ...
> 
> > +void clear_buffer(void);
> > +void get_buffer(void);
> > +void release_buffer(void);
> 
> ... and let those functions to get parameter to buffer.
> 
> E.g. get_buffer will return buffer and other two functions will take
> buffer parameter.

Before I spam everyone with another set of 15 patches, I'd like to
discuss this a bit further.  There is no point in passing the buffer to
release_buffer(), because it only unlocks a mutex.  I also see no point
in passing the buffer to clear_buffer() and dell_send_request(), because
there is always just one buffer to operate on.

A total of four functions have something to do with the SMBIOS buffer:

  * get_buffer()
  * clear_buffer()
  * release_buffer()
  * dell_send_request()

This rework is a chance to make them all consistent, i.e. remove the
SMBIOS buffer from their argument lists.  This way we can "signal" this
API's users that there is only one SMBIOS buffer ever involved while
still removing the extern and EXPORT_SYMBOL_GPL for the buffer.  BTW, I
also see little point in returning the buffer from dell_send_request()
as none of its callers in dell-laptop assign its return value to
anything (i.e. there is no "buffer = dell_send_request(buffer, ...)" in
the code).

To sum up, I'd suggest that function prototypes could look like this:

    struct calling_interface_buffer *dell_smbios_get_buffer(void);
    void dell_smbios_clear_buffer(void);
    void dell_smbios_release_buffer(void);
    void dell_smbios_send_request(int class, int select);

What do you think?

-- 
Best regards,
Michał Kępień

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

#1314000 — Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module

On Wednesday 20 January 2016 10:21:07 Michał Kępień wrote:
> > > +extern struct calling_interface_buffer *buffer;
> > > +extern struct calling_interface_token *da_tokens;
> > 
> > Better hide this variable in dell-smbios.c code ...
> > 
> > > +void clear_buffer(void);
> > > +void get_buffer(void);
> > > +void release_buffer(void);
> > 
> > ... and let those functions to get parameter to buffer.
> > 
> > E.g. get_buffer will return buffer and other two functions will take
> > buffer parameter.
> 
> Before I spam everyone with another set of 15 patches, I'd like to
> discuss this a bit further.  There is no point in passing the buffer to
> release_buffer(), because it only unlocks a mutex.  I also see no point
> in passing the buffer to clear_buffer() and dell_send_request(), because
> there is always just one buffer to operate on.
> 
> A total of four functions have something to do with the SMBIOS buffer:
> 
>   * get_buffer()
>   * clear_buffer()
>   * release_buffer()
>   * dell_send_request()
> 
> This rework is a chance to make them all consistent, i.e. remove the
> SMBIOS buffer from their argument lists.  This way we can "signal" this
> API's users that there is only one SMBIOS buffer ever involved while
> still removing the extern and EXPORT_SYMBOL_GPL for the buffer.  BTW, I
> also see little point in returning the buffer from dell_send_request()
> as none of its callers in dell-laptop assign its return value to
> anything (i.e. there is no "buffer = dell_send_request(buffer, ...)" in
> the code).
> 
> To sum up, I'd suggest that function prototypes could look like this:
> 
>     struct calling_interface_buffer *dell_smbios_get_buffer(void);
>     void dell_smbios_clear_buffer(void);
>     void dell_smbios_release_buffer(void);
>     void dell_smbios_send_request(int class, int select);
> 
> What do you think?
> 

In other scenario functions should do something like this:

struct buf *buf_alloc(void);
buf_clear(struct buf *buf);
buf_free(struct buf *buf);
buf_do_something(struct buf *buf, ...);

But here I do not know how hard is to create alloc/free functions and
what is cost for creating that buffer in first 4GB memory...

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

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

#1314189 — Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module

> > A total of four functions have something to do with the SMBIOS buffer:
> > 
> >   * get_buffer()
> >   * clear_buffer()
> >   * release_buffer()
> >   * dell_send_request()
> > 
> > This rework is a chance to make them all consistent, i.e. remove the
> > SMBIOS buffer from their argument lists.  This way we can "signal" this
> > API's users that there is only one SMBIOS buffer ever involved while
> > still removing the extern and EXPORT_SYMBOL_GPL for the buffer.  BTW, I
> > also see little point in returning the buffer from dell_send_request()
> > as none of its callers in dell-laptop assign its return value to
> > anything (i.e. there is no "buffer = dell_send_request(buffer, ...)" in
> > the code).
> > 
> > To sum up, I'd suggest that function prototypes could look like this:
> > 
> >     struct calling_interface_buffer *dell_smbios_get_buffer(void);
> >     void dell_smbios_clear_buffer(void);
> >     void dell_smbios_release_buffer(void);
> >     void dell_smbios_send_request(int class, int select);
> > 
> > What do you think?
> > 
> 
> In other scenario functions should do something like this:
> 
> struct buf *buf_alloc(void);
> buf_clear(struct buf *buf);
> buf_free(struct buf *buf);
> buf_do_something(struct buf *buf, ...);
> 
> But here I do not know how hard is to create alloc/free functions and
> what is cost for creating that buffer in first 4GB memory...

I'm guessing the cost is negligible, given that SMBIOS calls are not
present in any hot path.  Writing these functions is also pretty
straightforward, but the inconvenience of this approach is that it
forces the callers to do the error-checking for each buf_alloc() call.
It also seems pretty inefficient - notice we only need 36 bytes for the
calling interface buffer, yet we would be allocating a whole page in
each buf_alloc() call.

On the other hand, I believe returning a separate buffer for each
buf_alloc() caller makes it possible to drop buffer_mutex altogether.
Yet, the approach I suggested is more similar to what the Dell-supplied
dcdbas driver does internally (it manages a single, resizable buffer,
which is protected by a mutex and controllable from userspace through
sysfs), which is why I think it's a good idea to stick to that concept
for consistency.

As this patch series already touches a lot of code, I would like to
avoid changing the underlying concepts as much as possible.  If that's
okay with you, I'll post a v2 which includes your suggestion to make the
buffer pointer static while keeping the interface similar to the
original one.  If you would really like me to take a different path,
please let me know and I'll comply.

-- 
Best regards,
Michał Kępień

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

#1314192 — Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module

On Thursday 21 January 2016 14:06:03 Michał Kępień wrote:
> > > A total of four functions have something to do with the SMBIOS buffer:
> > > 
> > >   * get_buffer()
> > >   * clear_buffer()
> > >   * release_buffer()
> > >   * dell_send_request()
> > > 
> > > This rework is a chance to make them all consistent, i.e. remove the
> > > SMBIOS buffer from their argument lists.  This way we can "signal" this
> > > API's users that there is only one SMBIOS buffer ever involved while
> > > still removing the extern and EXPORT_SYMBOL_GPL for the buffer.  BTW, I
> > > also see little point in returning the buffer from dell_send_request()
> > > as none of its callers in dell-laptop assign its return value to
> > > anything (i.e. there is no "buffer = dell_send_request(buffer, ...)" in
> > > the code).
> > > 
> > > To sum up, I'd suggest that function prototypes could look like this:
> > > 
> > >     struct calling_interface_buffer *dell_smbios_get_buffer(void);
> > >     void dell_smbios_clear_buffer(void);
> > >     void dell_smbios_release_buffer(void);
> > >     void dell_smbios_send_request(int class, int select);
> > > 
> > > What do you think?
> > > 
> > 
> > In other scenario functions should do something like this:
> > 
> > struct buf *buf_alloc(void);
> > buf_clear(struct buf *buf);
> > buf_free(struct buf *buf);
> > buf_do_something(struct buf *buf, ...);
> > 
> > But here I do not know how hard is to create alloc/free functions and
> > what is cost for creating that buffer in first 4GB memory...
> 
> I'm guessing the cost is negligible, given that SMBIOS calls are not
> present in any hot path.  Writing these functions is also pretty
> straightforward, but the inconvenience of this approach is that it
> forces the callers to do the error-checking for each buf_alloc() call.
> It also seems pretty inefficient - notice we only need 36 bytes for the
> calling interface buffer, yet we would be allocating a whole page in
> each buf_alloc() call.
> 
> On the other hand, I believe returning a separate buffer for each
> buf_alloc() caller makes it possible to drop buffer_mutex altogether.
> Yet, the approach I suggested is more similar to what the Dell-supplied
> dcdbas driver does internally (it manages a single, resizable buffer,
> which is protected by a mutex and controllable from userspace through
> sysfs), which is why I think it's a good idea to stick to that concept
> for consistency.
> 
> As this patch series already touches a lot of code, I would like to
> avoid changing the underlying concepts as much as possible.  If that's
> okay with you, I'll post a v2 which includes your suggestion to make the
> buffer pointer static while keeping the interface similar to the
> original one.  If you would really like me to take a different path,
> please let me know and I'll comply.
> 

Another idea:

What about passing struct calling_interface_buffer from caller allocated
memory (either from stack or kernel alloc) to dell-smbios which will
copy it into own buffer under 4GB and then pass it to dcdbas?

This will avoid to use that get/release function and there will be only
one send_request.

But I will let decision for API to other people as I do not know what
the best API to use here...

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

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

#1314196 — Re: [PATCH 01/14] dell-laptop: extract SMBIOS-related code to a separate module

> Another idea:
> 
> What about passing struct calling_interface_buffer from caller allocated
> memory (either from stack or kernel alloc) to dell-smbios which will
> copy it into own buffer under 4GB and then pass it to dcdbas?
> 
> This will avoid to use that get/release function and there will be only
> one send_request.

Well, yes, these two functions could then be ripped out, but the callers
would have to do the error checking on their own.  Of course that's not
a bad thing per se, but it changes the currently used concept.

> But I will let decision for API to other people as I do not know what
> the best API to use here...

In order to avoid delaying this any further, I'll post a v2 soon and
hopefully it'll be good enough for your Acked-by.  If it turns out more
people have misgivings about it, I'll adjust the code.

-- 
Best regards,
Michał Kępień

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

#1307462 — [PATCH 03/14] dell-smbios: rename buffer to dell_smbios_buffer

As the SMBIOS buffer is exported from the module, it has to be renamed
to something less generic, so add a "dell_smbios_" prefix to the
variable name.

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

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 572bdca..aaef181 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -419,18 +419,18 @@ static int dell_rfkill_set(void *data, bool blocked)
 	get_buffer();
 
 	dell_send_request(17, 11);
-	ret = buffer->output[0];
-	status = buffer->output[1];
+	ret = dell_smbios_buffer->output[0];
+	status = dell_smbios_buffer->output[1];
 
 	if (ret != 0)
 		goto out;
 
 	clear_buffer();
 
-	buffer->input[0] = 0x2;
+	dell_smbios_buffer->input[0] = 0x2;
 	dell_send_request(17, 11);
-	ret = buffer->output[0];
-	hwswitch = buffer->output[1];
+	ret = dell_smbios_buffer->output[0];
+	hwswitch = dell_smbios_buffer->output[1];
 
 	/* If the hardware switch controls this radio, and the hardware
 	   switch is disabled, always disable the radio */
@@ -440,9 +440,9 @@ static int dell_rfkill_set(void *data, bool blocked)
 
 	clear_buffer();
 
-	buffer->input[0] = (1 | (radio<<8) | (disable << 16));
+	dell_smbios_buffer->input[0] = (1 | (radio << 8) | (disable << 16));
 	dell_send_request(17, 11);
-	ret = buffer->output[0];
+	ret = dell_smbios_buffer->output[0];
 
  out:
 	release_buffer();
@@ -457,7 +457,8 @@ static void dell_rfkill_update_sw_state(struct rfkill *rfkill, int radio,
 		/* Has hw-switch, sync sw_state to BIOS */
 		int block = rfkill_blocked(rfkill);
 		clear_buffer();
-		buffer->input[0] = (1 | (radio << 8) | (block << 16));
+		dell_smbios_buffer->input[0] = (1 | (radio << 8)
+						  | (block << 16));
 		dell_send_request(17, 11);
 	} else {
 		/* No hw-switch, sync BIOS state to sw_state */
@@ -482,8 +483,8 @@ static void dell_rfkill_query(struct rfkill *rfkill, void *data)
 	get_buffer();
 
 	dell_send_request(17, 11);
-	ret = buffer->output[0];
-	status = buffer->output[1];
+	ret = dell_smbios_buffer->output[0];
+	status = dell_smbios_buffer->output[1];
 
 	if (ret != 0 || !(status & BIT(0))) {
 		release_buffer();
@@ -492,10 +493,10 @@ static void dell_rfkill_query(struct rfkill *rfkill, void *data)
 
 	clear_buffer();
 
-	buffer->input[0] = 0x2;
+	dell_smbios_buffer->input[0] = 0x2;
 	dell_send_request(17, 11);
-	ret = buffer->output[0];
-	hwswitch = buffer->output[1];
+	ret = dell_smbios_buffer->output[0];
+	hwswitch = dell_smbios_buffer->output[1];
 
 	release_buffer();
 
@@ -522,15 +523,15 @@ static int dell_debugfs_show(struct seq_file *s, void *data)
 	get_buffer();
 
 	dell_send_request(17, 11);
-	ret = buffer->output[0];
-	status = buffer->output[1];
+	ret = dell_smbios_buffer->output[0];
+	status = dell_smbios_buffer->output[1];
 
 	clear_buffer();
 
-	buffer->input[0] = 0x2;
+	dell_smbios_buffer->input[0] = 0x2;
 	dell_send_request(17, 11);
-	hwswitch_ret = buffer->output[0];
-	hwswitch_state = buffer->output[1];
+	hwswitch_ret = dell_smbios_buffer->output[0];
+	hwswitch_state = dell_smbios_buffer->output[1];
 
 	release_buffer();
 
@@ -620,20 +621,20 @@ static void dell_update_rfkill(struct work_struct *ignored)
 	get_buffer();
 
 	dell_send_request(17, 11);
-	ret = buffer->output[0];
-	status = buffer->output[1];
+	ret = dell_smbios_buffer->output[0];
+	status = dell_smbios_buffer->output[1];
 
 	if (ret != 0)
 		goto out;
 
 	clear_buffer();
 
-	buffer->input[0] = 0x2;
+	dell_smbios_buffer->input[0] = 0x2;
 	dell_send_request(17, 11);
-	ret = buffer->output[0];
+	ret = dell_smbios_buffer->output[0];
 
 	if (ret == 0 && (status & BIT(0)))
-		hwswitch = buffer->output[1];
+		hwswitch = dell_smbios_buffer->output[1];
 
 	if (wifi_rfkill) {
 		dell_rfkill_update_hw_state(wifi_rfkill, 1, status, hwswitch);
@@ -711,8 +712,8 @@ static int __init dell_setup_rfkill(void)
 
 	get_buffer();
 	dell_send_request(17, 11);
-	ret = buffer->output[0];
-	status = buffer->output[1];
+	ret = dell_smbios_buffer->output[0];
+	status = dell_smbios_buffer->output[1];
 	release_buffer();
 
 	/* dell wireless info smbios call is not supported */
@@ -874,15 +875,15 @@ static int dell_send_intensity(struct backlight_device *bd)
 		return -ENODEV;
 
 	get_buffer();
-	buffer->input[0] = token;
-	buffer->input[1] = bd->props.brightness;
+	dell_smbios_buffer->input[0] = token;
+	dell_smbios_buffer->input[1] = bd->props.brightness;
 
 	if (power_supply_is_system_supplied() > 0)
 		dell_send_request(1, 2);
 	else
 		dell_send_request(1, 1);
 
-	ret = dell_smi_error(buffer->output[0]);
+	ret = dell_smi_error(dell_smbios_buffer->output[0]);
 
 	release_buffer();
 	return ret;
@@ -898,17 +899,17 @@ static int dell_get_intensity(struct backlight_device *bd)
 		return -ENODEV;
 
 	get_buffer();
-	buffer->input[0] = token;
+	dell_smbios_buffer->input[0] = token;
 
 	if (power_supply_is_system_supplied() > 0)
 		dell_send_request(0, 2);
 	else
 		dell_send_request(0, 1);
 
-	if (buffer->output[0])
-		ret = dell_smi_error(buffer->output[0]);
+	if (dell_smbios_buffer->output[0])
+		ret = dell_smi_error(dell_smbios_buffer->output[0]);
 	else
-		ret = buffer->output[1];
+		ret = dell_smbios_buffer->output[1];
 
 	release_buffer();
 	return ret;
@@ -1159,29 +1160,29 @@ static int kbd_get_info(struct kbd_info *info)
 
 	get_buffer();
 
-	buffer->input[0] = 0x0;
+	dell_smbios_buffer->input[0] = 0x0;
 	dell_send_request(4, 11);
-	ret = buffer->output[0];
+	ret = dell_smbios_buffer->output[0];
 
 	if (ret) {
 		ret = dell_smi_error(ret);
 		goto out;
 	}
 
-	info->modes = buffer->output[1] & 0xFFFF;
-	info->type = (buffer->output[1] >> 24) & 0xFF;
-	info->triggers = buffer->output[2] & 0xFF;
-	units = (buffer->output[2] >> 8) & 0xFF;
-	info->levels = (buffer->output[2] >> 16) & 0xFF;
+	info->modes = dell_smbios_buffer->output[1] & 0xFFFF;
+	info->type = (dell_smbios_buffer->output[1] >> 24) & 0xFF;
+	info->triggers = dell_smbios_buffer->output[2] & 0xFF;
+	units = (dell_smbios_buffer->output[2] >> 8) & 0xFF;
+	info->levels = (dell_smbios_buffer->output[2] >> 16) & 0xFF;
 
 	if (units & BIT(0))
-		info->seconds = (buffer->output[3] >> 0) & 0xFF;
+		info->seconds = (dell_smbios_buffer->output[3] >> 0) & 0xFF;
 	if (units & BIT(1))
-		info->minutes = (buffer->output[3] >> 8) & 0xFF;
+		info->minutes = (dell_smbios_buffer->output[3] >> 8) & 0xFF;
 	if (units & BIT(2))
-		info->hours = (buffer->output[3] >> 16) & 0xFF;
+		info->hours = (dell_smbios_buffer->output[3] >> 16) & 0xFF;
 	if (units & BIT(3))
-		info->days = (buffer->output[3] >> 24) & 0xFF;
+		info->days = (dell_smbios_buffer->output[3] >> 24) & 0xFF;
 
  out:
 	release_buffer();
@@ -1247,25 +1248,25 @@ static int kbd_get_state(struct kbd_state *state)
 
 	get_buffer();
 
-	buffer->input[0] = 0x1;
+	dell_smbios_buffer->input[0] = 0x1;
 	dell_send_request(4, 11);
-	ret = buffer->output[0];
+	ret = dell_smbios_buffer->output[0];
 
 	if (ret) {
 		ret = dell_smi_error(ret);
 		goto out;
 	}
 
-	state->mode_bit = ffs(buffer->output[1] & 0xFFFF);
+	state->mode_bit = ffs(dell_smbios_buffer->output[1] & 0xFFFF);
 	if (state->mode_bit != 0)
 		state->mode_bit--;
 
-	state->triggers = (buffer->output[1] >> 16) & 0xFF;
-	state->timeout_value = (buffer->output[1] >> 24) & 0x3F;
-	state->timeout_unit = (buffer->output[1] >> 30) & 0x3;
-	state->als_setting = buffer->output[2] & 0xFF;
-	state->als_value = (buffer->output[2] >> 8) & 0xFF;
-	state->level = (buffer->output[2] >> 16) & 0xFF;
+	state->triggers = (dell_smbios_buffer->output[1] >> 16) & 0xFF;
+	state->timeout_value = (dell_smbios_buffer->output[1] >> 24) & 0x3F;
+	state->timeout_unit = (dell_smbios_buffer->output[1] >> 30) & 0x3;
+	state->als_setting = dell_smbios_buffer->output[2] & 0xFF;
+	state->als_value = (dell_smbios_buffer->output[2] >> 8) & 0xFF;
+	state->level = (dell_smbios_buffer->output[2] >> 16) & 0xFF;
 
  out:
 	release_buffer();
@@ -1277,15 +1278,15 @@ static int kbd_set_state(struct kbd_state *state)
 	int ret;
 
 	get_buffer();
-	buffer->input[0] = 0x2;
-	buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
-	buffer->input[1] |= (state->triggers & 0xFF) << 16;
-	buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
-	buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
-	buffer->input[2] = state->als_setting & 0xFF;
-	buffer->input[2] |= (state->level & 0xFF) << 16;
+	dell_smbios_buffer->input[0] = 0x2;
+	dell_smbios_buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
+	dell_smbios_buffer->input[1] |= (state->triggers & 0xFF) << 16;
+	dell_smbios_buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
+	dell_smbios_buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
+	dell_smbios_buffer->input[2] = state->als_setting & 0xFF;
+	dell_smbios_buffer->input[2] |= (state->level & 0xFF) << 16;
 	dell_send_request(4, 11);
-	ret = buffer->output[0];
+	ret = dell_smbios_buffer->output[0];
 	release_buffer();
 
 	return dell_smi_error(ret);
@@ -1324,10 +1325,10 @@ static int kbd_set_token_bit(u8 bit)
 		return -EINVAL;
 
 	get_buffer();
-	buffer->input[0] = da_tokens[id].location;
-	buffer->input[1] = da_tokens[id].value;
+	dell_smbios_buffer->input[0] = da_tokens[id].location;
+	dell_smbios_buffer->input[1] = da_tokens[id].value;
 	dell_send_request(1, 0);
-	ret = buffer->output[0];
+	ret = dell_smbios_buffer->output[0];
 	release_buffer();
 
 	return dell_smi_error(ret);
@@ -1347,10 +1348,10 @@ static int kbd_get_token_bit(u8 bit)
 		return -EINVAL;
 
 	get_buffer();
-	buffer->input[0] = da_tokens[id].location;
+	dell_smbios_buffer->input[0] = da_tokens[id].location;
 	dell_send_request(0, 0);
-	ret = buffer->output[0];
-	val = buffer->output[1];
+	ret = dell_smbios_buffer->output[0];
+	val = dell_smbios_buffer->output[1];
 	release_buffer();
 
 	if (ret)
@@ -2018,10 +2019,10 @@ static int __init dell_init(void)
 	token = find_token_location(BRIGHTNESS_TOKEN);
 	if (token != -1) {
 		get_buffer();
-		buffer->input[0] = token;
+		dell_smbios_buffer->input[0] = token;
 		dell_send_request(0, 2);
-		if (buffer->output[0] == 0)
-			max_intensity = buffer->output[3];
+		if (dell_smbios_buffer->output[0] == 0)
+			max_intensity = dell_smbios_buffer->output[3];
 		release_buffer();
 	}
 
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 758680f..a3898f9 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -31,8 +31,8 @@ struct calling_interface_structure {
 } __packed;
 
 static DEFINE_MUTEX(buffer_mutex);
-struct calling_interface_buffer *buffer;
-EXPORT_SYMBOL_GPL(buffer);
+struct calling_interface_buffer *dell_smbios_buffer;
+EXPORT_SYMBOL_GPL(dell_smbios_buffer);
 
 static int da_command_address;
 static int da_command_code;
@@ -42,7 +42,7 @@ EXPORT_SYMBOL_GPL(da_tokens);
 
 void clear_buffer(void)
 {
-	memset(buffer, 0, sizeof(struct calling_interface_buffer));
+	memset(dell_smbios_buffer, 0, sizeof(struct calling_interface_buffer));
 }
 EXPORT_SYMBOL_GPL(clear_buffer);
 
@@ -91,15 +91,15 @@ struct calling_interface_buffer *dell_send_request(int class, int select)
 	command.magic = SMI_CMD_MAGIC;
 	command.command_address = da_command_address;
 	command.command_code = da_command_code;
-	command.ebx = virt_to_phys(buffer);
+	command.ebx = virt_to_phys(dell_smbios_buffer);
 	command.ecx = 0x42534931;
 
-	buffer->class = class;
-	buffer->select = select;
+	dell_smbios_buffer->class = class;
+	dell_smbios_buffer->select = select;
 
 	dcdbas_smi_request(&command);
 
-	return buffer;
+	return dell_smbios_buffer;
 }
 EXPORT_SYMBOL_GPL(dell_send_request);
 
@@ -162,8 +162,8 @@ static int __init dell_smbios_init(void)
 	 * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
 	 * is passed to SMI handler.
 	 */
-	buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
-	if (!buffer) {
+	dell_smbios_buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
+	if (!dell_smbios_buffer) {
 		ret = -ENOMEM;
 		goto fail_buffer;
 	}
@@ -178,7 +178,7 @@ fail_buffer:
 static void __exit dell_smbios_exit(void)
 {
 	kfree(da_tokens);
-	free_page((unsigned long)buffer);
+	free_page((unsigned long)dell_smbios_buffer);
 }
 
 subsys_initcall(dell_smbios_init);
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 0f58ce8..89c787c 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -35,7 +35,7 @@ struct calling_interface_token {
 	};
 };
 
-extern struct calling_interface_buffer *buffer;
+extern struct calling_interface_buffer *dell_smbios_buffer;
 extern struct calling_interface_token *da_tokens;
 
 void clear_buffer(void);
-- 
1.7.10.4

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

#1309737

On Tue, Jan 12, 2016 at 03:02:46PM +0100, Michał Kępień wrote:
> The Linux kernel tree currently contains two Dell laptop-related drivers
> issuing SMBIOS requests in different ways (dell-laptop in
> drivers/platform/x86 and dell-led in drivers/led).  As an upcoming patch
> series for the dell-wmi driver (also in drivers/platform/x86) will
> change it so that it also performs SMBIOS requests, I took the
> opportunity to unify the API used for issuing Dell SMBIOS requests
> throughout the kernel before any further code duplication happens.
> Credit for suggesting this goes to Pali Rohár.
> 
> This patch series is primarily intended for the platform-x86 subsystem,
> with only 2 final patches touching the LED subsystem.  I decided to send
> the whole series to everyone involved to provide context - my apologies
> if this is frowned upon.

I much prefer it, thank you.

> 
> As for making dell-led dependent on a driver in drivers/platform/x86,
> let me just hint that Pali and I think it could be possible to
> eventually move all of dell-led's code to drivers/platform/x86.  But
> first things first.
> 
> The first patch generates a lot of checkpatch warnings, but these are
> also raised for the original code and I decided that not changing the
> code while moving around large quantities of it is critical for
> reviewability.

Noted, thanks.

> 
> Alex, as I don't have the hardware to test the changes in dell-led
> (beyond compilation) and you contributed the parts of it which this
> patch series changes, is there any way you might test it on relevant
> hardware?

OK, before I dive into a review on this, I'm going to be looking for an ack from
Pali and some Tested-by from the usual suspects. We're already into the merge
window and this series needs to spend some time in next. We'll plan on getting
this into next after the merge window closes and have it land in 4.6. Hopefully
that will allow us to work through Andy's wmi rearchitecting at the same time
and catch any incompatibilities without introducing undue churn to mainline.

> 
>  drivers/leds/Kconfig               |    1 +
>  drivers/leds/dell-led.c            |  125 ++--------
>  drivers/platform/x86/Kconfig       |   12 +-
>  drivers/platform/x86/Makefile      |    1 +
>  drivers/platform/x86/dell-laptop.c |  444 ++++++++++++------------------------
>  drivers/platform/x86/dell-smbios.c |  179 +++++++++++++++
>  drivers/platform/x86/dell-smbios.h |   48 ++++
>  7 files changed, 395 insertions(+), 415 deletions(-)

My favorite kind of patch              ^ :-)

Thanks!

>  create mode 100644 drivers/platform/x86/dell-smbios.c
>  create mode 100644 drivers/platform/x86/dell-smbios.h
> 
> -- 
> 1.7.10.4
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web