[PATCH?] psmouse-base.c

[PATCH?] psmouse-base.c

[arief_mulya]

Dear Mr. Vojtech,


I learn that you are the author for PS/2 mouse driver on Linux 2.6.x 
kernel.

I just want to share a little change that  I've did to 
psmouse_pm_callback() which without this, my synaptics touchpad would 
prevent my laptop (IBM Thinkpad T30) from suspending.

I don't remember how the functions read at first. But this is how it 
become. This is not exactly a patch, huh? Sorry, but I hope it helps.

I also not sure if this patch already done the right thing and I haven't 
tested it on other machine. It works for me, but I can't promise if it 
blows up others. Maybe you could give it a test?

The kernel version was 2.6.0-test9 from Debian (not vanilla, but I'm 
sure the psmouse part is clean).

This is the function:

static int psmouse_pm_callback(struct pm_dev *dev, pm_request_t request, 
void *data)
{
        struct psmouse *psmouse = dev->data;
        struct serio_dev *ser_dev = psmouse->serio->dev;
                                                                               
 
        switch (request) {
        case PM_RESUME:
                psmouse->state = PSMOUSE_IGNORE;
                serio_rescan(psmouse->serio);
        default:
                return 0;
        }
}

Sorry if this just bothers you.
I'm cc-ing also to [linux-kernel], but I'm not subscribed to the list yet.


Best Regards,
-- 
arief_mulya <http://www.geocities.com/naida_s_rasha/>
Peace is beautiful.

Re: [PATCH?] psmouse-base.c

[Andrew Morton]

arief_mulya <arief_m_utama@telkomsel.co.id> wrote:
>
> static int psmouse_pm_callback(struct pm_dev *dev, pm_request_t request, 
>  void *data)
>  {
>          struct psmouse *psmouse = dev->data;
>          struct serio_dev *ser_dev = psmouse->serio->dev;
>                                                                                 
>   
>          switch (request) {
>          case PM_RESUME:
>                  psmouse->state = PSMOUSE_IGNORE;
>                  serio_rescan(psmouse->serio);
>          default:
>                  return 0;
>          }
>  }

What does the driver do without this change?  ie: what problem is this
fixing?

Why is it calling serio_rescan() rather than serio_reconnect()?

Re: [PATCH?] psmouse-base.c

[Dmitry Torokhov]

On Sunday 09 November 2003 11:12 pm, Andrew Morton wrote:
> arief_mulya <arief_m_utama@telkomsel.co.id> wrote:
> > static int psmouse_pm_callback(struct pm_dev *dev, pm_request_t
> > request, void *data)
> >  {
> >          struct psmouse *psmouse = dev->data;
> >          struct serio_dev *ser_dev = psmouse->serio->dev;
> >
> >
> >          switch (request) {
> >          case PM_RESUME:
> >                  psmouse->state = PSMOUSE_IGNORE;
> >                  serio_rescan(psmouse->serio);
> >          default:
> >                  return 0;
> >          }
> >  }
>
> What does the driver do without this change?  ie: what problem is this
> fixing?
>
> Why is it calling serio_rescan() rather than serio_reconnect()?
>

serio_reconnect() is only in your tree (-mm), it has not been pushed to
Linus yet... Unfortunately using rescan can cause input devices be shifted
if some program has them open while suspending.

Dmitry

Re: [PATCH?] psmouse-base.c

[Andrew Morton]

Dmitry Torokhov <dtor_core@ameritech.net> wrote:
>
> On Sunday 09 November 2003 11:12 pm, Andrew Morton wrote:
> > arief_mulya <arief_m_utama@telkomsel.co.id> wrote:
> > > static int psmouse_pm_callback(struct pm_dev *dev, pm_request_t
> > > request, void *data)
> > >  {
> > >          struct psmouse *psmouse = dev->data;
> > >          struct serio_dev *ser_dev = psmouse->serio->dev;
> > >
> > >
> > >          switch (request) {
> > >          case PM_RESUME:
> > >                  psmouse->state = PSMOUSE_IGNORE;
> > >                  serio_rescan(psmouse->serio);
> > >          default:
> > >                  return 0;
> > >          }
> > >  }
> >
> > What does the driver do without this change?  ie: what problem is this
> > fixing?
> >
> > Why is it calling serio_rescan() rather than serio_reconnect()?
> >
> 
> serio_reconnect() is only in your tree (-mm), it has not been pushed to
> Linus yet... Unfortunately using rescan can cause input devices be shifted
> if some program has them open while suspending.

Ah, I see.  So would you say that reconnect is the correct thing to use
here?

That would mean that the appropriate patch against -mm is

--- 25/drivers/input/mouse/psmouse-base.c~serio-pm-fix	2003-11-09 20:12:27.000000000 -0800
+++ 25-akpm/drivers/input/mouse/psmouse-base.c	2003-11-09 20:12:27.000000000 -0800
@@ -533,9 +533,10 @@ static int psmouse_pm_callback(struct pm
 {
 	struct psmouse *psmouse = dev->data;
 
-	psmouse->state = PSMOUSE_IGNORE;
-	serio_reconnect(psmouse->serio);
-
+	if (request == PM_RESUME) {
+		psmouse->state = PSMOUSE_IGNORE;
+		serio_reconnect(psmouse->serio);
+	}
 	return 0;
 }
 

_


Those serio patches have been in -mm for six weeks btw.  Was there some
problem with them?

Re: [PATCH?] psmouse-base.c

[arief_mulya]

Andrew Morton wrote:

>Dmitry Torokhov <dtor_core@ameritech.net> wrote:
>  
>
>>On Sunday 09 November 2003 11:12 pm, Andrew Morton wrote:
>>    
>>
>>>arief_mulya <arief_m_utama@telkomsel.co.id> wrote:
>>>      
>>>
>>>>static int psmouse_pm_callback(struct pm_dev *dev, pm_request_t
>>>>request, void *data)
>>>> {
>>>>         struct psmouse *psmouse = dev->data;
>>>>         struct serio_dev *ser_dev = psmouse->serio->dev;
>>>>
>>>>
>>>>         switch (request) {
>>>>         case PM_RESUME:
>>>>                 psmouse->state = PSMOUSE_IGNORE;
>>>>                 serio_rescan(psmouse->serio);
>>>>         default:
>>>>                 return 0;
>>>>         }
>>>> }
>>>>        
>>>>
>>>What does the driver do without this change?  ie: what problem is this
>>>fixing?
>>>
>>>Why is it calling serio_rescan() rather than serio_reconnect()?
>>>
>>>      
>>>
>>serio_reconnect() is only in your tree (-mm), it has not been pushed to
>>Linus yet... Unfortunately using rescan can cause input devices be shifted
>>if some program has them open while suspending.
>>    
>>
>
>  
>
Well, I don't know about this.
I was just trying to get my synaptics mouse working nicely through 
suspend-resume. And I haven't learn about linux kernel structure before, 
so I just spend all my sunshiny Sunday, researching everywhere with my 
lazy brain.

And finally, those (ugly?) patch come out ;-)

