linux-kernel - Re: [PATCH 1/7] input: alps: Set correct name of psmouse device in alps

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <201412240848.43326@pali>
Date:	Wed, 24 Dec 2014 08:48:42 +0100
From:	Pali Rohár <pali.rohar@...il.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	Hans de Goede <hdegoede@...hat.com>,
	Vadim Klishko <vadim@...que.com>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] input: alps: Set correct name of psmouse device in alps_init()

On Saturday 20 December 2014 09:53:38 Pali Rohár wrote:
> On Tuesday 16 December 2014 12:58:20 Pali Rohár wrote:
> > On Tuesday 16 December 2014 06:02:34 Dmitry Torokhov wrote:
> > > Hi Pali,
> > > 
> > > On Fri, Nov 14, 2014 at 08:38:20PM +0100, Pali Rohár wrote:
> > > > On some laptops after starting them from off state (not
> > > > after reboot), function alps_probe_trackstick_v3()
> > > > (called from function alps_identify()) does not detect
> > > > trackstick. To fix this problem we need to reset
> > > > device. But function alps_identify() is called also
> > > > from alps_detect() and we do not want to reset device
> > > > in detect function because it will slow down
> > > > initialization of all other non alps devices.
> > > > 
> > > > This patch moves code for setting correct device name &
> > > > protocol from function alps_detect() to alps_init()
> > > > which already doing full device reset.
> > > > 
> > > > So this patch removes need to do trackstick detection in
> > > > alps_detect() function.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali.rohar@...il.com>
> > > > ---
> > > > 
> > > >  drivers/input/mouse/alps.c |   17 ++++++++++++++---
> > > >  1 file changed, 14 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/input/mouse/alps.c
> > > > b/drivers/input/mouse/alps.c index 8d85c79..9ffa98d
> > > > 100644 --- a/drivers/input/mouse/alps.c
> > > > +++ b/drivers/input/mouse/alps.c
> > > > @@ -2392,6 +2392,10 @@ int alps_init(struct psmouse
> > > > *psmouse)
> > > > 
> > > >  	if (input_register_device(priv->dev2))
> > > >  	
> > > >  		goto init_fail;
> > > > 
> > > > +	if (!(priv->flags & ALPS_DUALPOINT))
> > > > +		psmouse->name = "GlidePoint TouchPad";
> > > > +	psmouse->model = priv->proto_version;
> > > > +
> > > > 
> > > >  	psmouse->protocol_handler = alps_process_byte;
> > > >  	psmouse->poll = alps_poll;
> > > >  	psmouse->disconnect = alps_disconnect;
> > > > 
> > > > @@ -2422,11 +2426,18 @@ int alps_detect(struct psmouse
> > > > *psmouse, bool set_properties)
> > > > 
> > > >  		return -1;
> > > >  	
> > > >  	if (set_properties) {
> > > > 
> > > > +		/*
> > > > +		 * NOTE: To detect model and trackstick presence we
> > 
> > need
> > 
> > > > to do +		 *       full device reset. To speed up
> > 
> > detection
> > 
> > > > and prevent +		 *       calling duplicate
> 
> initialization
> 
> > > > sequence (both in +		 *       alps_detect() and
> > > > alps_init()) we set model/protocol +		 *       version
> 
> and
> 
> > > > correct name in alps_init() (which will +		 *       
do
> > 
> > full
> > 
> > > > device reset). For now set name to DualPoint. +		 */
> > > > 
> > > >  		psmouse->vendor = "ALPS";
> > > > 
> > > > -		psmouse->name = dummy.flags & ALPS_DUALPOINT ?
> > > > -				"DualPoint TouchPad" : "GlidePoint";
> > > > -		psmouse->model = dummy.proto_version << 8;
> > > > +		psmouse->name = "DualPoint TouchPad";
> > > > 
> > > >  	}
> > > > 
> > > > +
> > > 
> > > I do not quite like the way we change the device
> > > description back and forth. Do you think we could
> > > allocate the "real" priv structure in alps_detect() and
> > > have alps_init() expect to find it (and free it if
> > > set_properties is false). This way we'd go through
> > > initialization once in detect, it will be authoritative,
> > > and we would set the name of the device properly from the
> > > beginning.
> > > 
> > > Thanks.
> > 
> > No without introducing another psmouse_reset call. I want to
> > reduce time of loading driver, so I think this is better.
> 
> Also psmouse-base.c call psmouse_reset between alps_detect()
> and alps_init(). We must do trackstick detection after
> psmouse_reset, but intorducing another psmouse_reset in
> alps_detect() will slow down initialization for ALPS devices.
> Also do not remember that psmouse_reset will lock both
> keyboard & mouse for one second (sometimes for two) on my
> laptop. I played with this and I think my patch is good
> approach how to do trackstick detection without slowdown or
> other side effects.
> 
> Also we do not need to know if ALPS touchpad has trackstick
> device in alps_detect() phase. In alps_detect() we just need
> to know if PS/2 device is ALPS or not. From psmouse/serio
> point of view there is only one ALPS device on PS/2/i8042
> bus. Just alps.ko driver parse PS/2 packets and detect which
> packet has touchpad header and which trackpoint. So we really
> do not need to know if trackstick is there or not in
> alps_detect() function.

Dmitry ping, can you finally review this patch series? I it there 
for more then month...

-- 
Pali Rohár
pali.rohar@...il.com

Download attachment "signature.asc " of type "application/pgp-signature" (199 bytes)