[PATCH v2 0/3] Input/HID: joydev fixes for motion sensors

[PATCH v2 0/3] Input/HID: joydev fixes for motion sensors

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Hi,

Some weeks ago we submitted an earlier version of this patch set,
which attempted to blacklist dualshock 3 / 4 motion sensor devices
from joydev. The motion sensor devices got picked up since the hid-sony
driver in recent months split the motion sensors of in separate devices.

The earlier version of this patch set, added a filter to joydev to
ignore devices which have INPUT_PROP_ACCELEROMETER set. Dmitry pointed
out that often you could use a motion sensor device as a joystick. He
felt the issue is with composite devices.

The discussion didn't result in a conclusion. This patch set only
filters out motion sensors if they are part of a composite device.
Since there is no way during driver initialization to determine
whether we are dealing with a composite device, we introduce a new
property INPUT_PROP_COMPOSITE to determine this. I think having such
flag is beneficial for userspace as well, since applications now get a hint
that a device is part of a composite device without having to infer
this from a EVIOCGPHYS / EVIOCGUINIQ match across devices.

Hopefully this patches will be accepted for 4.14, but maybe earlier if
still possible as the next wave of distributions will likely be on 4.13
with more users dealing with this issue.

Thanks,
Roderick


Roderick Colenbrander (3):
  Input: Add new property INPUT_PROP_COMPOSITE
  HID: sony: Set INPUT_PROP_COMPOSITE flag on sub devices.
  Input: joydev - ignore accelerometer devices

 Documentation/input/event-codes.rst    | 12 ++++++++++++
 drivers/hid/hid-sony.c                 |  8 ++++++++
 drivers/input/joydev.c                 |  5 +++++
 include/uapi/linux/input-event-codes.h |  1 +
 4 files changed, 26 insertions(+)

-- 
2.9.4

[PATCH 1/3] Input: Add new property INPUT_PROP_COMPOSITE

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

This new property can be set on subdevices part of a composite
input device. The purpose of the constant is to notify other
kernel components (e.g. joydev) and user space that a device is
a composite device.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 Documentation/input/event-codes.rst    | 12 ++++++++++++
 include/uapi/linux/input-event-codes.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
index a8c0873..d56a32e 100644
--- a/Documentation/input/event-codes.rst
+++ b/Documentation/input/event-codes.rst
@@ -356,6 +356,18 @@ can report through the rotational axes (absolute and/or relative rx, ry, rz).
 All other axes retain their meaning. A device must not mix
 regular directional axes and accelerometer axes on the same event node.
 
+INPUT_PROP_COMPOSITE
+--------------------
+
+The functionality of some advanced input devices is exposed through several
+event devices. For example a gamepad with motion sensors could be exposed as
+a gamepad device with axes / buttons and a separate motion sensor device.
+
+The INPUT_PROP_COMPOSITE flag is set on all sub devices making up a larger
+composite device. This flag allows user space applications in addition to
+other kernel drivers (e.g. joydev) to easily determine whether a device
+is part of a composite device.
+
 Guidelines
 ==========
 
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 1798910..da3fa83 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -26,6 +26,7 @@
 #define INPUT_PROP_TOPBUTTONPAD		0x04	/* softbuttons at top of pad */
 #define INPUT_PROP_POINTING_STICK	0x05	/* is a pointing stick */
 #define INPUT_PROP_ACCELEROMETER	0x06	/* has accelerometer */
+#define INPUT_PROP_COMPOSITE		0x07	/* is part of a composite device */
 
 #define INPUT_PROP_MAX			0x1f
 #define INPUT_PROP_CNT			(INPUT_PROP_MAX + 1)
-- 
2.9.4

Re: [PATCH 1/3] Input: Add new property INPUT_PROP_COMPOSITE

[Peter Hutterer]

