| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 18 Dec 2017 01:11:59 +0300
From: Dzianis Kahanovich <mahatma@...u.by>
To: linux-kernel@...r.kernel.org
Subject: [PATCH] hid: i2c-hid: Unify invariant post-power-on delay
This is result of debugging WCOM5020 on "Thinkpad 10 2nd" (2 HID devices -
finger & pen), have random failures on any delay. Quirk-style fixes
still wrong.
I found this delay was too invariant (different after
regulator_enable() & i2c command) and not even called in many
i2c power-on places (like PM & HID open/close, called multiple in my case).
All looks logically wrong.
Lets consolidate & serialize power-on code & delay, adding new module
parameter "post_power_delay_ms_default".
Side-effect: delay will be sleep twice (on init) - after regulator_enable()
and cmd power-on. There are no way to separately found "what is true power-on"
for abstract unknown device. But, AFAIK, delay on power-on is by specification.
This is just unified replacement of 2 different sleep attempt.
Now for me post_power_delay_ms_default=1000 is safe.
Signed-off-by: Dzianis Kahanovich <mahatma@...by>
--- a/drivers/hid/i2c-hid/i2c-hid.c 2017-12-17 23:34:28.067527024 +0300
+++ b/drivers/hid/i2c-hid/i2c-hid.c 2017-12-17 23:38:04.405071925 +0300
@@ -61,6 +61,12 @@ static bool debug;
module_param(debug, bool, 0444);
MODULE_PARM_DESC(debug, "print a lot of debug information");
+/* default post power on delay */
+/* for some of strange devices or i2c like WCOM5020 even 1000 is enough */
+static int post_power_delay_ms_default;
+module_param(post_power_delay_ms_default, int, 0444);
+MODULE_PARM_DESC(post_power_delay_ms_default, "default post power-on delay");
+
#define i2c_hid_dbg(ihid, fmt, arg...) \
do { \
if (debug) \
@@ -382,7 +388,24 @@ static int i2c_hid_set_or_send_report(st
return data_len;
}
-static int i2c_hid_set_power(struct i2c_client *client, int power_state)
+/* Agnostic calling twice: after regulator "power on" & hid cmd "power on" */
+static inline void i2c_hid_power_delay(struct i2c_hid *ihid)
+{
+ /*
+ * The HID over I2C specification states that if a DEVICE needs time
+ * after the PWR_ON request, it should utilise CLOCK stretching.
+ * However, it has been observered that the Windows driver provides a
+ * 1ms sleep between the PWR_ON and RESET requests and that some devices
+ * rely on this.
+ */
+ if (ihid->pdata.post_power_delay_ms)
+ msleep(ihid->pdata.post_power_delay_ms);
+ else
+ usleep_range(1000, 5000);
+
+}
+
+static int __i2c_hid_set_power(struct i2c_client *client, int power_state)
{
struct i2c_hid *ihid = i2c_get_clientdata(client);
int ret;
@@ -410,6 +433,23 @@ static int i2c_hid_set_power(struct i2c_
dev_err(&client->dev, "failed to change power setting.\n");
set_pwr_exit:
+ if (!ret && power_state == I2C_HID_PWR_ON)
+ i2c_hid_power_delay(ihid);
+ return ret;
+}
+
+static int i2c_hid_set_power(struct i2c_client *client, int power_state)
+{
+ struct i2c_hid *ihid = i2c_get_clientdata(client);
+ int ret;
+
+ /*
+ * Can be called from various places, including PM calls,
+ * strict behaviour & delay.
+ */
+ mutex_lock(&ihid->reset_lock);
+ ret=__i2c_hid_set_power(client,power_state);
+ mutex_unlock(&ihid->reset_lock);
return ret;
}
@@ -427,25 +467,17 @@ static int i2c_hid_hwreset(struct i2c_cl
*/
mutex_lock(&ihid->reset_lock);
- ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
+ ret = __i2c_hid_set_power(client, I2C_HID_PWR_ON);
if (ret)
goto out_unlock;
- /*
- * The HID over I2C specification states that if a DEVICE needs time
- * after the PWR_ON request, it should utilise CLOCK stretching.
- * However, it has been observered that the Windows driver provides a
- * 1ms sleep between the PWR_ON and RESET requests and that some devices
- * rely on this.
- */
- usleep_range(1000, 5000);
i2c_hid_dbg(ihid, "resetting...\n");
ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
if (ret) {
dev_err(&client->dev, "failed to reset device.\n");
- i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
+ __i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
}
out_unlock:
@@ -988,6 +1020,8 @@ static int i2c_hid_probe(struct i2c_clie
if (!ihid)
return -ENOMEM;
+ ihid->pdata.post_power_delay_ms = post_power_delay_ms_default;
+
if (client->dev.of_node) {
ret = i2c_hid_of_probe(client, &ihid->pdata);
if (ret)
@@ -1021,8 +1055,7 @@ static int i2c_hid_probe(struct i2c_clie
ret);
goto err;
}
- if (ihid->pdata.post_power_delay_ms)
- msleep(ihid->pdata.post_power_delay_ms);
+ i2c_hid_power_delay(ihid);
i2c_set_clientdata(client, ihid);
@@ -1198,8 +1231,7 @@ static int i2c_hid_resume(struct device
ret = regulator_enable(ihid->pdata.supply);
if (ret < 0)
hid_warn(hid, "Failed to enable supply: %d\n", ret);
- if (ihid->pdata.post_power_delay_ms)
- msleep(ihid->pdata.post_power_delay_ms);
+ i2c_hid_power_delay(ihid);
} else if (ihid->irq_wake_enabled) {
wake_status = disable_irq_wake(client->irq);
if (!wake_status)
--
WBR, Dzianis Kahanovich AKA Denis Kaganovich, http://mahatma.bspu.by/
Powered by blists - more mailing lists