[PATCH 0/5] HID: playstation: various DS4 and DualSense fixes

[PATCH 0/5] HID: playstation: various DS4 and DualSense fixes

[Roderick Colenbrander]

Hi,

This patch set contains a number of small fixes and stability improvements.

The stability patches are most critical as they prevent kernel crashes.
Over the years there have been various buggy devices usually clone devices,
but now apparently some official devices with wiped calibration data.
It is hard to handle all permutations of devices as some have constants,
some have broken constants (e.g. wrong sign of a coefficient). We disable
calibration when we see an invalid 0 denominator. The patch adds the same
logic to both the DualShock4 and DualSense code.

As part of the calibration stability improvements, a thorough review was
performed of the calibration code as it was suspected it was potentially
incorrect. It was found to be only slightly wrong for the handling of the
bias of the gyroscopes. Two patches fix this for both DualShock4 and DualSense.
There is only a marginal impact on practical values as the bias values
tend to be quite small.

Thanks,
Roderick Colenbrander
Sony Interactive Entertainment, LLC

Roderick Colenbrander (5):
  HID: playstation: fix DualShock4 unreachable calibration code.
  HID: playstation: sanity check DualShock4 calibration data.
  HID: playstation: sanity check DualSense calibration data.
  HID: playstation: correct DualShock4 gyro bias handling.
  HID: playstation: correct DualSense gyro bias handling.

 drivers/hid/hid-playstation.c | 104 +++++++++++++++++++++++++++-------
 1 file changed, 85 insertions(+), 19 deletions(-)

-- 
2.38.1

[PATCH 1/5] HID: playstation: fix DualShock4 unreachable calibration code.

[Roderick Colenbrander]

Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Reported-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-playstation.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index f399bf0d3c8c..866cc4e94320 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -1759,11 +1759,10 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
 				if (retries < 2) {
 					hid_warn(hdev, "Retrying DualShock 4 get calibration report (0x02) request\n");
 					continue;
-				} else {
-					ret = -EILSEQ;
-					goto err_free;
 				}
+
 				hid_err(hdev, "Failed to retrieve DualShock4 calibration info: %d\n", ret);
