Date:	Tue, 25 Jul 2006 12:06:45 -0700
From:	Andrew Morton <akpm@...l.org>
To:	Magnus Vigerlöf <wigge@...foot.com>
Cc:	linux-input@...ey.karlin.mff.cuni.cz, linux-kernel@...r.kernel.org,
	wigge@...foot.com, "Ping Cheng" <pingc@...om.com>
Subject: Re: [RFC] input: Wacom tablet driver for simple X hotplugging

On Fri, 21 Jul 2006 23:13:41 +0200
Magnus  Vigerlöf <wigge@...foot.com> wrote:

> Hi,
> 
> I've been working on a device driver that simplifies hotplugging Wacom
> tablets when running X.
> 
> I've posted the driver on the linuxwacom m-l and the response so far
> has been positive as it makes hot-plugging Wacom tablets even easier,
> but questions has been rised if this couldn't be made into a generic
> module for other devices than Wacom tablets.
> 
> The 'problem' is that the normal evdev-driver removes the inputX
> device when the physical device is unplugged and this causes the
> X-server to close the device and not re-open it until the user
> switches VT.
> 
> My device driver (patch attached below) works pretty much like the
> evdev-driver except that it it registers one device when it is loaded
> and simulates a Wacom tablet even when none is connected to the
> system. This way the X-server will never notice the absence of a
> tablet and whenever a tablet is (re)connected it will work without any
> tricks.
> 
> 
> I'd appreciate whether you think this is a viable idea to make it as a
> generic driver instead or should I continue with the Wacom-specific
> one. I know the 'right' thing would be to make X truly hot-plug aware,
> but this driver is something that would be possible to use in current
> systems without any problems.
> 
> If it is a viable idea; Which other devices/types of device do you
> think could be of interest to handle in a similar fashion? Tablets of
> different makes/models are obvious, but are there any others that
> would benefit from a similar driver?
> 
> And third, should something like this be a separate driver or should
> the functionallity be included in the evdev-driver?
> 
> 
> Note, the patch is included for idea visualization and not by any
> means something I consider ready to be submitted. I know I have a few
> issues that I must sort out first, however it does work in its current
> state.
> 

The patch adds rather a lot of trailing whitespace.  I trim that off when
applying; others do not.


> +#include <asm/semaphore.h

Semaphores are deprecated.  Unless you actually _need_ the sempahore's
counting feature, please use mutexes.

> +static int exclusivegrab = 0;
> +static int debugconnect = 0;
> +static short openfailcount = 0;

Avoid the initialisation-to-zero.  The C runtime will zero these anyway,
and the explicit initialisation will move these variables into the data
section and hence into the on-disk vmlinux inage.

> +#ifdef OPENFAILCOUNT
> +module_param(openfailcount, short, 0644);
> +MODULE_PARM_DESC(openfailcount, "Number of upcoming open that will fail"
> +		 " with -ENODEV.");
> +#endif

What is this??

> +static int str_to_user(const char *str, unsigned int maxlen,
> +		       void __user *p)
> +{
> +	int len;
> +
> +	if (!str)
> +		return -ENOENT;
> +
> +	len = strlen(str) + 1;
> +	if (len > maxlen)
> +		len = maxlen;
> +
> +	return copy_to_user(p, str, len) ? -EFAULT : len;
> +}

If (len > maxlen) this will send userspace a non-null-terminated string.

> +/* ################################################################### */
> +/*                  Module File Methods                                */
> +/* ################################################################### */
> +static int wp_open(struct inode *inodp, struct file *filp)
> +{
> +	struct wp_filenode *list;
> +
> +	if (openfailcount > 0) {
> +		openfailcount--;
> +		return -ENODEV;
> +	}
> +
> +	if (!(list = kzalloc(sizeof(*list), GFP_KERNEL)))
> +		return -ENOMEM;
> +
> +	filp->private_data = list;
> +	list->tabletid = wp_proxyhndl.tabletid;
> +
> +	down(&wp_sema);
> +	list_add_tail(&list->node, &wp_users);
> +	if (wp_currenthndl != &wp_proxyhndl) {
> +		if (!(wp_currenthndl->flags & WP_FLAG_OPEN)) {
> +			if (!input_open_device(&wp_currenthndl->handle))
> +				wp_currenthndl->flags |= WP_FLAG_OPEN;
> +			else
> +				printk(KERN_WARNING WP_MODNAME
> +				       ": Failed to open '%s'\n",
> +				       wp_currenthndl->devdesc);
> +		}
> +	}
> +	up(&wp_sema);
> +	return 0;
> +}

If input_open_device() fails then the whole open() should fail.  That way
there will be no close(), flush() or release().

> +		if(copy_to_user(ip, &wp_proxydev.id, sizeof(struct input_id)))

We put a space between the `if' and the `(' (review the entire patch for
this).

> +	/* the id as all must close/reopen again anyway. */
> +	else if (!list_empty(&wp_users)) {
> +		if (!input_open_device(&devh->handle)) {
> +			devh->flags |= WP_FLAG_OPEN;
> +			if (wp_devgrabcount > 0) {
> +				if (!input_grab_device(&devh->handle))
> +					devh->flags |= WP_FLAG_GRAB;
> +				else
> +					printk(KERN_WARNING WP_MODNAME
> +					       ": Failed to grab '%s'\n",
> +					       dev->name);
> +			}
> +		}
> +		else
> +			printk(KERN_WARNING WP_MODNAME
> +			       ": Failed to open '%s'\n", dev->name);
> +	}
> +}

Again, just emitting a printk seems insufficient here.


-
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/

Powered by blists - more mailing lists