>Ah, I see.  So would you say that reconnect is the correct thing to use
>here?
>
>That would mean that the appropriate patch against -mm is
>
>--- 25/drivers/input/mouse/psmouse-base.c~serio-pm-fix	2003-11-09 20:12:27.000000000 -0800
>+++ 25-akpm/drivers/input/mouse/psmouse-base.c	2003-11-09 20:12:27.000000000 -0800
>@@ -533,9 +533,10 @@ static int psmouse_pm_callback(struct pm
> {
> 	struct psmouse *psmouse = dev->data;
> 
>-	psmouse->state = PSMOUSE_IGNORE;
>-	serio_reconnect(psmouse->serio);
>-
>+	if (request == PM_RESUME) {
>+		psmouse->state = PSMOUSE_IGNORE;
>+		serio_reconnect(psmouse->serio);
>+	}
> 	return 0;
> }
> 
>
>_
>
>  
>
I guess I gotta try your tree now.

>Those serio patches have been in -mm for six weeks btw.  Was there some
>problem with them?
>  
>
I don't know. Has anybody tested it using an IBM T30 with Synaptics inside?

Best Regards.
-- 
arief_mulya

Re: [PATCH?] psmouse-base.c

[Dmitry Torokhov]

On Monday 10 November 2003 01:56 am, Andrew Morton wrote:
> Dmitry Torokhov <dtor_core@ameritech.net> wrote:

> >
> > serio_reconnect() is only in your tree (-mm), it has not been pushed
> > to Linus yet... Unfortunately using rescan can cause input devices be
> > shifted if some program has them open while suspending.
>
> Ah, I see.  So would you say that reconnect is the correct thing to use
> here?
>
> That would mean that the appropriate patch against -mm is
>
> --- 25/drivers/input/mouse/psmouse-base.c~serio-pm-fix	2003-11-09
> 20:12:27.000000000 -0800 +++
> 25-akpm/drivers/input/mouse/psmouse-base.c	2003-11-09
> 20:12:27.000000000 -0800 @@ -533,9 +533,10 @@ static int
> psmouse_pm_callback(struct pm
>  {
>  	struct psmouse *psmouse = dev->data;
>
> -	psmouse->state = PSMOUSE_IGNORE;
> -	serio_reconnect(psmouse->serio);
> -
> +	if (request == PM_RESUME) {
> +		psmouse->state = PSMOUSE_IGNORE;
> +		serio_reconnect(psmouse->serio);
> +	}
>  	return 0;
>  }
>

Yes, I believe this will work. And for vanilla 2.6 the patch below should 
do the trick. As you can see vanilla 2.6 has custom reconnect logic in PM 
handler but it does not work very well for devices connected to Synaptics
pass-through port - it will unregister it and register again potentially 
creating a new input device like serio does. The "main" mouse device will 
retain its device though.

===================================================================
ChangeSet@1.1423, 2003-11-11 00:06:11-05:00, dtor_core@ameritech.net
  Re-initialize mouse hardware on resume only.


 psmouse-base.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)


diff -Nru a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
--- a/drivers/input/mouse/psmouse-base.c	Tue Nov 11 00:07:50 2003
+++ b/drivers/input/mouse/psmouse-base.c	Tue Nov 11 00:07:50 2003
@@ -528,17 +528,19 @@
 	struct psmouse *psmouse = dev->data;
 	struct serio_dev *ser_dev = psmouse->serio->dev;
 
-	synaptics_disconnect(psmouse);
+	if (request == PM_RESUME) {
+		synaptics_disconnect(psmouse);
 
-	/* We need to reopen the serio port to reinitialize the i8042 controller */
-	serio_close(psmouse->serio);
-	serio_open(psmouse->serio, ser_dev);
+		/* We need to reopen the serio port to reinitialize the i8042 controller */
+		serio_close(psmouse->serio);
+		serio_open(psmouse->serio, ser_dev);
 
-	/* Probe and re-initialize the mouse */
-	psmouse_probe(psmouse);
-	psmouse_initialize(psmouse);
-	synaptics_pt_init(psmouse);
-	psmouse_activate(psmouse);
+		/* Probe and re-initialize the mouse */
+		psmouse_probe(psmouse);
+		psmouse_initialize(psmouse);
+		synaptics_pt_init(psmouse);
+		psmouse_activate(psmouse);
+	}
 
 	return 0;
 }

===================================================================

Unfortunately I do not suspend my laptop so I did not run it, just
made sure it compiles. Arief? could you give this patch a try?


Dmitry

Re: [PATCH?] psmouse-base.c

[arief_mulya]

Dmitry Torokhov wrote:

>On Monday 10 November 2003 01:56 am, Andrew Morton wrote:
>  
>
>>Dmitry Torokhov <dtor_core@ameritech.net> wrote:
>>    
>>
>
>  
>
>>>serio_reconnect() is only in your tree (-mm), it has not been pushed
>>>to Linus yet... Unfortunately using rescan can cause input devices be
>>>shifted if some program has them open while suspending.
>>>      
>>>
>>Ah, I see.  So would you say that reconnect is the correct thing to use
>>here?
>>
>>That would mean that the appropriate patch against -mm is
>>
>>--- 25/drivers/input/mouse/psmouse-base.c~serio-pm-fix	2003-11-09
>>20:12:27.000000000 -0800 +++
>>25-akpm/drivers/input/mouse/psmouse-base.c	2003-11-09
>>20:12:27.000000000 -0800 @@ -533,9 +533,10 @@ static int
>>psmouse_pm_callback(struct pm
>> {
>> 	struct psmouse *psmouse = dev->data;
>>
>>-	psmouse->state = PSMOUSE_IGNORE;
>>-	serio_reconnect(psmouse->serio);
>>-
>>+	if (request == PM_RESUME) {
>>+		psmouse->state = PSMOUSE_IGNORE;
>>+		serio_reconnect(psmouse->serio);
>>+	}
>> 	return 0;
>> }
>>
>>    
>>
>
>Yes, I believe this will work. And for vanilla 2.6 the patch below should 
>do the trick. As you can see vanilla 2.6 has custom reconnect logic in PM 
>handler but it does not work very well for devices connected to Synaptics
>pass-through port - it will unregister it and register again potentially 
>creating a new input device like serio does. The "main" mouse device will 
>retain its device though.
>
>===================================================================
>ChangeSet@1.1423, 2003-11-11 00:06:11-05:00, dtor_core@ameritech.net
>  Re-initialize mouse hardware on resume only.
>
>
> psmouse-base.c |   20 +++++++++++---------
> 1 files changed, 11 insertions(+), 9 deletions(-)
>
>
>diff -Nru a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
>--- a/drivers/input/mouse/psmouse-base.c	Tue Nov 11 00:07:50 2003
>+++ b/drivers/input/mouse/psmouse-base.c	Tue Nov 11 00:07:50 2003
>@@ -528,17 +528,19 @@
> 	struct psmouse *psmouse = dev->data;
> 	struct serio_dev *ser_dev = psmouse->serio->dev;
> 
>-	synaptics_disconnect(psmouse);
>+	if (request == PM_RESUME) {
>+		synaptics_disconnect(psmouse);
> 
>-	/* We need to reopen the serio port to reinitialize the i8042 controller */
>-	serio_close(psmouse->serio);
>-	serio_open(psmouse->serio, ser_dev);
>+		/* We need to reopen the serio port to reinitialize the i8042 controller */
>+		serio_close(psmouse->serio);
>+		serio_open(psmouse->serio, ser_dev);
> 
>-	/* Probe and re-initialize the mouse */
>-	psmouse_probe(psmouse);
>-	psmouse_initialize(psmouse);
>-	synaptics_pt_init(psmouse);
>-	psmouse_activate(psmouse);
>+		/* Probe and re-initialize the mouse */
>+		psmouse_probe(psmouse);
>+		psmouse_initialize(psmouse);
>+		synaptics_pt_init(psmouse);
>+		psmouse_activate(psmouse);
>+	}
> 
> 	return 0;
> }
>
>===================================================================
>
>Unfortunately I do not suspend my laptop so I did not run it, just
>made sure it compiles. Arief? could you give this patch a try?
>
>
>  
>
I have tested it before.
My first attempts looked quite just like that.

It didn't work quite nicely.
Especially with gpm, after resume, you cannot do Tap-to-Click behaviour 
with that patch. You can still move it, use left and right button, but 
no tap-to-click. I don't know why. That's why, finally, I use 
serio_rescan().

I haven't tested it with X, though, as I use gpm as a repeater, I 
thought this was unnecessary.

But I have try Andrew's tree. And it works flawlessly with the patch 
(case PM_RESUME: serio_reconnect()). I think I'm going to stick with mm 
tree, and dump my vanilla kernel.

