Re: [PATCH 4/4] Input: extend the number of event (and other) devices

Hi David,

On Sun, Oct 07, 2012 at 04:52:28PM +0200, David Herrmann wrote:

Hi Dmitry

On Wed, Oct 3, 2012 at 11:03 PM, Dmitry Torokhov
[off-list ref] wrote:

[snip]

quoted

@@ -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.

Right, but I prefer doing this to signify that the value in 'minor' is
actually an error code.

[snip]

quoted

@@ -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.

I believe this is OK and how the rest of Unix objects work - they do not
get freed until all users release them, but new users will get the new
instance.

[snip]

quoted

@@ -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?

My style preference: when the same variable is used to return error
codes and success/real values I call it retval. If I have separate error
handling path and explicit success path I use "return error;"/"return 0;".
Here we transitioned to explicitly returning success, so I changed the
name.

[snip]

quoted

@@ -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?).

I split it out, but we do not need it for stable. We still were
installing noop_llseek() fops handler so it should still work even
without nonseekable_open().

[snip]

Sorry for the bikeshedding. The patch looks really good and all I
found are just minor optional suggestions (or personal taste).

Thank you very mich for going over the code!

Thanks a lot for fixing this! It looks much better than my first approach.

Reviewed-by: David Herrmann <redacted>

Regards
David

-- 
Dmitry