[PATCH v3 0/7] HID: sony: game controller updates

[PATCH v3 0/7] HID: sony: game controller updates

[Roderick Colenbrander]

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

Hi,

This is a resubmit of the previous patches. Most patches are unchanged,
just for clarity the whole series is sent again.

Changes:
* 5/7: touch events patch calls input_mt_sync_frame and input_sync
       per pair of touch points.
* 7/7: removed 'v2' from update device ids patch title

Thanks,
Roderick

Roderick Colenbrander (7):
  HID: sony: Fix race condition in sony_probe
  HID: sony: Adjust HID report size name definitions
  HID: sony: Perform CRC check on bluetooth input packets
  HID: sony: Send ds4 output reports on output end-point
  HID: sony: Handle multiple touch events input record
  HID: sony: Adjust value range for motion sensors
  HID: sony: Update device ids

 drivers/hid/hid-core.c |   2 +
 drivers/hid/hid-ids.h  |   1 +
 drivers/hid/hid-sony.c | 277 +++++++++++++++++++++++++++++--------------------
 3 files changed, 166 insertions(+), 114 deletions(-)

-- 
2.7.4

[PATCH 1/7] HID: sony: Fix race condition in sony_probe

[Roderick Colenbrander]

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

Early on the sony_probe function calls hid_hw_start to start the hardware.
Afterwards it issues some hardware requests, initializes other functionality
like Force Feedback, power classes and others. However by the time
hid_hw_start returns, the device nodes have already been created, which leads
to a race condition by user space applications which may detect the device
prior to completion of initialization. We have observed this problem many
times, this patch fixes the problem.

This patch moves most of sony_probe to sony_input_configured, which is called
prior to device registration. This fixes the race condition and the same
approach is used in other HID drivers.

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index b0bb99a..afa8219 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1387,28 +1387,6 @@ static int sony_register_touchpad(struct hid_input *hi, int touch_count,
 	return 0;
 }
 
-static int sony_input_configured(struct hid_device *hdev,
-					struct hid_input *hidinput)
-{
-	struct sony_sc *sc = hid_get_drvdata(hdev);
-	int ret;
-
-	/*
-	 * The Dualshock 4 touchpad supports 2 touches and has a
-	 * resolution of 1920x942 (44.86 dots/mm).
-	 */
-	if (sc->quirks & DUALSHOCK4_CONTROLLER) {
-		ret = sony_register_touchpad(hidinput, 2, 1920, 942);
-		if (ret) {
-			hid_err(sc->hdev,
-				"Unable to initialize multi-touch slots: %d\n",
-				ret);
-			return ret;
-		}
-	}
-
-	return 0;
-}
 
 /*
  * Sending HID_REQ_GET_REPORT changes the operation mode of the ps3 controller
@@ -2329,45 +2307,12 @@ static inline void sony_cancel_work_sync(struct sony_sc *sc)
 		cancel_work_sync(&sc->state_worker);
 }
 
-static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
+static int sony_input_configured(struct hid_device *hdev,
+					struct hid_input *hidinput)
 {
-	int ret;
+	struct sony_sc *sc = hid_get_drvdata(hdev);
 	int append_dev_id;
-	unsigned long quirks = id->driver_data;
-	struct sony_sc *sc;
-	unsigned int connect_mask = HID_CONNECT_DEFAULT;
-
-	if (!strcmp(hdev->name, "FutureMax Dance Mat"))
-		quirks |= FUTUREMAX_DANCE_MAT;
-
-	sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
-	if (sc == NULL) {
-		hid_err(hdev, "can't alloc sony descriptor\n");
-		return -ENOMEM;
-	}
-
-	spin_lock_init(&sc->lock);
-
-	sc->quirks = quirks;
-	hid_set_drvdata(hdev, sc);
-	sc->hdev = hdev;
-
-	ret = hid_parse(hdev);
-	if (ret) {
-		hid_err(hdev, "parse failed\n");
-		return ret;
-	}
-
-	if (sc->quirks & VAIO_RDESC_CONSTANT)
-		connect_mask |= HID_CONNECT_HIDDEV_FORCE;
-	else if (sc->quirks & SIXAXIS_CONTROLLER)
-		connect_mask |= HID_CONNECT_HIDDEV_FORCE;
-
-	ret = hid_hw_start(hdev, connect_mask);
-	if (ret) {
-		hid_err(hdev, "hw start failed\n");
-		return ret;
-	}
+	int ret;
 
 	ret = sony_set_device_id(sc);
 	if (ret < 0) {
@@ -2427,6 +2372,18 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 			}
 		}
 
+		/*
+		 * The Dualshock 4 touchpad supports 2 touches and has a
+		 * resolution of 1920x942 (44.86 dots/mm).
+		 */
+		ret = sony_register_touchpad(hidinput, 2, 1920, 942);
+		if (ret) {
+			hid_err(sc->hdev,
+			"Unable to initialize multi-touch slots: %d\n",
+			ret);
+			return ret;
+		}
+
 		sony_init_output_report(sc, dualshock4_send_output_report);
 	} else if (sc->quirks & MOTION_CONTROLLER) {
 		sony_init_output_report(sc, motion_send_output_report);
@@ -2482,6 +2439,48 @@ err_stop:
 	return ret;
 }
 
+static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	int ret;
+	unsigned long quirks = id->driver_data;
+	struct sony_sc *sc;
+	unsigned int connect_mask = HID_CONNECT_DEFAULT;
+
+	if (!strcmp(hdev->name, "FutureMax Dance Mat"))
+		quirks |= FUTUREMAX_DANCE_MAT;
+
+	sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
+	if (sc == NULL) {
+		hid_err(hdev, "can't alloc sony descriptor\n");
+		return -ENOMEM;
+	}
+
+	spin_lock_init(&sc->lock);
+
+	sc->quirks = quirks;
+	hid_set_drvdata(hdev, sc);
+	sc->hdev = hdev;
+
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev, "parse failed\n");
+		return ret;
+	}
+
+	if (sc->quirks & VAIO_RDESC_CONSTANT)
+		connect_mask |= HID_CONNECT_HIDDEV_FORCE;
+	else if (sc->quirks & SIXAXIS_CONTROLLER)
+		connect_mask |= HID_CONNECT_HIDDEV_FORCE;
+
+	ret = hid_hw_start(hdev, connect_mask);
+	if (ret) {
+		hid_err(hdev, "hw start failed\n");
+		return ret;
+	}
+
+	return ret;
+}
+
 static void sony_remove(struct hid_device *hdev)
 {
 	struct sony_sc *sc = hid_get_drvdata(hdev);
-- 
2.7.4

Re: [PATCH 1/7] HID: sony: Fix race condition in sony_probe

[Roderick Colenbrander]

Hi,

We have been developing some new code building on top of this hid-sony patchset.
However during additional testing we noticed some issues between
swapping devices
between USB and Bluetooth. There is some code as part of 'sony_input_configured'
preventing two connections from the same device. The code is kind of
working as it
rejects the device, but there is some memory corruption due to 'sony_probe'
succeeding on this failure (later on 'sony_remove' double frees memory), which
was introduced by this patch. We intend to fix this, but I'm not sure
where the fix
should be. So far I'm leaning towards outside of this driver, but let
me explain.

On close inspection, the problem is that 'hid_hw_start' as called by
'sony_probe'
isn't returning an error on a 'sony_input_configured' failure.
Personally, I expected
it to behave like that, but it doesn't. As an outsider to the HID subsystem, I'm
not sure what expected behavior is.

I looked over some other drivers, which are emitting errors from
'input_configured'.
There are only 2 drivers returning errors and catching them through a
custom mechanism.
The drivers are 'hid-magicmouse' and 'hid-rmi'. The first kind of works around
'hid_hw_start' returning errors by setting some state (msc->input) from within
'input_configured' which 'probe' can pick up.
The 'hid-rmi' driver kind of relies on hid_hw_start not returning
errors. It leaves
the device in some in-between state with hidraw operational, so
someone could flash
a new firmware. For reference I have attached these code snippets to
the bottom of this
email.

I'm not exactly sure on what the best way to fix the issue I'm
experiencing is. My
feeling is that it should be fixed in a general way within
'hid_hw_start' and related
logic; not in a driver specific workaround. Outside of hid_hw_start
feels unclean
to me for various including keeping a device node around in some undefined state
for a short period of time.

The root cause seems to be that 'hid_connect' doesn't return a failure upon
'hidinput_connect' and only sets a flag:
    if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev,
                connect_mask & HID_CONNECT_HIDINPUT_FORCE))
        hdev->claimed |= HID_CLAIMED_INPUT;

I think it should return an error when hidinput_connect fails, which
would ultimately
bubble up to a return error in 'hid_hw_start'. I don't know the
intimate details of
all the HID code well, so I'm not sure if this is the best solution.

With such a change made, the 'if (!msc->input)' logic in magicmouse_probe could
be removed. The RMI driver would need some fixing. I would say it should call
'hid_hw_start' again on the first hid_hw_start failure, but with a different
connect_mask, so only hidraw is activated.


For reference two pieces of code, which are dealing with the same kind
of problem,
I'm seeing in hid-sony. I put some comments '<--' to show relevant pieces.

magicmouse_input_configured:
    ret = magicmouse_setup_input(msc->input, hdev);
    if (ret) {
        hid_err(hdev, "magicmouse setup input failed (%d)\n", ret);
        /* clean msc->input to notify probe() of the failure */
        msc->input = NULL; <-- Used to notify magicmouse_probe on
hid_hw_start failure
        return ret;
    }

