psmouse: touchpad doesn't reconnect after resume: Synaptics ps2 Bug: 551234

Hi All,

This is my first stab at correcting some PS/2 misbehavior when returning
from S3. Fire away :). Thanks.

Peter

P.S. Collaborated with Colin King on this one.

Peter M. Petrakis wrote:
> Hi All,
> 
> This is my first stab at correcting some PS/2 misbehavior when returning
> from S3. Fire away :). Thanks.
> 
> Peter
> 
> P.S. Collaborated with Colin King on this one.
> 
Hm, just a question. Have you tried using "i8042.reset=1" in the boot command
line while getting it to work(without your patch applied)? Knowing Coling likely
yes. But to be sure here...

Stefan

Stefan,

I think we tried that, but I retested again just to be sure
with a live image and it still fails with that option. Also,
removing and reinserting the psmouse mod doesn't make a
difference.

Peter

On 03/30/2010 05:46 AM, Stefan Bader wrote:
> Peter M. Petrakis wrote:
>> Hi All,
>>
>> This is my first stab at correcting some PS/2 misbehavior when returning
>> from S3. Fire away :). Thanks.
>>
>> Peter
>>
>> P.S. Collaborated with Colin King on this one.
>>
> Hm, just a question. Have you tried using "i8042.reset=1" in the boot command
> line while getting it to work(without your patch applied)? Knowing Coling likely
> yes. But to be sure here...
>
> Stefan

Peter M. Petrakis wrote:
> Stefan,
> 
> I think we tried that, but I retested again just to be sure
> with a live image and it still fails with that option. Also,
> removing and reinserting the psmouse mod doesn't make a
> difference.
> 
> Peter
> 
> On 03/30/2010 05:46 AM, Stefan Bader wrote:
>> Peter M. Petrakis wrote:
>>> Hi All,
>>>
>>> This is my first stab at correcting some PS/2 misbehavior when returning
>>> from S3. Fire away :). Thanks.
>>>
>>> Peter
>>>
>>> P.S. Collaborated with Colin King on this one.
>>>
>> Hm, just a question. Have you tried using "i8042.reset=1" in the boot command
>> line while getting it to work(without your patch applied)? Knowing Coling likely
>> yes. But to be sure here...
>>
>> Stefan
> 

Ok, sorry for the delayed response. First off, I assume this is requested for
Lucid, right?
Also such changes should rather go upstream, to be discussed there and then
could be accepted back. And for that it seems like we want code like that only
applied / executed on specific hardware. Which means it would need some sort of
quirk table code to only be used on that hardware.

Stefan

On Mon, Mar 29, 2010 at 05:18:35PM -0400, Peter M. Petrakis wrote:
> Hi All,
> 
> This is my first stab at correcting some PS/2 misbehavior when returning
> from S3. Fire away :). Thanks.
> 
> Peter
> 
> P.S. Collaborated with Colin King on this one.

