| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 23 Jun 2014 17:18:01 -0700
From: Ping Cheng <pinglinux@...il.com>
To: Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Jason Gerecke <killertofu@...il.com>,
linux-input <linux-input@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
linuxwacom-devel <linuxwacom-devel@...ts.sourceforge.net>
Subject: Re: [PATCH 1/5] Input - wacom: create a separate input device for pads
Hi Benjamin,
On Mon, Jun 23, 2014 at 1:57 PM, Benjamin Tissoires
<benjamin.tissoires@...hat.com> wrote:
> Currently, the pad events are sent through the stylus input device
> for the Intuos/Cintiqs, and through the touch input device for the
> Bamboos.
>
> To differentiate the buttons pressed on the pad from the ones pressed
> on the stylus, the Intuos/Cintiq uses MISC_SERIAL and ABS_MISC. This
> lead to a multiplexing of the events into one device, which are then
> splitted out in xf86-input-wacom. Bamboos are not using MISC events
> because the pad is attached to the touch interface, and only BTN_TOUCH
> is used for the finger (and DOUBLE_TAP, etc...). However, the user space
> driver still splits out the pad from the touch interface in the same
> way it does for the pro line devices.
>
> The other problem we can see with this fact is that some of the Intuos
> and Cintiq have a wheel, and the effective range of the reported values
> is [0..71]. Unfortunately, the airbrush stylus also sends wheel events
> (there is a small wheel on it), but in the range [0..1023]. From the user
> space point of view it is kind of difficult to understand that because
> the wheel on the pad are quite common, while the airbrush tool is not.
>
> A solution to fix all of these problems is to split out the pad device
> from the stylus/touch. This decision makes more sense because the pad is
> not linked to the absolute position of the finger or pen, and usually, the
> events from the pad are filtered out by the compositor, which then convert
> them into actions or keyboard shortcuts.
This is a very good solution. I like it.
> For backward compatibility with current xf86-input-wacom, the pad devices
> still present the ABS_X, ABS_Y and ABS_MISC events, but they can be
> completely ignored in the new implementation.
I do not think we need to keep ABS_X and ABS_Y for pad. We've already
supported a tablet (Intuos pen small, a pen only tablet with buttons
reported on touch interface) that does not set ABS_X and ABS_Y for
pad.
Unless you plan to use other means to tell userland those events are
from PAD tool, ABS_MISC is necessary and is a reasonable way to group
PAD events. So, I do not think it is for backward compatibility. It is
there to stay. With that said, the whole patchset is
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
Reviewed-by: Ping Cheng <pingc@...om.com>
Thank you, Benjamin, for your support.
Ping
> ---
> drivers/input/tablet/wacom.h | 2 ++
> drivers/input/tablet/wacom_sys.c | 63 +++++++++++++++++++++++++++++++++++-----
> drivers/input/tablet/wacom_wac.c | 27 ++++++++++++++++-
> drivers/input/tablet/wacom_wac.h | 2 ++
> 4 files changed, 85 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/tablet/wacom.h b/drivers/input/tablet/wacom.h
> index 9ebf0ed..caa59ca 100644
> --- a/drivers/input/tablet/wacom.h
> +++ b/drivers/input/tablet/wacom.h
> @@ -136,4 +136,6 @@ void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t len);
> void wacom_setup_device_quirks(struct wacom_features *features);
> int wacom_setup_input_capabilities(struct input_dev *input_dev,
> struct wacom_wac *wacom_wac);
> +int wacom_setup_pad_input_capabilities(struct input_dev *input_dev,
> + struct wacom_wac *wacom_wac);
> #endif
> diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
> index c993eee..b9bf37e 100644
> --- a/drivers/input/tablet/wacom_sys.c
> +++ b/drivers/input/tablet/wacom_sys.c
> @@ -135,6 +135,9 @@ static int wacom_open(struct input_dev *dev)
>
> mutex_lock(&wacom->lock);
>
> + if (wacom->open)
> + goto out;
> +
> if (usb_submit_urb(wacom->irq, GFP_KERNEL)) {
> retval = -EIO;
> goto out;
> @@ -157,9 +160,14 @@ static void wacom_close(struct input_dev *dev)
> autopm_error = usb_autopm_get_interface(wacom->intf);
>
> mutex_lock(&wacom->lock);
> + if (!wacom->open)
> + goto out;
> +
> usb_kill_urb(wacom->irq);
> wacom->open = false;
> wacom->intf->needs_remote_wakeup = 0;
> +
> +out:
> mutex_unlock(&wacom->lock);
>
> if (!autopm_error)
> @@ -1109,19 +1117,16 @@ static void wacom_destroy_battery(struct wacom *wacom)
> }
> }
>
> -static int wacom_register_input(struct wacom *wacom)
> +static struct input_dev *wacom_allocate_input(struct wacom *wacom)
> {
> struct input_dev *input_dev;
> struct usb_interface *intf = wacom->intf;
> struct usb_device *dev = interface_to_usbdev(intf);
> struct wacom_wac *wacom_wac = &(wacom->wacom_wac);
> - int error;
>
> input_dev = input_allocate_device();
> - if (!input_dev) {
> - error = -ENOMEM;
> - goto fail1;
> - }
> + if (!input_dev)
> + return NULL;
>
> input_dev->name = wacom_wac->name;
> input_dev->phys = wacom->phys;
> @@ -1131,21 +1136,59 @@ static int wacom_register_input(struct wacom *wacom)
> usb_to_input_id(dev, &input_dev->id);
> input_set_drvdata(input_dev, wacom);
>
> + return input_dev;
> +}
> +
> +static int wacom_register_input(struct wacom *wacom)
> +{
> + struct input_dev *input_dev, *pad_input_dev;
> + struct wacom_wac *wacom_wac = &(wacom->wacom_wac);
> + int error;
> +
> + input_dev = wacom_allocate_input(wacom);
> + pad_input_dev = wacom_allocate_input(wacom);
> + if (!input_dev || !pad_input_dev) {
> + error = -ENOMEM;
> + goto fail1;
> + }
> +
> wacom_wac->input = input_dev;
> + wacom_wac->pad_input = pad_input_dev;
> + wacom_wac->pad_input->name = wacom_wac->pad_name;
> +
> error = wacom_setup_input_capabilities(input_dev, wacom_wac);
> if (error)
> - goto fail1;
> + goto fail2;
>
> error = input_register_device(input_dev);
> if (error)
> goto fail2;
>
> + error = wacom_setup_pad_input_capabilities(pad_input_dev, wacom_wac);
> + if (error) {
> + /* no pad in use on this interface */
> + input_free_device(pad_input_dev);
> + wacom_wac->pad_input = NULL;
> + pad_input_dev = NULL;
> + } else {
> + error = input_register_device(pad_input_dev);
> + if (error)
> + goto fail3;
> + }
> +
> return 0;
>
> +fail3:
> + input_unregister_device(input_dev);
> + input_dev = NULL;
> fail2:
> - input_free_device(input_dev);
> wacom_wac->input = NULL;
> + wacom_wac->pad_input = NULL;
> fail1:
> + if (input_dev)
> + input_free_device(input_dev);
> + if (pad_input_dev)
> + input_free_device(pad_input_dev);
> return error;
> }
>
> @@ -1364,6 +1407,8 @@ static int wacom_probe(struct usb_interface *intf, const struct usb_device_id *i
> wacom_calculate_res(features);
>
> strlcpy(wacom_wac->name, features->name, sizeof(wacom_wac->name));
> + snprintf(wacom_wac->pad_name, sizeof(wacom_wac->pad_name),
> + "%s Pad", features->name);
>
> if (features->quirks & WACOM_QUIRK_MULTI_INPUT) {
> struct usb_device *other_dev;
> @@ -1438,6 +1483,8 @@ static void wacom_disconnect(struct usb_interface *intf)
> cancel_work_sync(&wacom->work);
> if (wacom->wacom_wac.input)
> input_unregister_device(wacom->wacom_wac.input);
> + if (wacom->wacom_wac.pad_input)
> + input_unregister_device(wacom->wacom_wac.pad_input);
> wacom_destroy_battery(wacom);
> wacom_destroy_leds(wacom);
> usb_free_urb(wacom->irq);
> diff --git a/drivers/input/tablet/wacom_wac.c b/drivers/input/tablet/wacom_wac.c
> index 977d05c..4b16a34 100644
> --- a/drivers/input/tablet/wacom_wac.c
> +++ b/drivers/input/tablet/wacom_wac.c
> @@ -1489,8 +1489,11 @@ void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t len)
> break;
> }
>
> - if (sync)
> + if (sync) {
> input_sync(wacom_wac->input);
> + if (wacom_wac->pad_input)
> + input_sync(wacom_wac->pad_input);
> + }
> }
>
> static void wacom_setup_cintiq(struct wacom_wac *wacom_wac)
> @@ -1939,6 +1942,28 @@ int wacom_setup_input_capabilities(struct input_dev *input_dev,
> return 0;
> }
>
> +int wacom_setup_pad_input_capabilities(struct input_dev *input_dev,
> + struct wacom_wac *wacom_wac)
> +{
> + struct wacom_features *features = &wacom_wac->features;
> +
> + input_dev->evbit[0] |= BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +
> + /* kept for making legacy xf86-input-wacom working with the wheels */
> + __set_bit(ABS_MISC, input_dev->absbit);
> +
> + /* kept for making legacy xf86-input-wacom accepting the pad */
> + input_set_abs_params(input_dev, ABS_X, 0, 1, 0, 0);
> + input_set_abs_params(input_dev, ABS_Y, 0, 1, 0, 0);
> +
> + switch (features->type) {
> + default:
> + /* no pad supported */
> + return 1;
> + }
> + return 0;
> +}
> +
> static const struct wacom_features wacom_features_0x00 =
> { "Wacom Penpartner", WACOM_PKGLEN_PENPRTN, 5040, 3780, 255,
> 0, PENPARTNER, WACOM_PENPRTN_RES, WACOM_PENPRTN_RES };
> diff --git a/drivers/input/tablet/wacom_wac.h b/drivers/input/tablet/wacom_wac.h
> index b2c9a9c..f48164c 100644
> --- a/drivers/input/tablet/wacom_wac.h
> +++ b/drivers/input/tablet/wacom_wac.h
> @@ -150,6 +150,7 @@ struct wacom_shared {
>
> struct wacom_wac {
> char name[WACOM_NAME_MAX];
> + char pad_name[WACOM_NAME_MAX];
> unsigned char *data;
> int tool[2];
> int id[2];
> @@ -157,6 +158,7 @@ struct wacom_wac {
> struct wacom_features features;
> struct wacom_shared *shared;
> struct input_dev *input;
> + struct input_dev *pad_input;
> int pid;
> int battery_capacity;
> int num_contacts_left;
> --
> 1.9.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists