Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752710AbbFVS0v (ORCPT ); Mon, 22 Jun 2015 14:26:51 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:36038 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750861AbbFVS0m (ORCPT ); Mon, 22 Jun 2015 14:26:42 -0400 Date: Mon, 22 Jun 2015 11:26:48 -0700 From: Darren Hart To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Matthew Garrett , platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/4] dell-laptop: Check return value of all SMBIOS calls Message-ID: <20150622182648.GC58421@vmdeb7> References: <1434875967-13370-1-git-send-email-pali.rohar@gmail.com> <1434875967-13370-3-git-send-email-pali.rohar@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1434875967-13370-3-git-send-email-pali.rohar@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3777 Lines: 146 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 > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in Please read the FAQ at http://www.tux.org/lkml/