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

[PATCH v2] 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.

Fixes: 549b4930f057658dc50d8010e66219233119a4d8
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..a7b1419 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_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;
@@ -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_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 */
@@ -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_fill_request(&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_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)));
@@ -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_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;
@@ -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_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);
@@ -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_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);
@@ -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_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)
@@ -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_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;
 
@@ -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_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;
 }
 
@@ -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_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;
 }
@@ -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_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;
@@ -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_fill_request(&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_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;
@@ -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_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;
@@ -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_fill_request(&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_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) {
@@ -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 v2] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

[Andy Shevchenko]

On Wed, Jan 31, 2018 at 7:47 PM, Mario Limonciello
<mario.limonciello@dell.com> 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.
>

Pushed to my review and testing queue, thanks!

> Fixes: 549b4930f057658dc50d8010e66219233119a4d8
> 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..a7b1419 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_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;
> @@ -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_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 */
> @@ -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_fill_request(&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_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)));
> @@ -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_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;
> @@ -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_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);
> @@ -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_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);
> @@ -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_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)
> @@ -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_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;
>
> @@ -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_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;
>  }
>
> @@ -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_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;
>  }
> @@ -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_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;
> @@ -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_fill_request(&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_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;
> @@ -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_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;
> @@ -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_fill_request(&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_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) {
> @@ -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
>



-- 
With Best Regards,
Andy Shevchenko

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

[Pali Rohár]

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

On Wednesday 31 January 2018 11:47:35 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.
> 
> Fixes: 549b4930f057658dc50d8010e66219233119a4d8
> Suggested-by: Pali Rohar <pali.rohar@gmail.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---

Ok, you can add my:

Reviewed-by: Pali Rohár <pali.rohar@gmail.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..a7b1419 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_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;
> @@ -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_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 */
> @@ -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_fill_request(&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_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)));
> @@ -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_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;
> @@ -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_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);
> @@ -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_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);
> @@ -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_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)
> @@ -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_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;
>  
> @@ -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_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;
>  }
>  
> @@ -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_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;
>  }
> @@ -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_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;
> @@ -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_fill_request(&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_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;
> @@ -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_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;
> @@ -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_fill_request(&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_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) {
> @@ -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);

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

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

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

[Andy Shevchenko]

On Sun, Feb 4, 2018 at 2:09 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Wednesday 31 January 2018 11:47:35 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.
>>
>> Fixes: 549b4930f057658dc50d8010e66219233119a4d8

He-h, I had to notice this earlier...

> Ok, you can add my:
> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

Thanks and sorry, Pali, it's in for-next already, can't rebase.

-- 
With Best Regards,
Andy Shevchenko

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

[Mario.Limonciello]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Sunday, February 4, 2018 8:28 AM
> To: Pali Rohár <pali.rohar@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 v2] platform/x86: dell-laptop: Allocate buffer on heap rather
> than globally
> 
> On Sun, Feb 4, 2018 at 2:09 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > On Wednesday 31 January 2018 11:47:35 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.
> >>
> >> Fixes: 549b4930f057658dc50d8010e66219233119a4d8
> 
> He-h, I had to notice this earlier...
> 
> > Ok, you can add my:
> > Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
> 
> Thanks and sorry, Pali, it's in for-next already, can't rebase.

Andy,
Since it's already in for-next it's probably too late to add the stable CC too right?

So what's the proper time now to send this to @stable?  And should I just forward existing
patch?

Thanks,

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

[Darren Hart]

On Mon, Feb 05, 2018 at 04:49:35PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> > Sent: Sunday, February 4, 2018 8:28 AM
> > To: Pali Rohár <pali.rohar@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 v2] platform/x86: dell-laptop: Allocate buffer on heap rather
> > than globally
> > 
> > On Sun, Feb 4, 2018 at 2:09 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > On Wednesday 31 January 2018 11:47:35 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.
> > >>
> > >> Fixes: 549b4930f057658dc50d8010e66219233119a4d8
> > 
> > He-h, I had to notice this earlier...
> > 
> > > Ok, you can add my:
> > > Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
> > 
> > Thanks and sorry, Pali, it's in for-next already, can't rebase.
> 
> Andy,
> Since it's already in for-next it's probably too late to add the stable CC too right?
> 
> So what's the proper time now to send this to @stable?  And should I just forward existing
> patch?

As a general rule, Andy and I should be adding Cc stable to most anything that
includes a Fixes tag that isn't from this review cycle. I've forgotten in the
past as well - sorry about that. Something we should add some tooling around I
think, so we don't miss it when checking things in to our review branches.

As to timing. As soon as this is merged to Linus' master, it can go to stable.
Instructions for doing this are in Documentation/process/stable-kernel-rules.rst

-- 
Darren Hart
VMware Open Source Technology Center

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

[Mario.Limonciello]

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Monday, February 5, 2018 6:58 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: andy.shevchenko@gmail.com; pali.rohar@gmail.com; linux-
> kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org
> Subject: Re: [PATCH v2] platform/x86: dell-laptop: Allocate buffer on heap rather
> than globally
> 
> On Mon, Feb 05, 2018 at 04:49:35PM +0000, Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> > > Sent: Sunday, February 4, 2018 8:28 AM
> > > To: Pali Rohár <pali.rohar@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 v2] platform/x86: dell-laptop: Allocate buffer on heap
> rather
> > > than globally
> > >
> > > On Sun, Feb 4, 2018 at 2:09 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > On Wednesday 31 January 2018 11:47:35 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.
> > > >>
> > > >> Fixes: 549b4930f057658dc50d8010e66219233119a4d8
> > >
> > > He-h, I had to notice this earlier...
> > >
> > > > Ok, you can add my:
> > > > Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
> > >
> > > Thanks and sorry, Pali, it's in for-next already, can't rebase.
> >
> > Andy,
> > Since it's already in for-next it's probably too late to add the stable CC too right?
> >
> > So what's the proper time now to send this to @stable?  And should I just forward
> existing
> > patch?
> 
> As a general rule, Andy and I should be adding Cc stable to most anything that
> includes a Fixes tag that isn't from this review cycle. I've forgotten in the
> past as well - sorry about that. Something we should add some tooling around I
> think, so we don't miss it when checking things in to our review branches.
> 
> As to timing. As soon as this is merged to Linus' master, it can go to stable.
> Instructions for doing this are in Documentation/process/stable-kernel-rules.rst
> 
> --

Thanks, I see it's in Linuses' tree today so I sent something to stable for it.