lkml.org 
[lkml]   [2006]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC] input: Wacom tablet driver for simple X hotplugging
On Fri, 21 Jul 2006 23:13:41 +0200
Magnus Vigerlöf <wigge@bigfoot.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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-07-25 21:11    [W:0.246 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site