linux-kernel - Re: [PATCH] input: Synaptics USB device driver

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: <20120105024634.5c594566@greyhound>
Date:	Thu, 5 Jan 2012 02:46:34 +0000
From:	Jan Steinhoff <mail@...-steinhoff.de>
To:	Oliver Neukum <oneukum@...e.de>
Cc:	Alessandro Rubini <rubini@...vis.unipv.it>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	linux-input@...r.kernel.org, Jiri Kosina <jkosina@...e.cz>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] input: Synaptics USB device driver

On Wed, 4 Jan 2012 09:25:59 +0100
Oliver Neukum <oneukum@...e.de> wrote:

> Am Dienstag, 3. Januar 2012, 19:40:33 schrieb Jan Steinhoff:  
> > From: Jan Steinhoff <mail@...-steinhoff.de>
> > 
> > This patch adds a driver for Synaptics USB touchpad or pointing
> > stick devices. These USB devices emulate an USB mouse by default,
> > so one can also use the usbhid driver. However, in combination with
> > special user space drivers this kernel driver allows one to
> > customize the behaviour of the device.  
> 
> Hi,
> 
> thank you for this driver. There are a few issues which I have
> commented on in the text.  

Hi,

I agree with most of the comments, but I have some further
questions/remarks below.

Best,
Jan

[...]
> [..]  
> > +
> > +/*
> > + * initialization of usb data structures
> > + */
> > +
> > +static int synusb_setup_iurb(struct synusb *synusb,
> > +			     struct usb_endpoint_descriptor
> > *endpoint) +{
> > +	char *buf;
> > +
> > +	if (endpoint->wMaxPacketSize < 8)
> > +		return 0;  
> 
> How could this happen?  

The id table can be modified via sysfs, so it is easily possible to try
this driver with officially unsupported devices. (At least for Synaptics
devices, there is a good chance this driver works just the same.) The
condition I put there was the only one I could think of to select when
it is not worth trying.

[...]
> > +static int synusb_suspend(struct usb_interface *intf, pm_message_t
> > message) +{
> > +	struct synusb *synusb = usb_get_intfdata(intf);
> > +
> > +	if (synusb == NULL)
> > +		return 0;  
> 
> How can this happen?  

According to the Documentation this can not happen. However, this check
is present in usb-skeleton.c, so I put it there just in case I
overlooked something.

[...]
> > +static struct usb_driver synusb_driver = {
> > +	.name =		"synaptics_usb",
> > +	.probe =	synusb_probe,
> > +	.disconnect =	synusb_disconnect,
> > +	.id_table =	synusb_idtable,
> > +	.suspend =	synusb_suspend,
> > +	.resume =	synusb_resume,
> > +	.pre_reset =	synusb_pre_reset,
> > +	.post_reset =	synusb_post_reset,
> > +	.reset_resume = synusb_reset_resume,
> > +	.supports_autosuspend = 1,  
> 
> Yet you do not manage the busy state. Do you really want to play
> ping-pong with the power state?  

The last time I tested autosuspend its timer was reseted "automatically"
every time an int urb arrived. Isn't this all that is needed for an
input device?

The autosuspend delay should of course be set to a reasonably large
value. Then there should be no ping-pong with the power state while the
device is actually used/touched. On the other hand, if the pad is not
touched for a while, then no int urb arrives and the device will
autosuspend (and, thanks to remote wakeup, autoresume nicely when
touched again).
--
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/