> From 006e55bacfe64f5fe6b8c12af071f7b2bf528c3a Mon Sep 17 00:00:00 2001
> From: Peter M. Petrakis <peter.petrakis@canonical.com>
> Date: Fri, 26 Mar 2010 17:23:43 -0400
> Subject: [PATCH] Perform a reset should psmouse_probe fail for any reason. This is necessary for some PS/2 devices who can't even be identified when returning from sleep (S3).
> 
> ---
>  psmouse-base.c |   28 +++++++++++++++++++++++++---
>  1 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/psmouse-base.c b/psmouse-base.c
> index b407b35..3b8e63c 100644
> --- a/psmouse-base.c
> +++ b/psmouse-base.c
> @@ -856,12 +856,12 @@ static const struct psmouse_protocol *psmouse_protocol_by_name(const char *name,
>  	return NULL;
>  }
>  
> -
>  /*
> - * psmouse_probe() probes for a PS/2 mouse.
> + * __psmouse_probe() probes for a PS/2 mouse.
> + * Wrapped by psmouse_probe() for clean reset code.
>   */
>  
> -static int psmouse_probe(struct psmouse *psmouse)
> +static int __psmouse_probe(struct psmouse *psmouse)
>  {
>  	struct ps2dev *ps2dev = &psmouse->ps2dev;
>  	unsigned char param[2];
> @@ -892,6 +892,28 @@ static int psmouse_probe(struct psmouse *psmouse)
>  }
>  
>  /*
> + * Wrapper for probe routine to cleanly reset the device should
> + * the initial probe fail for any reason.
> + */
> +static int psmouse_probe(struct psmouse *psmouse) {
> +	struct ps2dev *ps2dev = &psmouse->ps2dev;
> +	int tries = 3;
> +	int ret = -1;
> +
> +retry:
> +	if (tries > 0 && ((ret = __psmouse_probe(psmouse)) != 0)) {
> +		printk(KERN_ERR "psmouse.c: Error encountered while probing PS/2 device on %s "
> +			"reseting...\n", ps2dev->serio->phys);
> +		psmouse_reset(psmouse);
> +		msleep(500);
> +		tries--;
> +		goto retry;
> +	}

How does this behave if the machine does not have a psmouse on this
port?  Do we still call here?  If so that sounds like it might add 1.5s
to boot/resume?

Is 500ms a number picked out of the air or does it have some basis in an
errata?  It seems like a long time in the real world.

Also what machines have this issue, what devices?  Can we quirk this on
only for the machines which are broken? 

Finally, what releases is this aimed at?

> +
> +	return ret;
> +}
> +
> +/*
>   * Here we set the mouse resolution.
>   */

-apw

On Thu, Apr 1, 2010 at 5:24 AM, Andy Whitcroft <apw@canonical.com> wrote:
> On Mon, Mar 29, 2010 at 05:18:35PM -0400, Peter M. Petrakis wrote:
>> Hi All,
>>
>> This is my first stab at correcting some PS/2 misbehavior when returning
>> from S3. Fire away :). Thanks.
>>
>> Peter
>>
>> P.S. Collaborated with Colin King on this one.
>
>> From 006e55bacfe64f5fe6b8c12af071f7b2bf528c3a Mon Sep 17 00:00:00 2001
>> From: Peter M. Petrakis <peter.petrakis@canonical.com>
>> Date: Fri, 26 Mar 2010 17:23:43 -0400
>> Subject: [PATCH] Perform a reset should psmouse_probe fail for any reason. This is necessary for some PS/2 devices who can't even be identified when returning from sleep (S3).
>>
>> ---
>>  psmouse-base.c |   28 +++++++++++++++++++++++++---
>>  1 files changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/psmouse-base.c b/psmouse-base.c
>> index b407b35..3b8e63c 100644
>> --- a/psmouse-base.c
>> +++ b/psmouse-base.c
>> @@ -856,12 +856,12 @@ static const struct psmouse_protocol *psmouse_protocol_by_name(const char *name,
>>       return NULL;
>>  }
>>
>> -
>>  /*
>> - * psmouse_probe() probes for a PS/2 mouse.
>> + * __psmouse_probe() probes for a PS/2 mouse.
>> + * Wrapped by psmouse_probe() for clean reset code.
>>   */
>>
>> -static int psmouse_probe(struct psmouse *psmouse)
>> +static int __psmouse_probe(struct psmouse *psmouse)
>>  {
>>       struct ps2dev *ps2dev = &psmouse->ps2dev;
>>       unsigned char param[2];
>> @@ -892,6 +892,28 @@ static int psmouse_probe(struct psmouse *psmouse)
>>  }
>>
>>  /*
>> + * Wrapper for probe routine to cleanly reset the device should
>> + * the initial probe fail for any reason.
>> + */
>> +static int psmouse_probe(struct psmouse *psmouse) {
>> +     struct ps2dev *ps2dev = &psmouse->ps2dev;
>> +     int tries = 3;
>> +     int ret = -1;
>> +
>> +retry:
>> +     if (tries > 0 && ((ret = __psmouse_probe(psmouse)) != 0)) {
>> +             printk(KERN_ERR "psmouse.c: Error encountered while probing PS/2 device on %s "
>> +                     "reseting...\n", ps2dev->serio->phys);
>> +             psmouse_reset(psmouse);
>> +             msleep(500);
>> +             tries--;
>> +             goto retry;
>> +     }

Stylistically, I think this is better off as a while loop instead of a
loop created by using goto.

-- Chase

On 04/01/2010 05:20 AM, Stefan Bader wrote:
> Peter M. Petrakis wrote:
>> Stefan,
>>
>> I think we tried that, but I retested again just to be sure
>> with a live image and it still fails with that option. Also,
>> removing and reinserting the psmouse mod doesn't make a
>> difference.
>>
>> Peter
>>
>> On 03/30/2010 05:46 AM, Stefan Bader wrote:
>>> Peter M. Petrakis wrote:
>>>> Hi All,
>>>>
>>>> This is my first stab at correcting some PS/2 misbehavior when returning
>>>> from S3. Fire away :). Thanks.
>>>>
>>>> Peter
>>>>
>>>> P.S. Collaborated with Colin King on this one.
>>>>
>>> Hm, just a question. Have you tried using "i8042.reset=1" in the boot command
>>> line while getting it to work(without your patch applied)? Knowing Coling likely
>>> yes. But to be sure here...
>>>
>>> Stefan
>>
>
> Ok, sorry for the delayed response. First off, I assume this is requested for
> Lucid, right?
> Also such changes should rather go upstream, to be discussed there and then
> could be accepted back. And for that it seems like we want code like that only
> applied / executed on specific hardware. Which means it would need some sort of
> quirk table code to only be used on that hardware.
> Stefan

