Re: [PATCH 2/4] dell-laptop: Check return value of all SMBIOS calls

[Darren Hart <dvhart@infradead.org>]

On Sun, Jun 21, 2015 at 10:39:27AM +0200, Pali Rohár wrote:
> Make sure that return value of all SMBIOS calls are properly checked and
> do not continue of processing (received) information if call failed.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/platform/x86/dell-laptop.c |   61 +++++++++++++++++++++++++++++++-----
>  1 file changed, 54 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c

...

> @@ -677,11 +690,16 @@ static const struct file_operations dell_debugfs_fops = {
>  static void dell_update_rfkill(struct work_struct *ignored)
>  {
>  	int status;
> +	int ret;
>  
>  	get_buffer();
>  	dell_send_request(buffer, 17, 11);
> +	ret = buffer->output[0];
>  	status = buffer->output[1];

Parallel to previous blocks, you can release_buffer() here...

>  
> +	if (ret != 0)
> +		goto out;

And just return here

> +
>  	if (wifi_rfkill) {
>  		dell_rfkill_update_hw_state(wifi_rfkill, 1, status);
>  		dell_rfkill_update_sw_state(wifi_rfkill, 1, status);
> @@ -695,6 +713,7 @@ static void dell_update_rfkill(struct work_struct *ignored)
>  		dell_rfkill_update_sw_state(wwan_rfkill, 3, status);
>  	}
>  
> + out:

And drop this label.

>  	release_buffer();
>  }
>  static DECLARE_DELAYED_WORK(dell_rfkill_work, dell_update_rfkill);
> @@ -755,13 +774,35 @@ static int __init dell_setup_rfkill(void)
>  		return 0;
>  
>  	get_buffer();
> +
>  	dell_send_request(buffer, 17, 11);
> +	ret = buffer->output[0];
>  	status = buffer->output[1];
> +	if (ret != 0) {
> +		/* dell wireless info smbios call is not working */
> +		/* so there is no support for rfkill */

Please follow coding style for multi-line comments.

> +		release_buffer();
> +		return 0;
> +	}
> +
>  	buffer->input[0] = 0x2;
>  	dell_send_request(buffer, 17, 11);
> +	ret = buffer->output[0];
>  	hwswitch_state = buffer->output[1];
> +
>  	release_buffer();
>  
> +	if (ret != 0) {

Just "if (ret)" is more typical

> +		/* dell wireless switch smbios call is not working */
> +		if (force_rfkill) {
> +			/* clear all hw-controlled bits */
> +			hwswitch_state &= ~7;
> +		} else {
> +			/* rfkill is only tested on laptops with a hwswitch */
> +			return 0;
> +		}

Save an additional indent block and all the braces with:

		/* rfkill is only tested on laptops with a hwswitch */
		if (!force_rfkill)
			return 0

		/* clear all hw-controlled bits */
		hwswitch_state &= ~7;


> +	}
> +
>  	if (!(status & BIT(0))) {
>  		if (force_rfkill) {
>  			/* No hwsitch, clear all hw-controlled bits */
> @@ -931,6 +972,8 @@ static int dell_send_intensity(struct backlight_device *bd)
>  	else
>  		dell_send_request(buffer, 1, 1);
>  
> +	ret = dell_smi_error(buffer->output[0]);
> +
>   out:
>  	release_buffer();
>  	return ret;
> @@ -953,7 +996,10 @@ static int dell_get_intensity(struct backlight_device *bd)
>  	else
>  		dell_send_request(buffer, 0, 1);
>  
> -	ret = buffer->output[1];
> +	if (buffer->output[0])
> +		ret = dell_smi_error(buffer->output[0]);
> +	else
> +		ret = buffer->output[1];

This is OK, but this block reverses the ret/status terms applied to output[0]
and output[1] which is a little confusing.

>  
>   out:
>  	release_buffer();
> @@ -2087,7 +2133,8 @@ static int __init dell_init(void)
>  	buffer->input[0] = find_token_location(BRIGHTNESS_TOKEN);
>  	if (buffer->input[0] != -1) {
>  		dell_send_request(buffer, 0, 2);
> -		max_intensity = buffer->output[3];
> +		if (buffer->output[0] == 0)
> +			max_intensity = buffer->output[3];
>  	}
>  	release_buffer();
>  
> -- 
> 1.7.9.5
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center