RE: [PATCH] platform/x86: dell-laptop: Guard SMBIOS calls with a mutex

From: Mario.Limonciello
Date: Tue Jan 30 2018 - 10:35:51 EST

Next message: Will Deacon: "Re: [PATCH v6 0/4] x86, kasan: add KASAN checks to atomic operations"
Previous message: Colin King: "[PATCH] NFC: fdp: fix signed less or equal zero check in u8 max_size"
In reply to: Pali RohÃr: "Re: [PATCH] platform/x86: dell-laptop: Guard SMBIOS calls with a mutex"
Next in thread: Andy Shevchenko: "Re: [PATCH] platform/x86: dell-laptop: Guard SMBIOS calls with a mutex"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: platform-driver-x86-owner@xxxxxxxxxxxxxxx [mailto:platform-driver-x86-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Pali RohÃr
> Sent: Tuesday, January 30, 2018 5:03 AM
> To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> Cc: dvhart@xxxxxxxxxxxxx; Andy Shevchenko <andy.shevchenko@xxxxxxxxx>;
> LKML <linux-kernel@xxxxxxxxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] platform/x86: dell-laptop: Guard SMBIOS calls with a mutex
>
> On Monday 29 January 2018 17:15:56 Mario Limonciello wrote:
> > Suggested-by: Pali Rohar <pali.rohar@xxxxxxxxx>
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxxx>
> > ---
> > drivers/platform/x86/dell-laptop.c | 67 ++++++++++++++++++++++++++++++-------
> -
> > 1 file changed, 53 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-
> laptop.c
> > index fc2dfc8..f8e2bd8 100644
> > --- a/drivers/platform/x86/dell-laptop.c
> > +++ b/drivers/platform/x86/dell-laptop.c
> > @@ -85,6 +85,7 @@ static struct rfkill *wifi_rfkill;
> > static struct rfkill *bluetooth_rfkill;
> > static struct rfkill *wwan_rfkill;
> > static bool force_rfkill;
> > +static DEFINE_MUTEX(buffer_mutex);
> >
> > module_param(force_rfkill, bool, 0444);
> > MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
> > @@ -472,16 +473,17 @@ static int dell_rfkill_set(void *data, bool blocked)
> > int status;
> > int ret;
> >
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(0, 0, 0, 0);
> > ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
> > if (ret)
> > - return ret;
> > + goto out;
> > status = buffer->output[1];
> >
> > dell_set_arguments(0x2, 0, 0, 0);
> > ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
>
> Hi! I'm looking at this code, and do we need shared global buffer with
> mutex protection at all? Is not buffer allocated on stack enough?

Oh you mean rather than create buffer mutex to just remove global
buffer and allocate in each function? That seems like a workable
approach to me too.

I'm fine with either way.

Andy or Darren, what's your preference in this area?

