Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755813AbbHYPE0 (ORCPT ); Tue, 25 Aug 2015 11:04:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52600 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755410AbbHYPEY (ORCPT ); Tue, 25 Aug 2015 11:04:24 -0400 Date: Tue, 25 Aug 2015 11:04:21 -0400 From: Benjamin Tissoires To: David Herrmann Cc: Dmitry Torokhov , Peter Hutterer , "open list:HID CORE LAYER" , linux-kernel Subject: Re: [PATCH v2] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl Message-ID: <20150825150421.GB12421@mail.corp.redhat.com> References: <1440167783-5628-1-git-send-email-benjamin.tissoires@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13012 Lines: 312 On Aug 24 2015 or thereabouts, David Herrmann wrote: > Hi > > On Fri, Aug 21, 2015 at 4:36 PM, Benjamin Tissoires > wrote: > > This adds two new ioctls, UINPUT_DEV_SETUP and UI_ABS_SETUP, that > > replaces the old device setup method (by write()'ing "struct > > uinput_user_dev" to the node). The old method is not easily extendable > > and requires huge payloads. Furthermore, overloading write() without > > properly versioned objects is error-prone. > > > > Therefore, we introduce two new ioctls to replace the old method. > > These ioctls support all features of the old method, plus a "resolution" > > field for absinfo. Furthermore, it's properly forward-compatible to new > > ABS codes and a growing "struct input_absinfo" structure. > > > > UI_ABS_SETUP also allows user-space to skip unknown axes if not set. > > There is no need to copy the whole array temporarily into the kernel, > > but instead the caller issues several ioctl where we copy each value > > manually. > > > > Signed-off-by: David Herrmann > > Signed-off-by: Benjamin Tissoires > > --- > > > > Hi, > > > > this is the v2 of the patch David submitted last year. I tried to take > > Dmitry's remarks into account and came out with this. > > > > Cheers, > > Benjamin > > > > drivers/input/misc/uinput.c | 83 +++++++++++++++++++++++++++++++++++++++++++-- > > include/linux/uinput.h | 5 +++ > > include/uapi/linux/uinput.h | 83 +++++++++++++++++++++++++++++++++++++++++++-- > > 3 files changed, 165 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c > > index 345df9b..afbcc41 100644 > > --- a/drivers/input/misc/uinput.c > > +++ b/drivers/input/misc/uinput.c > > @@ -370,8 +370,76 @@ static int uinput_allocate_device(struct uinput_device *udev) > > return 0; > > } > > > > -static int uinput_setup_device(struct uinput_device *udev, > > - const char __user *buffer, size_t count) > > +static int uinput_dev_setup(struct uinput_device *udev, > > + struct uinput_setup __user *arg) > > +{ > > + struct uinput_setup setup; > > + struct input_dev *dev; > > + int retval; > > + > > + if (udev->state == UIST_CREATED) > > + return -EINVAL; > > + if (copy_from_user(&setup, arg, sizeof(setup))) > > + return -EFAULT; > > + if (!setup.name[0]) > > + return -EINVAL; > > + > > + dev = udev->dev; > > + dev->id = setup.id; > > + udev->ff_effects_max = setup.ff_effects_max; > > + > > + kfree(dev->name); > > + dev->name = kstrndup(setup.name, UINPUT_MAX_NAME_SIZE, GFP_KERNEL); > > + if (!dev->name) > > + return -ENOMEM; > > + > > + retval = uinput_validate_absbits(dev); > > + if (retval < 0) > > + return retval; > > + > > + udev->state = UIST_SETUP_COMPLETE; > > + return 0; > > +} > > + > > +static int uinput_abs_setup(struct uinput_device *udev, > > + struct uinput_setup __user *arg, size_t size) > > +{ > > + struct uinput_abs_setup setup; > > + struct input_dev *dev; > > + > > + memset(&setup, 0, sizeof(setup)); > > This could be simplified as: > > struct uinput_abs_setup setup = {}; > > > + > > + if (size > sizeof(setup)) > > + return -EINVAL; > > Maybe use something more specific here. This is something user-space > may want to match for if the ioctl struct gets extended. E2BIG? > > > + if (udev->state == UIST_CREATED) > > + return -EINVAL; > > + if (copy_from_user(&setup, arg, size)) > > + return -EFAULT; > > + if (setup.code > ABS_MAX) > > + return -ERANGE; > > + > > + dev = udev->dev; > > + > > + input_alloc_absinfo(dev); > > + if (!dev->absinfo) > > + return -ENOMEM; > > + > > + set_bit(setup.code, dev->absbit); > > + > > If you do a second round of this patch, maybe drop that newline? > > > + dev->absinfo[setup.code] = setup.absinfo; > > + > > + /* > > + * We restore the state to UIST_NEW_DEVICE because the user has to call > > + * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the > > + * validity of the absbits. > > + */ > > + udev->state = UIST_NEW_DEVICE; > > + return 0; > > +} > > + > > +/* legacy setup via write() */ > > +static int uinput_setup_device_legacy(struct uinput_device *udev, > > + const char __user *buffer, size_t count) > > { > > struct uinput_user_dev *user_dev; > > struct input_dev *dev; > > @@ -474,7 +542,7 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer, > > > > retval = udev->state == UIST_CREATED ? > > uinput_inject_events(udev, buffer, count) : > > - uinput_setup_device(udev, buffer, count); > > + uinput_setup_device_legacy(udev, buffer, count); > > > > mutex_unlock(&udev->mutex); > > > > @@ -735,6 +803,12 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd, > > uinput_destroy_device(udev); > > goto out; > > > > + case UI_DEV_SETUP: > > + retval = uinput_dev_setup(udev, p); > > + goto out; > > + > > + /* UI_ABS_SETUP is handled in the variable size ioctls */ > > + > > case UI_SET_EVBIT: > > retval = uinput_set_bit(arg, evbit, EV_MAX); > > goto out; > > @@ -879,6 +953,9 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd, > > name = dev_name(&udev->dev->dev); > > retval = uinput_str_to_user(p, name, size); > > goto out; > > + case UI_ABS_SETUP & ~IOCSIZE_MASK: > > + retval = uinput_abs_setup(udev, p, size); > > + goto out; > > } > > > > retval = -EINVAL; > > diff --git a/include/linux/uinput.h b/include/linux/uinput.h > > index 0994c0d..75de43d 100644 > > --- a/include/linux/uinput.h > > +++ b/include/linux/uinput.h > > @@ -20,6 +20,11 @@ > > * Author: Aristeu Sergio Rozanski Filho > > * > > * Changes/Revisions: > > + * 0.5 08/13/2015 (David Herrmann & > > + * Benjamin Tissoires ) > > + * - add UI_DEV_SETUP ioctl > > + * - add UI_ABS_SETUP ioctl > > + * - add UI_GET_VERSION ioctl > > * 0.4 01/09/2014 (Benjamin Tissoires ) > > * - add UI_GET_SYSNAME ioctl > > * 0.3 24/05/2006 (Anssi Hannula ) > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h > > index 013c9d8..ef6c9f5 100644 > > --- a/include/uapi/linux/uinput.h > > +++ b/include/uapi/linux/uinput.h > > @@ -20,6 +20,11 @@ > > * Author: Aristeu Sergio Rozanski Filho > > * > > * Changes/Revisions: > > + * 0.5 08/13/2015 (David Herrmann & > > + * Benjamin Tissoires ) > > + * - add UI_DEV_SETUP ioctl > > + * - add UI_ABS_SETUP ioctl > > + * - add UI_GET_VERSION ioctl > > * 0.4 01/09/2014 (Benjamin Tissoires ) > > * - add UI_GET_SYSNAME ioctl > > * 0.3 24/05/2006 (Anssi Hannula ) > > @@ -37,8 +42,8 @@ > > #include > > #include > > > > -#define UINPUT_VERSION 4 > > - > > +#define UINPUT_VERSION 5 > > +#define UINPUT_MAX_NAME_SIZE 80 > > > > struct uinput_ff_upload { > > __u32 request_id; > > @@ -58,6 +63,79 @@ struct uinput_ff_erase { > > #define UI_DEV_CREATE _IO(UINPUT_IOCTL_BASE, 1) > > #define UI_DEV_DESTROY _IO(UINPUT_IOCTL_BASE, 2) > > > > +struct uinput_setup { > > + struct input_id id; > > + char name[UINPUT_MAX_NAME_SIZE]; > > + __u32 ff_effects_max; > > +}; > > + > > +/** > > + * UI_DEV_SETUP - Set device parameters for setup > > + * > > + * This ioctl sets parameters for the input-device to be created. It must be > > + * issued *before* calling UI_DEV_CREATE or it will fail. This ioctl supersedes > > + * the old "struct uinput_user_dev" method, which wrote this data via write(). > > + * To actually set the absolute axes, you also need to call the ioctl > > + * UI_ABS_SETUP *before* calling this ioctl. > > + * > > + * This ioctl takes a "struct uinput_setup" object as argument. The fields of > > + * this object are as follows: > > + * id: See the description of "struct input_id". This field is > > + * copied unchanged into the new device. > > + * name: This is used unchanged as name for the new device. > > + * ff_effects_max: This limits the maximum numbers of force-feedback effects. > > + * See below for a description of FF with uinput. > > + * > > + * This ioctl can be called multiple times and will overwrite previous values. > > + * If this ioctl fails with -EINVAL, you're recommended to use the old > > + * "uinput_user_dev" method via write() as fallback, in case you run on an old > > + * kernel that does not support this ioctl. > > + * > > + * This ioctl may fail with -EINVAL if it is not supported or if you passed > > + * incorrect values, -ENOMEM if the kernel runs out of memory or -EFAULT if the > > + * passed uinput_setup object cannot be read/written. > > + * If this call fails, partial data may have already been applied to the > > + * internal device. > > + */ > > +#define UI_DEV_SETUP _IOW(UINPUT_IOCTL_BASE, 3, struct uinput_setup) > > + > > +struct uinput_abs_setup { > > + __u16 code; /* axis code */ > > + /* __u16 filler; */ > > + struct input_absinfo absinfo; > > +}; > > + > > +/** > > + * UI_ABS_SETUP - Set absolute axis information for the device to setup > > + * > > + * This ioctl sets one absolute axis information for the input-device to be > > + * created. It must be issued *before* calling UI_DEV_SETUP and UI_DEV_CREATE > > + * for every absolute axis the device exports. > > + * This ioctl supersedes the old "struct uinput_user_dev" method, which wrote > > + * part of this data and the content of UI_DEV_SETUP via write(). > > + * > > + * This ioctl takes a "struct uinput_abs_setup" object as argument. The fields > > + * of this object are as follows: > > + * code: The corresponding input code associated with this axis > > + * (ABS_X, ABS_Y, etc...) > > + * absinfo: See "struct input_absinfo" for a description of this field. > > + * This field is copied unchanged into the kernel for the > > + * specified axis. If the axis is not enabled via > > + * UI_SET_ABSBIT, this ioctl will enable it. > > + * > > + * This ioctl can be called multiple times and will overwrite previous values. > > + * If this ioctl fails with -EINVAL, you're recommended to use the old > > + * "uinput_user_dev" method via write() as fallback, in case you run on an old > > + * kernel that does not support this ioctl. > > + * > > + * This ioctl may fail with -EINVAL if it is not supported or if you passed > > + * incorrect values, -ENOMEM if the kernel runs out of memory or -EFAULT if the > > + * passed uinput_setup object cannot be read/written. > > + * If this call fails, partial data may have already been applied to the > > + * internal device. > > + */ > > +#define UI_ABS_SETUP _IOW(UINPUT_IOCTL_BASE, 4, struct uinput_abs_setup) > > + > > #define UI_SET_EVBIT _IOW(UINPUT_IOCTL_BASE, 100, int) > > #define UI_SET_KEYBIT _IOW(UINPUT_IOCTL_BASE, 101, int) > > #define UI_SET_RELBIT _IOW(UINPUT_IOCTL_BASE, 102, int) > > @@ -144,7 +222,6 @@ struct uinput_ff_erase { > > #define UI_FF_UPLOAD 1 > > #define UI_FF_ERASE 2 > > > > -#define UINPUT_MAX_NAME_SIZE 80 > > struct uinput_user_dev { > > char name[UINPUT_MAX_NAME_SIZE]; > > struct input_id id; > > Looks good to me! Thanks for picking it up! Feel free to append: > > Reviewed-by: David Herrmann Thanks for your comments David. I'll send a new version right away with your suggestions. Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/