magicmouse_probe:
    ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
    if (ret) {
        hid_err(hdev, "magicmouse hw start failed\n");
        return ret;
    }

    if (!msc->input) { <-- workaround to detect
magicmouse_input_configured failure
        hid_err(hdev, "magicmouse input not registered\n");
        ret = -ENOMEM;
        goto err_stop_hw;
    }


rmi_probe:
    ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
    if (ret) {
        hid_err(hdev, "hw start failed\n");
        return ret;
    }

    if ((data->device_flags & RMI_DEVICE) &&
        !test_bit(RMI_STARTED, &data->flags)) <-- input_configure sets
this bit on success
        /*
         * The device maybe in the bootloader if rmi_input_configured
         * failed to find F11 in the PDT. Print an error, but don't
         * return an error from rmi_probe so that hidraw will be
         * accessible from userspace. That way a userspace tool
         * can be used to reload working firmware on the touchpad.
         */
        hid_err(hdev, "Device failed to be properly configured\n");


Thanks,
Roderick

Re: [PATCH 1/7] HID: sony: Fix race condition in sony_probe

[Benjamin Tissoires]

On Oct 10 2016 or thereabouts, Roderick Colenbrander wrote:
> Hi,
> 
> We have been developing some new code building on top of this hid-sony patchset.
> However during additional testing we noticed some issues between
> swapping devices
> between USB and Bluetooth. There is some code as part of 'sony_input_configured'
> preventing two connections from the same device. The code is kind of
> working as it
> rejects the device, but there is some memory corruption due to 'sony_probe'
> succeeding on this failure (later on 'sony_remove' double frees memory), which
> was introduced by this patch. We intend to fix this, but I'm not sure
> where the fix
> should be. So far I'm leaning towards outside of this driver, but let
> me explain.
> 
> On close inspection, the problem is that 'hid_hw_start' as called by
> 'sony_probe'
> isn't returning an error on a 'sony_input_configured' failure.
> Personally, I expected
> it to behave like that, but it doesn't. As an outsider to the HID subsystem, I'm
> not sure what expected behavior is.
> 
> I looked over some other drivers, which are emitting errors from
> 'input_configured'.
> There are only 2 drivers returning errors and catching them through a
> custom mechanism.
> The drivers are 'hid-magicmouse' and 'hid-rmi'. The first kind of works around
> 'hid_hw_start' returning errors by setting some state (msc->input) from within
> 'input_configured' which 'probe' can pick up.

Well, hid-magicmouse is not the best of the art in term of HID driver.
But this quirk works OK-ish.

> The 'hid-rmi' driver kind of relies on hid_hw_start not returning
> errors. It leaves
> the device in some in-between state with hidraw operational, so
> someone could flash
> a new firmware. For reference I have attached these code snippets to
> the bottom of this
> email.

And hid-rmi is also a "temporary" driver and we (Andrew mostly) are
trying to clean it up and forward all the RMI4 logic to RMI4 core now
upstream in the drivers/input/rmi4 directory.

> 
> I'm not exactly sure on what the best way to fix the issue I'm
> experiencing is. My
> feeling is that it should be fixed in a general way within
> 'hid_hw_start' and related
> logic; not in a driver specific workaround. Outside of hid_hw_start
> feels unclean
> to me for various including keeping a device node around in some undefined state
> for a short period of time.

I am not entirely sure having hidinput_connect failing being an actual
error. It can be useful to still present the HID node through hidraw if
the device fails for whatever reasons to bind on the input side.

If I am not wrong, you can simply check after hid_hw_start() if
hdev->claimed matches HID_CLAIMED_INPUT, and if not just bail out.

> 
> The root cause seems to be that 'hid_connect' doesn't return a failure upon
> 'hidinput_connect' and only sets a flag:
>     if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev,
>                 connect_mask & HID_CONNECT_HIDINPUT_FORCE))
>         hdev->claimed |= HID_CLAIMED_INPUT;
> 
> I think it should return an error when hidinput_connect fails, which
> would ultimately
> bubble up to a return error in 'hid_hw_start'. I don't know the
> intimate details of
> all the HID code well, so I'm not sure if this is the best solution.
> 
> With such a change made, the 'if (!msc->input)' logic in magicmouse_probe could
> be removed. The RMI driver would need some fixing. I would say it should call
> 'hid_hw_start' again on the first hid_hw_start failure, but with a different
> connect_mask, so only hidraw is activated.

Again, I wouldn't bother much for these 2 drivers. The first one works
and this wouldn't add much, and the second one is doomed to be changed,
so as long as it ain't broken... :)

Cheers,
Benjamin

> 
> 
> For reference two pieces of code, which are dealing with the same kind
> of problem,
> I'm seeing in hid-sony. I put some comments '<--' to show relevant pieces.
> 
> magicmouse_input_configured:
>     ret = magicmouse_setup_input(msc->input, hdev);
>     if (ret) {
>         hid_err(hdev, "magicmouse setup input failed (%d)\n", ret);
>         /* clean msc->input to notify probe() of the failure */
>         msc->input = NULL; <-- Used to notify magicmouse_probe on
> hid_hw_start failure
>         return ret;
>     }
> 
> magicmouse_probe:
>     ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>     if (ret) {
>         hid_err(hdev, "magicmouse hw start failed\n");
>         return ret;
>     }
> 
>     if (!msc->input) { <-- workaround to detect
> magicmouse_input_configured failure
>         hid_err(hdev, "magicmouse input not registered\n");
>         ret = -ENOMEM;
>         goto err_stop_hw;
>     }
> 
> 
> rmi_probe:
>     ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>     if (ret) {
>         hid_err(hdev, "hw start failed\n");
>         return ret;
>     }
> 
>     if ((data->device_flags & RMI_DEVICE) &&
>         !test_bit(RMI_STARTED, &data->flags)) <-- input_configure sets
> this bit on success
>         /*
>          * The device maybe in the bootloader if rmi_input_configured
>          * failed to find F11 in the PDT. Print an error, but don't
>          * return an error from rmi_probe so that hidraw will be
>          * accessible from userspace. That way a userspace tool
>          * can be used to reload working firmware on the touchpad.
>          */
>         hid_err(hdev, "Device failed to be properly configured\n");
> 
> 
> Thanks,
> Roderick

Re: [PATCH 1/7] HID: sony: Fix race condition in sony_probe

[Roderick Colenbrander]

On Tue, Oct 11, 2016 at 8:43 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> On Oct 10 2016 or thereabouts, Roderick Colenbrander wrote:
>> Hi,
>>
>> We have been developing some new code building on top of this hid-sony patchset.
>> However during additional testing we noticed some issues between
>> swapping devices
>> between USB and Bluetooth. There is some code as part of 'sony_input_configured'
>> preventing two connections from the same device. The code is kind of
>> working as it
>> rejects the device, but there is some memory corruption due to 'sony_probe'
>> succeeding on this failure (later on 'sony_remove' double frees memory), which
>> was introduced by this patch. We intend to fix this, but I'm not sure
>> where the fix
>> should be. So far I'm leaning towards outside of this driver, but let
>> me explain.
>>
>> On close inspection, the problem is that 'hid_hw_start' as called by
>> 'sony_probe'
>> isn't returning an error on a 'sony_input_configured' failure.
>> Personally, I expected
>> it to behave like that, but it doesn't. As an outsider to the HID subsystem, I'm
>> not sure what expected behavior is.
>>
>> I looked over some other drivers, which are emitting errors from
>> 'input_configured'.
>> There are only 2 drivers returning errors and catching them through a
>> custom mechanism.
>> The drivers are 'hid-magicmouse' and 'hid-rmi'. The first kind of works around
>> 'hid_hw_start' returning errors by setting some state (msc->input) from within
>> 'input_configured' which 'probe' can pick up.
>
> Well, hid-magicmouse is not the best of the art in term of HID driver.
> But this quirk works OK-ish.
>
>> The 'hid-rmi' driver kind of relies on hid_hw_start not returning
>> errors. It leaves
>> the device in some in-between state with hidraw operational, so
>> someone could flash
>> a new firmware. For reference I have attached these code snippets to
>> the bottom of this
>> email.
>
> And hid-rmi is also a "temporary" driver and we (Andrew mostly) are
> trying to clean it up and forward all the RMI4 logic to RMI4 core now
> upstream in the drivers/input/rmi4 directory.
>
>>
>> I'm not exactly sure on what the best way to fix the issue I'm
>> experiencing is. My
>> feeling is that it should be fixed in a general way within
>> 'hid_hw_start' and related
>> logic; not in a driver specific workaround. Outside of hid_hw_start
>> feels unclean
>> to me for various including keeping a device node around in some undefined state
>> for a short period of time.
>
> I am not entirely sure having hidinput_connect failing being an actual
> error. It can be useful to still present the HID node through hidraw if
> the device fails for whatever reasons to bind on the input side.
>
> If I am not wrong, you can simply check after hid_hw_start() if
> hdev->claimed matches HID_CLAIMED_INPUT, and if not just bail out.

Yep that's correct. We actually implemented that method already, but
weren't sure if it was a workaround or a solution. What concerned me
is that doing it this way still leaves a 'transient' device node
around (although for a very short while until probe really fails). If
you think something like this is fine over, maybe modifying the other
HID code, I'm fine with that.

Thanks,
Roderick

[PATCH 2/7] HID: sony: Adjust HID report size name definitions

[Roderick Colenbrander]

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

Put the report type (feature / output) in the report size definitions.
This prevents name collisions later on for other different reports, which use
the same report id, but have a different size.

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index afa8219..43bb24c 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1019,10 +1019,10 @@ struct motion_output_report_02 {
 	u8 rumble;
 };
 
-#define DS4_REPORT_0x02_SIZE 37
-#define DS4_REPORT_0x05_SIZE 32
-#define DS4_REPORT_0x11_SIZE 78
-#define DS4_REPORT_0x81_SIZE 7
+#define DS4_FEATURE_REPORT_0x02_SIZE 37
+#define DS4_FEATURE_REPORT_0x81_SIZE 7
+#define DS4_OUTPUT_REPORT_0x05_SIZE 32
+#define DS4_OUTPUT_REPORT_0x11_SIZE 78
 #define SIXAXIS_REPORT_0xF2_SIZE 17
 #define SIXAXIS_REPORT_0xF5_SIZE 8
 #define MOTION_REPORT_0x02_SIZE 49
@@ -1461,11 +1461,11 @@ static int dualshock4_set_operational_bt(struct hid_device *hdev)
 	u8 *buf;
 	int ret;
 
-	buf = kmalloc(DS4_REPORT_0x02_SIZE, GFP_KERNEL);
+	buf = kmalloc(DS4_FEATURE_REPORT_0x02_SIZE, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	ret = hid_hw_raw_request(hdev, 0x02, buf, DS4_REPORT_0x02_SIZE,
+	ret = hid_hw_raw_request(hdev, 0x02, buf, DS4_FEATURE_REPORT_0x02_SIZE,
 				HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
 
 	kfree(buf);
@@ -1870,12 +1870,12 @@ static void dualshock4_send_output_report(struct sony_sc *sc)
 	 * 0xD0 - 66hz
 	 */
 	if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
-		memset(buf, 0, DS4_REPORT_0x05_SIZE);
+		memset(buf, 0, DS4_OUTPUT_REPORT_0x05_SIZE);
 		buf[0] = 0x05;
 		buf[1] = 0xFF;
 		offset = 4;
 	} else {
-		memset(buf, 0, DS4_REPORT_0x11_SIZE);
+		memset(buf, 0, DS4_OUTPUT_REPORT_0x11_SIZE);
 		buf[0] = 0x11;
 		buf[1] = 0x80;
 		buf[3] = 0x0F;
@@ -1903,9 +1903,9 @@ static void dualshock4_send_output_report(struct sony_sc *sc)
 	buf[offset++] = sc->led_delay_off[3];
 
 	if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
-		hid_hw_output_report(hdev, buf, DS4_REPORT_0x05_SIZE);
+		hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x05_SIZE);
 	else
-		hid_hw_raw_request(hdev, 0x11, buf, DS4_REPORT_0x11_SIZE,
+		hid_hw_raw_request(hdev, 0x11, buf, DS4_OUTPUT_REPORT_0x11_SIZE,
 				HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
 }
 
@@ -1950,10 +1950,10 @@ static int sony_allocate_output_report(struct sony_sc *sc)
 			kmalloc(sizeof(union sixaxis_output_report_01),
 				GFP_KERNEL);
 	else if (sc->quirks & DUALSHOCK4_CONTROLLER_BT)
-		sc->output_report_dmabuf = kmalloc(DS4_REPORT_0x11_SIZE,
+		sc->output_report_dmabuf = kmalloc(DS4_OUTPUT_REPORT_0x11_SIZE,
 						GFP_KERNEL);
 	else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
-		sc->output_report_dmabuf = kmalloc(DS4_REPORT_0x05_SIZE,
+		sc->output_report_dmabuf = kmalloc(DS4_OUTPUT_REPORT_0x05_SIZE,
 						GFP_KERNEL);
 	else if (sc->quirks & MOTION_CONTROLLER)
 		sc->output_report_dmabuf = kmalloc(MOTION_REPORT_0x02_SIZE,
@@ -2198,7 +2198,7 @@ static int sony_check_add(struct sony_sc *sc)
 			return 0;
 		}
 	} else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
-		buf = kmalloc(DS4_REPORT_0x81_SIZE, GFP_KERNEL);
+		buf = kmalloc(DS4_FEATURE_REPORT_0x81_SIZE, GFP_KERNEL);
 		if (!buf)
 			return -ENOMEM;
 
@@ -2208,10 +2208,10 @@ static int sony_check_add(struct sony_sc *sc)
 		 * offset 1.
 		 */
 		ret = hid_hw_raw_request(sc->hdev, 0x81, buf,
-				DS4_REPORT_0x81_SIZE, HID_FEATURE_REPORT,
+				DS4_FEATURE_REPORT_0x81_SIZE, HID_FEATURE_REPORT,
 				HID_REQ_GET_REPORT);
 
-		if (ret != DS4_REPORT_0x81_SIZE) {
+		if (ret != DS4_FEATURE_REPORT_0x81_SIZE) {
 			hid_err(sc->hdev, "failed to retrieve feature report 0x81 with the DualShock 4 MAC address\n");
 			ret = ret < 0 ? ret : -EINVAL;
 			goto out_free;
-- 
2.7.4

[PATCH 3/7] HID: sony: Perform CRC check on bluetooth input packets

[Roderick Colenbrander]

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

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 43bb24c..34988ce 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -36,6 +36,8 @@
 #include <linux/list.h>
 #include <linux/idr.h>
 #include <linux/input/mt.h>
+#include <linux/crc32.h>
+#include <asm/unaligned.h>
 
 #include "hid-ids.h"
 
@@ -1021,6 +1023,7 @@ struct motion_output_report_02 {
 
 #define DS4_FEATURE_REPORT_0x02_SIZE 37
 #define DS4_FEATURE_REPORT_0x81_SIZE 7
+#define DS4_INPUT_REPORT_0x11_SIZE 78
 #define DS4_OUTPUT_REPORT_0x05_SIZE 32
 #define DS4_OUTPUT_REPORT_0x11_SIZE 78
 #define SIXAXIS_REPORT_0xF2_SIZE 17
@@ -1324,6 +1327,21 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
 	} else if (((sc->quirks & DUALSHOCK4_CONTROLLER_USB) && rd[0] == 0x01 &&
 			size == 64) || ((sc->quirks & DUALSHOCK4_CONTROLLER_BT)
 			&& rd[0] == 0x11 && size == 78)) {
+		if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
+			/* CRC check */
+			u8 bthdr = 0xA1;
+			u32 crc;
+			u32 report_crc;
+
+			crc = crc32_le(0xFFFFFFFF, &bthdr, 1);
+			crc = ~crc32_le(crc, rd, DS4_INPUT_REPORT_0x11_SIZE-4);
+			report_crc = get_unaligned_le32(&rd[DS4_INPUT_REPORT_0x11_SIZE-4]);
+			if (crc != report_crc) {
+				hid_dbg(sc->hdev, "DualShock 4 input report's CRC check failed, received crc 0x%0x != 0x%0x\n",
+					report_crc, crc);
+				return -EILSEQ;
+			}
+		}
 		dualshock4_parse_report(sc, rd, size);
 	}
 
-- 
2.7.4

[PATCH 4/7] HID: sony: Send ds4 output reports on output end-point

[Roderick Colenbrander]

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

Add a CRC value to each output report. This removes the need for the
'no output reports on interrupt end-point' quirk.

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 34988ce..24f7d19 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1895,7 +1895,7 @@ static void dualshock4_send_output_report(struct sony_sc *sc)
 	} else {
 		memset(buf, 0, DS4_OUTPUT_REPORT_0x11_SIZE);
 		buf[0] = 0x11;
-		buf[1] = 0x80;
+		buf[1] = 0xC0; /* HID + CRC */
 		buf[3] = 0x0F;
 		offset = 6;
 	}
@@ -1922,9 +1922,16 @@ static void dualshock4_send_output_report(struct sony_sc *sc)
 
 	if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
 		hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x05_SIZE);
-	else
-		hid_hw_raw_request(hdev, 0x11, buf, DS4_OUTPUT_REPORT_0x11_SIZE,
-				HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
+	else {
+		/* CRC generation */
+		u8 bthdr = 0xA2;
+		u32 crc;
+
+		crc = crc32_le(0xFFFFFFFF, &bthdr, 1);
+		crc = ~crc32_le(crc, buf, DS4_OUTPUT_REPORT_0x11_SIZE-4);
+		put_unaligned_le32(crc, &buf[74]);
+		hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x11_SIZE);
+	}
 }
 
 static void motion_send_output_report(struct sony_sc *sc)
@@ -2378,11 +2385,6 @@ static int sony_input_configured(struct hid_device *hdev,
 		sony_init_output_report(sc, sixaxis_send_output_report);
 	} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
 		if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
-			/*
-			 * The DualShock 4 wants output reports sent on the ctrl
-			 * endpoint when connected via Bluetooth.
-			 */
-			hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP;
 			ret = dualshock4_set_operational_bt(hdev);
 			if (ret < 0) {
 				hid_err(hdev, "failed to set the Dualshock 4 operational mode\n");
-- 
2.7.4

[PATCH v2 5/7] HID: sony: Handle multiple touch events input record

[Roderick Colenbrander]

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

Read the touch history field in the HID descriptor and use this value
to determine how many touch events to read from the report. As part
of this patch, we did a first attempt of making the offset calculation
code less magical.

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 24f7d19..b206d85 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1030,6 +1030,12 @@ struct motion_output_report_02 {
 #define SIXAXIS_REPORT_0xF5_SIZE 8
 #define MOTION_REPORT_0x02_SIZE 49
 
+/* Offsets relative to USB input report (0x1). Bluetooth (0x11) requires an
+ * additional +2.
+ */
+#define DS4_INPUT_REPORT_BATTERY_OFFSET  30
+#define DS4_INPUT_REPORT_TOUCHPAD_OFFSET 33
+
 static DEFINE_SPINLOCK(sony_dev_list_lock);
 static LIST_HEAD(sony_device_list);
 static DEFINE_IDA(sony_device_id_allocator);
@@ -1226,19 +1232,17 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
 						struct hid_input, list);
 	struct input_dev *input_dev = hidinput->input;
 	unsigned long flags;
-	int n, offset;
+	int n, m, offset, num_touch_data, max_touch_data;
 	u8 cable_state, battery_capacity, battery_charging;
 
-	/*
-	 * Battery and touchpad data starts at byte 30 in the USB report and
-	 * 32 in Bluetooth report.
-	 */
-	offset = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 30 : 32;
+	/* When using Bluetooth the header is 2 bytes longer, so skip these. */
+	int data_offset = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 0 : 2;
 
 	/*
-	 * The lower 4 bits of byte 30 contain the battery level
+	 * The lower 4 bits of byte 30 (or 32 for BT) contain the battery level
 	 * and the 5th bit contains the USB cable state.
 	 */
+	offset = data_offset + DS4_INPUT_REPORT_BATTERY_OFFSET;
 	cable_state = (rd[offset] >> 4) & 0x01;
 	battery_capacity = rd[offset] & 0x0F;
 
@@ -1265,30 +1269,52 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	sc->battery_charging = battery_charging;
 	spin_unlock_irqrestore(&sc->lock, flags);
 
-	offset += 5;
-
 	/*
-	 * The Dualshock 4 multi-touch trackpad data starts at offset 35 on USB
-	 * and 37 on Bluetooth.
-	 * The first 7 bits of the first byte is a counter and bit 8 is a touch
-	 * indicator that is 0 when pressed and 1 when not pressed.
-	 * The next 3 bytes are two 12 bit touch coordinates, X and Y.
-	 * The data for the second touch is in the same format and immediatly
-	 * follows the data for the first.
+	 * The Dualshock 4 multi-touch trackpad data starts at offset 33 on USB
+	 * and 35 on Bluetooth.
+	 * The first byte indicates the number of touch data in the report.
+	 * Trackpad data starts 2 bytes later (e.g. 35 for USB).
 	 */
-	for (n = 0; n < 2; n++) {
-		u16 x, y;
+	offset = data_offset + DS4_INPUT_REPORT_TOUCHPAD_OFFSET;
+	max_touch_data = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 3 : 4;
+	if (rd[offset] > 0 && rd[offset] <= max_touch_data)
+		num_touch_data = rd[offset];
+	else
+		num_touch_data = 1;
+	offset += 1;
 
-		x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8);
-		y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4);
+	for (m = 0; m < num_touch_data; m++) {
+		/* Skip past timestamp */
+		offset += 1;
 
-		input_mt_slot(input_dev, n);
-		input_mt_report_slot_state(input_dev, MT_TOOL_FINGER,
-					!(rd[offset] >> 7));
-		input_report_abs(input_dev, ABS_MT_POSITION_X, x);
-		input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
+		/*
+		 * The first 7 bits of the first byte is a counter and bit 8 is
+		 * a touch indicator that is 0 when pressed and 1 when not
+		 * pressed.
+		 * The next 3 bytes are two 12 bit touch coordinates, X and Y.
+		 * The data for the second touch is in the same format and
+		 * immediately follows the data for the first.
+		 */
+		for (n = 0; n < 2; n++) {
+			u16 x, y;
+			bool active;
+
+			x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8);
+			y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4);
+
+			active = !(rd[offset] >> 7);
+			input_mt_slot(input_dev, n);
+			input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, active);
 
-		offset += 4;
+			if (active) {
+				input_report_abs(input_dev, ABS_MT_POSITION_X, x);
+				input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
+			}
+
+			offset += 4;
+		}
+		input_mt_sync_frame(input_dev);
+		input_sync(input_dev);
 	}
 }
 