>
> > if (ret)
> > - return ret;
> > + goto out;
> > hwswitch = buffer->output[1];
> >
> > /* If the hardware switch controls this radio, and the hardware
> > @@ -492,6 +494,9 @@ static int dell_rfkill_set(void *data, bool blocked)
> >
> > dell_set_arguments(1 | (radio<<8) | (disable << 16), 0, 0, 0);
> > ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
> > +
> > +out:
> > + mutex_unlock(&buffer_mutex);
> > return ret;
> > }
> >
> > @@ -501,8 +506,10 @@ static void dell_rfkill_update_sw_state(struct rfkill
> *rfkill, int radio,
> > if (status & BIT(0)) {
> > /* Has hw-switch, sync sw_state to BIOS */
> > int block = rfkill_blocked(rfkill);
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(1 | (radio << 8) | (block << 16), 0, 0, 0);
> > dell_send_request(CLASS_INFO, SELECT_RFKILL);
> > + mutex_unlock(&buffer_mutex);
> > } else {
> > /* No hw-switch, sync BIOS state to sw_state */
> > rfkill_set_sw_state(rfkill, !!(status & BIT(radio + 16)));
> > @@ -523,22 +530,24 @@ static void dell_rfkill_query(struct rfkill *rfkill, void
> *data)
> > int status;
> > int ret;
> >
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(0, 0, 0, 0);
> > ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
> > status = buffer->output[1];
> >
> > - if (ret != 0 || !(status & BIT(0))) {
> > - return;
> > - }
> > + if (ret != 0 || !(status & BIT(0)))
> > + goto out;
> >
> > dell_set_arguments(0, 0x2, 0, 0);
> > ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
> > hwswitch = buffer->output[1];
> >
> > if (ret != 0)
> > - return;
> > + goto out;
> >
> > dell_rfkill_update_hw_state(rfkill, radio, status, hwswitch);
> > +out:
> > + mutex_unlock(&buffer_mutex);
> > }
> >
> > static const struct rfkill_ops dell_rfkill_ops = {
> > @@ -555,16 +564,19 @@ static int dell_debugfs_show(struct seq_file *s, void
> *data)
> > int status;
> > int ret;
> >
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(0, 0, 0, 0);
> > ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
> > if (ret)
> > - return ret;
> > + goto out;
> > status = buffer->output[1];
> >
> > dell_set_arguments(0, 0x2, 0, 0);
> > hwswitch_ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
> > - if (hwswitch_ret)
> > - return hwswitch_ret;
> > + if (hwswitch_ret) {
> > + ret = hwswitch_ret;
> > + goto out;
> > + }
> > hwswitch_state = buffer->output[1];
> >
> > seq_printf(s, "return:\t%d\n", ret);
> > @@ -628,7 +640,9 @@ static int dell_debugfs_show(struct seq_file *s, void
> *data)
> > seq_printf(s, "Bit 15: Wifi locator setting locked: %lu\n",
> > (hwswitch_state & BIT(15)) >> 15);
> >
> > - return 0;
> > +out:
> > + mutex_unlock(&buffer_mutex);
> > + return ret;
> > }
> >
> > static int dell_debugfs_open(struct inode *inode, struct file *file)
> > @@ -650,12 +664,13 @@ static void dell_update_rfkill(struct work_struct
> *ignored)
> > int status;
> > int ret;
> >
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(0, 0, 0, 0);
> > ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
> > status = buffer->output[1];
> >
> > if (ret != 0)
> > - return;
> > + goto out;
> >
> > dell_set_arguments(0, 0x2, 0, 0);
> > ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
> > @@ -676,6 +691,8 @@ static void dell_update_rfkill(struct work_struct
> *ignored)
> > dell_rfkill_update_hw_state(wwan_rfkill, 3, status, hwswitch);
> > dell_rfkill_update_sw_state(wwan_rfkill, 3, status);
> > }
> > +out:
> > + mutex_unlock(&buffer_mutex);
> > }
> > static DECLARE_DELAYED_WORK(dell_rfkill_work, dell_update_rfkill);
> >
> > @@ -734,9 +751,11 @@ static int __init dell_setup_rfkill(void)
> > if (!force_rfkill && !whitelisted)
> > return 0;
> >
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(0, 0, 0, 0);
> > ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
> > status = buffer->output[1];
> > + mutex_unlock(&buffer_mutex);
> >
> > /* dell wireless info smbios call is not supported */
> > if (ret != 0)
> > @@ -896,11 +915,13 @@ static int dell_send_intensity(struct backlight_device
> *bd)
> > if (!token)
> > return -ENODEV;
> >
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(token->location, bd->props.brightness, 0, 0);
> > if (power_supply_is_system_supplied() > 0)
> > ret = dell_send_request(CLASS_TOKEN_WRITE,
> SELECT_TOKEN_AC);
> > else
> > ret = dell_send_request(CLASS_TOKEN_WRITE,
> SELECT_TOKEN_BAT);
> > + mutex_unlock(&buffer_mutex);
> >
> > return ret;
> > }
> > @@ -914,6 +935,7 @@ static int dell_get_intensity(struct backlight_device *bd)
> > if (!token)
> > return -ENODEV;
> >
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(token->location, 0, 0, 0);
> > if (power_supply_is_system_supplied() > 0)
> > ret = dell_send_request(CLASS_TOKEN_READ, SELECT_TOKEN_AC);
> > @@ -922,6 +944,8 @@ static int dell_get_intensity(struct backlight_device *bd)
> >
> > if (ret == 0)
> > ret = buffer->output[1];
> > + mutex_unlock(&buffer_mutex);
> > +
> > return ret;
> > }
> >
> > @@ -1189,10 +1213,11 @@ static int kbd_get_info(struct kbd_info *info)
> > u8 units;
> > int ret;
> >
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(0, 0, 0, 0);
> > ret = dell_send_request(CLASS_KBD_BACKLIGHT,
> SELECT_KBD_BACKLIGHT);
> > if (ret)
> > - return ret;
> > + goto out;
> >
> > info->modes = buffer->output[1] & 0xFFFF;
> > info->type = (buffer->output[1] >> 24) & 0xFF;
> > @@ -1212,6 +1237,8 @@ static int kbd_get_info(struct kbd_info *info)
> > if (units & BIT(3))
> > info->days = (buffer->output[3] >> 24) & 0xFF;
> >
> > +out:
> > + mutex_unlock(&buffer_mutex);
> > return ret;
> > }
> >
> > @@ -1272,10 +1299,11 @@ static int kbd_get_state(struct kbd_state *state)
> > {
> > int ret;
> >
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(0x1, 0, 0, 0);
> > ret = dell_send_request(CLASS_KBD_BACKLIGHT,
> SELECT_KBD_BACKLIGHT);
> > if (ret)
> > - return ret;
> > + goto out;
> >
> > state->mode_bit = ffs(buffer->output[1] & 0xFFFF);
> > if (state->mode_bit != 0)
> > @@ -1289,7 +1317,8 @@ static int kbd_get_state(struct kbd_state *state)
> > 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;
> > -
> > +out:
> > + mutex_unlock(&buffer_mutex);
> > return ret;
> > }
> >
> > @@ -1307,8 +1336,10 @@ 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;
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(0x2, input1, input2, 0);
> > ret = dell_send_request(CLASS_KBD_BACKLIGHT,
> SELECT_KBD_BACKLIGHT);
> > + mutex_unlock(&buffer_mutex);
> >
> > return ret;
> > }
> > @@ -1345,8 +1376,10 @@ static int kbd_set_token_bit(u8 bit)
> > if (!token)
> > return -EINVAL;
> >
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(token->location, token->value, 0, 0);
> > ret = dell_send_request(CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> > + mutex_unlock(&buffer_mutex);
> >
> > return ret;
> > }
> > @@ -1364,9 +1397,11 @@ static int kbd_get_token_bit(u8 bit)
> > if (!token)
> > return -EINVAL;
> >
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(token->location, 0, 0, 0);
> > ret = dell_send_request(CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> > val = buffer->output[1];
> > + mutex_unlock(&buffer_mutex);
> >
> > if (ret)
> > return ret;
> > @@ -2114,8 +2149,10 @@ int dell_micmute_led_set(int state)
> > if (!token)
> > return -ENODEV;
> >
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(token->location, token->value, 0, 0);
> > dell_send_request(CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> > + mutex_unlock(&buffer_mutex);
> >
> > return state;
> > }
> > @@ -2177,10 +2214,12 @@ static int __init dell_init(void)
> >
> > token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
> > if (token) {
> > + mutex_lock(&buffer_mutex);
> > dell_set_arguments(token->location, 0, 0, 0);
> > ret = dell_send_request(CLASS_TOKEN_READ, SELECT_TOKEN_AC);
> > if (ret)
> > max_intensity = buffer->output[3];
> > + mutex_unlock(&buffer_mutex);
> > }
> >
> > if (max_intensity) {
>
> --
> Pali RohÃr
> pali.rohar@xxxxxxxxx

Next message: Will Deacon: "Re: [PATCH v6 0/4] x86, kasan: add KASAN checks to atomic operations"
Previous message: Colin King: "[PATCH] NFC: fdp: fix signed less or equal zero check in u8 max_size"
In reply to: Pali RohÃr: "Re: [PATCH] platform/x86: dell-laptop: Guard SMBIOS calls with a mutex"
Next in thread: Andy Shevchenko: "Re: [PATCH] platform/x86: dell-laptop: Guard SMBIOS calls with a mutex"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]