On Thu, Aug 17, 2017 at 07:01:54PM -0700, Roderick Colenbrander wrote:
> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> This new property can be set on subdevices part of a composite
> input device. The purpose of the constant is to notify other
> kernel components (e.g. joydev) and user space that a device is
> a composite device.
> 
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> ---
>  Documentation/input/event-codes.rst    | 12 ++++++++++++
>  include/uapi/linux/input-event-codes.h |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
> index a8c0873..d56a32e 100644
> --- a/Documentation/input/event-codes.rst
> +++ b/Documentation/input/event-codes.rst
> @@ -356,6 +356,18 @@ can report through the rotational axes (absolute and/or relative rx, ry, rz).
>  All other axes retain their meaning. A device must not mix
>  regular directional axes and accelerometer axes on the same event node.
>  
> +INPUT_PROP_COMPOSITE
> +--------------------
> +
> +The functionality of some advanced input devices is exposed through several
> +event devices. For example a gamepad with motion sensors could be exposed as
> +a gamepad device with axes / buttons and a separate motion sensor device.
> +
> +The INPUT_PROP_COMPOSITE flag is set on all sub devices making up a larger
> +composite device. This flag allows user space applications in addition to
> +other kernel drivers (e.g. joydev) to easily determine whether a device
> +is part of a composite device.

honestly, I would love for this to be paired with an ioctl that can give me
the sysnames of the other devices. Or (probably safer) just a unique
number/string that all composite devices share.

That way I can reassemble this in userspace. We already do something like
this for the libinput device groups and I think just having the input
property isn't quite enough. We can already guess a lot of this from the
PHYS attributes but there are several devices where having this filled by
the kernel is going to be useful. Wacom devices for example where the pen
and touch device don't share the same pid.

Right now, it's a bit too simple - all this flag gives me is the
notification that I have to start looking for other devices. Works fine with
one composited device connected, but once you look at more than that it
doesn't seem to be overly useful.

Cheers,
   Peter


> +
>  Guidelines
>  ==========
>  
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 1798910..da3fa83 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -26,6 +26,7 @@
>  #define INPUT_PROP_TOPBUTTONPAD		0x04	/* softbuttons at top of pad */
>  #define INPUT_PROP_POINTING_STICK	0x05	/* is a pointing stick */
>  #define INPUT_PROP_ACCELEROMETER	0x06	/* has accelerometer */
> +#define INPUT_PROP_COMPOSITE		0x07	/* is part of a composite device */
>  
>  #define INPUT_PROP_MAX			0x1f
>  #define INPUT_PROP_CNT			(INPUT_PROP_MAX + 1)
> -- 
> 2.9.4

Re: [PATCH 1/3] Input: Add new property INPUT_PROP_COMPOSITE

[Colenbrander, Roelof]

On 08/17/2017 08:25 PM, Peter Hutterer wrote:
> On Thu, Aug 17, 2017 at 07:01:54PM -0700, Roderick Colenbrander wrote:
>> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>>
>> This new property can be set on subdevices part of a composite
>> input device. The purpose of the constant is to notify other
>> kernel components (e.g. joydev) and user space that a device is
>> a composite device.
>>
>> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
>> ---
>>   Documentation/input/event-codes.rst    | 12 ++++++++++++
>>   include/uapi/linux/input-event-codes.h |  1 +
>>   2 files changed, 13 insertions(+)
>>
>> diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
>> index a8c0873..d56a32e 100644
>> --- a/Documentation/input/event-codes.rst
>> +++ b/Documentation/input/event-codes.rst
>> @@ -356,6 +356,18 @@ can report through the rotational axes (absolute and/or relative rx, ry, rz).
>>   All other axes retain their meaning. A device must not mix
>>   regular directional axes and accelerometer axes on the same event node.
>>   
>> +INPUT_PROP_COMPOSITE
>> +--------------------
>> +
>> +The functionality of some advanced input devices is exposed through several
>> +event devices. For example a gamepad with motion sensors could be exposed as
>> +a gamepad device with axes / buttons and a separate motion sensor device.
>> +
>> +The INPUT_PROP_COMPOSITE flag is set on all sub devices making up a larger
>> +composite device. This flag allows user space applications in addition to
>> +other kernel drivers (e.g. joydev) to easily determine whether a device
>> +is part of a composite device.
> 
> honestly, I would love for this to be paired with an ioctl that can give me
> the sysnames of the other devices. Or (probably safer) just a unique
> number/string that all composite devices share.

I agree enumerating composite devices is a pain, which requires looping 
over all evdev nodes and attempting to stitch them together based on 
ioctl return info or by manually scanning sysfs. On a sidenote, the 
process of finding sysfs node for a device as this often needed too for 
e.g. LEDs and device specific options is tricky too and could use 
improvements.

I'm trying to envision how a new ioctl for composite device enumeration 
would work. Even right now there are already fun race conditions as the 
composite devices come up in different order just, because they 
completed initialization at slightly different times.

