Hi Dmitry
On Wed, Oct 3, 2012 at 11:03 PM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
[snip]
> @@ -991,43 +950,47 @@ static void evdev_cleanup(struct evdev *evdev)
>
> /*
> * Create new evdev device. Note that input core serializes calls
> - * to connect and disconnect so we don't need to lock evdev_table here.
> + * to connect and disconnect.
> */
> static int evdev_connect(struct input_handler *handler, struct input_dev *dev,
> const struct input_device_id *id)
> {
> struct evdev *evdev;
> int minor;
> + int dev_no;
> int error;
>
> - for (minor = 0; minor < EVDEV_MINORS; minor++)
> - if (!evdev_table[minor])
> - break;
> -
> - if (minor == EVDEV_MINORS) {
> - pr_err("no more free evdev devices\n");
> - return -ENFILE;
> + minor = input_get_new_minor(EVDEV_MINOR_BASE, EVDEV_MINORS, true);
> + if (minor < 0) {
> + error = minor;
> + pr_err("failed to reserve new minor: %d\n", error);
> + return error;
You could also do:
return minor;
So you can drop that "error = minor;" line.
[snip]
> @@ -1062,6 +1028,7 @@ static void evdev_disconnect(struct input_handle *handle)
>
> device_del(&evdev->dev);
> evdev_cleanup(evdev);
> + input_free_minor(MINOR(evdev->dev.devt));
I was wondering whether we should free the minors in evdev_free()
instead. Because if we free them here, we might end up with two
user-space applications listening to the same cdev (based on
major/minor) but to different input devices that drive the cdev.
The older of both cdevs would be already dead and I don't think this
is a big issue, but I just wanted to mention it if others can think of
corner cases where this would be bad.
[snip]
> @@ -2016,22 +2017,14 @@ EXPORT_SYMBOL(input_unregister_device);
> int input_register_handler(struct input_handler *handler)
> {
> struct input_dev *dev;
> - int retval;
> + int error;
>
> - retval = mutex_lock_interruptible(&input_mutex);
> - if (retval)
> - return retval;
> + error = mutex_lock_interruptible(&input_mutex);
> + if (error)
> + return error;
Still wondering why you change that variable name here?
[snip]
> @@ -576,13 +556,14 @@ static int mousedev_open(struct inode *inode, struct file *file)
> goto err_free_client;
>
> file->private_data = client;
> + nonseekable_open(inode, file);
Ouh, seems like we never called this for mousedevs, isn't that a bug
that should also go to stable? It just sets some flags but I am not
sure what happends if we don't set them.
It isn't related to this patch (I think?).
[snip]
Sorry for the bikeshedding. The patch looks really good and all I
found are just minor optional suggestions (or personal taste).
Thanks a lot for fixing this! It looks much better than my first approach.
Reviewed-by: David Herrmann <dh.herrmann@xxxxxxxxxxxxxx>
Regards
David
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
