Skip to contentSkip to conversation

Uinput update

PATCH In review Dmitry Torokhov 3 participants 6 messages Nov 19, 2005 Input drivers

lore
Share & tools
lore

Dmitry Torokhov

author
#0 lore raw

cover Uinput update

Hi,

The following patches update uinput driver to perform dynamic input allocation, add some locking and ioctl to allow setting EV_SW.

Any testing will be greatly appreciated.

Thanks!

--
Dmitry

author
#1 lore raw

PATCH 1/3 Uinput: convert to dynalloc allocation

Input: uinput - convert to dynalloc allocation

Also introduce proper locking when creating/deleting device.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/input/misc/uinput.c |  308 +++++++++++++++++++++++---------------------
 include/linux/uinput.h      |   12 -
 2 files changed, 168 insertions(+), 152 deletions(-)
drivers/input/misc/uinput.c +162 -146
Index: work/drivers/input/misc/uinput.c
===================================================================
@@ -152,67 +152,62 @@ static int uinput_dev_erase_effect(struc
 	return retval;
 }
 
-static int uinput_create_device(struct uinput_device *udev)
+static void uinput_destroy_device(struct uinput_device *udev)
 {
-	if (!udev->dev->name) {
-		printk(KERN_DEBUG "%s: write device info first\n", UINPUT_NAME);
-		return -EINVAL;
-	}
+	const char *name, *phys;
 
-	udev->dev->event = uinput_dev_event;
-	udev->dev->upload_effect = uinput_dev_upload_effect;
-	udev->dev->erase_effect = uinput_dev_erase_effect;
-	udev->dev->private = udev;
-
-	init_waitqueue_head(&udev->waitq);
-
-	input_register_device(udev->dev);
-
-	set_bit(UIST_CREATED, &udev->state);
+	if (udev->dev) {
+		name = udev->dev->name;
+		phys = udev->dev->phys;
+		if (udev->state == UIST_CREATED)
+			input_unregister_device(udev->dev);
+		else
+			input_free_device(udev->dev);
+		kfree(name);
+		kfree(phys);
+		udev->dev = NULL;
+	}
 
-	return 0;
+	udev->state = UIST_NEW_DEVICE;
 }
 
