ACPI: thinkpad-acpi: refactor hotkey_get and hotkey_set (v2)

Refactor and organize the code a bit for the NVRAM polling support:

1. Split hotkey_get/set into hotkey_status_get/set and hotkey_mask_get/set;
2. Cache the status of hot key mask for later driver use;
3. Make sure the cache of hot key mask is refreshed when needed;
4. log a printk notice when the firmware doesn't set the hot key
   mask to exactly what we asked it to;
5. Add proper locking to the data structures.

Only (4) should be user-noticeable, but there is a chance (5) fixes
some unknown/unreported race conditions.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Signed-off-by: Len Brown <len.brown@intel.com>

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 7b1080f..49d4f4a 100644 (file)
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -777,6 +777,7 @@ static int hotkey_orig_status;
 static u32 hotkey_orig_mask;
 static u32 hotkey_all_mask;
 static u32 hotkey_reserved_mask;
+static u32 hotkey_mask;
 
 static u16 *hotkey_keycode_map;
 
@@ -789,15 +790,76 @@ static int hotkey_get_wlsw(int *status)
        return 0;
 }
 
+/*
+ * Call with hotkey_mutex held
+ */
+static int hotkey_mask_get(void)
+{
+       if (tp_features.hotkey_mask) {
+               if (!acpi_evalf(hkey_handle, &hotkey_mask, "DHKN", "d"))
+                       return -EIO;
+       }
+
+       return 0;
+}
+
+/*
+ * Call with hotkey_mutex held
+ */
+static int hotkey_mask_set(u32 mask)
+{
+       int i;
+       int rc = 0;
+
+       if (tp_features.hotkey_mask) {
+               for (i = 0; i < 32; i++) {
+                       u32 m = 1 << i;
+                       if (!acpi_evalf(hkey_handle,
+                                       NULL, "MHKM", "vdd", i + 1,
+                                       !!(mask & m))) {
+                               rc = -EIO;
+                               break;
+                       } else {
+                               hotkey_mask = (hotkey_mask & ~m) | (mask & m);
+                       }
+               }
+
+               /* hotkey_mask_get must be called unconditionally below */
+               if (!hotkey_mask_get() && !rc && hotkey_mask != mask) {
+                       printk(IBM_NOTICE
+                              "requested hot key mask 0x%08x, but "
+                              "firmware forced it to 0x%08x\n",
+                              mask, hotkey_mask);
+               }
+       }
+
+       return rc;
+}
+
+static int hotkey_status_get(int *status)
+{
+       if (!acpi_evalf(hkey_handle, status, "DHKC", "d"))
+               return -EIO;
+
+       return 0;
+}
+
+static int hotkey_status_set(int status)
+{
+       if (!acpi_evalf(hkey_handle, NULL, "MHKC", "vd", status))
+               return -EIO;
+
+       return 0;
+}
+
 /* sysfs hotkey enable ------------------------------------------------- */
 static ssize_t hotkey_enable_show(struct device *dev,
                           struct device_attribute *attr,
                           char *buf)
 {
        int res, status;
-       u32 mask;
 
-       res = hotkey_get(&status, &mask);
+       res = hotkey_status_get(&status);
        if (res)
                return res;
 
@@ -809,15 +871,12 @@ static ssize_t hotkey_enable_store(struct device *dev,
                            const char *buf, size_t count)
 {
        unsigned long t;
-       int res, status;
-       u32 mask;
+       int res;
 
        if (parse_strtoul(buf, 1, &t))
                return -EINVAL;
 
-       res = hotkey_get(&status, &mask);
-       if (!res)
-               res = hotkey_set(t, mask);
+       res = hotkey_status_set(t);
 
        return (res) ? res : count;
 }
@@ -831,14 +890,15 @@ static ssize_t hotkey_mask_show(struct device *dev,
                           struct device_attribute *attr,
                           char *buf)
 {
-       int res, status;
-       u32 mask;
+       int res;
 
-       res = hotkey_get(&status, &mask);
-       if (res)
-               return res;
+       if (mutex_lock_interruptible(&hotkey_mutex))
+               return -ERESTARTSYS;
+       res = hotkey_mask_get();
+       mutex_unlock(&hotkey_mutex);
 
-       return snprintf(buf, PAGE_SIZE, "0x%08x\n", mask);
+       return (res)?
+               res : snprintf(buf, PAGE_SIZE, "0x%08x\n", hotkey_mask);
 }
 
 static ssize_t hotkey_mask_store(struct device *dev,
@@ -846,15 +906,16 @@ static ssize_t hotkey_mask_store(struct device *dev,
                            const char *buf, size_t count)
 {
        unsigned long t;
-       int res, status;
-       u32 mask;
+       int res;
 
        if (parse_strtoul(buf, 0xffffffffUL, &t))
                return -EINVAL;
 
-       res = hotkey_get(&status, &mask);
-       if (!res)
-               hotkey_set(status, t);
+       if (mutex_lock_interruptible(&hotkey_mutex))
+               return -ERESTARTSYS;
+
+       res = hotkey_mask_set(t);
+       mutex_unlock(&hotkey_mutex);
 
        return (res) ? res : count;
 }
@@ -1123,11 +1184,16 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
                        }
                }
 
-               res = hotkey_get(&hotkey_orig_status, &hotkey_orig_mask);
+               res = hotkey_status_get(&hotkey_orig_status);
                if (!res && tp_features.hotkey_mask) {
-                       res = add_many_to_attr_set(hotkey_dev_attributes,
-                               hotkey_mask_attributes,
-                               ARRAY_SIZE(hotkey_mask_attributes));
+                       res = hotkey_mask_get();
+                       hotkey_orig_mask = hotkey_mask;
+                       if (!res) {
+                               res = add_many_to_attr_set(
+                                       hotkey_dev_attributes,
+                                       hotkey_mask_attributes,
+                                       ARRAY_SIZE(hotkey_mask_attributes));
+                       }
                }
 
                /* Not all thinkpads have a hardware radio switch */
@@ -1191,7 +1257,10 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 
                dbg_printk(TPACPI_DBG_INIT,
                                "enabling hot key handling\n");
-               res = hotkey_set(1, (hotkey_all_mask & ~hotkey_reserved_mask)
+               res = hotkey_status_set(1);
+               if (res)
+                       return res;
+               res = hotkey_mask_set((hotkey_all_mask & ~hotkey_reserved_mask)
                                        | hotkey_orig_mask);
                if (res)
                        return res;
@@ -1207,13 +1276,12 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 
 static void hotkey_exit(void)
 {
-       int res;
-
        if (tp_features.hotkey) {
-               dbg_printk(TPACPI_DBG_EXIT, "restoring original hotkey mask\n");
-               res = hotkey_set(hotkey_orig_status, hotkey_orig_mask);
-               if (res)
-                       printk(IBM_ERR "failed to restore hotkey to BIOS defaults\n");
+               dbg_printk(TPACPI_DBG_EXIT, "restoring original hot key mask\n");
+               /* no short-circuit boolean operator below! */
+               if ((hotkey_mask_set(hotkey_orig_mask) |
+                    hotkey_status_set(hotkey_orig_status)) != 0)
+                       printk(IBM_ERR "failed to restore hot key mask to BIOS defaults\n");
        }
 
        if (hotkey_dev_attributes) {
@@ -1349,50 +1417,15 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 
 static void hotkey_resume(void)
 {
+       if (hotkey_mask_get())
+               printk(IBM_ERR "error while trying to read hot key mask from firmware\n");
        tpacpi_input_send_radiosw();
 }
 
-/*
- * Call with hotkey_mutex held
- */
-static int hotkey_get(int *status, u32 *mask)
-{
-       if (!acpi_evalf(hkey_handle, status, "DHKC", "d"))
-               return -EIO;
-
-       if (tp_features.hotkey_mask)
-               if (!acpi_evalf(hkey_handle, mask, "DHKN", "d"))
-                       return -EIO;
-
-       return 0;
-}
-
-/*
- * Call with hotkey_mutex held
- */
-static int hotkey_set(int status, u32 mask)
-{
-       int i;
-
-       if (!acpi_evalf(hkey_handle, NULL, "MHKC", "vd", status))
-               return -EIO;
-
-       if (tp_features.hotkey_mask)
-               for (i = 0; i < 32; i++) {
-                       int bit = ((1 << i) & mask) != 0;
-                       if (!acpi_evalf(hkey_handle,
-                                       NULL, "MHKM", "vdd", i + 1, bit))
-                               return -EIO;
-               }
-
-       return 0;
-}
-
 /* procfs -------------------------------------------------------------- */
 static int hotkey_read(char *p)
 {
        int res, status;
-       u32 mask;
        int len = 0;
 
        if (!tp_features.hotkey) {
@@ -1402,14 +1435,16 @@ static int hotkey_read(char *p)
 
        if (mutex_lock_interruptible(&hotkey_mutex))
                return -ERESTARTSYS;
-       res = hotkey_get(&status, &mask);
+       res = hotkey_status_get(&status);
+       if (!res)
+               res = hotkey_mask_get();
        mutex_unlock(&hotkey_mutex);
        if (res)
                return res;
 
        len += sprintf(p + len, "status:\t\t%s\n", enabled(status, 0));
        if (tp_features.hotkey_mask) {
-               len += sprintf(p + len, "mask:\t\t0x%08x\n", mask);
+               len += sprintf(p + len, "mask:\t\t0x%08x\n", hotkey_mask);
                len += sprintf(p + len,
                               "commands:\tenable, disable, reset, <mask>\n");
        } else {
@@ -1425,7 +1460,6 @@ static int hotkey_write(char *buf)
        int res, status;
        u32 mask;
        char *cmd;
-       int do_cmd = 0;
 
        if (!tp_features.hotkey)
                return -ENODEV;
@@ -1433,9 +1467,8 @@ static int hotkey_write(char *buf)
        if (mutex_lock_interruptible(&hotkey_mutex))
                return -ERESTARTSYS;
 
-       res = hotkey_get(&status, &mask);
-       if (res)
-               goto errexit;
+       status = -1;
+       mask = hotkey_mask;
 
        res = 0;
        while ((cmd = next_cmd(&buf))) {
@@ -1454,11 +1487,12 @@ static int hotkey_write(char *buf)
                        res = -EINVAL;
                        goto errexit;
                }
-               do_cmd = 1;
        }
+       if (status != -1)
+               res = hotkey_status_set(status);
 
-       if (do_cmd)
-               res = hotkey_set(status, mask);
+       if (!res && mask != hotkey_mask)
+               res = hotkey_mask_set(mask);
 
 errexit:
        mutex_unlock(&hotkey_mutex);

diff --git a/drivers/misc/thinkpad_acpi.h b/drivers/misc/thinkpad_acpi.h
index 8fba2bb..3b03134 100644 (file)
--- a/drivers/misc/thinkpad_acpi.h
+++ b/drivers/misc/thinkpad_acpi.h
@@ -461,8 +461,6 @@ static struct mutex hotkey_mutex;
 
 static int hotkey_init(struct ibm_init_struct *iibm);
 static void hotkey_exit(void);
-static int hotkey_get(int *status, u32 *mask);
-static int hotkey_set(int status, u32 mask);
 static void hotkey_notify(struct ibm_struct *ibm, u32 event);
 static int hotkey_read(char *p);
 static int hotkey_write(char *buf);