Maybe there would be some 'evdev device group' concept with each evdev 
node storing this unique id. This would map to a unique location in 
sysfs, which contains the list of devices? The ioctl would just be used 
to query this id?


> 
> That way I can reassemble this in userspace. We already do something like
> this for the libinput device groups and I think just having the input
> property isn't quite enough. We can already guess a lot of this from the
> PHYS attributes but there are several devices where having this filled by
> the kernel is going to be useful. Wacom devices for example where the pen
> and touch device don't share the same pid.

I'm not too familiar with the Wacom devices, the composite device show 
up as different devices on a bus (e.g. USB / Bluetooth / ..) or is there 
a single physical device, but the driver somehow exposes different sub 
devices through some vendor specific method?

> 
> Right now, it's a bit too simple - all this flag gives me is the
> notification that I have to start looking for other devices. Works fine with
> one composited device connected, but once you look at more than that it
> doesn't seem to be overly useful.

I understand a more sophisticated solution is desired and we don't help 
creating that. For us the original problem was that we expose dualshock 
3 / 4 subdevices in recent kernels for which the subdevices got picked 
up by joydev. We proposed code to limit accelerometer devices, but 
during review it was felt that the code didn't narrow it down enough. We 
felt joydev would cause issues for users and then recently we had this 
reddit thread 
(https://www.reddit.com/r/linux_gaming/comments/6rxhj8/what_happened_with_the_dualshock_4_and_the_new/) 
with complaining users (SDL2 we took care of last year, but they still 
haven't released 2.0.6 yet). Since 4.13 or 4.14 will be used in a lot of 
upcoming distributions, we like to consider at least getting this 
initial part in, if it is considered to be a stepping stone towards the 
full solution of adding an ioctl as well.


> 
> Cheers,
>     Peter
> 
> 
>> +
>>   Guidelines
>>   ==========
>>   
>> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
>> index 1798910..da3fa83 100644
>> --- a/include/uapi/linux/input-event-codes.h
>> +++ b/include/uapi/linux/input-event-codes.h
>> @@ -26,6 +26,7 @@
>>   #define INPUT_PROP_TOPBUTTONPAD		0x04	/* softbuttons at top of pad */
>>   #define INPUT_PROP_POINTING_STICK	0x05	/* is a pointing stick */
>>   #define INPUT_PROP_ACCELEROMETER	0x06	/* has accelerometer */
>> +#define INPUT_PROP_COMPOSITE		0x07	/* is part of a composite device */
>>   
>>   #define INPUT_PROP_MAX			0x1f
>>   #define INPUT_PROP_CNT			(INPUT_PROP_MAX + 1)
>> -- 
>> 2.9.4
> 
> 

Re: [PATCH 1/3] Input: Add new property INPUT_PROP_COMPOSITE

[Peter Hutterer]

On Tue, Aug 22, 2017 at 11:34:17PM +0000, Colenbrander, Roelof wrote:
> On 08/17/2017 08:25 PM, Peter Hutterer wrote:
> > On Thu, Aug 17, 2017 at 07:01:54PM -0700, Roderick Colenbrander wrote:
> >> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> >>
> >> This new property can be set on subdevices part of a composite
> >> input device. The purpose of the constant is to notify other
> >> kernel components (e.g. joydev) and user space that a device is
> >> a composite device.
> >>
> >> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> >> ---
> >>   Documentation/input/event-codes.rst    | 12 ++++++++++++
> >>   include/uapi/linux/input-event-codes.h |  1 +
> >>   2 files changed, 13 insertions(+)
> >>
> >> diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
> >> index a8c0873..d56a32e 100644
> >> --- a/Documentation/input/event-codes.rst
> >> +++ b/Documentation/input/event-codes.rst
> >> @@ -356,6 +356,18 @@ can report through the rotational axes (absolute and/or relative rx, ry, rz).
> >>   All other axes retain their meaning. A device must not mix
> >>   regular directional axes and accelerometer axes on the same event node.
> >>   
> >> +INPUT_PROP_COMPOSITE
> >> +--------------------
> >> +
> >> +The functionality of some advanced input devices is exposed through several
> >> +event devices. For example a gamepad with motion sensors could be exposed as
> >> +a gamepad device with axes / buttons and a separate motion sensor device.
> >> +
> >> +The INPUT_PROP_COMPOSITE flag is set on all sub devices making up a larger
> >> +composite device. This flag allows user space applications in addition to
> >> +other kernel drivers (e.g. joydev) to easily determine whether a device
> >> +is part of a composite device.
> > 
> > honestly, I would love for this to be paired with an ioctl that can give me
> > the sysnames of the other devices. Or (probably safer) just a unique
> > number/string that all composite devices share.
> 
> I agree enumerating composite devices is a pain, which requires looping 
> over all evdev nodes and attempting to stitch them together based on 
> ioctl return info or by manually scanning sysfs. On a sidenote, the 
> process of finding sysfs node for a device as this often needed too for 
> e.g. LEDs and device specific options is tricky too and could use 
> improvements.
> 
> I'm trying to envision how a new ioctl for composite device enumeration 
> would work. Even right now there are already fun race conditions as the 
> composite devices come up in different order just, because they 
> completed initialization at slightly different times.
> 
> Maybe there would be some 'evdev device group' concept with each evdev 
> node storing this unique id. This would map to a unique location in 
> sysfs, which contains the list of devices? The ioctl would just be used 
> to query this id?

I think realistically the device group approach is the only one not prone to
race conditions. It can also easily be added to the udev properties so it's
accessible from userspace. I don't think a sysfs location to have a list of
device is feasable, it's a lot easier to just export this and let the
client cobble it back together.

As soon as you try to provide a list, you're likely running into similar
race conditions.

> > That way I can reassemble this in userspace. We already do something like
> > this for the libinput device groups and I think just having the input
> > property isn't quite enough. We can already guess a lot of this from the
> > PHYS attributes but there are several devices where having this filled by
> > the kernel is going to be useful. Wacom devices for example where the pen
> > and touch device don't share the same pid.
> 
> I'm not too familiar with the Wacom devices, the composite device show 
> up as different devices on a bus (e.g. USB / Bluetooth / ..) or is there 
> a single physical device, but the driver somehow exposes different sub 
> devices through some vendor specific method?

it's one big physical device, but internally it's two devices (touch and
the tablet), and the tablet is usually on two evdev nodes (pen and pad).
On some devices the touch has a different pid.

right now libinput creates a device group based on the PHYS string we get
back with minor modifications. That usually contains the pid/vid but we have
libwacom, so we can modify the pid for the tablet device to match the touch
and everything is some value of happy again.

it's a fairly simple approach, see the mirror here:
https://github.com/wayland-project/libinput/blob/master/udev/libinput-device-group.c

> > Right now, it's a bit too simple - all this flag gives me is the
> > notification that I have to start looking for other devices. Works fine with
> > one composited device connected, but once you look at more than that it
> > doesn't seem to be overly useful.
> 
> I understand a more sophisticated solution is desired and we don't help 
> creating that. For us the original problem was that we expose dualshock 
> 3 / 4 subdevices in recent kernels for which the subdevices got picked 
> up by joydev. We proposed code to limit accelerometer devices, but 
> during review it was felt that the code didn't narrow it down enough. We 
> felt joydev would cause issues for users and then recently we had this 
> reddit thread 
> (https://www.reddit.com/r/linux_gaming/comments/6rxhj8/what_happened_with_the_dualshock_4_and_the_new/) 
> with complaining users (SDL2 we took care of last year, but they still 
> haven't released 2.0.6 yet). Since 4.13 or 4.14 will be used in a lot of 
> upcoming distributions, we like to consider at least getting this 
> initial part in, if it is considered to be a stepping stone towards the 
> full solution of adding an ioctl as well.

well, the thing is, if we add something like EVIOCGGROUP, then we don't need
an additional property. So I'm not sure we should double up here.

Cheers,
   Peter

> >> +
> >>   Guidelines
> >>   ==========
> >>   
> >> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> >> index 1798910..da3fa83 100644
> >> --- a/include/uapi/linux/input-event-codes.h
> >> +++ b/include/uapi/linux/input-event-codes.h
> >> @@ -26,6 +26,7 @@
> >>   #define INPUT_PROP_TOPBUTTONPAD		0x04	/* softbuttons at top of pad */
> >>   #define INPUT_PROP_POINTING_STICK	0x05	/* is a pointing stick */
> >>   #define INPUT_PROP_ACCELEROMETER	0x06	/* has accelerometer */
> >> +#define INPUT_PROP_COMPOSITE		0x07	/* is part of a composite device */
> >>   
> >>   #define INPUT_PROP_MAX			0x1f
> >>   #define INPUT_PROP_CNT			(INPUT_PROP_MAX + 1)
> >> -- 
> >> 2.9.4
> > 
> > 
> 
> 

[PATCH 2/3] HID: sony: Set INPUT_PROP_COMPOSITE flag on sub devices.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Configure the touchpad and motion sensor sub devices with the
INPUT_PROP_COMPOSITE flag. This flag will be consumed by joydev
to filter out the motion sensor device.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-sony.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index d03203a..a161093 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1276,6 +1276,7 @@ static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
 	__set_bit(EV_KEY, sc->touchpad->evbit);
 	__set_bit(BTN_LEFT, sc->touchpad->keybit);
 	__set_bit(INPUT_PROP_BUTTONPAD, sc->touchpad->propbit);
+	__set_bit(INPUT_PROP_COMPOSITE, sc->touchpad->propbit);
 
 	input_set_abs_params(sc->touchpad, ABS_MT_POSITION_X, 0, w, 0, 0);
 	input_set_abs_params(sc->touchpad, ABS_MT_POSITION_Y, 0, h, 0, 0);
@@ -1375,6 +1376,7 @@ static int sony_register_sensors(struct sony_sc *sc)
 	}
 
 	__set_bit(INPUT_PROP_ACCELEROMETER, sc->sensor_dev->propbit);
+	__set_bit(INPUT_PROP_COMPOSITE, sc->sensor_dev->propbit);
 
 	ret = input_register_device(sc->sensor_dev);
 	if (ret < 0)
@@ -2561,6 +2563,8 @@ static int sony_input_configured(struct hid_device *hdev,
 
 		sony_init_output_report(sc, sixaxis_send_output_report);
 	} else if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
+		__set_bit(INPUT_PROP_COMPOSITE, hidinput->input->propbit);
+
 		/*
 		 * The Sony Sixaxis does not handle HID Output Reports on the
 		 * Interrupt EP and the device only becomes active when the
@@ -2586,6 +2590,8 @@ static int sony_input_configured(struct hid_device *hdev,
 
 		sony_init_output_report(sc, sixaxis_send_output_report);
 	} else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
+		__set_bit(INPUT_PROP_COMPOSITE, hidinput->input->propbit);
+
 		/*
 		 * The Sixaxis wants output reports sent on the ctrl endpoint
 		 * when connected via Bluetooth.
@@ -2607,6 +2613,8 @@ static int sony_input_configured(struct hid_device *hdev,
 
 		sony_init_output_report(sc, sixaxis_send_output_report);
 	} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
+		__set_bit(INPUT_PROP_COMPOSITE, hidinput->input->propbit);
+
 		ret = dualshock4_get_calibration_data(sc);
 		if (ret < 0) {
 			hid_err(hdev, "Failed to get calibration data from Dualshock 4\n");
-- 
2.9.4

Re: [PATCH 2/3] HID: sony: Set INPUT_PROP_COMPOSITE flag on sub devices.

[Bastien Nocera]

On Thu, 2017-08-17 at 19:01 -0700, Roderick Colenbrander wrote:
> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> Configure the touchpad and motion sensor sub devices with the
> INPUT_PROP_COMPOSITE flag. This flag will be consumed by joydev
> to filter out the motion sensor device.

Could you please add the same to the hid-udraw-ps3.c driver?

Re: [PATCH 2/3] HID: sony: Set INPUT_PROP_COMPOSITE flag on sub devices.

[Roderick Colenbrander]

On Wed, Aug 23, 2017 at 1:50 AM, Bastien Nocera <hadess@hadess.net> wrote:
> On Thu, 2017-08-17 at 19:01 -0700, Roderick Colenbrander wrote:
>> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>>
>> Configure the touchpad and motion sensor sub devices with the
>> INPUT_PROP_COMPOSITE flag. This flag will be consumed by joydev
>> to filter out the motion sensor device.
>
> Could you please add the same to the hid-udraw-ps3.c driver?

Sure, if the flag gets accepted we could add it there.

[PATCH 3/3] Input: joydev - ignore accelerometer devices

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Gamepads like DualShock 3 / 4 as of 4.12 started reporting motion
sensors on a separate evdev node. Joydev is picking these devices
up as well, but they don't make sense for the joydev interface.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/input/joydev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 29d677c..9b70fe0 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -819,6 +819,11 @@ static bool joydev_match(struct input_handler *handler, struct input_dev *dev)
 	if (joydev_dev_is_absolute_mouse(dev))
 		return false;
 
+	/* Avoid accelerometers on composite devices. */
+	if (test_bit(INPUT_PROP_ACCELEROMETER, dev->propbit) &&
+	    test_bit(INPUT_PROP_COMPOSITE, dev->propbit))
+		return false;
+
 	return true;
 }
 
-- 
2.9.4

Re: [PATCH 3/3] Input: joydev - ignore accelerometer devices

[Bastien Nocera]

On Thu, 2017-08-17 at 19:01 -0700, Roderick Colenbrander wrote:
> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> Gamepads like DualShock 3 / 4 as of 4.12 started reporting motion
> sensors on a separate evdev node. Joydev is picking these devices
> up as well, but they don't make sense for the joydev interface.
> 
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> ---
>  drivers/input/joydev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
> index 29d677c..9b70fe0 100644
> --- a/drivers/input/joydev.c
> +++ b/drivers/input/joydev.c
> @@ -819,6 +819,11 @@ static bool joydev_match(struct input_handler
> *handler, struct input_dev *dev)
>  	if (joydev_dev_is_absolute_mouse(dev))
>  		return false;
>  
> +	/* Avoid accelerometers on composite devices. */
> +	if (test_bit(INPUT_PROP_ACCELEROMETER, dev->propbit) &&
> +	    test_bit(INPUT_PROP_COMPOSITE, dev->propbit))
> +		return false;

I don't understand how making a laptop's builtin hard-drive drop sensor
into a joystick device is acceptable, but the same device inside an
external joypad isn't.

Either all accelerometers are blocked through this interface, or none
are.

Re: [PATCH 3/3] Input: joydev - ignore accelerometer devices

[Roderick Colenbrander]

On Wed, Aug 23, 2017 at 1:45 AM, Bastien Nocera <hadess@hadess.net> wrote:
> On Thu, 2017-08-17 at 19:01 -0700, Roderick Colenbrander wrote:
>> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>>
>> Gamepads like DualShock 3 / 4 as of 4.12 started reporting motion
>> sensors on a separate evdev node. Joydev is picking these devices
>> up as well, but they don't make sense for the joydev interface.
>>
>> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
>> ---
>>  drivers/input/joydev.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
>> index 29d677c..9b70fe0 100644
>> --- a/drivers/input/joydev.c
>> +++ b/drivers/input/joydev.c
>> @@ -819,6 +819,11 @@ static bool joydev_match(struct input_handler
>> *handler, struct input_dev *dev)
>>       if (joydev_dev_is_absolute_mouse(dev))
>>               return false;
>>
>> +     /* Avoid accelerometers on composite devices. */
>> +     if (test_bit(INPUT_PROP_ACCELEROMETER, dev->propbit) &&
>> +         test_bit(INPUT_PROP_COMPOSITE, dev->propbit))
>> +             return false;
>
> I don't understand how making a laptop's builtin hard-drive drop sensor
> into a joystick device is acceptable, but the same device inside an
> external joypad isn't.
>
> Either all accelerometers are blocked through this interface, or none
> are.

I agree. The main question is probably what devices do we really want
to support through this legacy interface? It should mostly be used by
old applications by now, which probably can't handle motion anyway, so
why not drop it in any case. Also only 3 drivers are setting this flag
right now.

Re: [PATCH v2 0/3] Input/HID: joydev fixes for motion sensors

[Bastien Nocera]

On Thu, 2017-08-17 at 19:01 -0700, Roderick Colenbrander wrote:
> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> Hi,
> 
> Some weeks ago we submitted an earlier version of this patch set,
> which attempted to blacklist dualshock 3 / 4 motion sensor devices
> from joydev. The motion sensor devices got picked up since the hid-
> sony
> driver in recent months split the motion sensors of in separate
> devices.
> 
> The earlier version of this patch set, added a filter to joydev to
> ignore devices which have INPUT_PROP_ACCELEROMETER set. Dmitry
> pointed
> out that often you could use a motion sensor device as a joystick. He
> felt the issue is with composite devices.
> 
> The discussion didn't result in a conclusion. This patch set only
> filters out motion sensors if they are part of a composite device.
> Since there is no way during driver initialization to determine
> whether we are dealing with a composite device, we introduce a new
> property INPUT_PROP_COMPOSITE to determine this.

Would have a way to know how many pieces make up that composite device
be useful?

> I think having such
> flag is beneficial for userspace as well, since applications now get
> a hint
> that a device is part of a composite device without having to infer
> this from a EVIOCGPHYS / EVIOCGUINIQ match across devices.

Not that much, udev already does all that for user-space.

> Hopefully this patches will be accepted for 4.14, but maybe earlier
> if
> still possible as the next wave of distributions will likely be on
> 4.13
> with more users dealing with this issue.

I'm not sure I fully understand the problem you're trying to solve.

Is it "old software which relies on joydev is too basic to allow
selecting the correct joystick device"? Or "some system components are
picking up the joypad's accelerometer like it inferred the screen's
positioning"?

Cheers

Re: [PATCH v2 0/3] Input/HID: joydev fixes for motion sensors

[Roderick Colenbrander]

On Wed, Aug 23, 2017 at 2:04 AM, Bastien Nocera <hadess@hadess.net> wrote:
> On Thu, 2017-08-17 at 19:01 -0700, Roderick Colenbrander wrote:
>> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>>
>> Hi,
>>
>> Some weeks ago we submitted an earlier version of this patch set,
>> which attempted to blacklist dualshock 3 / 4 motion sensor devices
>> from joydev. The motion sensor devices got picked up since the hid-
>> sony
>> driver in recent months split the motion sensors of in separate
>> devices.
>>
>> The earlier version of this patch set, added a filter to joydev to
>> ignore devices which have INPUT_PROP_ACCELEROMETER set. Dmitry
>> pointed
>> out that often you could use a motion sensor device as a joystick. He
>> felt the issue is with composite devices.
>>
>> The discussion didn't result in a conclusion. This patch set only
>> filters out motion sensors if they are part of a composite device.
>> Since there is no way during driver initialization to determine
>> whether we are dealing with a composite device, we introduce a new
>> property INPUT_PROP_COMPOSITE to determine this.
>
> Would have a way to know how many pieces make up that composite device
> be useful?

Yes that would be quite useful, see the discussion with Peter in '1/3'
where he is proposing an ioctl as well.

>
>> I think having such
>> flag is beneficial for userspace as well, since applications now get
>> a hint
>> that a device is part of a composite device without having to infer
>> this from a EVIOCGPHYS / EVIOCGUINIQ match across devices.
>
> Not that much, udev already does all that for user-space.

We deal a lot with platforms on which we don't have udev and could see
using this. Also applications like Wine directly read evdev or
libraries like SDL2 it can be helpful maybe.

>
>> Hopefully this patches will be accepted for 4.14, but maybe earlier
>> if
>> still possible as the next wave of distributions will likely be on
>> 4.13
>> with more users dealing with this issue.
>
> I'm not sure I fully understand the problem you're trying to solve.
>
> Is it "old software which relies on joydev is too basic to allow
> selecting the correct joystick device"? Or "some system components are
> picking up the joypad's accelerometer like it inferred the screen's
> positioning"?
>

The problem is more like the first. Joydev is too basic to allow
applications to determine they are dealing with a motion sensor device
(axes are just numbers from its perspective). Due to recent hid-sony
restructure, its motion sensors are now picked up by joydev as it
passes joydev its heuristics.

We felt that no motion sensors should probably get picked up by the
legacy joydev interface as it caused issues for legacy applications
(and users reported issues to me). Originally we proposed filtering in
joydev based on just 'INPUT_PROP_ACCELEROMETER' as it has similar
filters to filter out touchpad device and others. Discussion on that
patch led to the composite device discussion as Dmitry felt that the
ability to use a standalone motion sensor device as a joystick could
potentially make sense.

Re: [PATCH v2 0/3] Input/HID: joydev fixes for motion sensors

[Bastien Nocera]

On Wed, 2017-08-23 at 16:57 -0700, Roderick Colenbrander wrote:
> On Wed, Aug 23, 2017 at 2:04 AM, Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
<snip>
> > I'm not sure I fully understand the problem you're trying to solve.
> > 
> > Is it "old software which relies on joydev is too basic to allow
> > selecting the correct joystick device"? Or "some system components
> > are
> > picking up the joypad's accelerometer like it inferred the screen's
> > positioning"?
> > 
> 
> The problem is more like the first. Joydev is too basic to allow
> applications to determine they are dealing with a motion sensor
> device
> (axes are just numbers from its perspective). Due to recent hid-sony
> restructure, its motion sensors are now picked up by joydev as it
> passes joydev its heuristics.
> 
> We felt that no motion sensors should probably get picked up by the
> legacy joydev interface as it caused issues for legacy applications
> (and users reported issues to me). Originally we proposed filtering
> in
> joydev based on just 'INPUT_PROP_ACCELEROMETER' as it has similar
> filters to filter out touchpad device and others. Discussion on that
> patch led to the composite device discussion as Dmitry felt that the
> ability to use a standalone motion sensor device as a joystick could
> potentially make sense.

In which case a INPUT_PROP_JOYDEV_IGNORE property would be much much
more descriptive of what you want to achieve with this patchset, and
avoids confusion.

I think that INPUT_PROP_COMPOSITE might have its place (and it would be
useful to have supported in toolkits, or wayland compositors), but not
for this purpose.

I'd say repurpose the whole patchset around INPUT_PROP_JOYDEV_IGNORE
and we can look into a more complete INPUT_PROP_COMPOSITE with ioctl()
and co.

Cheers

Re: [PATCH v2 0/3] Input/HID: joydev fixes for motion sensors

[Roderick Colenbrander]

On Wed, Aug 23, 2017 at 5:08 PM, Bastien Nocera <hadess@hadess.net> wrote:
> On Wed, 2017-08-23 at 16:57 -0700, Roderick Colenbrander wrote:
>> On Wed, Aug 23, 2017 at 2:04 AM, Bastien Nocera <hadess@hadess.net>
>> wrote:
>> >
> <snip>
>> > I'm not sure I fully understand the problem you're trying to solve.
>> >
>> > Is it "old software which relies on joydev is too basic to allow
>> > selecting the correct joystick device"? Or "some system components
>> > are
>> > picking up the joypad's accelerometer like it inferred the screen's
>> > positioning"?
>> >
>>
>> The problem is more like the first. Joydev is too basic to allow
>> applications to determine they are dealing with a motion sensor
>> device
>> (axes are just numbers from its perspective). Due to recent hid-sony
>> restructure, its motion sensors are now picked up by joydev as it
>> passes joydev its heuristics.
>>
>> We felt that no motion sensors should probably get picked up by the
>> legacy joydev interface as it caused issues for legacy applications
>> (and users reported issues to me). Originally we proposed filtering
>> in
>> joydev based on just 'INPUT_PROP_ACCELEROMETER' as it has similar
>> filters to filter out touchpad device and others. Discussion on that
>> patch led to the composite device discussion as Dmitry felt that the
>> ability to use a standalone motion sensor device as a joystick could
>> potentially make sense.
>
> In which case a INPUT_PROP_JOYDEV_IGNORE property would be much much
> more descriptive of what you want to achieve with this patchset, and
> avoids confusion.
>
> I think that INPUT_PROP_COMPOSITE might have its place (and it would be
> useful to have supported in toolkits, or wayland compositors), but not
> for this purpose.
>
> I'd say repurpose the whole patchset around INPUT_PROP_JOYDEV_IGNORE
> and we can look into a more complete INPUT_PROP_COMPOSITE with ioctl()
> and co.
>
> Cheers

That sounds like a good idea. The only downside is that this flag
would also get exposed to user space, but I guess that could be okay.

Re: [PATCH v2 0/3] Input/HID: joydev fixes for motion sensors

[Bastien Nocera]

On Thu, 2017-08-17 at 19:01 -0700, Roderick Colenbrander wrote:
> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
<snip>
> The earlier version of this patch set, added a filter to joydev to
> ignore devices which have INPUT_PROP_ACCELEROMETER set. Dmitry
> pointed
> out that often you could use a motion sensor device as a joystick. He
> felt the issue is with composite devices.
> 
> The discussion didn't result in a conclusion. This patch set only
> filters out motion sensors if they are part of a composite device.
> Since there is no way during driver initialization to determine
> whether we are dealing with a composite device, we introduce a new
> property INPUT_PROP_COMPOSITE to determine this. I think having such
> flag is beneficial for userspace as well, since applications now get
> a hint
> that a device is part of a composite device without having to infer
> this from a EVIOCGPHYS / EVIOCGUINIQ match across devices.
> 
> Hopefully this patches will be accepted for 4.14, but maybe earlier
> if
> still possible as the next wave of distributions will likely be on
> 4.13
> with more users dealing with this issue.

FWIW, in iio-sensor-proxy, we'd probably be more interested in knowing
that the accelerometer is external to the machine, inside an external
device, than to know it's part of a "multi-device device", as most
sensors already are part of blocks containing other sensors such as
gyro, compass, etc.