-static int uinput_destroy_device(struct uinput_device *udev)
+static int uinput_create_device(struct uinput_device *udev)
 {
-	if (!test_bit(UIST_CREATED, &udev->state)) {
-		printk(KERN_WARNING "%s: create the device first\n", UINPUT_NAME);
+	int error;
+
+	if (udev->state != UIST_SETUP_COMPLETE) {
+		printk(KERN_DEBUG "%s: write device info first\n", UINPUT_NAME);
 		return -EINVAL;
 	}
 
-	input_unregister_device(udev->dev);
+	error = input_register_device(udev->dev);
+	if (error) {
+		uinput_destroy_device(udev);
+		return error;
+	}
 
-	clear_bit(UIST_CREATED, &udev->state);
+	udev->state = UIST_CREATED;
 
 	return 0;
 }
 
 static int uinput_open(struct inode *inode, struct file *file)
 {
-	struct uinput_device	*newdev;
-	struct input_dev	*newinput;
+	struct uinput_device *newdev;
 
-	newdev = kmalloc(sizeof(struct uinput_device), GFP_KERNEL);
+	newdev = kzalloc(sizeof(struct uinput_device), GFP_KERNEL);
 	if (!newdev)
-		goto error;
-	memset(newdev, 0, sizeof(struct uinput_device));
+		return -ENOMEM;
+
+	init_MUTEX(&newdev->sem);
 	spin_lock_init(&newdev->requests_lock);
 	init_waitqueue_head(&newdev->requests_waitq);
-
-	newinput = kmalloc(sizeof(struct input_dev), GFP_KERNEL);
-	if (!newinput)
-		goto cleanup;
-	memset(newinput, 0, sizeof(struct input_dev));
-
-	newdev->dev = newinput;
+	init_waitqueue_head(&newdev->waitq);
+	newdev->state = UIST_NEW_DEVICE;
 
 	file->private_data = newdev;
 
 	return 0;
-cleanup:
-	kfree(newdev);
-error:
-	return -ENOMEM;
 }
 
 static int uinput_validate_absbits(struct input_dev *dev)

@@ -246,34 +241,55 @@ static int uinput_validate_absbits(struc
 	return retval;
 }
 
-static int uinput_alloc_device(struct file *file, const char __user *buffer, size_t count)
+static int uinput_allocate_device(struct uinput_device *udev)
+{
+	udev->dev = input_allocate_device();
+	if (!udev->dev)
+		return -ENOMEM;
+
+	udev->dev->event = uinput_dev_event;
+	udev->dev->upload_effect = uinput_dev_upload_effect;
+	udev->dev->erase_effect = uinput_dev_erase_effect;
+	udev->dev->private = udev;
+
+	return 0;
+}
+
+static int uinput_setup_device(struct uinput_device *udev, const char __user *buffer, size_t count)
 {
 	struct uinput_user_dev	*user_dev;
 	struct input_dev	*dev;
-	struct uinput_device	*udev;
 	char			*name;
 	int			size;
 	int			retval;
 
-	retval = count;
+	if (count != sizeof(struct uinput_user_dev))
+		return -EINVAL;
+
+	if (!udev->dev) {
+		retval = uinput_allocate_device(udev);
+		if (retval)
+			return retval;
+	}
 
-	udev = file->private_data;
 	dev = udev->dev;
 
 	user_dev = kmalloc(sizeof(struct uinput_user_dev), GFP_KERNEL);
-	if (!user_dev) {
-		retval = -ENOMEM;
-		goto exit;
-	}
+	if (!user_dev)
+		return -ENOMEM;
 
 	if (copy_from_user(user_dev, buffer, sizeof(struct uinput_user_dev))) {
 		retval = -EFAULT;
 		goto exit;
 	}
 
-	kfree(dev->name);
-
 	size = strnlen(user_dev->name, UINPUT_MAX_NAME_SIZE) + 1;
+	if (!size) {
+		retval = -EINVAL;
+		goto exit;
+	}
+
+	kfree(dev->name);
 	dev->name = name = kmalloc(size, GFP_KERNEL);
 	if (!name) {
 		retval = -ENOMEM;

@@ -296,32 +312,50 @@ static int uinput_alloc_device(struct fi
 	/* check if absmin/absmax/absfuzz/absflat are filled as
 	 * told in Documentation/input/input-programming.txt */
 	if (test_bit(EV_ABS, dev->evbit)) {
-		int err = uinput_validate_absbits(dev);
-		if (err < 0) {
-			retval = err;
-			kfree(dev->name);
-		}
+		retval = uinput_validate_absbits(dev);
+		if (retval < 0)
+			goto exit;
 	}
 
-exit:
+	udev->state = UIST_SETUP_COMPLETE;
+	retval = count;
+
+ exit:
 	kfree(user_dev);
 	return retval;
 }
 
+static inline ssize_t uinput_inject_event(struct uinput_device *udev, const char __user *buffer, size_t count)
+{
+	struct input_event ev;
+
+	if (count != sizeof(struct input_event))
+		return -EINVAL;
+
+	if (copy_from_user(&ev, buffer, sizeof(struct input_event)))
+		return -EFAULT;
+
+	input_event(udev->dev, ev.type, ev.code, ev.value);
+
+	return sizeof(struct input_event);
+}
+
 static ssize_t uinput_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos)
 {
 	struct uinput_device *udev = file->private_data;
+	int retval;
 
-	if (test_bit(UIST_CREATED, &udev->state)) {
-		struct input_event	ev;
+	retval = down_interruptible(&udev->sem);
+	if (retval)
+		return retval;
 
-		if (copy_from_user(&ev, buffer, sizeof(struct input_event)))
-			return -EFAULT;
-		input_event(udev->dev, ev.type, ev.code, ev.value);
-	} else
-		count = uinput_alloc_device(file, buffer, count);
+	retval = udev->state == UIST_CREATED ?
+			uinput_inject_event(udev, buffer, count) :
+			uinput_setup_device(udev, buffer, count);
 
-	return count;
+	up(&udev->sem);
+
+	return retval;
 }
 
 static ssize_t uinput_read(struct file *file, char __user *buffer, size_t count, loff_t *ppos)

@@ -329,28 +363,38 @@ static ssize_t uinput_read(struct file *
 	struct uinput_device *udev = file->private_data;
 	int retval = 0;
 
-	if (!test_bit(UIST_CREATED, &udev->state))
+	if (udev->state != UIST_CREATED)
 		return -ENODEV;
 
 	if (udev->head == udev->tail && (file->f_flags & O_NONBLOCK))
 		return -EAGAIN;
 
 	retval = wait_event_interruptible(udev->waitq,
-			udev->head != udev->tail || !test_bit(UIST_CREATED, &udev->state));
+			udev->head != udev->tail || udev->state != UIST_CREATED);
 	if (retval)
 		return retval;
 
-	if (!test_bit(UIST_CREATED, &udev->state))
-		return -ENODEV;
+	retval = down_interruptible(&udev->sem);
+	if (retval)
+		return retval;
 
-	while ((udev->head != udev->tail) &&
-	    (retval + sizeof(struct input_event) <= count)) {
-		if (copy_to_user(buffer + retval, &udev->buff[udev->tail], sizeof(struct input_event)))
-			return -EFAULT;
+	if (udev->state != UIST_CREATED) {
+		retval = -ENODEV;
+		goto out;
+	}
+
+	while (udev->head != udev->tail && retval + sizeof(struct input_event) <= count) {
+		if (copy_to_user(buffer + retval, &udev->buff[udev->tail], sizeof(struct input_event))) {
+			retval = -EFAULT;
+			goto out;
+		}
 		udev->tail = (udev->tail + 1) % UINPUT_BUFFER_SIZE;
 		retval += sizeof(struct input_event);
 	}
 
+ out:
+	up(&udev->sem);
+
 	return retval;
 }
 

@@ -366,28 +410,30 @@ static unsigned int uinput_poll(struct f
 	return 0;
 }
 
-static int uinput_burn_device(struct uinput_device *udev)
+static int uinput_release(struct inode *inode, struct file *file)
 {
-	if (test_bit(UIST_CREATED, &udev->state))
-		uinput_destroy_device(udev);
+	struct uinput_device *udev = file->private_data;
 
-	kfree(udev->dev->name);
-	kfree(udev->dev->phys);
-	kfree(udev->dev);
+	uinput_destroy_device(udev);
 	kfree(udev);
 
 	return 0;
 }
 
-static int uinput_close(struct inode *inode, struct file *file)
-{
-	uinput_burn_device(file->private_data);
-	return 0;
-}
+#define uinput_set_bit(_arg, _bit, _max)		\
+({							\
+	int __ret = 0;					\
+	if (udev->state == UIST_CREATED)		\
+		__ret =  -EINVAL;			\
+	else if ((_arg) > (_max))			\
+		__ret = -EINVAL;			\
+	else set_bit((_arg), udev->dev->_bit);		\
+	__ret;						\
+})
 
-static int uinput_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
+static long uinput_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	int			retval = 0;
+	int			retval;
 	struct uinput_device	*udev;
 	void __user             *p = (void __user *)arg;
 	struct uinput_ff_upload ff_up;

@@ -398,19 +444,14 @@ static int uinput_ioctl(struct inode *in
 
 	udev = file->private_data;
 
-	/* device attributes can not be changed after the device is created */
-	switch (cmd) {
-		case UI_SET_EVBIT:
-		case UI_SET_KEYBIT:
-		case UI_SET_RELBIT:
-		case UI_SET_ABSBIT:
-		case UI_SET_MSCBIT:
-		case UI_SET_LEDBIT:
-		case UI_SET_SNDBIT:
-		case UI_SET_FFBIT:
-		case UI_SET_PHYS:
-			if (test_bit(UIST_CREATED, &udev->state))
-				return -EINVAL;
+	retval = down_interruptible(&udev->sem);
+	if (retval)
+		return retval;
+
+	if (!udev->dev) {
+		retval = uinput_allocate_device(udev);
+		if (retval)
+			goto out;
 	}
 
 	switch (cmd) {

@@ -419,74 +460,46 @@ static int uinput_ioctl(struct inode *in
 			break;
 
 		case UI_DEV_DESTROY:
-			retval = uinput_destroy_device(udev);
+			uinput_destroy_device(udev);
 			break;
 
 		case UI_SET_EVBIT:
-			if (arg > EV_MAX) {
-				retval = -EINVAL;
-				break;
-			}
-			set_bit(arg, udev->dev->evbit);
+			retval = uinput_set_bit(arg, evbit, EV_MAX);
 			break;
 
 		case UI_SET_KEYBIT:
-			if (arg > KEY_MAX) {
-				retval = -EINVAL;
-				break;
-			}
-			set_bit(arg, udev->dev->keybit);
+			retval = uinput_set_bit(arg, keybit, KEY_MAX);
 			break;
 
 		case UI_SET_RELBIT:
-			if (arg > REL_MAX) {
-				retval = -EINVAL;
-				break;
-			}
-			set_bit(arg, udev->dev->relbit);
+			retval = uinput_set_bit(arg, relbit, REL_MAX);
 			break;
 
 		case UI_SET_ABSBIT:
-			if (arg > ABS_MAX) {
-				retval = -EINVAL;
-				break;
-			}
-			set_bit(arg, udev->dev->absbit);
+			retval = uinput_set_bit(arg, absbit, ABS_MAX);
 			break;
 
 		case UI_SET_MSCBIT:
-			if (arg > MSC_MAX) {
-				retval = -EINVAL;
-				break;
-			}
-			set_bit(arg, udev->dev->mscbit);
+			retval = uinput_set_bit(arg, mscbit, MSC_MAX);
 			break;
 
 		case UI_SET_LEDBIT:
-			if (arg > LED_MAX) {
-				retval = -EINVAL;
-				break;
-			}
-			set_bit(arg, udev->dev->ledbit);
+			retval = uinput_set_bit(arg, ledbit, LED_MAX);
 			break;
 
 		case UI_SET_SNDBIT:
-			if (arg > SND_MAX) {
-				retval = -EINVAL;
-				break;
-			}
-			set_bit(arg, udev->dev->sndbit);
+			retval = uinput_set_bit(arg, sndbit, SND_MAX);
 			break;
 
 		case UI_SET_FFBIT:
-			if (arg > FF_MAX) {
-				retval = -EINVAL;
-				break;
-			}
-			set_bit(arg, udev->dev->ffbit);
+			retval = uinput_set_bit(arg, ffbit, FF_MAX);
 			break;
 
 		case UI_SET_PHYS:
+			if (udev->state == UIST_CREATED) {
+				retval = -EINVAL;
+				goto out;
+			}
 			length = strnlen_user(p, 1024);
 			if (length <= 0) {
 				retval = -EFAULT;

@@ -575,23 +588,26 @@ static int uinput_ioctl(struct inode *in
 		default:
 			retval = -EINVAL;
 	}
+
+ out:
+	up(&udev->sem);
 	return retval;
 }
 
 static struct file_operations uinput_fops = {
-	.owner =	THIS_MODULE,
-	.open =		uinput_open,
-	.release =	uinput_close,
-	.read =		uinput_read,
-	.write =	uinput_write,
-	.poll =		uinput_poll,
-	.ioctl =	uinput_ioctl,
+	.owner		= THIS_MODULE,
+	.open		= uinput_open,
+	.release	= uinput_release,
+	.read		= uinput_read,
+	.write		= uinput_write,
+	.poll		= uinput_poll,
+	.unlocked_ioctl	= uinput_ioctl,
 };
 
 static struct miscdevice uinput_misc = {
-	.fops =		&uinput_fops,
-	.minor =	UINPUT_MINOR,
-	.name =		UINPUT_NAME,
+	.fops		= &uinput_fops,
+	.minor		= UINPUT_MINOR,
+	.name		= UINPUT_NAME,
 };
 
 static int __init uinput_init(void)
include/linux/uinput.h +6 -6
Index: work/include/linux/uinput.h
===================================================================
@@ -34,8 +34,7 @@
 #define UINPUT_BUFFER_SIZE	16
 #define UINPUT_NUM_REQUESTS	16
 
-/* state flags => bit index for {set|clear|test}_bit ops */
-#define UIST_CREATED		0
+enum uinput_state { UIST_NEW_DEVICE, UIST_SETUP_COMPLETE, UIST_CREATED };
 
 struct uinput_request {
 	int			id;

@@ -52,11 +51,12 @@ struct uinput_request {
 
 struct uinput_device {
 	struct input_dev	*dev;
-	unsigned long		state;
+	struct semaphore	sem;
+	enum uinput_state	state;
 	wait_queue_head_t	waitq;
-	unsigned char		ready,
-				head,
-				tail;
+	unsigned char		ready;
+	unsigned char		head;
+	unsigned char		tail;
 	struct input_event	buff[UINPUT_BUFFER_SIZE];
 
 	struct uinput_request	*requests[UINPUT_NUM_REQUESTS];
author
#2 lore raw

PATCH 2/3 Uinput: add UI_SET_SWBIT ioctl

In reply to Dmitry Torokhov

Input: uinput - add UI_SET_SWBIT ioctl

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/input/misc/uinput.c |    4 ++++
 include/linux/uinput.h      |    1 +
 2 files changed, 5 insertions(+)
drivers/input/misc/uinput.c +4 -0
Index: work/drivers/input/misc/uinput.c
===================================================================
@@ -495,6 +495,10 @@ static long uinput_ioctl(struct file *fi
 			retval = uinput_set_bit(arg, ffbit, FF_MAX);
 			break;
 
+		case UI_SET_SWBIT:
+			retval = uinput_set_bit(arg, swbit, SW_MAX);
+			break;
+
 		case UI_SET_PHYS:
 			if (udev->state == UIST_CREATED) {
 				retval = -EINVAL;
include/linux/uinput.h +1 -0
Index: work/include/linux/uinput.h
===================================================================
@@ -91,6 +91,7 @@ struct uinput_ff_erase {
 #define UI_SET_SNDBIT		_IOW(UINPUT_IOCTL_BASE, 106, int)
 #define UI_SET_FFBIT		_IOW(UINPUT_IOCTL_BASE, 107, int)
 #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
+#define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
 
 #define UI_BEGIN_FF_UPLOAD	_IOWR(UINPUT_IOCTL_BASE, 200, struct uinput_ff_upload)
 #define UI_END_FF_UPLOAD	_IOW(UINPUT_IOCTL_BASE, 201, struct uinput_ff_upload)
author
#3 lore raw

PATCH 3/3 Uinput: dont use "interruptible" in FF code

In reply to Dmitry Torokhov

Input: uinput - don't use "interruptible" in FF code

If thread that submitted FF request gets interrupted somehow it will release request structure and ioctl handler will work with freed memory. TO prevent that from happening switch to using wait_for_completion instead of wait_for_completion_interruptible.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/input/misc/uinput.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)
drivers/input/misc/uinput.c +3 -8
Index: work/drivers/input/misc/uinput.c
===================================================================
@@ -92,24 +92,19 @@ static void uinput_request_done(struct u
 {
 	/* Mark slot as available */
 	udev->requests[request->id] = NULL;
-	wake_up_interruptible(&udev->requests_waitq);
+	wake_up(&udev->requests_waitq);
 
 	complete(&request->done);
 }
 
 static int uinput_request_submit(struct input_dev *dev, struct uinput_request *request)
 {
-	int retval;
-
 	/* Tell our userspace app about this new request by queueing an input event */
 	uinput_dev_event(dev, EV_UINPUT, request->code, request->id);
 
 	/* Wait for the request to complete */
-	retval = wait_for_completion_interruptible(&request->done);
-	if (!retval)
-		retval = request->retval;
-
-	return retval;
+	wait_for_completion(&request->done);
+	return request->retval;
 }
 
 static int uinput_dev_upload_effect(struct input_dev *dev, struct ff_effect *effect)
#4 lore raw
In reply to Dmitry Torokhov
Quoted 6 lines · Dmitry Torokhov

Hi,

The following patches update uinput driver to perform dynamic input allocation, add some locking and ioctl to allow setting EV_SW.

Any testing will be greatly appreciated.

Thanks for the patches!

Signature · 2 lines
Vojtech Pavlik
SuSE Labs, SuSE CR
#5 lore raw
In reply to Dmitry Torokhov
Quoted 6 lines · On Friday November 18, [email protected] wrote:

Hi,

The following patches update uinput driver to perform dynamic input allocation, add some locking and ioctl to allow setting EV_SW.

Any testing will be greatly appreciated.

Well it certainly solved the problems I was having with uinput, Thanks!

NeilBrown