Re: [PATCH] HID: i2c-hid: Prevent sending reports from racing with device reset

[Nish Aravamudan]

On Mon, Dec 21, 2015 at 5:26 AM, Mika Westerberg
 wrote:

> Below is what happens to touchpad on Lenovo Yoga 900 during resume from
> system sleep:



> Reported-by: Nish Aravamudan 
> Reported-by: Linus Torvalds 
> Suggested-by: Benjamin Tissoires 
> Signed-off-by: Mika Westerberg 

Probably not necessary, but this can be updated to

Reported-and-tested-by: Nish Aravamudan 

-Nish
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] HID: i2c-hid: Prevent sending reports from racing with device reset

[Jiri Kosina]

On Mon, 21 Dec 2015, Mika Westerberg wrote:

> When an i2c-hid device is resumed from system sleep the driver resets
> the device to be sure it is in known state. The device is expected to
> issue an interrupt when reset is complete.
[ ... snip ... ]
> Prevent sending of feature/output reports while the device is being
> reset by adding a mutex which is held during that time.

Thanks Mika. Applied to for-4.4/upstream-fixes.

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] HID: i2c-hid: Prevent sending reports from racing with device reset

[Linus Torvalds]

On Mon, Dec 21, 2015 at 5:26 AM, Mika Westerberg
 wrote:
>
> Below is what happens to touchpad on Lenovo Yoga 900 during resume from
> system sleep:

Ok, since my daughter is home for xmas, I can report that it seems to
indeed fix the problem on her Yoga 900 too.

  Reported-and-tested-by: Linus Torvalds 

Thanks,

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] HID: i2c-hid: Prevent sending reports from racing with device reset

[Benjamin Tissoires]

On Dec 21 2015 or thereabouts, Mika Westerberg wrote:
> When an i2c-hid device is resumed from system sleep the driver resets
> the device to be sure it is in known state. The device is expected to
> issue an interrupt when reset is complete.
> 
> This reset might take few milliseconds to complete so if the HID driver
> on top (hid-rmi) starts to set up the device by sending feature reports
> etc. the device might not issue the reset complete interrupt anymore.
> 
> Below is what happens to touchpad on Lenovo Yoga 900 during resume from
> system sleep:
> 
>   [   24.790951] i2c_hid i2c-SYNA2B29:00: i2c_hid_hwreset
>   [   24.790973] i2c_hid i2c-SYNA2B29:00: i2c_hid_set_power
>   [   24.790982] i2c_hid i2c-SYNA2B29:00: __i2c_hid_command: cmd=22 00 00 08
>   [   24.793011] i2c_hid i2c-SYNA2B29:00: resetting...
>   [   24.793016] i2c_hid i2c-SYNA2B29:00: __i2c_hid_command: cmd=22 00 00 01
> 
> Here i2c-hid sends reset command to the touchpad.
> 
>   [   24.794012] i2c_hid i2c-SYNA2B29:00: input: 06 00 01 00 00 00
>   [   24.794051] i2c_hid i2c-SYNA2B29:00: i2c_hid_set_or_send_report
>   [   24.794059] i2c_hid i2c-SYNA2B29:00: __i2c_hid_command:
>  cmd=22 00 3f 03 0f 23 00 04 00 0f 01
> 
> Now hid-rmi puts the touchpad to correct mode by sending it a feature
> report. This makes the touchpad not to issue reset complete interrupt.
> 
>   [   24.796092] i2c_hid i2c-SYNA2B29:00: __i2c_hid_command: waiting...
> 
> i2c-hid starts to wait for the reset interrupt to trigger which never
> happens.
> 
>   [   24.798304] i2c_hid i2c-SYNA2B29:00: i2c_hid_set_or_send_report
>   [   24.798313] i2c_hid i2c-SYNA2B29:00: __i2c_hid_command:
>  cmd=25 00 17 00 09 01 42 00 2e 00 19 19 00 10 cc 06 74 04 0f
>  19 00 00 00 00 00
> 
> Yet another output report from hid-rmi driver.
> 
>   [   29.795630] i2c_hid i2c-SYNA2B29:00: __i2c_hid_command: finished.
>   [   29.795637] i2c_hid i2c-SYNA2B29:00: failed to reset device.
> 
> After 5 seconds i2c-hid driver times out.
> 
>   [   29.795642] i2c_hid i2c-SYNA2B29:00: i2c_hid_set_power
>   [   29.795649] i2c_hid i2c-SYNA2B29:00: __i2c_hid_command: cmd=22 00 01 08
>   [   29.797576] dpm_run_callback(): i2c_hid_resume+0x0/0xb0 returns -61
>   [   29.797584] PM: Device i2c-SYNA2B29:00 failed to resume: error -61
> 
> After this the touchpad does not work anymore (and also resume itself
> gets slowed down because of the timeout).
> 
> Prevent sending of feature/output reports while the device is being
> reset by adding a mutex which is held during that time.
> 
> Reported-by: Nish Aravamudan 
> Reported-by: Linus Torvalds 
> Suggested-by: Benjamin Tissoires 
> Signed-off-by: Mika Westerberg 
> ---

Looks good to me.

Reviewed-by: Benjamin Tissoires 

Thanks for the patch Mika!

Cheers,
Benjamin

>  drivers/hid/i2c-hid/i2c-hid.c | 21 ++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 10bd8e6e4c9c..fc5ef1c25ed4 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -151,6 +151,7 @@ struct i2c_hid {
>   struct i2c_hid_platform_data pdata;
>  
>   boolirq_wake_enabled;
> + struct mutexreset_lock;
>  };
>  
>  static int __i2c_hid_command(struct i2c_client *client,
> @@ -356,9 +357,16 @@ static int i2c_hid_hwreset(struct i2c_client *client)
>  
>   i2c_hid_dbg(ihid, "%s\n", __func__);
>  
> + /*
> +  * This prevents sending feature reports while the device is
> +  * being reset. Otherwise we may lose the reset complete
> +  * interrupt.
> +  */
> + mutex_lock(>reset_lock);
> +
>   ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
>   if (ret)
> - return ret;
> + goto out_unlock;
>  
>   i2c_hid_dbg(ihid, "resetting...\n");
>  
> @@ -366,10 +374,11 @@ static int i2c_hid_hwreset(struct i2c_client *client)
>   if (ret) {
>   dev_err(>dev, "failed to reset device.\n");
>   i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
> - return ret;
>   }
>  
> - return 0;
> +out_unlock:
> + mutex_unlock(>reset_lock);
> + return ret;
>  }
>  
>  static void i2c_hid_get_input(struct i2c_hid *ihid)
> @@ -587,12 +596,15 @@ static int i2c_hid_output_raw_report(struct hid_device 
> *hid, __u8 *buf,
>   size_t count, unsigned char report_type, bool use_data)
>  {
>   struct i2c_client *client = hid->driver_data;
> + struct i2c_hid *ihid = i2c_get_clientdata(client);
>   int report_id = buf[0];
>   int ret;
>  
>   if (report_type == HID_INPUT_REPORT)
>   return -EINVAL;
>  
> + mutex_lock(>reset_lock);
> +
>   if (report_id) {
>   buf++;
>