-- 
2.7.4

[PATCH 6/7] HID: sony: Adjust value range for motion sensors

[Roderick Colenbrander]

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

The motion sensor values are 16-bit, so make the value range match.
It is hard to reach the upper values, but they can be reached. At
least the current accelerometer value of 8192 is very easy to pass.

It is still not nice that the motion sensors live in no man's land
in between ABS_MISC and ABS_MT_SLOT, but that's something for another
time, which the proposed ABS_ACCEL_*/ABS_GYRO_* were meant for.

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index b206d85..bd84790 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -405,14 +405,14 @@ static u8 dualshock4_usb_rdesc[] = {
 	0x19, 0x40,         /*      Usage Minimum (40h),            */
 	0x29, 0x42,         /*      Usage Maximum (42h),            */
 	0x16, 0x00, 0x80,   /*      Logical Minimum (-32768),       */
-	0x26, 0x00, 0x7F,   /*      Logical Maximum (32767),        */
+	0x26, 0xFF, 0x7F,   /*      Logical Maximum (32767),        */
 	0x75, 0x10,         /*      Report Size (16),               */
 	0x95, 0x03,         /*      Report Count (3),               */
 	0x81, 0x02,         /*      Input (Variable),               */
 	0x19, 0x43,         /*      Usage Minimum (43h),            */
 	0x29, 0x45,         /*      Usage Maximum (45h),            */
-	0x16, 0x00, 0xE0,   /*      Logical Minimum (-8192),        */
-	0x26, 0xFF, 0x1F,   /*      Logical Maximum (8191),         */
+	0x16, 0x00, 0x80,   /*      Logical Minimum (-32768),       */
+	0x26, 0xFF, 0x7F,   /*      Logical Maximum (32767),        */
 	0x95, 0x03,         /*      Report Count (3),               */
 	0x81, 0x02,         /*      Input (Variable),               */
 	0x06, 0x00, 0xFF,   /*      Usage Page (FF00h),             */
@@ -714,14 +714,14 @@ static u8 dualshock4_bt_rdesc[] = {
 	0x19, 0x40,         /*      Usage Minimum (40h),            */
 	0x29, 0x42,         /*      Usage Maximum (42h),            */
 	0x16, 0x00, 0x80,   /*      Logical Minimum (-32768),       */
-	0x26, 0x00, 0x7F,   /*      Logical Maximum (32767),        */
+	0x26, 0xFF, 0x7F,   /*      Logical Maximum (32767),        */
 	0x75, 0x10,         /*      Report Size (16),               */
 	0x95, 0x03,         /*      Report Count (3),               */
 	0x81, 0x02,         /*      Input (Variable),               */
 	0x19, 0x43,         /*      Usage Minimum (43h),            */
 	0x29, 0x45,         /*      Usage Maximum (45h),            */
-	0x16, 0x00, 0xE0,   /*      Logical Minimum (-8192),        */
-	0x26, 0xFF, 0x1F,   /*      Logical Maximum (8191),         */
+	0x16, 0x00, 0x80,   /*      Logical Minimum (-32768),       */
+	0x26, 0xFF, 0x7F,   /*      Logical Maximum (32767),        */
 	0x95, 0x03,         /*      Report Count (3),               */
 	0x81, 0x02,         /*      Input (Variable),               */
 	0x06, 0x00, 0xFF,   /*      Usage Page (FF00h),             */
-- 
2.7.4

[PATCH v3 7/7] HID: sony: Update device ids

[Roderick Colenbrander]

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

Support additional DS4 model.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-core.c | 2 ++
 drivers/hid/hid-ids.h  | 1 +
 drivers/hid/hid-sony.c | 4 ++++
 3 files changed, 7 insertions(+)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 2b89c70..5ed2f57 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2059,6 +2059,8 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER) },
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGX_MOUSE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGP_MOUSE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SINO_LITE, USB_DEVICE_ID_SINO_LITE_CONTROLLER) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index cd59c79..27f82cc 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -899,6 +899,7 @@
 #define USB_DEVICE_ID_SONY_PS3_BDREMOTE		0x0306
 #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_MOTION_CONTROLLER	0x03d5
 #define USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER	0x042f
 #define USB_DEVICE_ID_SONY_BUZZ_CONTROLLER		0x0002
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index bd84790..14763cd 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -2641,6 +2641,10 @@ static const struct hid_device_id sony_devices[] = {
 		.driver_data = DUALSHOCK4_CONTROLLER_USB },
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER),
 		.driver_data = DUALSHOCK4_CONTROLLER_BT },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2),
