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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxxx>
> ---

Ok, you can add my:

Reviewed-by: Pali Rohár <pali.rohar@xxxxxxxxx>

>  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@xxxxxxxxx

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux