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
> <[email protected]> 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.
Right, but I prefer doing this to signify that the value in 'minor' is
actually an error code.
>
> [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.
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]
>
> > @@ -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]
>
> > @@ -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 <[email protected]>
>
> Regards
> David
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
