by Darren Hart

[permalink] [raw]

Subject: Re: [PATCH v2] dell-laptop: Check return value of all SMBIOS calls and do not cache hwswitch state

On Sat, Jun 27, 2015 at 09:34:43AM +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.
>
> Also do not chache hwswitch wireless state as it can be changed at runtime
> (e.g from userspace smbios applications).

This "Also do.." tripled the size of the patch. This should really be two
patches.

>
> Signed-off-by: Pali Roh?r <[email protected]>
> ---
> Changes since v1:
> * Call clear_buffer before each sequential SMBIOS call (we expect zero-filled buffer)

Another good independent patch candidate

> * Do not cache hwswitch state as it can be modified at runtime by userspace
> * simplify some conditions
> ---
> drivers/platform/x86/dell-laptop.c | 173 ++++++++++++++++++++++++++----------
> 1 file changed, 127 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 35758cb..99f28d3 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c

...

> static void dell_update_rfkill(struct work_struct *ignored)
> {
> + int hwswitch;
> int status;
> + int ret;
>
> get_buffer();
> +
> dell_send_request(buffer, 17, 11);
> + ret = buffer->output[0];
> status = buffer->output[1];
>
> + if (ret != 0)
> + goto out;
> +
> + clear_buffer();
> +
> + buffer->input[0] = 0x2;
> + dell_send_request(buffer, 17, 11);
> + ret = buffer->output[0];
> +
> + if (ret == 0 && (status & BIT(0)))
> + hwswitch = buffer->output[1];
> + else
> + hwswitch = 0;

Initializing hwswitch to 0 above saves the else and assignment line here.
Generally preferred.

...

>
> static int dell_get_intensity(struct backlight_device *bd)
> {
> - int ret = 0;
> + int ret;
> + int token;

Since we're talking respin, declare in order of descending line length please,
just as you did later when adding token to a function.

--
Darren Hart
Intel Open Source Technology Center

2015-07-01 18:04:58

It can be changed at runtime so make sure that dell-laptop knows always
current state. It can be modified by userspace utility smbios-wireless-ctl.

Signed-off-by: Pali Rohár <[email protected]>
---
drivers/platform/x86/dell-laptop.c | 85 ++++++++++++++++++++++++++----------
1 file changed, 61 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index d9710c1..89d6004 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -308,8 +308,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
static struct calling_interface_buffer *buffer;
static DEFINE_MUTEX(buffer_mutex);

-static int hwswitch_state;
-
static void clear_buffer(void)
{
memset(buffer, 0, sizeof(struct calling_interface_buffer));
@@ -552,20 +550,30 @@ 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;
+ int hwswitch;
+ int status;
int ret;

get_buffer();

dell_send_request(buffer, 17, 11);
ret = buffer->output[0];
+ status = buffer->output[1];

if (ret != 0)
goto out;

+ clear_buffer();
+
+ buffer->input[0] = 0x2;
+ dell_send_request(buffer, 17, 11);
+ ret = buffer->output[0];
+ hwswitch = buffer->output[1];
+
/* If the hardware switch controls this radio, and the hardware
switch is disabled, always disable the radio */
- if ((hwswitch_state & BIT(hwswitch_bit)) &&
- !(buffer->output[1] & BIT(16)))
+ if (ret == 0 && (hwswitch & BIT(hwswitch_bit)) &&
+ (status & BIT(0)) && !(status & BIT(16)))
disable = 1;

clear_buffer();
@@ -596,27 +604,43 @@ static void dell_rfkill_update_sw_state(struct rfkill *rfkill, int radio,
}

static void dell_rfkill_update_hw_state(struct rfkill *rfkill, int radio,
- int status)
+ int status, int hwswitch)
{
- if (hwswitch_state & (BIT(radio - 1)))
+ if (hwswitch & (BIT(radio - 1)))
rfkill_set_hw_state(rfkill, !(status & BIT(16)));
}

static void dell_rfkill_query(struct rfkill *rfkill, void *data)
{
+ int radio = ((unsigned long)data & 0xF);
+ int hwswitch;
int status;
int ret;

get_buffer();
+
dell_send_request(buffer, 17, 11);
ret = buffer->output[0];
status = buffer->output[1];
+
+ if (ret != 0 || !(status & BIT(0))) {
+ release_buffer();
+ return;
+ }
+
+ clear_buffer();
+
+ buffer->input[0] = 0x2;
+ dell_send_request(buffer, 17, 11);
+ ret = buffer->output[0];
+ hwswitch = buffer->output[1];
+
release_buffer();

if (ret != 0)
return;

- dell_rfkill_update_hw_state(rfkill, (unsigned long)data, status);
+ dell_rfkill_update_hw_state(rfkill, radio, status, hwswitch);
}

static const struct rfkill_ops dell_rfkill_ops = {
@@ -628,13 +652,24 @@ static struct dentry *dell_laptop_dir;

static int dell_debugfs_show(struct seq_file *s, void *data)
{
+ int hwswitch_state;
+ int hwswitch_ret;
int status;
int ret;

get_buffer();
+
dell_send_request(buffer, 17, 11);
ret = buffer->output[0];
status = buffer->output[1];
+
+ clear_buffer();
+
+ buffer->input[0] = 0x2;
+ dell_send_request(buffer, 17, 11);
+ hwswitch_ret = buffer->output[0];
+ hwswitch_state = buffer->output[1];
+
release_buffer();

seq_printf(s, "return:\t%d\n", ret);
@@ -679,7 +714,8 @@ static int dell_debugfs_show(struct seq_file *s, void *data)
seq_printf(s, "Bit 21: WiGig is blocked: %lu\n",
(status & BIT(21)) >> 21);

- seq_printf(s, "\nhwswitch_state:\t0x%X\n", hwswitch_state);
+ seq_printf(s, "\nhwswitch_return:\t%d\n", hwswitch_ret);
+ seq_printf(s, "hwswitch_state:\t0x%X\n", hwswitch_state);
seq_printf(s, "Bit 0 : Wifi controlled by switch: %lu\n",
hwswitch_state & BIT(0));
seq_printf(s, "Bit 1 : Bluetooth controlled by switch: %lu\n",
@@ -715,6 +751,7 @@ static const struct file_operations dell_debugfs_fops = {

static void dell_update_rfkill(struct work_struct *ignored)
{
+ int hwswitch = 0;
int status;
int ret;

@@ -727,16 +764,26 @@ static void dell_update_rfkill(struct work_struct *ignored)
if (ret != 0)
goto out;

+ clear_buffer();
+
+ buffer->input[0] = 0x2;
+ dell_send_request(buffer, 17, 11);
+ ret = buffer->output[0];
+
+ if (ret == 0 && (status & BIT(0)))
+ hwswitch = buffer->output[1];
+
if (wifi_rfkill) {
- dell_rfkill_update_hw_state(wifi_rfkill, 1, status);
+ dell_rfkill_update_hw_state(wifi_rfkill, 1, status, hwswitch);
dell_rfkill_update_sw_state(wifi_rfkill, 1, status);
}
if (bluetooth_rfkill) {
- dell_rfkill_update_hw_state(bluetooth_rfkill, 2, status);
+ dell_rfkill_update_hw_state(bluetooth_rfkill, 2, status,
+ hwswitch);
dell_rfkill_update_sw_state(bluetooth_rfkill, 2, status);
}
if (wwan_rfkill) {
- dell_rfkill_update_hw_state(wwan_rfkill, 3, status);
+ dell_rfkill_update_hw_state(wwan_rfkill, 3, status, hwswitch);
dell_rfkill_update_sw_state(wwan_rfkill, 3, status);
}

@@ -804,25 +851,15 @@ static int __init dell_setup_rfkill(void)
dell_send_request(buffer, 17, 11);
ret = buffer->output[0];
status = buffer->output[1];
- clear_buffer();
- buffer->input[0] = 0x2;
- dell_send_request(buffer, 17, 11);
- hwswitch_state = buffer->output[1];
release_buffer();

/* dell wireless info smbios call is not supported */
if (ret != 0)
return 0;

- if (!(status & BIT(0))) {
- if (force_rfkill) {
- /* No hwsitch, clear all hw-controlled bits */
- hwswitch_state &= ~7;
- } else {
- /* rfkill is only tested on laptops with a hwswitch */
- return 0;
- }
- }
+ /* rfkill is only tested on laptops with a hwswitch */
+ if (!(status & BIT(0)) && !force_rfkill)
+ return 0;

if ((status & (1<<2|1<<8)) == (1<<2|1<<8)) {
wifi_rfkill = rfkill_alloc("dell-wifi", &platform_device->dev,
--
1.7.9.5

2015-07-06 10:10:07

On Tue, Jul 07, 2015 at 04:04:57PM +0200, Pali Roh?r wrote:
> On Monday 06 July 2015 15:39:37 Darren Hart wrote:
> > On Mon, Jul 06, 2015 at 12:08:55PM +0200, Pali Roh?r wrote:
> > > Make sure that before initializing SMBIOS call input buffer does not
> > > contain any garbage (e.g. values from previous SMBIOS call). This fix
> > > problem with passing undefined/random parameters to SMBIOS functions.
> > >
> > > Signed-off-by: Pali Roh?r <[email protected]>
> >
> > Queued all 3 to fixes. Thanks Pali.
> >
>
> Ok.
>
> > (I made a few minor grammatical fixes tot he commit messages, review the changes
> > if you like in the fixes branch)
> >
>
> It is OK, I'm not native speaker and I have lot of troubles with English
> language. So feel free to fix any grammatical mistakes...

You do better than I do with any other language I attempt :-) I don't feel it's
reasonable for me to nag you about commit messages, but I do need to ensure they
are clear. I'll continue to update them going forward.

--
Darren Hart
Intel Open Source Technology Center