linux-kernel - [PATCH] hid: i2c-hid: Unify invariant post-power-on delay

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <795e3db4-9d25-8619-53b2-8a66077d4a16@bspu.by>
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/