+		.driver_data = DUALSHOCK4_CONTROLLER_USB },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER_2),
+		.driver_data = DUALSHOCK4_CONTROLLER_BT },
 	/* Nyko Core Controller for PS3 */
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SINO_LITE, USB_DEVICE_ID_SINO_LITE_CONTROLLER),
 		.driver_data = SIXAXIS_CONTROLLER_USB | SINO_LITE_CONTROLLER },
-- 
2.7.4

Re: [PATCH v3 0/7] HID: sony: game controller updates

[Jiri Kosina]

On Fri, 7 Oct 2016, Roderick Colenbrander wrote:

> This is a resubmit of the previous patches. Most patches are unchanged,
> just for clarity the whole series is sent again.

This is now queued in hid.git#for-4.10/sony. Given the slight confusion 
wrt. how this series has been (partially) (re-)submitted multiple times, I 
hope I got everything right; please check the branch.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 0/7] HID: sony: game controller updates

[Roderick Colenbrander]

On Mon, Oct 10, 2016 at 1:45 AM, Jiri Kosina <jikos@kernel.org> wrote:
> On Fri, 7 Oct 2016, Roderick Colenbrander wrote:
>
>> This is a resubmit of the previous patches. Most patches are unchanged,
>> just for clarity the whole series is sent again.
>
> This is now queued in hid.git#for-4.10/sony. Given the slight confusion
> wrt. how this series has been (partially) (re-)submitted multiple times, I
> hope I got everything right; please check the branch.
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>

Hi Jiri,

I have had a look over the branch and it looks correct to me. Thanks
for merging!

Thanks,
Roderick