Message ID | 4BB1192B.8030109@canonical.com |
---|---|
State | Superseded |
Delegated to: | Andy Whitcroft |
Headers | show |
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
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; + } + + return ret; +} + +/* * Here we set the mouse resolution. */ -- 1.6.3.3