linux-kernel - Re: [git pull] Input updates for 2.6.33-rc2

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>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20091231210654.GA3874@core.coreip.homeip.net>
Date:	Thu, 31 Dec 2009 13:06:54 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>
Subject: Re: [git pull] Input updates for 2.6.33-rc2

On Thu, Dec 31, 2009 at 11:50:59AM -0800, Linus Torvalds wrote:
> 
> 
> On Wed, 30 Dec 2009, Dmitry Torokhov wrote:
> >
> > Dmitry Torokhov (7):
> >       Input: speed up suspend/shutdown for PS/2 mice and keyboards
> 
> Btw, the thing I like best about this commit is how it adds some comments 
> on the keyboard commands.
> 
> That said, the naming and the comments aren't all that wonderful. I don't 
> think you should use the word "RESET" for command f5/f6: the command is 
> called "Set Default" (and ".. Disable" for f5), and there is no actual 
> reset involved. 
> 

I guess it is just a matter of wording. For me "reset" and "revert to
initial state" are pretty much the same.

> So I suspect the command should be renamed to
> 
> 	#define ATKBD_CMD_SET_DEFAULTS_AND_DISABLE	0x00f5

ATKBD_CMD_SET_DEFAULTS_AND_DISABLE_DATA_REPORTING :) I was just trying
to follow the existing naming style. It was there before me and it does
make sense to me still so I did not feel the neet to change it.

> 	#define ATKBD_CMD_SET_DEFAULTS			0x00f6
> 
> and then the comments wouldn't even be needed (and anybody reading the 
> code would not have to look them up in the header file).
> 
> And then "ATKBD_CMD_RESET_BAT" should probably just be "ATKBD_CMD_RESET" 
> (I wonder what the "BAT" part is all about?)

That came from Vojtech but I am pretty sure this stands for Basic
Assurance Test since full reset for touchpads for example involves
recalibration. See this excerpt from Synaptics docs:

"At power-on, the PS/2 TouchPad performs a self-test and calibration,
then transmits the completion code $AA and ID code $00. If the TouchPad
fails its self-test, it transmits error code $FC and ID code $00.  This
processing also occurs when a software Reset ($FF) command is received.
The host should not attempt to send commands to the TouchPad until the
calibration/self-test is complete."

> 
> Oh, and command F5h is not always a "set defaults and disable". For some 
> (all?) PS/2 mice it seems to be _just_ a "disable",

Exactly, that is why it is called PSMOUSE_CMD_DISABLE when used in mouse
context. However snippets of docs I have indicate that for keyboards it
also causes them to reset to defaults.

> and I'm not at all 
> sure you should have used it for the psmouse_cleanup() function.

I am using PSMOUSE_CMD_RESET_DIS (0xf6) in psmouse_cleanup().

> You used 
> to do a
> 
> 	psmouse_reset(psmouse);
> 
> which sent a full reset to the mouse (and waits for the two-byte ACK). It 
> did _not_ disable the mouse - 

It does actually, at least for some devices (incidentally the ones that
may give trouble to the BIOS if left as is):

"The reset state of the TouchPad is as follows:
- Reported sample rate is 100 samples per second (see page 30).
- Reported resolution is 4 counts per mm (see page 32).
- Scaling is 1:1.
- Stream mode is selected.
- Data reporting is disabled. <---------------
- Absolute mode is disabled."

> that happened earlier in 
> psmouse_deactivate(), when you sent it the PSMOUSE_CMD_DISABLE command.

What happened before doing psmouse_reset() (which is PSMOUSE_CMD_RESET_BAT)
should not matter because all previous state should be reset after it.

> 
> You changed that 'psmouse_reset()' to
> 
> 	/*
> 	 * Reset the mouse to defaults (bare PS/2 protocol).
> 	 */
> 	        ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_RESET_DIS);
> 
> which now disables it again, only to then later do
>         
> 	/*      
> 	 * Some boxes, such as HP nx7400, get terribly confused if mouse
> 	 * is not fully enabled before suspending/shutting down.
> 	 */     
> 	        ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> 
> which seems odd. IOW, Why do you do a "set defaults and disable" followed 
> by a "enable", when you already had it disabled?

We do not have "light reset" option, so we have to use "light reset and
disable" to return to the bare 3-byte PS/2 protocol. Then we have to
re-enable the device because BIOS on some HP notebooks would get
confused and not suspend (or resume, I don't quite remember) if mouse
left disabled.

> 
> I think the PSMOUSE_CMD_RESET that _used_ to be there would re-enable most 
> mice, but now that you don't even do that any more, I get the feeling that 
> the "set defaults and disable" should be just a "set defaults".

There is no such command for mice unfortunately.

> 
> (You may have good reasons for having picked the "and disable" command. I 
> just wonder what they are)
> 
> 		Linus

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/