Yes, we'd like to target lucid. With regards to a quirks table, we can't 
even identify what's on the other side of this port so a quirks
table won't help us here. This is more of a general discovery hardening
fix that just happens to manifest itself with this synaptics touchpad.

      0 kseriod(41): -> ps2_init
ps2_init args [ps2dev=0xf006de08 serio=0xf6d36200 ]
exit 17 kseriod(41): -> ps2_init
ps2_init args []
      0 kseriod(41): -> ps2_command
ps2_command args [ps2dev=0xf006de08 param=0xf75bbeaa command=0x2f2 ]
(ps2_command) libps2.c:172
(ps2_command) libps2.c:177
(ps2_command) libps2.c:182
(ps2_command) libps2.c:197
     42 kseriod(41): -> ps2_sendbyte
ps2_sendbyte args [ps2dev=0xf006de08 byte=0xf2 timeout=0xc8 ]
exit 200048 kseriod(41): -> ps2_sendbyte
ps2_sendbyte args [return=0xffffffffffffffff ]
(ps2_command) libps2.c:224
exit 200070 kseriod(41): -> ps2_command
ps2_command args [return=0xffffffffffffffff ]

According to the comments in psmouse-base.c:

psmouse_probe proper:

/*
  * First, we check if it's a mouse. It should send 0x00 or 0x03
  * in case of an IntelliMouse in 4-byte mode or 0x04 for IM Explorer.
  * Sunrex K8561 IR Keyboard/Mouse reports 0xff on second and subsequent
  * ID queries, probably due to a firmware bug.
  */

         param[0] = 0xa5;
         if (ps2_command(ps2dev, param, PSMOUSE_CMD_GETID))
                 return -1;

and we're getting ff back. Ideally, I'd like to keep a list of all
active serial devices and simply walk that list with device specific
reconnects after resume. The current implementation seems to forget
completely about what's connected and tries to treat it like a fresh
device.

Peter