+				ret = -EILSEQ;
 				goto err_free;
 			} else {
 				break;
-- 
2.38.1

Re: [PATCH 1/5] HID: playstation: fix DualShock4 unreachable calibration code.

[Jiri Kosina]

On Thu, 5 Jan 2023, Roderick Colenbrander wrote:

> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Reported-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>

Applied to for-6.3/sony.

-- 
Jiri Kosina
SUSE Labs

[PATCH 2/5] HID: playstation: sanity check DualShock4 calibration data.

[Roderick Colenbrander]

Some DualShock4 devices report invalid calibration data resulting
in kernel oopses due to division by zero during report handling.

The devices affected generally appear to be clone devices, which don't
implement all reports properly and don't populate proper calibration
data. The issue may have been seen on an official device with erased
calibration reports.

This patch prevents the crashes by essentially disabling calibration
when invalid values are detected.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
Tested-by: Alain Carlucci <alain.carlucci@gmail.com>
---
 drivers/hid/hid-playstation.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 866cc4e94320..783ce9fd396e 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -1737,6 +1737,7 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
 	int speed_2x;
 	int range_2g;
 	int ret = 0;
+	int i;
 	uint8_t *buf;
 
 	if (ds4->base.hdev->bus == BUS_USB) {
@@ -1829,6 +1830,21 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
 	ds4->gyro_calib_data[2].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
 	ds4->gyro_calib_data[2].sens_denom = gyro_roll_plus - gyro_roll_minus;
 
+	/*
+	 * Sanity check gyro calibration data. This is needed to prevent crashes
+	 * during report handling of virtual, clone or broken devices not implementing
+	 * calibration data properly.
+	 */
+	for (i = 0; i < ARRAY_SIZE(ds4->gyro_calib_data); i++) {
+		if (ds4->gyro_calib_data[i].sens_denom == 0) {
+			hid_warn(hdev, "Invalid gyro calibration data for axis (%d), disabling calibration.",
+					ds4->gyro_calib_data[i].abs_code);
+			ds4->gyro_calib_data[i].bias = 0;
+			ds4->gyro_calib_data[i].sens_numer = DS4_GYRO_RANGE;
+			ds4->gyro_calib_data[i].sens_denom = S16_MAX;
+		}
+	}
+
 	/*
 	 * Set accelerometer calibration and normalization parameters.
 	 * Data values will be normalized to 1/DS4_ACC_RES_PER_G g.
@@ -1851,6 +1867,21 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
 	ds4->accel_calib_data[2].sens_numer = 2*DS4_ACC_RES_PER_G;
 	ds4->accel_calib_data[2].sens_denom = range_2g;
 
+	/*
+	 * Sanity check accelerometer calibration data. This is needed to prevent crashes
+	 * during report handling of virtual, clone or broken devices not implementing calibration
+	 * data properly.
+	 */
+	for (i = 0; i < ARRAY_SIZE(ds4->accel_calib_data); i++) {
+		if (ds4->accel_calib_data[i].sens_denom == 0) {
+			hid_warn(hdev, "Invalid accelerometer calibration data for axis (%d), disabling calibration.",
+					ds4->accel_calib_data[i].abs_code);
+			ds4->accel_calib_data[i].bias = 0;
+			ds4->accel_calib_data[i].sens_numer = DS4_ACC_RANGE;
+			ds4->accel_calib_data[i].sens_denom = S16_MAX;
+		}
+	}
+
 err_free:
 	kfree(buf);
 	return ret;
-- 
2.38.1

Re: [PATCH 2/5] HID: playstation: sanity check DualShock4 calibration data.

[Jiri Kosina]

On Thu, 5 Jan 2023, Roderick Colenbrander wrote:

> Some DualShock4 devices report invalid calibration data resulting
> in kernel oopses due to division by zero during report handling.
> 
> The devices affected generally appear to be clone devices, which don't
> implement all reports properly and don't populate proper calibration
> data. The issue may have been seen on an official device with erased
> calibration reports.
> 
> This patch prevents the crashes by essentially disabling calibration
> when invalid values are detected.
> 
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> Tested-by: Alain Carlucci <alain.carlucci@gmail.com>

Applied to for-6.2/upstream-fixes.

-- 
Jiri Kosina
SUSE Labs

[PATCH 3/5] HID: playstation: sanity check DualSense calibration data.

[Roderick Colenbrander]

Make sure calibration values are defined to prevent potential kernel
crashes. This fixes a hypothetical issue for virtual or clone devices
inspired by a similar fix for DS4.

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

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 783ce9fd396e..11bb6caf7412 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -944,6 +944,7 @@ ATTRIBUTE_GROUPS(ps_device);
 
 static int dualsense_get_calibration_data(struct dualsense *ds)
 {
+	struct hid_device *hdev = ds->base.hdev;
 	short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
 	short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
 	short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
@@ -954,6 +955,7 @@ static int dualsense_get_calibration_data(struct dualsense *ds)
 	int speed_2x;
 	int range_2g;
 	int ret = 0;
+	int i;
 	uint8_t *buf;
 
 	buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
@@ -1005,6 +1007,21 @@ static int dualsense_get_calibration_data(struct dualsense *ds)
 	ds->gyro_calib_data[2].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
 	ds->gyro_calib_data[2].sens_denom = gyro_roll_plus - gyro_roll_minus;
 
+	/*
+	 * Sanity check gyro calibration data. This is needed to prevent crashes
+	 * during report handling of virtual, clone or broken devices not implementing
+	 * calibration data properly.
+	 */
+	for (i = 0; i < ARRAY_SIZE(ds->gyro_calib_data); i++) {
+		if (ds->gyro_calib_data[i].sens_denom == 0) {
+			hid_warn(hdev, "Invalid gyro calibration data for axis (%d), disabling calibration.",
+					ds->gyro_calib_data[i].abs_code);
+			ds->gyro_calib_data[i].bias = 0;
+			ds->gyro_calib_data[i].sens_numer = DS_GYRO_RANGE;
+			ds->gyro_calib_data[i].sens_denom = S16_MAX;
+		}
+	}
+
 	/*
 	 * Set accelerometer calibration and normalization parameters.
 	 * Data values will be normalized to 1/DS_ACC_RES_PER_G g.
@@ -1027,6 +1044,21 @@ static int dualsense_get_calibration_data(struct dualsense *ds)
 	ds->accel_calib_data[2].sens_numer = 2*DS_ACC_RES_PER_G;
 	ds->accel_calib_data[2].sens_denom = range_2g;
 
+	/*
+	 * Sanity check accelerometer calibration data. This is needed to prevent crashes
+	 * during report handling of virtual, clone or broken devices not implementing calibration
+	 * data properly.
+	 */
+	for (i = 0; i < ARRAY_SIZE(ds->accel_calib_data); i++) {
+		if (ds->accel_calib_data[i].sens_denom == 0) {
+			hid_warn(hdev, "Invalid accelerometer calibration data for axis (%d), disabling calibration.",
+					ds->accel_calib_data[i].abs_code);
+			ds->accel_calib_data[i].bias = 0;
+			ds->accel_calib_data[i].sens_numer = DS_ACC_RANGE;
+			ds->accel_calib_data[i].sens_denom = S16_MAX;
+		}
+	}
+
 err_free:
 	kfree(buf);
 	return ret;
-- 
2.38.1

Re: [PATCH 3/5] HID: playstation: sanity check DualSense calibration data.

[Jiri Kosina]

On Thu, 5 Jan 2023, Roderick Colenbrander wrote:

> Make sure calibration values are defined to prevent potential kernel
> crashes. This fixes a hypothetical issue for virtual or clone devices
> inspired by a similar fix for DS4.
> 
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>

Applied to for-6.2/upstream-fixes.

-- 
Jiri Kosina
SUSE Labs

[PATCH 4/5] HID: playstation: correct DualShock4 gyro bias handling.

[Roderick Colenbrander]

The bias for the gyroscope is not used correctly. The sensor bias
needs to be used in calculation of the 'sensivity' instead of being
an offset.

In practice this has little input on the values as the bias values
tends to be small (+/- 20).

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

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 11bb6caf7412..1001eab6ff75 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -1848,19 +1848,22 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
 	 */
 	speed_2x = (gyro_speed_plus + gyro_speed_minus);
 	ds4->gyro_calib_data[0].abs_code = ABS_RX;
-	ds4->gyro_calib_data[0].bias = gyro_pitch_bias;
+	ds4->gyro_calib_data[0].bias = 0;
 	ds4->gyro_calib_data[0].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
-	ds4->gyro_calib_data[0].sens_denom = gyro_pitch_plus - gyro_pitch_minus;
+	ds4->gyro_calib_data[0].sens_denom = abs(gyro_pitch_plus - gyro_pitch_bias) +
+			abs(gyro_pitch_minus - gyro_pitch_bias);
 
 	ds4->gyro_calib_data[1].abs_code = ABS_RY;
-	ds4->gyro_calib_data[1].bias = gyro_yaw_bias;
+	ds4->gyro_calib_data[1].bias = 0;
 	ds4->gyro_calib_data[1].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
-	ds4->gyro_calib_data[1].sens_denom = gyro_yaw_plus - gyro_yaw_minus;
+	ds4->gyro_calib_data[1].sens_denom = abs(gyro_yaw_plus - gyro_yaw_bias) +
+			abs(gyro_yaw_minus - gyro_yaw_bias);
 
 	ds4->gyro_calib_data[2].abs_code = ABS_RZ;
-	ds4->gyro_calib_data[2].bias = gyro_roll_bias;
+	ds4->gyro_calib_data[2].bias = 0;
 	ds4->gyro_calib_data[2].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
-	ds4->gyro_calib_data[2].sens_denom = gyro_roll_plus - gyro_roll_minus;
+	ds4->gyro_calib_data[2].sens_denom = abs(gyro_roll_plus - gyro_roll_bias) +
+			abs(gyro_roll_minus - gyro_roll_bias);
 
 	/*
 	 * Sanity check gyro calibration data. This is needed to prevent crashes
@@ -2241,8 +2244,7 @@ static int dualshock4_parse_report(struct ps_device *ps_dev, struct hid_report *
 	for (i = 0; i < ARRAY_SIZE(ds4_report->gyro); i++) {
 		int raw_data = (short)le16_to_cpu(ds4_report->gyro[i]);
 		int calib_data = mult_frac(ds4->gyro_calib_data[i].sens_numer,
-					   raw_data - ds4->gyro_calib_data[i].bias,
-					   ds4->gyro_calib_data[i].sens_denom);
+					   raw_data, ds4->gyro_calib_data[i].sens_denom);
 
 		input_report_abs(ds4->sensors, ds4->gyro_calib_data[i].abs_code, calib_data);
 	}
-- 
2.38.1

Re: [PATCH 4/5] HID: playstation: correct DualShock4 gyro bias handling.

[Jiri Kosina]

On Thu, 5 Jan 2023, Roderick Colenbrander wrote:

> The bias for the gyroscope is not used correctly. The sensor bias
> needs to be used in calculation of the 'sensivity' instead of being
> an offset.
> 
> In practice this has little input on the values as the bias values
> tends to be small (+/- 20).
> 
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>

Applied to for-6.3/sony.

-- 
Jiri Kosina
SUSE Labs

[PATCH 5/5] HID: playstation: correct DualSense gyro bias handling.

[Roderick Colenbrander]

The bias for the gyroscope is not used correctly. The sensor bias
needs to be used in calculation of the 'sensivity' instead of being
an offset.

In practice this has little input on the values as the bias values
tends to be small (+/- 20).

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

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 1001eab6ff75..8ac8f7b8e317 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -993,19 +993,22 @@ static int dualsense_get_calibration_data(struct dualsense *ds)
 	 */
 	speed_2x = (gyro_speed_plus + gyro_speed_minus);
 	ds->gyro_calib_data[0].abs_code = ABS_RX;
-	ds->gyro_calib_data[0].bias = gyro_pitch_bias;
+	ds->gyro_calib_data[0].bias = 0;
 	ds->gyro_calib_data[0].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
-	ds->gyro_calib_data[0].sens_denom = gyro_pitch_plus - gyro_pitch_minus;
+	ds->gyro_calib_data[0].sens_denom = abs(gyro_pitch_plus - gyro_pitch_bias) +
+			abs(gyro_pitch_minus - gyro_pitch_bias);
 
 	ds->gyro_calib_data[1].abs_code = ABS_RY;
-	ds->gyro_calib_data[1].bias = gyro_yaw_bias;
+	ds->gyro_calib_data[1].bias = 0;
 	ds->gyro_calib_data[1].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
-	ds->gyro_calib_data[1].sens_denom = gyro_yaw_plus - gyro_yaw_minus;
+	ds->gyro_calib_data[1].sens_denom = abs(gyro_yaw_plus - gyro_yaw_bias) +
+			abs(gyro_yaw_minus - gyro_yaw_bias);
 
 	ds->gyro_calib_data[2].abs_code = ABS_RZ;
-	ds->gyro_calib_data[2].bias = gyro_roll_bias;
+	ds->gyro_calib_data[2].bias = 0;
 	ds->gyro_calib_data[2].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
-	ds->gyro_calib_data[2].sens_denom = gyro_roll_plus - gyro_roll_minus;
+	ds->gyro_calib_data[2].sens_denom = abs(gyro_roll_plus - gyro_roll_bias) +
+			abs(gyro_roll_minus - gyro_roll_bias);
 
 	/*
 	 * Sanity check gyro calibration data. This is needed to prevent crashes
@@ -1388,8 +1391,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	for (i = 0; i < ARRAY_SIZE(ds_report->gyro); i++) {
 		int raw_data = (short)le16_to_cpu(ds_report->gyro[i]);
 		int calib_data = mult_frac(ds->gyro_calib_data[i].sens_numer,
-					   raw_data - ds->gyro_calib_data[i].bias,
-					   ds->gyro_calib_data[i].sens_denom);
+					   raw_data, ds->gyro_calib_data[i].sens_denom);
 
 		input_report_abs(ds->sensors, ds->gyro_calib_data[i].abs_code, calib_data);
 	}
-- 
2.38.1

Re: [PATCH 5/5] HID: playstation: correct DualSense gyro bias handling.

[Jiri Kosina]

On Thu, 5 Jan 2023, Roderick Colenbrander wrote:

> The bias for the gyroscope is not used correctly. The sensor bias
> needs to be used in calculation of the 'sensivity' instead of being
> an offset.
> 
> In practice this has little input on the values as the bias values
> tends to be small (+/- 20).
> 
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>

Applied to for-6.3/sony.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 0/5] HID: playstation: various DS4 and DualSense fixes

[Jiri Kosina]

On Thu, 5 Jan 2023, Roderick Colenbrander wrote:

> Hi,
> 
> This patch set contains a number of small fixes and stability improvements.
> 
> The stability patches are most critical as they prevent kernel crashes.
> Over the years there have been various buggy devices usually clone devices,
> but now apparently some official devices with wiped calibration data.
> It is hard to handle all permutations of devices as some have constants,
> some have broken constants (e.g. wrong sign of a coefficient). We disable
> calibration when we see an invalid 0 denominator. The patch adds the same
> logic to both the DualShock4 and DualSense code.
> 
> As part of the calibration stability improvements, a thorough review was
> performed of the calibration code as it was suspected it was potentially
> incorrect. It was found to be only slightly wrong for the handling of the
> bias of the gyroscopes. Two patches fix this for both DualShock4 and DualSense.
> There is only a marginal impact on practical values as the bias values
> tend to be quite small.
> 
> Thanks,
> Roderick Colenbrander
> Sony Interactive Entertainment, LLC

I am splitting the stability ones (for 6.2) from the small improvements / 
cleanups (for 6.3).

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 0/5] HID: playstation: various DS4 and DualSense fixes

[Roderick Colenbrander]

On Wed, Jan 18, 2023 at 2:12 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Thu, 5 Jan 2023, Roderick Colenbrander wrote:
>
> > Hi,
> >
> > This patch set contains a number of small fixes and stability improvements.
> >
> > The stability patches are most critical as they prevent kernel crashes.
> > Over the years there have been various buggy devices usually clone devices,
> > but now apparently some official devices with wiped calibration data.
> > It is hard to handle all permutations of devices as some have constants,
> > some have broken constants (e.g. wrong sign of a coefficient). We disable
> > calibration when we see an invalid 0 denominator. The patch adds the same
> > logic to both the DualShock4 and DualSense code.
> >
> > As part of the calibration stability improvements, a thorough review was
> > performed of the calibration code as it was suspected it was potentially
> > incorrect. It was found to be only slightly wrong for the handling of the
> > bias of the gyroscopes. Two patches fix this for both DualShock4 and DualSense.
> > There is only a marginal impact on practical values as the bias values
> > tend to be quite small.
> >
> > Thanks,
> > Roderick Colenbrander
> > Sony Interactive Entertainment, LLC
>
> I am splitting the stability ones (for 6.2) from the small improvements /
> cleanups (for 6.3).
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>

Thanks, that's how the patches were kind of set up. I wasn't sure due
to timing where they would land, but thanks for pulling in the
stability fixes!

Thanks,
Roderick