Re: [RFC v3 1/1] Input: Add userio module
flat view
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2015-07-22 21:40:07
Also in:
linux-input, lkml
Hi Stephen, On Wed, Jul 22, 2015 at 03:52:53PM -0400, Stephen Chandler Paul wrote:
quoted hunk ↗ jump to hunk
+static int userio_char_open(struct inode *inode, struct file *file) +{ + struct userio_device *userio; + + userio = devm_kzalloc(userio_misc.this_device, + sizeof(struct userio_device), GFP_KERNEL); + if (!userio) + return -ENOMEM;
Never ever use devm_ API outside of device probe() calls because devm-managed resources only freed after failed probe() or after remove() callback on driver model device and not any other device (block, char, etc) in the system.
quoted hunk ↗ jump to hunk
+ + spin_lock_init(&userio->head_lock); + mutex_init(&userio->tail_lock); + init_waitqueue_head(&userio->waitq); + + userio->serio = kzalloc(sizeof(struct serio), GFP_KERNEL); + if (!userio->serio) { + devm_kfree(userio_misc.this_device, userio);
And indeed here you have ot manually release the acquired resource. So the only benefit you got form devm* is wasted resousrces.
quoted hunk ↗ jump to hunk
+ return -ENOMEM; + } + + userio->serio->write = userio_device_write; + userio->serio->port_data = userio; + + file->private_data = userio; + + return 0; +} + +static int userio_char_release(struct inode *inode, struct file *file) +{ + struct userio_device *userio = file->private_data; + + if (userio->serio) { + userio->serio->port_data = NULL;
No need to reset, it is going away.
quoted hunk ↗ jump to hunk
+ + if (userio->running) + serio_unregister_port(userio->serio); + else + kfree(userio->serio); + } + + devm_kfree(userio_misc.this_device, userio); + + return 0; +} + +static ssize_t userio_char_read(struct file *file, char __user *buffer, + size_t count, loff_t *ppos) +{ + struct userio_device *userio = file->private_data; + int ret; + size_t nonwrap_len, copylen; + + if (!count) + return 0; + + if (file->f_flags & O_NONBLOCK && userio->head == userio->tail) + return -EAGAIN; + else {
This is racy. If there was data and other thread "stole" it here then your O_NONBLOCK will block. See evdev_read() how you supposed to handle this. Thanks. -- Dmitry