On 04/01/2010 05:24 AM, Andy Whitcroft wrote:
> On Mon, Mar 29, 2010 at 05:18:35PM -0400, Peter M. Petrakis wrote:
>> Hi All,
>>
>> This is my first stab at correcting some PS/2 misbehavior when returning
>> from S3. Fire away :). Thanks.
>>
>> Peter
>>
>> P.S. Collaborated with Colin King on this one.
>
>>  From 006e55bacfe64f5fe6b8c12af071f7b2bf528c3a Mon Sep 17 00:00:00 2001
>> From: Peter M. Petrakis<peter.petrakis@canonical.com>
>> Date: Fri, 26 Mar 2010 17:23:43 -0400
>> Subject: [PATCH] Perform a reset should psmouse_probe fail for any reason. This is necessary for some PS/2 devices who can't even be identified when returning from sleep (S3).
>>
>> ---
>>   psmouse-base.c |   28 +++++++++++++++++++++++++---
>>   1 files changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/psmouse-base.c b/psmouse-base.c
>> index b407b35..3b8e63c 100644
>> --- a/psmouse-base.c
>> +++ b/psmouse-base.c
>> @@ -856,12 +856,12 @@ static const struct psmouse_protocol *psmouse_protocol_by_name(const char *name,
>>   	return NULL;
>>   }
>>
>> -
>>   /*
>> - * psmouse_probe() probes for a PS/2 mouse.
>> + * __psmouse_probe() probes for a PS/2 mouse.
>> + * Wrapped by psmouse_probe() for clean reset code.
>>    */
>>
>> -static int psmouse_probe(struct psmouse *psmouse)
>> +static int __psmouse_probe(struct psmouse *psmouse)
>>   {
>>   	struct ps2dev *ps2dev =&psmouse->ps2dev;
>>   	unsigned char param[2];
>> @@ -892,6 +892,28 @@ static int psmouse_probe(struct psmouse *psmouse)
>>   }
>>
>>   /*
>> + * Wrapper for probe routine to cleanly reset the device should
>> + * the initial probe fail for any reason.
>> + */
>> +static int psmouse_probe(struct psmouse *psmouse) {
>> +	struct ps2dev *ps2dev =&psmouse->ps2dev;
>> +	int tries = 3;
>> +	int ret = -1;
>> +
>> +retry:
>> +	if (tries>  0&&  ((ret = __psmouse_probe(psmouse)) != 0)) {
>> +		printk(KERN_ERR "psmouse.c: Error encountered while probing PS/2 device on %s "
>> +			"reseting...\n", ps2dev->serio->phys);
>> +		psmouse_reset(psmouse);
>> +		msleep(500);
>> +		tries--;
>> +		goto retry;
>> +	}


> How does this behave if the machine does not have a psmouse on this
> port?  Do we still call here?  If so that sounds like it might add 1.5s
> to boot/resume?

Didn't test that and good point. There are some other sleeps in the 
mouse code and their sum including retries doesn't seem to exceed 1s. 
I'll retest and see if I can do without it. I was hoping someone on
the list would be able to point a better value for settling time
after issuing the reset.

> Is 500ms a number picked out of the air or does it have some basis in an
> errata?  It seems like a long time in the real world.

Plucked out of the air indeed :), The longest sleep I could find in this
could was 200ms per try, I'll start there and ratchet down.

> Also what machines have this issue, what devices?  Can we quirk this on
> only for the machines which are broken?

Specific Dell laptops with Synaptics touchpads. We  could quirk this per 
platform... where would that live? dell_laptop?

> Finally, what releases is this aimed at?

lucid, though if it could be pulled into the next SRU for karmic that 
would be good too.

Peter

>> +
>> +	return ret;
>> +}
>> +
>> +/*
>>    * Here we set the mouse resolution.
>>    */
>
> -apw

