[PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Mario Limonciello]

There may be race conditions with multiple different functions working
on a module wide buffer causing one function to get the wrong results.

Suggested-by: Pali Rohar <pali.rohar@gmail.com>
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/platform/x86/dell-laptop.c | 188 ++++++++++++++++++++-----------------
 1 file changed, 103 insertions(+), 85 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index fc2dfc8..ab58373 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -78,7 +78,6 @@ static struct platform_driver platform_driver = {
 	}
 };
 
-static struct calling_interface_buffer *buffer;
 static struct platform_device *platform_device;
 static struct backlight_device *dell_backlight_device;
 static struct rfkill *wifi_rfkill;
@@ -322,7 +321,8 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 	{ }
 };
 
-static void dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3)
+static void dell_set_arguments(struct calling_interface_buffer *buffer,
+			       u32 arg0, u32 arg1, u32 arg2, u32 arg3)
 {
 	memset(buffer, 0, sizeof(struct calling_interface_buffer));
 	buffer->input[0] = arg0;
@@ -331,7 +331,8 @@ static void dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3)
 	buffer->input[3] = arg3;
 }
 
-static int dell_send_request(u16 class, u16 select)
+static int dell_send_request(struct calling_interface_buffer *buffer,
+			     u16 class, u16 select)
 {
 	int ret;
 
@@ -468,21 +469,22 @@ static int dell_rfkill_set(void *data, bool blocked)
 	int disable = blocked ? 1 : 0;
 	unsigned long radio = (unsigned long)data;
 	int hwswitch_bit = (unsigned long)data - 1;
+	struct calling_interface_buffer buffer;
 	int hwswitch;
 	int status;
 	int ret;
 
-	dell_set_arguments(0, 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
+	dell_set_arguments(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 	if (ret)
 		return ret;
-	status = buffer->output[1];
+	status = buffer.output[1];
 
-	dell_set_arguments(0x2, 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
+	dell_set_arguments(&buffer, 0x2, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 	if (ret)
 		return ret;
-	hwswitch = buffer->output[1];
+	hwswitch = buffer.output[1];
 
 	/* If the hardware switch controls this radio, and the hardware
 	   switch is disabled, always disable the radio */
@@ -490,8 +492,8 @@ static int dell_rfkill_set(void *data, bool blocked)
 	    (status & BIT(0)) && !(status & BIT(16)))
 		disable = 1;
 
-	dell_set_arguments(1 | (radio<<8) | (disable << 16), 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
+	dell_set_arguments(&buffer, 1 | (radio<<8) | (disable << 16), 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 	return ret;
 }
 
@@ -500,9 +502,11 @@ static void dell_rfkill_update_sw_state(struct rfkill *rfkill, int radio,
 {
 	if (status & BIT(0)) {
 		/* Has hw-switch, sync sw_state to BIOS */
+		struct calling_interface_buffer buffer;
 		int block = rfkill_blocked(rfkill);
-		dell_set_arguments(1 | (radio << 8) | (block << 16), 0, 0, 0);
-		dell_send_request(CLASS_INFO, SELECT_RFKILL);
+		dell_set_arguments(&buffer,
+				   1 | (radio << 8) | (block << 16), 0, 0, 0);
+		dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 	} else {
 		/* No hw-switch, sync BIOS state to sw_state */
 		rfkill_set_sw_state(rfkill, !!(status & BIT(radio + 16)));
@@ -519,21 +523,22 @@ static void dell_rfkill_update_hw_state(struct rfkill *rfkill, int radio,
 static void dell_rfkill_query(struct rfkill *rfkill, void *data)
 {
 	int radio = ((unsigned long)data & 0xF);
+	struct calling_interface_buffer buffer;
 	int hwswitch;
 	int status;
 	int ret;
 
-	dell_set_arguments(0, 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
-	status = buffer->output[1];
+	dell_set_arguments(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	status = buffer.output[1];
 
 	if (ret != 0 || !(status & BIT(0))) {
 		return;
 	}
 
-	dell_set_arguments(0, 0x2, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
-	hwswitch = buffer->output[1];
+	dell_set_arguments(&buffer, 0, 0x2, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	hwswitch = buffer.output[1];
 
 	if (ret != 0)
 		return;
@@ -550,22 +555,23 @@ static struct dentry *dell_laptop_dir;
 
 static int dell_debugfs_show(struct seq_file *s, void *data)
 {
+	struct calling_interface_buffer buffer;
 	int hwswitch_state;
 	int hwswitch_ret;
 	int status;
 	int ret;
 
-	dell_set_arguments(0, 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
+	dell_set_arguments(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 	if (ret)
 		return ret;
-	status = buffer->output[1];
+	status = buffer.output[1];
 
-	dell_set_arguments(0, 0x2, 0, 0);
-	hwswitch_ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
+	dell_set_arguments(&buffer, 0, 0x2, 0, 0);
+	hwswitch_ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 	if (hwswitch_ret)
 		return hwswitch_ret;
-	hwswitch_state = buffer->output[1];
+	hwswitch_state = buffer.output[1];
 
 	seq_printf(s, "return:\t%d\n", ret);
 	seq_printf(s, "status:\t0x%X\n", status);
@@ -646,22 +652,23 @@ static const struct file_operations dell_debugfs_fops = {
 
 static void dell_update_rfkill(struct work_struct *ignored)
 {
+	struct calling_interface_buffer buffer;
 	int hwswitch = 0;
 	int status;
 	int ret;
 
-	dell_set_arguments(0, 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
-	status = buffer->output[1];
+	dell_set_arguments(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	status = buffer.output[1];
 
 	if (ret != 0)
 		return;
 
-	dell_set_arguments(0, 0x2, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
+	dell_set_arguments(&buffer, 0, 0x2, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 
 	if (ret == 0 && (status & BIT(0)))
-		hwswitch = buffer->output[1];
+		hwswitch = buffer.output[1];
 
 	if (wifi_rfkill) {
 		dell_rfkill_update_hw_state(wifi_rfkill, 1, status, hwswitch);
@@ -719,6 +726,7 @@ static struct notifier_block dell_laptop_rbtn_notifier = {
 
 static int __init dell_setup_rfkill(void)
 {
+	struct calling_interface_buffer buffer;
 	int status, ret, whitelisted;
 	const char *product;
 
@@ -734,9 +742,9 @@ static int __init dell_setup_rfkill(void)
 	if (!force_rfkill && !whitelisted)
 		return 0;
 
-	dell_set_arguments(0, 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
-	status = buffer->output[1];
+	dell_set_arguments(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	status = buffer.output[1];
 
 	/* dell wireless info smbios call is not supported */
 	if (ret != 0)
@@ -889,6 +897,7 @@ static void dell_cleanup_rfkill(void)
 
 static int dell_send_intensity(struct backlight_device *bd)
 {
+	struct calling_interface_buffer buffer;
 	struct calling_interface_token *token;
 	int ret;
 
@@ -896,17 +905,21 @@ static int dell_send_intensity(struct backlight_device *bd)
 	if (!token)
 		return -ENODEV;
 
-	dell_set_arguments(token->location, bd->props.brightness, 0, 0);
+	dell_set_arguments(&buffer,
+			   token->location, bd->props.brightness, 0, 0);
 	if (power_supply_is_system_supplied() > 0)
-		ret = dell_send_request(CLASS_TOKEN_WRITE, SELECT_TOKEN_AC);
+		ret = dell_send_request(&buffer,
+					CLASS_TOKEN_WRITE, SELECT_TOKEN_AC);
 	else
-		ret = dell_send_request(CLASS_TOKEN_WRITE, SELECT_TOKEN_BAT);
+		ret = dell_send_request(&buffer,
+					CLASS_TOKEN_WRITE, SELECT_TOKEN_BAT);
 
 	return ret;
 }
 
 static int dell_get_intensity(struct backlight_device *bd)
 {
+	struct calling_interface_buffer buffer;
 	struct calling_interface_token *token;
 	int ret;
 
@@ -914,14 +927,17 @@ static int dell_get_intensity(struct backlight_device *bd)
 	if (!token)
 		return -ENODEV;
 
-	dell_set_arguments(token->location, 0, 0, 0);
+	dell_set_arguments(&buffer, token->location, 0, 0, 0);
 	if (power_supply_is_system_supplied() > 0)
-		ret = dell_send_request(CLASS_TOKEN_READ, SELECT_TOKEN_AC);
+		ret = dell_send_request(&buffer,
+					CLASS_TOKEN_READ, SELECT_TOKEN_AC);
 	else
-		ret = dell_send_request(CLASS_TOKEN_READ, SELECT_TOKEN_BAT);
+		ret = dell_send_request(&buffer,
+					CLASS_TOKEN_READ, SELECT_TOKEN_BAT);
 
 	if (ret == 0)
-		ret = buffer->output[1];
+		ret = buffer.output[1];
+
 	return ret;
 }
 
@@ -1186,31 +1202,33 @@ static enum led_brightness kbd_led_level;
 
 static int kbd_get_info(struct kbd_info *info)
 {
+	struct calling_interface_buffer buffer;
 	u8 units;
 	int ret;
 
-	dell_set_arguments(0, 0, 0, 0);
-	ret = dell_send_request(CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
+	dell_set_arguments(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer,
+				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
 	if (ret)
 		return ret;
 
-	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 = 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;
 
 	if (quirks && quirks->kbd_led_levels_off_1 && info->levels)
 		info->levels--;
 
 	if (units & BIT(0))
-		info->seconds = (buffer->output[3] >> 0) & 0xFF;
+		info->seconds = (buffer.output[3] >> 0) & 0xFF;
 	if (units & BIT(1))
-		info->minutes = (buffer->output[3] >> 8) & 0xFF;
+		info->minutes = (buffer.output[3] >> 8) & 0xFF;
 	if (units & BIT(2))
-		info->hours = (buffer->output[3] >> 16) & 0xFF;
+		info->hours = (buffer.output[3] >> 16) & 0xFF;
 	if (units & BIT(3))
-		info->days = (buffer->output[3] >> 24) & 0xFF;
+		info->days = (buffer.output[3] >> 24) & 0xFF;
 
 	return ret;
 }
@@ -1270,31 +1288,34 @@ static int kbd_set_level(struct kbd_state *state, u8 level)
 
 static int kbd_get_state(struct kbd_state *state)
 {
+	struct calling_interface_buffer buffer;
 	int ret;
 
-	dell_set_arguments(0x1, 0, 0, 0);
-	ret = dell_send_request(CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
+	dell_set_arguments(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer,
+				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
 	if (ret)
 		return ret;
 
-	state->mode_bit = ffs(buffer->output[1] & 0xFFFF);
+	state->mode_bit = ffs(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->timeout_value_ac = (buffer->output[2] >> 24) & 0x3F;
-	state->timeout_unit_ac = (buffer->output[2] >> 30) & 0x3;
+	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->timeout_value_ac = (buffer.output[2] >> 24) & 0x3F;
+	state->timeout_unit_ac = (buffer.output[2] >> 30) & 0x3;
 
 	return ret;
 }
 
 static int kbd_set_state(struct kbd_state *state)
 {
+	struct calling_interface_buffer buffer;
 	int ret;
 	u32 input1;
 	u32 input2;
@@ -1307,8 +1328,9 @@ static int kbd_set_state(struct kbd_state *state)
 	input2 |= (state->level & 0xFF) << 16;
 	input2 |= (state->timeout_value_ac & 0x3F) << 24;
 	input2 |= (state->timeout_unit_ac & 0x3) << 30;
-	dell_set_arguments(0x2, input1, input2, 0);
-	ret = dell_send_request(CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
+	dell_set_arguments(&buffer, 0x2, input1, input2, 0);
+	ret = dell_send_request(&buffer,
+				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
 
 	return ret;
 }
@@ -1335,6 +1357,7 @@ static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
 
 static int kbd_set_token_bit(u8 bit)
 {
+	struct calling_interface_buffer buffer;
 	struct calling_interface_token *token;
 	int ret;
 
@@ -1345,14 +1368,15 @@ static int kbd_set_token_bit(u8 bit)
 	if (!token)
 		return -EINVAL;
 
-	dell_set_arguments(token->location, token->value, 0, 0);
-	ret = dell_send_request(CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
+	dell_set_arguments(&buffer, token->location, token->value, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
 
 	return ret;
 }
 
 static int kbd_get_token_bit(u8 bit)
 {
+	struct calling_interface_buffer buffer;
 	struct calling_interface_token *token;
 	int ret;
 	int val;
@@ -1364,9 +1388,9 @@ static int kbd_get_token_bit(u8 bit)
 	if (!token)
 		return -EINVAL;
 
-	dell_set_arguments(token->location, 0, 0, 0);
-	ret = dell_send_request(CLASS_TOKEN_READ, SELECT_TOKEN_STD);
-	val = buffer->output[1];
+	dell_set_arguments(&buffer, token->location, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD);
+	val = buffer.output[1];
 
 	if (ret)
 		return ret;
@@ -2102,6 +2126,7 @@ static struct notifier_block dell_laptop_notifier = {
 
 int dell_micmute_led_set(int state)
 {
+	struct calling_interface_buffer buffer;
 	struct calling_interface_token *token;
 
 	if (state == 0)
@@ -2114,8 +2139,8 @@ int dell_micmute_led_set(int state)
 	if (!token)
 		return -ENODEV;
 
-	dell_set_arguments(token->location, token->value, 0, 0);
-	dell_send_request(CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
+	dell_set_arguments(&buffer, token->location, token->value, 0, 0);
+	dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
 
 	return state;
 }
@@ -2146,13 +2171,6 @@ static int __init dell_init(void)
 	if (ret)
 		goto fail_platform_device2;
 
-	buffer = kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
-	if (!buffer) {
-		ret = -ENOMEM;
-		goto fail_buffer;
-	}
-
-
 	ret = dell_setup_rfkill();
 
 	if (ret) {
@@ -2177,10 +2195,13 @@ static int __init dell_init(void)
 
 	token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
 	if (token) {
-		dell_set_arguments(token->location, 0, 0, 0);
-		ret = dell_send_request(CLASS_TOKEN_READ, SELECT_TOKEN_AC);
+		struct calling_interface_buffer buffer;
+
+		dell_set_arguments(&buffer, token->location, 0, 0, 0);
+		ret = dell_send_request(&buffer,
+					CLASS_TOKEN_READ, SELECT_TOKEN_AC);
 		if (ret)
-			max_intensity = buffer->output[3];
+			max_intensity = buffer.output[3];
 	}
 
 	if (max_intensity) {
@@ -2214,8 +2235,6 @@ static int __init dell_init(void)
 fail_get_brightness:
 	backlight_device_unregister(dell_backlight_device);
 fail_backlight:
-	kfree(buffer);
-fail_buffer:
 	dell_cleanup_rfkill();
 fail_rfkill:
 	platform_device_del(platform_device);
@@ -2235,7 +2254,6 @@ static void __exit dell_exit(void)
 		touchpad_led_exit();
 	kbd_led_exit();
 	backlight_device_unregister(dell_backlight_device);
-	kfree(buffer);
 	dell_cleanup_rfkill();
 	if (platform_device) {
 		platform_device_unregister(platform_device);
-- 
2.7.4

Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 2695 bytes --]

On Tuesday 30 January 2018 10:59:00 Mario Limonciello wrote:
> There may be race conditions with multiple different functions working
> on a module wide buffer causing one function to get the wrong results.

Yes, this is better. We really do not need to allocate shared buffer in
dell-laptop anymore. Before this buffer was specially allocated in first
4GB addresses space because its physical addresses was passed into SMM.
But now this buffer is not used by SMM anymore (another one is allocated
in dell-smbios* code), so in dell-laptop we can really get rid of shared
buffers, which also simplify code. Working with mutexes and concurrency
always leads to problems and it is better to avoid it.

> Suggested-by: Pali Rohar <pali.rohar@gmail.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
>  drivers/platform/x86/dell-laptop.c | 188 ++++++++++++++++++++-----------------
>  1 file changed, 103 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index fc2dfc8..ab58373 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -78,7 +78,6 @@ static struct platform_driver platform_driver = {
>  	}
>  };
>  
> -static struct calling_interface_buffer *buffer;
>  static struct platform_device *platform_device;
>  static struct backlight_device *dell_backlight_device;
>  static struct rfkill *wifi_rfkill;
> @@ -322,7 +321,8 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
>  	{ }
>  };
>  
> -static void dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3)
> +static void dell_set_arguments(struct calling_interface_buffer *buffer,
> +			       u32 arg0, u32 arg1, u32 arg2, u32 arg3)

Hm... this function has too many parameters :-(

What about following API?

static struct calling_interface_buffer dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3);

It would return filled structure (not a pointer !) and caller would be
able to use it as:

  struct calling_interface_buffer buffer;
  buffer = dell_set_arguments(0x2, 0, 0, 0);
  ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);

And maybe after this change, function dell_set_arguments() could have
better name, e.g. dell_prepare_request() (or dell_prepare_request_buffer)
as "set arguments" is not really what would function do (as it return
something).

  struct calling_interface_buffer buffer;
  buffer = dell_prepare_request_buffer(0x2, 0, 0, 0);
  ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);

Andy, any suggestion or opinion?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

On Tuesday 30 January 2018 10:59:00 Mario Limonciello wrote:
> There may be race conditions with multiple different functions working
> on a module wide buffer causing one function to get the wrong results.
> 
> Suggested-by: Pali Rohar <pali.rohar@gmail.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>

And Fixes: 549b4930f057658dc50d8010e66219233119a4d8

I think fix for such race condition should be forwarded to stable
kernel(s). Apparently only 4.15 is affected.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Andy Shevchenko]

On Tue, Jan 30, 2018 at 7:17 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Tuesday 30 January 2018 10:59:00 Mario Limonciello wrote:
>> There may be race conditions with multiple different functions working
>> on a module wide buffer causing one function to get the wrong results.
>
> Yes, this is better. We really do not need to allocate shared buffer in
> dell-laptop anymore. Before this buffer was specially allocated in first
> 4GB addresses space because its physical addresses was passed into SMM.

+1.

>> -static void dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3)
>> +static void dell_set_arguments(struct calling_interface_buffer *buffer,
>> +                            u32 arg0, u32 arg1, u32 arg2, u32 arg3)
>
> Hm... this function has too many parameters :-(
>
> What about following API?
>
> static struct calling_interface_buffer dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3);
>
> It would return filled structure (not a pointer !)

I do not think it's a good idea. Either we allocate request on a heap
and return a pointer, or we fill the struct with some data on spot.

To naming:

for allocation: ..._alloc_request()
for filling: _fill_request() / _prepare_request()

or alike.

_set_arguments() not good enough to me.

> and caller would be
> able to use it as:
>
>   struct calling_interface_buffer buffer;
>   buffer = dell_set_arguments(0x2, 0, 0, 0);
>   ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);

See above.

> And maybe after this change, function dell_set_arguments() could have
> better name, e.g. dell_prepare_request() (or dell_prepare_request_buffer)
> as "set arguments" is not really what would function do (as it return
> something).

Agree.

>
>   struct calling_interface_buffer buffer;
>   buffer = dell_prepare_request_buffer(0x2, 0, 0, 0);
>   ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
>
> Andy, any suggestion or opinion?

See above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Pali Rohár]

On Tuesday 30 January 2018 20:14:26 Andy Shevchenko wrote:
> >> -static void dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3)
> >> +static void dell_set_arguments(struct calling_interface_buffer *buffer,
> >> +                            u32 arg0, u32 arg1, u32 arg2, u32 arg3)
> >
> > Hm... this function has too many parameters :-(
> >
> > What about following API?
> >
> > static struct calling_interface_buffer dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3);
> >
> > It would return filled structure (not a pointer !)
> 
> I do not think it's a good idea. Either we allocate request on a heap
> and return a pointer, or we fill the struct with some data on spot.
> 
> To naming:
> 
> for allocation: ..._alloc_request()
> for filling: _fill_request() / _prepare_request()
> 
> or alike.
> 
> _set_arguments() not good enough to me.

Ok. Then we need to stick with 5 arguments... What about name
dell_fill_request()? E.g.

  struct calling_interface_buffer buffer;
  dell_fill_request(&buffer, 0x2, 0, 0, 0);
  ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);

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

RE: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Wednesday, January 31, 2018 3:08 AM
> To: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; Darren Hart
> <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Platform Driver
> <platform-driver-x86@vger.kernel.org>
> Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than
> globally
> 
> On Tuesday 30 January 2018 20:14:26 Andy Shevchenko wrote:
> > >> -static void dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3)
> > >> +static void dell_set_arguments(struct calling_interface_buffer *buffer,
> > >> +                            u32 arg0, u32 arg1, u32 arg2, u32 arg3)
> > >
> > > Hm... this function has too many parameters :-(
> > >
> > > What about following API?
> > >
> > > static struct calling_interface_buffer dell_set_arguments(u32 arg0, u32 arg1,
> u32 arg2, u32 arg3);
> > >
> > > It would return filled structure (not a pointer !)
> >
> > I do not think it's a good idea. Either we allocate request on a heap
> > and return a pointer, or we fill the struct with some data on spot.
> >
> > To naming:
> >
> > for allocation: ..._alloc_request()
> > for filling: _fill_request() / _prepare_request()
> >
> > or alike.
> >
> > _set_arguments() not good enough to me.
> 
> Ok. Then we need to stick with 5 arguments... What about name
> dell_fill_request()? E.g.
> 
>   struct calling_interface_buffer buffer;
>   dell_fill_request(&buffer, 0x2, 0, 0, 0);
>   ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> 

The other alternative is to just define the input of the structure immediately with
an initializer, no multi argument filler function.  Like this:

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index ab58373..e7a971b 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -469,13 +469,15 @@ static int dell_rfkill_set(void *data, bool blocked)
        int disable = blocked ? 1 : 0;
        unsigned long radio = (unsigned long)data;
        int hwswitch_bit = (unsigned long)data - 1;
-       struct calling_interface_buffer buffer;
+       struct calling_interface_buffer buffer = {CLASS_INFO,
+                                                 SELECT_RFKILL,
+                                                 {0, 0, 0, 0},
+                                                 {0, 0, 0, 0}};
        int hwswitch;
        int status;
        int ret;
 
-       dell_set_arguments(&buffer, 0, 0, 0, 0);
-       ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+       ret = dell_send_request(&buffer);
        if (ret)
                return ret;
        status = buffer.output[1];

RE: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Wednesday, January 31, 2018 3:08 AM
> To: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; Darren Hart
> <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Platform Driver
> <platform-driver-x86@vger.kernel.org>
> Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than
> globally
> 
> On Tuesday 30 January 2018 20:14:26 Andy Shevchenko wrote:
> > >> -static void dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3)
> > >> +static void dell_set_arguments(struct calling_interface_buffer *buffer,
> > >> +                            u32 arg0, u32 arg1, u32 arg2, u32 arg3)
> > >
> > > Hm... this function has too many parameters :-(
> > >
> > > What about following API?
> > >
> > > static struct calling_interface_buffer dell_set_arguments(u32 arg0, u32 arg1,
> u32 arg2, u32 arg3);
> > >
> > > It would return filled structure (not a pointer !)
> >
> > I do not think it's a good idea. Either we allocate request on a heap
> > and return a pointer, or we fill the struct with some data on spot.
> >
> > To naming:
> >
> > for allocation: ..._alloc_request()
> > for filling: _fill_request() / _prepare_request()
> >
> > or alike.
> >
> > _set_arguments() not good enough to me.
> 
> Ok. Then we need to stick with 5 arguments... What about name
> dell_fill_request()? E.g.
> 
>   struct calling_interface_buffer buffer;
>   dell_fill_request(&buffer, 0x2, 0, 0, 0);
>   ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> 

The other alternative is to just define the input of the structure immediately with
an initializer, no multi argument filler function.  Like this:

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index ab58373..e7a971b 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -469,13 +469,15 @@ static int dell_rfkill_set(void *data, bool blocked)
        int disable = blocked ? 1 : 0;
        unsigned long radio = (unsigned long)data;
        int hwswitch_bit = (unsigned long)data - 1;
-       struct calling_interface_buffer buffer;
+       struct calling_interface_buffer buffer = {CLASS_INFO,
+                                                 SELECT_RFKILL,
+                                                 {0, 0, 0, 0},
+                                                 {0, 0, 0, 0}};
        int hwswitch;
        int status;
        int ret;
 
-       dell_set_arguments(&buffer, 0, 0, 0, 0);
-       ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+       ret = dell_send_request(&buffer);
        if (ret)
                return ret;
        status = buffer.output[1];

Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Andy Shevchenko]

On Wed, Jan 31, 2018 at 6:46 PM,  <Mario.Limonciello@dell.com> wrote:

>> > for allocation: ..._alloc_request()
>> > for filling: _fill_request() / _prepare_request()
>> >
>> > or alike.
>> >
>> > _set_arguments() not good enough to me.
>>
>> Ok. Then we need to stick with 5 arguments... What about name
>> dell_fill_request()? E.g.
>>
>>   struct calling_interface_buffer buffer;
>>   dell_fill_request(&buffer, 0x2, 0, 0, 0);
>>   ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
>>
>
> The other alternative is to just define the input of the structure immediately with
> an initializer, no multi argument filler function.  Like this:

Either would work for me, though one comment below.

> -       struct calling_interface_buffer buffer;


> +       struct calling_interface_buffer buffer = {CLASS_INFO,
> +                                                 SELECT_RFKILL,
> +                                                 {0, 0, 0, 0},
> +                                                 {0, 0, 0, 0}};

Looking to this approach I would rather provide a macro then.

#define FILL_REQUEST(a,b,c,d,...) \ // variant FILL_RFKILL_REQUEST(a,b,c,d)
(struct calling_interface_buffer) { \
 ... \
}

But then it is macro(s) vs. function(s) debate.

> -       dell_set_arguments(&buffer, 0, 0, 0, 0);
> -       ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> +       ret = dell_send_request(&buffer);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 1720 bytes --]

On Wednesday 31 January 2018 18:53:19 Andy Shevchenko wrote:
> On Wed, Jan 31, 2018 at 6:46 PM,  <Mario.Limonciello@dell.com> wrote:
> 
> >> > for allocation: ..._alloc_request()
> >> > for filling: _fill_request() / _prepare_request()
> >> >
> >> > or alike.
> >> >
> >> > _set_arguments() not good enough to me.
> >>
> >> Ok. Then we need to stick with 5 arguments... What about name
> >> dell_fill_request()? E.g.
> >>
> >>   struct calling_interface_buffer buffer;
> >>   dell_fill_request(&buffer, 0x2, 0, 0, 0);
> >>   ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> >>
> >
> > The other alternative is to just define the input of the structure immediately with
> > an initializer, no multi argument filler function.  Like this:
> 
> Either would work for me, though one comment below.
> 
> > -       struct calling_interface_buffer buffer;
> 
> 
> > +       struct calling_interface_buffer buffer = {CLASS_INFO,
> > +                                                 SELECT_RFKILL,
> > +                                                 {0, 0, 0, 0},
> > +                                                 {0, 0, 0, 0}};
> 
> Looking to this approach I would rather provide a macro then.
> 
> #define FILL_REQUEST(a,b,c,d,...) \ // variant FILL_RFKILL_REQUEST(a,b,c,d)
> (struct calling_interface_buffer) { \
>  ... \
> }
> 
> But then it is macro(s) vs. function(s) debate.

Does not matter, I'm fine with either macro or function.

> 
> > -       dell_set_arguments(&buffer, 0, 0, 0, 0);
> > -       ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> > +       ret = dell_send_request(&buffer);
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Andy Shevchenko]

On Wed, Jan 31, 2018 at 7:06 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Wednesday 31 January 2018 18:53:19 Andy Shevchenko wrote:
>> On Wed, Jan 31, 2018 at 6:46 PM,  <Mario.Limonciello@dell.com> wrote:

>> >> Ok. Then we need to stick with 5 arguments... What about name
>> >> dell_fill_request()? E.g.

>> > +       struct calling_interface_buffer buffer = {CLASS_INFO,
>> > +                                                 SELECT_RFKILL,
>> > +                                                 {0, 0, 0, 0},
>> > +                                                 {0, 0, 0, 0}};
>>
>> Looking to this approach I would rather provide a macro then.
>>
>> #define FILL_REQUEST(a,b,c,d,...) \ // variant FILL_RFKILL_REQUEST(a,b,c,d)
>> (struct calling_interface_buffer) { \
>>  ... \
>> }
>>
>> But then it is macro(s) vs. function(s) debate.
>
> Does not matter, I'm fine with either macro or function.

Mario, it's now up to you. Choose one of the option and send new version.
Thanks!

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Mario.Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, February 27, 2018 12:31 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: stable@vger.kernel.org; andriy.shevchenko@linux.intel.com
> Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than
> globally
> 
> On Tue, Feb 27, 2018 at 03:01:24PM +0000, Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Limonciello, Mario
> > > Sent: Thursday, February 15, 2018 8:50 AM
> > > To: 'Greg KH' <gregkh@linuxfoundation.org>
> > > Cc: stable@vger.kernel.org; andriy.shevchenko@linux.intel.com
> > > Subject: RE: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather
> than
> > > globally
> > >
> > > > -----Original Message-----
> > > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > > Sent: Thursday, February 15, 2018 8:28 AM
> > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > > Cc: stable@vger.kernel.org; andriy.shevchenko@linux.intel.com
> > > > Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather
> > > than
> > > > globally
> > > >
> > > > On Tue, Feb 13, 2018 at 07:54:10PM +0000, Mario.Limonciello@dell.com
> wrote:
> > > > > > -----Original Message-----
> > > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > > > > Sent: Tuesday, February 13, 2018 1:52 PM
> > > > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > > > > Cc: linux-stable <stable@vger.kernel.org>; Andy Shevchenko
> > > > > > <andriy.shevchenko@linux.intel.com>
> > > > > > Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap
> rather
> > > > than
> > > > > > globally
> > > > > >
> > > > > > On Tue, Feb 13, 2018 at 01:39:19PM -0600, Mario Limonciello wrote:
> > > > > > > There is no longer a need for the buffer to be defined in
> > > > > > > first 4GB physical address space.
> > > > > > >
> > > > > > > Furthermore there may be race conditions with multiple different
> functions
> > > > > > > working on a module wide buffer causing incorrect results.
> > > > > > >
> > > > > > > This commit has been backported from:
> > > > > > > commit 9862b43624 upstream
> > > > > > >
> > > > > > > Fixes: 549b4930f057658dc50d8010e66219233119a4d8
> > > > > > > Suggested-by: Pali Rohar <pali.rohar@gmail.com>
> > > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > > > ---
> > > > > > >  drivers/platform/x86/dell-laptop.c | 188 ++++++++++++++++++++---------
> -----
> > > --
> > > > -
> > > > > > >  1 file changed, 103 insertions(+), 85 deletions(-)
> > > > > >
> > > > > > Why is this a stable patch?  What bug does it fix?
> > > > > >
> > > > > > confused,
> > > > > >
> > > > > > greg k-h
> > > > >
> > > > > Greg,
> > > > >
> > > > > I realized I forgot to send this 2 weeks ago when you said a clean backport
> > > > > didn't work.  This was a follow up from a race condition that was seen in
> > > > > the wild and submitted to platform-x86.
> > > >
> > > > Ok, what kernel tree(s) do you want it applied to?  I need a hint please :)
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > 4.15.y please.
> > >
> > > Thank you,
> >
> > Hi Greg,
> >
> > I just wanted to follow up on this one because I noticed it's not in 4.15.y still
> > And there are some bug traffics about problems.
> 
> Ah, ooops, this fell to the bottom of the heap, sorry.  Will pick it up
> in the next release.

No worry, glad I checked.

> 
> > There's also another patch that was CC'ed to stable that came after it that
> > should be going to 4.15.y.
> >
> > I can resubmit it with those bug URLs and squash that patch if it's helpful to
> > you.
> 
> No, no squashing patches, I want the "identical" patch that is in
> Linus's tree where ever possible.
> 

OK thanks for confirming.
If you want to refer to the open Bugzilla that came up around this in commit
message when you add here is the URL:
https://bugzilla.redhat.com/show_bug.cgi?id=1546464#c11

This is the other patch that got CC'ed to stable that goes on top of mine in 4.15.y.
https://www.spinics.net/lists/stable/msg215268.html

Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Greg KH]

On Tue, Feb 27, 2018 at 03:01:24PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Limonciello, Mario
> > Sent: Thursday, February 15, 2018 8:50 AM
> > To: 'Greg KH' <gregkh@linuxfoundation.org>
> > Cc: stable@vger.kernel.org; andriy.shevchenko@linux.intel.com
> > Subject: RE: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than
> > globally
> > 
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > Sent: Thursday, February 15, 2018 8:28 AM
> > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > Cc: stable@vger.kernel.org; andriy.shevchenko@linux.intel.com
> > > Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather
> > than
> > > globally
> > >
> > > On Tue, Feb 13, 2018 at 07:54:10PM +0000, Mario.Limonciello@dell.com wrote:
> > > > > -----Original Message-----
> > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > > > Sent: Tuesday, February 13, 2018 1:52 PM
> > > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > > > Cc: linux-stable <stable@vger.kernel.org>; Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com>
> > > > > Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather
> > > than
> > > > > globally
> > > > >
> > > > > On Tue, Feb 13, 2018 at 01:39:19PM -0600, Mario Limonciello wrote:
> > > > > > There is no longer a need for the buffer to be defined in
> > > > > > first 4GB physical address space.
> > > > > >
> > > > > > Furthermore there may be race conditions with multiple different functions
> > > > > > working on a module wide buffer causing incorrect results.
> > > > > >
> > > > > > This commit has been backported from:
> > > > > > commit 9862b43624 upstream
> > > > > >
> > > > > > Fixes: 549b4930f057658dc50d8010e66219233119a4d8
> > > > > > Suggested-by: Pali Rohar <pali.rohar@gmail.com>
> > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/platform/x86/dell-laptop.c | 188 ++++++++++++++++++++--------------
> > --
> > > -
> > > > > >  1 file changed, 103 insertions(+), 85 deletions(-)
> > > > >
> > > > > Why is this a stable patch?  What bug does it fix?
> > > > >
> > > > > confused,
> > > > >
> > > > > greg k-h
> > > >
> > > > Greg,
> > > >
> > > > I realized I forgot to send this 2 weeks ago when you said a clean backport
> > > > didn't work.  This was a follow up from a race condition that was seen in
> > > > the wild and submitted to platform-x86.
> > >
> > > Ok, what kernel tree(s) do you want it applied to?  I need a hint please :)
> > >
> > > thanks,
> > >
> > > greg k-h
> > 
> > 4.15.y please.
> > 
> > Thank you,
> 
> Hi Greg,
> 
> I just wanted to follow up on this one because I noticed it's not in 4.15.y still
> And there are some bug traffics about problems.

Ah, ooops, this fell to the bottom of the heap, sorry.  Will pick it up
in the next release.

> There's also another patch that was CC'ed to stable that came after it that
> should be going to 4.15.y.
> 
> I can resubmit it with those bug URLs and squash that patch if it's helpful to
> you.

No, no squashing patches, I want the "identical" patch that is in
Linus's tree where ever possible.

thanks,

greg k-h

RE: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Mario.Limonciello]

> -----Original Message-----
> From: Limonciello, Mario
> Sent: Thursday, February 15, 2018 8:50 AM
> To: 'Greg KH' <gregkh@linuxfoundation.org>
> Cc: stable@vger.kernel.org; andriy.shevchenko@linux.intel.com
> Subject: RE: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than
> globally
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Thursday, February 15, 2018 8:28 AM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: stable@vger.kernel.org; andriy.shevchenko@linux.intel.com
> > Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather
> than
> > globally
> >
> > On Tue, Feb 13, 2018 at 07:54:10PM +0000, Mario.Limonciello@dell.com wrote:
> > > > -----Original Message-----
> > > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > > Sent: Tuesday, February 13, 2018 1:52 PM
> > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > > Cc: linux-stable <stable@vger.kernel.org>; Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com>
> > > > Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather
> > than
> > > > globally
> > > >
> > > > On Tue, Feb 13, 2018 at 01:39:19PM -0600, Mario Limonciello wrote:
> > > > > There is no longer a need for the buffer to be defined in
> > > > > first 4GB physical address space.
> > > > >
> > > > > Furthermore there may be race conditions with multiple different functions
> > > > > working on a module wide buffer causing incorrect results.
> > > > >
> > > > > This commit has been backported from:
> > > > > commit 9862b43624 upstream
> > > > >
> > > > > Fixes: 549b4930f057658dc50d8010e66219233119a4d8
> > > > > Suggested-by: Pali Rohar <pali.rohar@gmail.com>
> > > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > ---
> > > > >  drivers/platform/x86/dell-laptop.c | 188 ++++++++++++++++++++--------------
> --
> > -
> > > > >  1 file changed, 103 insertions(+), 85 deletions(-)
> > > >
> > > > Why is this a stable patch?  What bug does it fix?
> > > >
> > > > confused,
> > > >
> > > > greg k-h
> > >
> > > Greg,
> > >
> > > I realized I forgot to send this 2 weeks ago when you said a clean backport
> > > didn't work.  This was a follow up from a race condition that was seen in
> > > the wild and submitted to platform-x86.
> >
> > Ok, what kernel tree(s) do you want it applied to?  I need a hint please :)
> >
> > thanks,
> >
> > greg k-h
> 
> 4.15.y please.
> 
> Thank you,

Hi Greg,

I just wanted to follow up on this one because I noticed it's not in 4.15.y still
And there are some bug traffics about problems.

There's also another patch that was CC'ed to stable that came after it that
should be going to 4.15.y.

I can resubmit it with those bug URLs and squash that patch if it's helpful to
you.

Thanks

RE: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Mario.Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, February 15, 2018 8:28 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: stable@vger.kernel.org; andriy.shevchenko@linux.intel.com
> Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than
> globally
> 
> On Tue, Feb 13, 2018 at 07:54:10PM +0000, Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > Sent: Tuesday, February 13, 2018 1:52 PM
> > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > Cc: linux-stable <stable@vger.kernel.org>; Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com>
> > > Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather
> than
> > > globally
> > >
> > > On Tue, Feb 13, 2018 at 01:39:19PM -0600, Mario Limonciello wrote:
> > > > There is no longer a need for the buffer to be defined in
> > > > first 4GB physical address space.
> > > >
> > > > Furthermore there may be race conditions with multiple different functions
> > > > working on a module wide buffer causing incorrect results.
> > > >
> > > > This commit has been backported from:
> > > > commit 9862b43624 upstream
> > > >
> > > > Fixes: 549b4930f057658dc50d8010e66219233119a4d8
> > > > Suggested-by: Pali Rohar <pali.rohar@gmail.com>
> > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > ---
> > > >  drivers/platform/x86/dell-laptop.c | 188 ++++++++++++++++++++----------------
> -
> > > >  1 file changed, 103 insertions(+), 85 deletions(-)
> > >
> > > Why is this a stable patch?  What bug does it fix?
> > >
> > > confused,
> > >
> > > greg k-h
> >
> > Greg,
> >
> > I realized I forgot to send this 2 weeks ago when you said a clean backport
> > didn't work.  This was a follow up from a race condition that was seen in
> > the wild and submitted to platform-x86.
> 
> Ok, what kernel tree(s) do you want it applied to?  I need a hint please :)
> 
> thanks,
> 
> greg k-h

4.15.y please.

Thank you,

Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Greg KH]

On Tue, Feb 13, 2018 at 07:54:10PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Tuesday, February 13, 2018 1:52 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: linux-stable <stable@vger.kernel.org>; Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com>
> > Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than
> > globally
> > 
> > On Tue, Feb 13, 2018 at 01:39:19PM -0600, Mario Limonciello wrote:
> > > There is no longer a need for the buffer to be defined in
> > > first 4GB physical address space.
> > >
> > > Furthermore there may be race conditions with multiple different functions
> > > working on a module wide buffer causing incorrect results.
> > >
> > > This commit has been backported from:
> > > commit 9862b43624 upstream
> > >
> > > Fixes: 549b4930f057658dc50d8010e66219233119a4d8
> > > Suggested-by: Pali Rohar <pali.rohar@gmail.com>
> > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  drivers/platform/x86/dell-laptop.c | 188 ++++++++++++++++++++-----------------
> > >  1 file changed, 103 insertions(+), 85 deletions(-)
> > 
> > Why is this a stable patch?  What bug does it fix?
> > 
> > confused,
> > 
> > greg k-h
> 
> Greg,
> 
> I realized I forgot to send this 2 weeks ago when you said a clean backport
> didn't work.  This was a follow up from a race condition that was seen in
> the wild and submitted to platform-x86.

Ok, what kernel tree(s) do you want it applied to?  I need a hint please :)

thanks,

greg k-h

RE: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Mario.Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, February 13, 2018 1:52 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: linux-stable <stable@vger.kernel.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>
> Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than
> globally
> 
> On Tue, Feb 13, 2018 at 01:39:19PM -0600, Mario Limonciello wrote:
> > There is no longer a need for the buffer to be defined in
> > first 4GB physical address space.
> >
> > Furthermore there may be race conditions with multiple different functions
> > working on a module wide buffer causing incorrect results.
> >
> > This commit has been backported from:
> > commit 9862b43624 upstream
> >
> > Fixes: 549b4930f057658dc50d8010e66219233119a4d8
> > Suggested-by: Pali Rohar <pali.rohar@gmail.com>
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/platform/x86/dell-laptop.c | 188 ++++++++++++++++++++-----------------
> >  1 file changed, 103 insertions(+), 85 deletions(-)
> 
> Why is this a stable patch?  What bug does it fix?
> 
> confused,
> 
> greg k-h

Greg,

I realized I forgot to send this 2 weeks ago when you said a clean backport
didn't work.  This was a follow up from a race condition that was seen in
the wild and submitted to platform-x86.

No kernel Bugzilla as far as I could tell was filed for it though.

Thanks,

Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Greg KH]

On Tue, Feb 13, 2018 at 01:39:19PM -0600, Mario Limonciello wrote:
> There is no longer a need for the buffer to be defined in
> first 4GB physical address space.
> 
> Furthermore there may be race conditions with multiple different functions
> working on a module wide buffer causing incorrect results.
> 
> This commit has been backported from:
> commit 9862b43624 upstream
> 
> Fixes: 549b4930f057658dc50d8010e66219233119a4d8
> Suggested-by: Pali Rohar <pali.rohar@gmail.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/platform/x86/dell-laptop.c | 188 ++++++++++++++++++++-----------------
>  1 file changed, 103 insertions(+), 85 deletions(-)

Why is this a stable patch?  What bug does it fix?

confused,

greg k-h

[PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Mario Limonciello]

There is no longer a need for the buffer to be defined in
first 4GB physical address space.

Furthermore there may be race conditions with multiple different functions
working on a module wide buffer causing incorrect results.

This commit has been backported from:
commit 9862b43624 upstream

Fixes: 549b4930f057658dc50d8010e66219233119a4d8
Suggested-by: Pali Rohar <pali.rohar@gmail.com>
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/dell-laptop.c | 188 ++++++++++++++++++++-----------------
 1 file changed, 103 insertions(+), 85 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index cd4725e..c1b9e11 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -78,7 +78,6 @@ static struct platform_driver platform_driver = {
 	}
 };
 
-static struct calling_interface_buffer *buffer;
 static struct platform_device *platform_device;
 static struct backlight_device *dell_backlight_device;
 static struct rfkill *wifi_rfkill;
@@ -286,7 +285,8 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 	{ }
 };
 
-void dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3)
+static void dell_fill_request(struct calling_interface_buffer *buffer,
+			      u32 arg0, u32 arg1, u32 arg2, u32 arg3)
 {
 	memset(buffer, 0, sizeof(struct calling_interface_buffer));
 	buffer->input[0] = arg0;
@@ -295,7 +295,8 @@ void dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3)
 	buffer->input[3] = arg3;
 }
 
-int dell_send_request(u16 class, u16 select)
+static int dell_send_request(struct calling_interface_buffer *buffer,
+			     u16 class, u16 select)
 {
 	int ret;
 
@@ -432,21 +433,22 @@ static int dell_rfkill_set(void *data, bool blocked)
 	int disable = blocked ? 1 : 0;
 	unsigned long radio = (unsigned long)data;
 	int hwswitch_bit = (unsigned long)data - 1;
+	struct calling_interface_buffer buffer;
 	int hwswitch;
 	int status;
 	int ret;
 
-	dell_set_arguments(0, 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
+	dell_fill_request(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 	if (ret)
 		return ret;
-	status = buffer->output[1];
+	status = buffer.output[1];
 
-	dell_set_arguments(0x2, 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
+	dell_fill_request(&buffer, 0x2, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 	if (ret)
 		return ret;
-	hwswitch = buffer->output[1];
+	hwswitch = buffer.output[1];
 
 	/* If the hardware switch controls this radio, and the hardware
 	   switch is disabled, always disable the radio */
@@ -454,8 +456,8 @@ static int dell_rfkill_set(void *data, bool blocked)
 	    (status & BIT(0)) && !(status & BIT(16)))
 		disable = 1;
 
-	dell_set_arguments(1 | (radio<<8) | (disable << 16), 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
+	dell_fill_request(&buffer, 1 | (radio<<8) | (disable << 16), 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 	return ret;
 }
 
@@ -464,9 +466,11 @@ static void dell_rfkill_update_sw_state(struct rfkill *rfkill, int radio,
 {
 	if (status & BIT(0)) {
 		/* Has hw-switch, sync sw_state to BIOS */
+		struct calling_interface_buffer buffer;
 		int block = rfkill_blocked(rfkill);
-		dell_set_arguments(1 | (radio << 8) | (block << 16), 0, 0, 0);
-		dell_send_request(CLASS_INFO, SELECT_RFKILL);
+		dell_fill_request(&buffer,
+				   1 | (radio << 8) | (block << 16), 0, 0, 0);
+		dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 	} else {
 		/* No hw-switch, sync BIOS state to sw_state */
 		rfkill_set_sw_state(rfkill, !!(status & BIT(radio + 16)));
@@ -483,21 +487,22 @@ static void dell_rfkill_update_hw_state(struct rfkill *rfkill, int radio,
 static void dell_rfkill_query(struct rfkill *rfkill, void *data)
 {
 	int radio = ((unsigned long)data & 0xF);
+	struct calling_interface_buffer buffer;
 	int hwswitch;
 	int status;
 	int ret;
 
-	dell_set_arguments(0, 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
-	status = buffer->output[1];
+	dell_fill_request(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	status = buffer.output[1];
 
 	if (ret != 0 || !(status & BIT(0))) {
 		return;
 	}
 
-	dell_set_arguments(0, 0x2, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
-	hwswitch = buffer->output[1];
+	dell_fill_request(&buffer, 0, 0x2, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	hwswitch = buffer.output[1];
 
 	if (ret != 0)
 		return;
@@ -514,22 +519,23 @@ static struct dentry *dell_laptop_dir;
 
 static int dell_debugfs_show(struct seq_file *s, void *data)
 {
+	struct calling_interface_buffer buffer;
 	int hwswitch_state;
 	int hwswitch_ret;
 	int status;
 	int ret;
 
-	dell_set_arguments(0, 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
+	dell_fill_request(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 	if (ret)
 		return ret;
-	status = buffer->output[1];
+	status = buffer.output[1];
 
-	dell_set_arguments(0, 0x2, 0, 0);
-	hwswitch_ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
+	dell_fill_request(&buffer, 0, 0x2, 0, 0);
+	hwswitch_ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 	if (hwswitch_ret)
 		return hwswitch_ret;
-	hwswitch_state = buffer->output[1];
+	hwswitch_state = buffer.output[1];
 
 	seq_printf(s, "return:\t%d\n", ret);
 	seq_printf(s, "status:\t0x%X\n", status);
@@ -610,22 +616,23 @@ static const struct file_operations dell_debugfs_fops = {
 
 static void dell_update_rfkill(struct work_struct *ignored)
 {
+	struct calling_interface_buffer buffer;
 	int hwswitch = 0;
 	int status;
 	int ret;
 
-	dell_set_arguments(0, 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
-	status = buffer->output[1];
+	dell_fill_request(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	status = buffer.output[1];
 
 	if (ret != 0)
 		return;
 
-	dell_set_arguments(0, 0x2, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
+	dell_fill_request(&buffer, 0, 0x2, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
 
 	if (ret == 0 && (status & BIT(0)))
-		hwswitch = buffer->output[1];
+		hwswitch = buffer.output[1];
 
 	if (wifi_rfkill) {
 		dell_rfkill_update_hw_state(wifi_rfkill, 1, status, hwswitch);
@@ -683,6 +690,7 @@ static struct notifier_block dell_laptop_rbtn_notifier = {
 
 static int __init dell_setup_rfkill(void)
 {
+	struct calling_interface_buffer buffer;
 	int status, ret, whitelisted;
 	const char *product;
 
@@ -698,9 +706,9 @@ static int __init dell_setup_rfkill(void)
 	if (!force_rfkill && !whitelisted)
 		return 0;
 
-	dell_set_arguments(0, 0, 0, 0);
-	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
-	status = buffer->output[1];
+	dell_fill_request(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	status = buffer.output[1];
 
 	/* dell wireless info smbios call is not supported */
 	if (ret != 0)
@@ -853,6 +861,7 @@ static void dell_cleanup_rfkill(void)
 
 static int dell_send_intensity(struct backlight_device *bd)
 {
+	struct calling_interface_buffer buffer;
 	struct calling_interface_token *token;
 	int ret;
 
@@ -860,17 +869,21 @@ static int dell_send_intensity(struct backlight_device *bd)
 	if (!token)
 		return -ENODEV;
 
-	dell_set_arguments(token->location, bd->props.brightness, 0, 0);
+	dell_fill_request(&buffer,
+			   token->location, bd->props.brightness, 0, 0);
 	if (power_supply_is_system_supplied() > 0)
-		ret = dell_send_request(CLASS_TOKEN_WRITE, SELECT_TOKEN_AC);
+		ret = dell_send_request(&buffer,
+					CLASS_TOKEN_WRITE, SELECT_TOKEN_AC);
 	else
-		ret = dell_send_request(CLASS_TOKEN_WRITE, SELECT_TOKEN_BAT);
+		ret = dell_send_request(&buffer,
+					CLASS_TOKEN_WRITE, SELECT_TOKEN_BAT);
 
 	return ret;
 }
 
 static int dell_get_intensity(struct backlight_device *bd)
 {
+	struct calling_interface_buffer buffer;
 	struct calling_interface_token *token;
 	int ret;
 
@@ -878,14 +891,17 @@ static int dell_get_intensity(struct backlight_device *bd)
 	if (!token)
 		return -ENODEV;
 
-	dell_set_arguments(token->location, 0, 0, 0);
+	dell_fill_request(&buffer, token->location, 0, 0, 0);
 	if (power_supply_is_system_supplied() > 0)
-		ret = dell_send_request(CLASS_TOKEN_READ, SELECT_TOKEN_AC);
+		ret = dell_send_request(&buffer,
+					CLASS_TOKEN_READ, SELECT_TOKEN_AC);
 	else
-		ret = dell_send_request(CLASS_TOKEN_READ, SELECT_TOKEN_BAT);
+		ret = dell_send_request(&buffer,
+					CLASS_TOKEN_READ, SELECT_TOKEN_BAT);
 
 	if (ret == 0)
-		ret = buffer->output[1];
+		ret = buffer.output[1];
+
 	return ret;
 }
 
@@ -1149,31 +1165,33 @@ static DEFINE_MUTEX(kbd_led_mutex);
 
 static int kbd_get_info(struct kbd_info *info)
 {
+	struct calling_interface_buffer buffer;
 	u8 units;
 	int ret;
 
-	dell_set_arguments(0, 0, 0, 0);
-	ret = dell_send_request(CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
+	dell_fill_request(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer,
+				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
 	if (ret)
 		return ret;
 
-	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 = 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;
 
 	if (quirks && quirks->kbd_led_levels_off_1 && info->levels)
 		info->levels--;
 
 	if (units & BIT(0))
-		info->seconds = (buffer->output[3] >> 0) & 0xFF;
+		info->seconds = (buffer.output[3] >> 0) & 0xFF;
 	if (units & BIT(1))
-		info->minutes = (buffer->output[3] >> 8) & 0xFF;
+		info->minutes = (buffer.output[3] >> 8) & 0xFF;
 	if (units & BIT(2))
-		info->hours = (buffer->output[3] >> 16) & 0xFF;
+		info->hours = (buffer.output[3] >> 16) & 0xFF;
 	if (units & BIT(3))
-		info->days = (buffer->output[3] >> 24) & 0xFF;
+		info->days = (buffer.output[3] >> 24) & 0xFF;
 
 	return ret;
 }
@@ -1233,31 +1251,34 @@ static int kbd_set_level(struct kbd_state *state, u8 level)
 
 static int kbd_get_state(struct kbd_state *state)
 {
+	struct calling_interface_buffer buffer;
 	int ret;
 
-	dell_set_arguments(0x1, 0, 0, 0);
-	ret = dell_send_request(CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
+	dell_fill_request(&buffer, 0, 0, 0, 0);
+	ret = dell_send_request(&buffer,
+				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
 	if (ret)
 		return ret;
 
-	state->mode_bit = ffs(buffer->output[1] & 0xFFFF);
+	state->mode_bit = ffs(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->timeout_value_ac = (buffer->output[2] >> 24) & 0x3F;
-	state->timeout_unit_ac = (buffer->output[2] >> 30) & 0x3;
+	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->timeout_value_ac = (buffer.output[2] >> 24) & 0x3F;
+	state->timeout_unit_ac = (buffer.output[2] >> 30) & 0x3;
 
 	return ret;
 }
 
 static int kbd_set_state(struct kbd_state *state)
 {
+	struct calling_interface_buffer buffer;
 	int ret;
 	u32 input1;
 	u32 input2;
@@ -1270,8 +1291,9 @@ static int kbd_set_state(struct kbd_state *state)
 	input2 |= (state->level & 0xFF) << 16;
 	input2 |= (state->timeout_value_ac & 0x3F) << 24;
 	input2 |= (state->timeout_unit_ac & 0x3) << 30;
-	dell_set_arguments(0x2, input1, input2, 0);
-	ret = dell_send_request(CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
+	dell_fill_request(&buffer, 0x2, input1, input2, 0);
+	ret = dell_send_request(&buffer,
+				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
 
 	return ret;
 }
@@ -1298,6 +1320,7 @@ static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
 
 static int kbd_set_token_bit(u8 bit)
 {
+	struct calling_interface_buffer buffer;
 	struct calling_interface_token *token;
 	int ret;
 
@@ -1308,14 +1331,15 @@ static int kbd_set_token_bit(u8 bit)
 	if (!token)
 		return -EINVAL;
 
-	dell_set_arguments(token->location, token->value, 0, 0);
-	ret = dell_send_request(CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
+	dell_fill_request(&buffer, token->location, token->value, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
 
 	return ret;
 }
 
 static int kbd_get_token_bit(u8 bit)
 {
+	struct calling_interface_buffer buffer;
 	struct calling_interface_token *token;
 	int ret;
 	int val;
@@ -1327,9 +1351,9 @@ static int kbd_get_token_bit(u8 bit)
 	if (!token)
 		return -EINVAL;
 
-	dell_set_arguments(token->location, 0, 0, 0);
-	ret = dell_send_request(CLASS_TOKEN_READ, SELECT_TOKEN_STD);
-	val = buffer->output[1];
+	dell_fill_request(&buffer, token->location, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD);
+	val = buffer.output[1];
 
 	if (ret)
 		return ret;
@@ -2046,6 +2070,7 @@ static struct notifier_block dell_laptop_notifier = {
 
 int dell_micmute_led_set(int state)
 {
+	struct calling_interface_buffer buffer;
 	struct calling_interface_token *token;
 
 	if (state == 0)
@@ -2058,8 +2083,8 @@ int dell_micmute_led_set(int state)
 	if (!token)
 		return -ENODEV;
 
-	dell_set_arguments(token->location, token->value, 0, 0);
-	dell_send_request(CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
+	dell_fill_request(&buffer, token->location, token->value, 0, 0);
+	dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
 
 	return state;
 }
@@ -2090,13 +2115,6 @@ static int __init dell_init(void)
 	if (ret)
 		goto fail_platform_device2;
 
-	buffer = kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
-	if (!buffer) {
-		ret = -ENOMEM;
-		goto fail_buffer;
-	}
-
-
 	ret = dell_setup_rfkill();
 
 	if (ret) {
@@ -2121,10 +2139,13 @@ static int __init dell_init(void)
 
 	token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
 	if (token) {
-		dell_set_arguments(token->location, 0, 0, 0);
-		ret = dell_send_request(CLASS_TOKEN_READ, SELECT_TOKEN_AC);
+		struct calling_interface_buffer buffer;
+
+		dell_fill_request(&buffer, token->location, 0, 0, 0);
+		ret = dell_send_request(&buffer,
+					CLASS_TOKEN_READ, SELECT_TOKEN_AC);
 		if (ret)
-			max_intensity = buffer->output[3];
+			max_intensity = buffer.output[3];
 	}
 
 	if (max_intensity) {
@@ -2158,8 +2179,6 @@ static int __init dell_init(void)
 fail_get_brightness:
 	backlight_device_unregister(dell_backlight_device);
 fail_backlight:
-	kfree(buffer);
-fail_buffer:
 	dell_cleanup_rfkill();
 fail_rfkill:
 	platform_device_del(platform_device);
@@ -2179,7 +2198,6 @@ static void __exit dell_exit(void)
 		touchpad_led_exit();
 	kbd_led_exit();
 	backlight_device_unregister(dell_backlight_device);
-	kfree(buffer);
 	dell_cleanup_rfkill();
 	if (platform_device) {
 		platform_device_unregister(platform_device);
-- 
2.7.4