One more think, I also sets "psmouse_resetafter" to 1 at the 
declaration. Without that, I get too many ugly message saying "Synaptics 
lost sync at 1 byte..." or something like that. As it is a module 
parameter, but on menuconfig synaptics does not available as module, so 
I set it directly on the source. I don't know if I can set it on boot 
time, can it?

Best Regards
-- 
arief_mulya
Peace is Beautiful.

Re: [PATCH?] psmouse-base.c

[Dmitry Torokhov]

On Tuesday 11 November 2003 01:54 am, arief_mulya wrote:
> Dmitry Torokhov wrote:

> >
> >Unfortunately I do not suspend my laptop so I did not run it, just
> >made sure it compiles. Arief? could you give this patch a try?
>
> I have tested it before.
> My first attempts looked quite just like that.
>
> It didn't work quite nicely.
> Especially with gpm, after resume, you cannot do Tap-to-Click behaviour
> with that patch. You can still move it, use left and right button, but
> no tap-to-click. I don't know why. That's why, finally, I use
> serio_rescan().
>
> I haven't tested it with X, though, as I use gpm as a repeater, I
> thought this was unnecessary.
>
> But I have try Andrew's tree. And it works flawlessly with the patch
> (case PM_RESUME: serio_reconnect()). I think I'm going to stick with mm
> tree, and dump my vanilla kernel.
>
> One more think, I also sets "psmouse_resetafter" to 1 at the
> declaration. Without that, I get too many ugly message saying
> "Synaptics lost sync at 1 byte..." or something like that. As it is a
> module parameter, but on menuconfig synaptics does not available as
> module, so I set it directly on the source. I don't know if I can set
> it on boot time, can it?
>

Ok, I somewhat confused as the sequence in psmouse_pm_callback is pretty
much the same as in rescan/reconnect. Wait...
 
Does suspend/resume work at all if you don't set psmouse_resetafter to 1??
The reason I am asking is that we have alot of different PM interfaces and
this one is marked as deprecated. If it is not called during resume it would
leave the touchpad in relative mode while kernel expects absolute and spews 
"lost sync" messages... Until Synaptics decides that it's time to reset 
after X bad packets. Does it make any sense?

Btw, Synaptics is intergated into psmouse module so you don't really need
to edit the source to set resetafter parameter. Either pass 
"psmouse_resetafter=X" to the kernel on boot if psmouse compiled in or
pass it to modprobe.

Dmitry

Re: [PATCH?] psmouse-base.c

[arief_mulya]

Dmitry Torokhov wrote:

>On Tuesday 11 November 2003 01:54 am, arief_mulya wrote:
>  
>
>>Dmitry Torokhov wrote:
>>    
>>
>
>  
>
>>>Unfortunately I do not suspend my laptop so I did not run it, just
>>>made sure it compiles. Arief? could you give this patch a try?
>>>      
>>>
>>I have tested it before.
>>My first attempts looked quite just like that.
>>
>>It didn't work quite nicely.
>>Especially with gpm, after resume, you cannot do Tap-to-Click behaviour
>>with that patch. You can still move it, use left and right button, but
>>no tap-to-click. I don't know why. That's why, finally, I use
>>serio_rescan().
>>
>>I haven't tested it with X, though, as I use gpm as a repeater, I
>>thought this was unnecessary.
>>
>>But I have try Andrew's tree. And it works flawlessly with the patch
>>(case PM_RESUME: serio_reconnect()). I think I'm going to stick with mm
>>tree, and dump my vanilla kernel.
>>
>>One more think, I also sets "psmouse_resetafter" to 1 at the
>>declaration. Without that, I get too many ugly message saying
>>"Synaptics lost sync at 1 byte..." or something like that. As it is a
>>module parameter, but on menuconfig synaptics does not available as
>>module, so I set it directly on the source. I don't know if I can set
>>it on boot time, can it?
>>
>>    
>>
>
>Ok, I somewhat confused as the sequence in psmouse_pm_callback is pretty
>much the same as in rescan/reconnect. Wait...
> 
>  
>
This is what  I thought, too.
Why did it work with serio_rescan() or serio_reconnect() but not with 
the one above?