On 04/01/2010 08:38 AM, Chase Douglas wrote:
> On Thu, Apr 1, 2010 at 5:24 AM, Andy Whitcroft<apw@canonical.com>  wrote:
>> On Mon, Mar 29, 2010 at 05:18:35PM -0400, Peter M. Petrakis wrote:
>>> Hi All,
>>>
>>> This is my first stab at correcting some PS/2 misbehavior when returning
>>> from S3. Fire away :). Thanks.
>>>
>>> Peter
>>>
>>> P.S. Collaborated with Colin King on this one.
>>
>>>  From 006e55bacfe64f5fe6b8c12af071f7b2bf528c3a Mon Sep 17 00:00:00 2001
>>> From: Peter M. Petrakis<peter.petrakis@canonical.com>
>>> Date: Fri, 26 Mar 2010 17:23:43 -0400
>>> Subject: [PATCH] Perform a reset should psmouse_probe fail for any reason. This is necessary for some PS/2 devices who can't even be identified when returning from sleep (S3).
>>>
>>> ---
>>>   psmouse-base.c |   28 +++++++++++++++++++++++++---
>>>   1 files changed, 25 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/psmouse-base.c b/psmouse-base.c
>>> index b407b35..3b8e63c 100644
>>> --- a/psmouse-base.c
>>> +++ b/psmouse-base.c
>>> @@ -856,12 +856,12 @@ static const struct psmouse_protocol *psmouse_protocol_by_name(const char *name,
>>>        return NULL;
>>>   }
>>>
>>> -
>>>   /*
>>> - * psmouse_probe() probes for a PS/2 mouse.
>>> + * __psmouse_probe() probes for a PS/2 mouse.
>>> + * Wrapped by psmouse_probe() for clean reset code.
>>>    */
>>>
>>> -static int psmouse_probe(struct psmouse *psmouse)
>>> +static int __psmouse_probe(struct psmouse *psmouse)
>>>   {
>>>        struct ps2dev *ps2dev =&psmouse->ps2dev;
>>>        unsigned char param[2];
>>> @@ -892,6 +892,28 @@ static int psmouse_probe(struct psmouse *psmouse)
>>>   }
>>>
>>>   /*
>>> + * Wrapper for probe routine to cleanly reset the device should
>>> + * the initial probe fail for any reason.
>>> + */
>>> +static int psmouse_probe(struct psmouse *psmouse) {
>>> +     struct ps2dev *ps2dev =&psmouse->ps2dev;
>>> +     int tries = 3;
>>> +     int ret = -1;
>>> +
>>> +retry:
>>> +     if (tries>  0&&  ((ret = __psmouse_probe(psmouse)) != 0)) {
>>> +             printk(KERN_ERR "psmouse.c: Error encountered while probing PS/2 device on %s "
>>> +                     "reseting...\n", ps2dev->serio->phys);
>>> +             psmouse_reset(psmouse);
>>> +             msleep(500);
>>> +             tries--;
>>> +             goto retry;
>>> +     }
>
> Stylistically, I think this is better off as a while loop instead of a
> loop created by using goto.

Sorry about that, it started as goto when it was hacked into 
psmouse_probe to begin with. Fixed.

> -- Chase

Peter M. Petrakis wrote:
> Hi All,
> 
> This is my first stab at correcting some PS/2 misbehavior when returning
> from S3. Fire away :). Thanks.
> 
> Peter
> 
> P.S. Collaborated with Colin King on this one.
> 

Just to check on this. I believe we agreed that this should go via upstream
because of the potential to miss something and regress. Peter, did you work on
this and what it the current status there?

Stefan

Still working this upstream, actually have another patch
to test today. The linux-input maintainers have been very
responsive.

Peter

On 04/30/2010 05:49 AM, Stefan Bader wrote:
> Peter M. Petrakis wrote:
>> Hi All,
>>
>> This is my first stab at correcting some PS/2 misbehavior when returning
>> from S3. Fire away :). Thanks.
>>
>> Peter
>>
>> P.S. Collaborated with Colin King on this one.
>>
> 
> Just to check on this. I believe we agreed that this should go via upstream
> because of the potential to miss something and regress. Peter, did you work on
> this and what it the current status there?
> 
> Stefan