[PATCH 0/1] Input: joydev blacklist feature

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

Hi,

The joydev changes introduced by this patch are the result of a discussion
on linux-input about handling of features in newer devices, which classic
joydev applications may not pick up and confuse. The original motivation
was dualshock 3 /4 and thq udraw devices, which expose motion sensors
on a subdevice. These got picked up by joydev and confused older applications
and users and has been a regression since 4.12.

The original patch proposed to filter out motion sensor capability from joydev,
but this was 'too wide' of a filter. Ultimately Dmitry suggested for joydev
to reject certain devices.

This patch introduces an initial blacklist mechanism for joydev. It is used
to filter out the motion sensor subdevice on dualshock 3 / 4 and thq udraw.

We hope this feature could still make it into 4.14 to minimize impact on our
users as this will be a long LTS release.

Thanks,
Roderick

Roderick Colenbrander (1):
  Input: joydev - blacklist ds3/ds4/udraw motion sensors

 drivers/input/joydev.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

-- 
2.13.6

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

Introduce a device table used for blacklisting devices. We currently
blacklist the motion sensor subdevice of THQ Udraw and Sony ds3/ds4.

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

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 29d677c714d2..e2a0f63d5656 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c

@@ -70,6 +70,40 @@ struct joydev_client {
 	struct list_head node;
 };
 
+/* These codes are copied from from hid-ids.h, unfortunately there is no common
+ * usb_ids/bt_ids.h header.
+ */
+#define USB_VENDOR_ID_SONY 0x54c
+#define USB_VENDOR_ID_THQ		0x20d6
+#define USB_DEVICE_ID_SONY_PS3_CONTROLLER	0x0268
+#define USB_DEVICE_ID_SONY_PS4_CONTROLLER	0x05c4
+#define USB_DEVICE_ID_SONY_PS4_CONTROLLER_2	0x09cc
+#define USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE	0x0ba0
+#define USB_DEVICE_ID_THQ_PS3_UDRAW	0xcb17
+
+/* List of devices blacklisted from joydev. A reason for blacklisting a
+ * device is to support (legacy) software supporting joydev, but which will
+ * never get updated to support these devices or features. An example would
+ * be handling of motion sensors, which these applications could not handle
+ * resulting in undefined behavior.
+ */
+static const struct joydev_blacklist {
+	__u16 bustype;
+	__u16 vendor;
+	__u16 product;
+	unsigned long propbit; /* Allow for filtering based on device properties. */
+} joydev_blacklist[] = {
+	{ BUS_BLUETOOTH, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER, BIT(INPUT_PROP_ACCELEROMETER) },
+	{ BUS_BLUETOOTH, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER, BIT(INPUT_PROP_ACCELEROMETER) },
+	{ BUS_BLUETOOTH, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2, BIT(INPUT_PROP_ACCELEROMETER) },
+	{ BUS_USB, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER, BIT(INPUT_PROP_ACCELEROMETER) },
+	{ BUS_USB, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER, BIT(INPUT_PROP_ACCELEROMETER) },
+	{ BUS_USB, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2, BIT(INPUT_PROP_ACCELEROMETER) },
+	{ BUS_USB, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE, BIT(INPUT_PROP_ACCELEROMETER) },
+	{ BUS_USB, USB_VENDOR_ID_THQ, USB_DEVICE_ID_THQ_PS3_UDRAW, BIT(INPUT_PROP_ACCELEROMETER) },
+	{ 0, 0, 0, 0}
+};
+
 static int joydev_correct(int value, struct js_corr *corr)
 {
 	switch (corr->type) {

@@ -805,6 +839,25 @@ static bool joydev_dev_is_absolute_mouse(struct input_dev *dev)
 	return true;
 }
 
+
+static bool joydev_dev_is_blacklisted(struct input_dev *dev)
+{
+	int i;
+
+	for (i = 0; joydev_blacklist[i].vendor; i++) {
+		if (dev->id.bustype == joydev_blacklist[i].bustype &&
+		    dev->id.vendor == joydev_blacklist[i].vendor &&
+		    dev->id.product == joydev_blacklist[i].product &&
+		    dev->propbit[0] == joydev_blacklist[i].propbit)
+		{
+			dev_info(&dev->dev, "joydev: Blacklisting '%s'\n", dev->name);
+			return true;
+		}
+	}
+
+	return false;
+}
+
 static bool joydev_match(struct input_handler *handler, struct input_dev *dev)
 {
 	/* Avoid touchpads and touchscreens */

@@ -819,6 +872,10 @@ static bool joydev_match(struct input_handler *handler, struct input_dev *dev)
 	if (joydev_dev_is_absolute_mouse(dev))
 		return false;
 
+	/* Disable blacklisted devices */
+	if (joydev_dev_is_blacklisted(dev))
+		return false;
+
 	return true;
 }
 

-- 
2.13.6

Hi Roderick,

On Thu, Oct 05, 2017 at 06:03:33PM -0700, Roderick Colenbrander wrote:

I would assume that regardless of connection (USB, BT, SPI, I2C, the
accelerometer in PS4 controller should not be handled by joydev.  I
wonder if we should not factor out input device id matching from
input_match_device(), add propbit handling to input device id and use it
here? Then your blacklist would be:

#define ACCEL_DEV(vendor, product)				\
	{								\
		.flags		= INPUT_DEVICE_ID_MATCH_VENDOR |	\
					INPUT_DEVICE_ID_MATCH_PRODUCT | \
					INPUT_DEVICE_ID_MATCH_PROPBIT,	\
		.vendor		= (vendor),				\
		.product	= (product),				\
		.propbit[0]	= BIT(INPUT_PROP_ACCELEROMETER),	\
	}

static const struct input_device_id joydev_blacklist {
	ACCEL_DEV(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER),
	ACCEL_DEV(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER),
	...
	{ }
};

You probably want "&" and not strict match?

Thanks.

-- 
Dmitry

Hi Dmitry,

Thanks for your suggestions the feedback looks good. Just got back to office after a long weekend, then was surprised to see you already made a few changes to the series. The changes look. Thanks for the assistance!

Thanks,
Roderick
________________________________________
From: Dmitry Torokhov [dmitry.torokhov@gmail.com]
Sent: Saturday, October 07, 2017 11:44 AM
To: roderick@gaikai.com
Cc: linux-input@vger.kernel.org; Benjamin Tissoires; Jiri Kosina; Colenbrander, Roelof; stable@vger.kernel.org
Subject: Re: [PATCH 1/1] Input: joydev - blacklist ds3/ds4/udraw motion sensors

Hi Roderick,

On Thu, Oct 05, 2017 at 06:03:33PM -0700, Roderick Colenbrander wrote:

I would assume that regardless of connection (USB, BT, SPI, I2C, the
accelerometer in PS4 controller should not be handled by joydev.  I
wonder if we should not factor out input device id matching from
input_match_device(), add propbit handling to input device id and use it
here? Then your blacklist would be:

#define ACCEL_DEV(vendor, product)                              \
        {                                                               \
                .flags          = INPUT_DEVICE_ID_MATCH_VENDOR |        \
                                        INPUT_DEVICE_ID_MATCH_PRODUCT | \
                                        INPUT_DEVICE_ID_MATCH_PROPBIT,  \
                .vendor         = (vendor),                             \
                .product        = (product),                            \
                .propbit[0]     = BIT(INPUT_PROP_ACCELEROMETER),        \
        }

static const struct input_device_id joydev_blacklist {
        ACCEL_DEV(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER),
        ACCEL_DEV(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER),
        ...
        { }
};

You probably want "&" and not strict match?

Thanks.

--
Dmitry

Hi Roderick,

On Wed, Oct 11, 2017 at 12:53:53AM +0000, Colenbrander, Roelof wrote:

Hi Dmitry,

Thanks for your suggestions the feedback looks good. Just got back to office after a long weekend, then was surprised to see you already made a few changes to the series. The changes look. Thanks for the assistance!

OK, if that works for you then I will queue it up.

Thanks.

Thanks,
Roderick
________________________________________
From: Dmitry Torokhov [dmitry.torokhov@gmail.com]
Sent: Saturday, October 07, 2017 11:44 AM
To: roderick@gaikai.com
Cc: linux-input@vger.kernel.org; Benjamin Tissoires; Jiri Kosina; Colenbrander, Roelof; stable@vger.kernel.org
Subject: Re: [PATCH 1/1] Input: joydev - blacklist ds3/ds4/udraw motion sensors

Hi Roderick,

On Thu, Oct 05, 2017 at 06:03:33PM -0700, Roderick Colenbrander wrote:

quoted

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

Introduce a device table used for blacklisting devices. We currently
blacklist the motion sensor subdevice of THQ Udraw and Sony ds3/ds4.

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

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 29d677c714d2..e2a0f63d5656 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c

@@ -70,6 +70,40 @@ struct joydev_client {
      struct list_head node;
 };

+/* These codes are copied from from hid-ids.h, unfortunately there is no common
+ * usb_ids/bt_ids.h header.
+ */
+#define USB_VENDOR_ID_SONY 0x54c
+#define USB_VENDOR_ID_THQ            0x20d6
+#define USB_DEVICE_ID_SONY_PS3_CONTROLLER    0x0268
+#define USB_DEVICE_ID_SONY_PS4_CONTROLLER    0x05c4
+#define USB_DEVICE_ID_SONY_PS4_CONTROLLER_2  0x09cc
+#define USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE     0x0ba0
+#define USB_DEVICE_ID_THQ_PS3_UDRAW  0xcb17
+
+/* List of devices blacklisted from joydev. A reason for blacklisting a
+ * device is to support (legacy) software supporting joydev, but which will
+ * never get updated to support these devices or features. An example would
+ * be handling of motion sensors, which these applications could not handle
+ * resulting in undefined behavior.
+ */
+static const struct joydev_blacklist {
+     __u16 bustype;
+     __u16 vendor;
+     __u16 product;
+     unsigned long propbit; /* Allow for filtering based on device properties. */
+} joydev_blacklist[] = {
+     { BUS_BLUETOOTH, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_BLUETOOTH, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_BLUETOOTH, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_USB, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_USB, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_USB, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_USB, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_USB, USB_VENDOR_ID_THQ, USB_DEVICE_ID_THQ_PS3_UDRAW, BIT(INPUT_PROP_ACCELEROMETER) },
+     { 0, 0, 0, 0}

I would assume that regardless of connection (USB, BT, SPI, I2C, the
accelerometer in PS4 controller should not be handled by joydev.  I
wonder if we should not factor out input device id matching from
input_match_device(), add propbit handling to input device id and use it
here? Then your blacklist would be:

#define ACCEL_DEV(vendor, product)                              \
        {                                                               \
                .flags          = INPUT_DEVICE_ID_MATCH_VENDOR |        \
                                        INPUT_DEVICE_ID_MATCH_PRODUCT | \
                                        INPUT_DEVICE_ID_MATCH_PROPBIT,  \
                .vendor         = (vendor),                             \
                .product        = (product),                            \
                .propbit[0]     = BIT(INPUT_PROP_ACCELEROMETER),        \
        }

static const struct input_device_id joydev_blacklist {
        ACCEL_DEV(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER),
        ACCEL_DEV(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER),
        ...
        { }
};

quoted

+};
+
 static int joydev_correct(int value, struct js_corr *corr)
 {
      switch (corr->type) {

@@ -805,6 +839,25 @@ static bool joydev_dev_is_absolute_mouse(struct input_dev *dev)
      return true;
 }

+
+static bool joydev_dev_is_blacklisted(struct input_dev *dev)
+{
+     int i;
+
+     for (i = 0; joydev_blacklist[i].vendor; i++) {
+             if (dev->id.bustype == joydev_blacklist[i].bustype &&
+                 dev->id.vendor == joydev_blacklist[i].vendor &&
+                 dev->id.product == joydev_blacklist[i].product &&
+                 dev->propbit[0] == joydev_blacklist[i].propbit)

You probably want "&" and not strict match?

quoted

+             {
+                     dev_info(&dev->dev, "joydev: Blacklisting '%s'\n", dev->name);
+                     return true;
+             }
+     }
+
+     return false;
+}
+
 static bool joydev_match(struct input_handler *handler, struct input_dev *dev)
 {
      /* Avoid touchpads and touchscreens */

@@ -819,6 +872,10 @@ static bool joydev_match(struct input_handler *handler, struct input_dev *dev)
      if (joydev_dev_is_absolute_mouse(dev))
              return false;

+     /* Disable blacklisted devices */
+     if (joydev_dev_is_blacklisted(dev))
+             return false;
+
      return true;
 }

--

2.13.6

Thanks.

--
Dmitry

-- 
Dmitry

Hi Dmitry,

I would like to confirm that the changes are working correctly. The code 
looked code before, but hadn't validated the changes yet. We did so 
today and things work accordingly, so I think it is ready to merge.

Thanks for the help!

Thanks,
Roderick


On 10/13/2017 04:50 PM, Dmitry Torokhov wrote:

Hi Roderick,

On Wed, Oct 11, 2017 at 12:53:53AM +0000, Colenbrander, Roelof wrote:

quoted

Hi Dmitry,

Thanks for your suggestions the feedback looks good. Just got back to office after a long weekend, then was surprised to see you already made a few changes to the series. The changes look. Thanks for the assistance!

OK, if that works for you then I will queue it up.

Thanks.

quoted

Thanks,
Roderick
________________________________________
From: Dmitry Torokhov [dmitry.torokhov@gmail.com]
Sent: Saturday, October 07, 2017 11:44 AM
To: roderick@gaikai.com
Cc: linux-input@vger.kernel.org; Benjamin Tissoires; Jiri Kosina; Colenbrander, Roelof; stable@vger.kernel.org
Subject: Re: [PATCH 1/1] Input: joydev - blacklist ds3/ds4/udraw motion sensors

Hi Roderick,

On Thu, Oct 05, 2017 at 06:03:33PM -0700, Roderick Colenbrander wrote:

quoted

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

Introduce a device table used for blacklisting devices. We currently
blacklist the motion sensor subdevice of THQ Udraw and Sony ds3/ds4.

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

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 29d677c714d2..e2a0f63d5656 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c

@@ -70,6 +70,40 @@ struct joydev_client {
       struct list_head node;
  };

+/* These codes are copied from from hid-ids.h, unfortunately there is no common
+ * usb_ids/bt_ids.h header.
+ */
+#define USB_VENDOR_ID_SONY 0x54c
+#define USB_VENDOR_ID_THQ            0x20d6
+#define USB_DEVICE_ID_SONY_PS3_CONTROLLER    0x0268
+#define USB_DEVICE_ID_SONY_PS4_CONTROLLER    0x05c4
+#define USB_DEVICE_ID_SONY_PS4_CONTROLLER_2  0x09cc
+#define USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE     0x0ba0
+#define USB_DEVICE_ID_THQ_PS3_UDRAW  0xcb17
+
+/* List of devices blacklisted from joydev. A reason for blacklisting a
+ * device is to support (legacy) software supporting joydev, but which will
+ * never get updated to support these devices or features. An example would
+ * be handling of motion sensors, which these applications could not handle
+ * resulting in undefined behavior.
+ */
+static const struct joydev_blacklist {
+     __u16 bustype;
+     __u16 vendor;
+     __u16 product;
+     unsigned long propbit; /* Allow for filtering based on device properties. */
+} joydev_blacklist[] = {
+     { BUS_BLUETOOTH, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_BLUETOOTH, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_BLUETOOTH, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_USB, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_USB, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_USB, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_USB, USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE, BIT(INPUT_PROP_ACCELEROMETER) },
+     { BUS_USB, USB_VENDOR_ID_THQ, USB_DEVICE_ID_THQ_PS3_UDRAW, BIT(INPUT_PROP_ACCELEROMETER) },
+     { 0, 0, 0, 0}

I would assume that regardless of connection (USB, BT, SPI, I2C, the
accelerometer in PS4 controller should not be handled by joydev.  I
wonder if we should not factor out input device id matching from
input_match_device(), add propbit handling to input device id and use it
here? Then your blacklist would be:

#define ACCEL_DEV(vendor, product)                              \
         {                                                               \
                 .flags          = INPUT_DEVICE_ID_MATCH_VENDOR |        \
                                         INPUT_DEVICE_ID_MATCH_PRODUCT | \
                                         INPUT_DEVICE_ID_MATCH_PROPBIT,  \
                 .vendor         = (vendor),                             \
                 .product        = (product),                            \
                 .propbit[0]     = BIT(INPUT_PROP_ACCELEROMETER),        \
         }

static const struct input_device_id joydev_blacklist {
         ACCEL_DEV(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER),
         ACCEL_DEV(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER),
         ...
         { }
};

quoted

+};
+
  static int joydev_correct(int value, struct js_corr *corr)
  {
       switch (corr->type) {

@@ -805,6 +839,25 @@ static bool joydev_dev_is_absolute_mouse(struct input_dev *dev)
       return true;
  }

+
+static bool joydev_dev_is_blacklisted(struct input_dev *dev)
+{
+     int i;
+
+     for (i = 0; joydev_blacklist[i].vendor; i++) {
+             if (dev->id.bustype == joydev_blacklist[i].bustype &&
+                 dev->id.vendor == joydev_blacklist[i].vendor &&
+                 dev->id.product == joydev_blacklist[i].product &&
+                 dev->propbit[0] == joydev_blacklist[i].propbit)

You probably want "&" and not strict match?

quoted

+             {
+                     dev_info(&dev->dev, "joydev: Blacklisting '%s'\n", dev->name);
+                     return true;
+             }
+     }
+
+     return false;
+}
+
  static bool joydev_match(struct input_handler *handler, struct input_dev *dev)
  {
       /* Avoid touchpads and touchscreens */

@@ -819,6 +872,10 @@ static bool joydev_match(struct input_handler *handler, struct input_dev *dev)
       if (joydev_dev_is_absolute_mouse(dev))
               return false;

+     /* Disable blacklisted devices */
+     if (joydev_dev_is_blacklisted(dev))
+             return false;
+
       return true;
  }

--

2.13.6

Thanks.

--
Dmitry