But on second thought, I didn't know how the kernel works, not then. Not 
now. So when it works for me, my lazy brain, said. That's enough.  ;-)

>Does suspend/resume work at all if you don't set psmouse_resetafter to 1??
>  
>
Nope. At least, I didn't wait that long. (I hate seeing lots of "lost 
sync" messages in my console forever, so I always reboot. And in X, you 
just get un-moveable mouse.)

>The reason I am asking is that we have alot of different PM interfaces and
>this one is marked as deprecated. If it is not called during resume it would
>leave the touchpad in relative mode while kernel expects absolute and spews 
>"lost sync" messages... Until Synaptics decides that it's time to reset 
>after X bad packets. Does it make any sense?
>
>  
>
Personally, I don't think it makes any sense.
If something is broken. Why do I need to see lots of funny messages 
(which I didn't even know what it means) if then it could be fixed. Or 
is it has something to do with other serio devices?

>Btw, Synaptics is intergated into psmouse module so you don't really need
>to edit the source to set resetafter parameter. Either pass 
>"psmouse_resetafter=X" to the kernel on boot if psmouse compiled in or
>pass it to modprobe.
>
>  
>
I thought so, too.
But how do you pass to modprobe something that does not available as 
Module in menuconfig?

Best Regards.
-- 
arief_mulya
Peace is Beautiful.

>Dmitry
>
>  
>

Re: [PATCH?] psmouse-base.c

[Dmitry Torokhov]

On Tuesday 11 November 2003 09:28 pm, arief_mulya wrote:
> Dmitry Torokhov wrote:

>
> >Does suspend/resume work at all if you don't set psmouse_resetafter to
> > 1??
>
> Nope. At least, I didn't wait that long. (I hate seeing lots of "lost
> sync" messages in my console forever, so I always reboot. And in X, you
> just get un-moveable mouse.)
>

OK, thank you for confirming it.

> >The reason I am asking is that we have alot of different PM interfaces
> > and this one is marked as deprecated. If it is not called during
> > resume it would leave the touchpad in relative mode while kernel
> > expects absolute and spews "lost sync" messages... Until Synaptics
> > decides that it's time to reset after X bad packets. Does it make any
> > sense?
>
> Personally, I don't think it makes any sense.
> If something is broken. Why do I need to see lots of funny messages
> (which I didn't even know what it means) if then it could be fixed. Or
> is it has something to do with other serio devices?
>

These messages tell you that something bad happened and data stream that
is coming from the mouse hardware does not match selected protocol. In your
case it turns out that PM callback isn't get called at all. This leaves
the touchpad in relative mode (after BIOS init) while Synaptics driver
expects so called absolute protocol. Normally you should not see these
messages, they signal that something went wrong.

Now, you activated psmouse_resetafter option. I coded it to take care of
my docking station resetting the touchpad back into relative mode without
telling anyone. If it is active then Synaptics tries to re-initialize 
itself after so many bad packets (1 in your case).

What I am trying to tell is that pm_callback does not affect anything in
your case, its psmouse_resetafter that does re-initialization.

Andrew, the patch, although probably not needed should be still valid as 
there is no point of trying to reconnect when suspending.
 
> >Btw, Synaptics is intergated into psmouse module so you don't really
> > need to edit the source to set resetafter parameter. Either pass
> >"psmouse_resetafter=X" to the kernel on boot if psmouse compiled in or
> >pass it to modprobe.
>
> I thought so, too.
> But how do you pass to modprobe something that does not available as
> Module in menuconfig?
>

There is only one module called psmouse that supports different flavors
of PS/2 protocol along with (optional) Synaptics protocol. You should try
to "modprobe psmouse psmouse_resetafter=X".

Dmitry