All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] HID: sony: Fix division by zero
@ 2022-12-26  0:07 Alain Carlucci
  2022-12-27 16:17 ` Roderick Colenbrander
  0 siblings, 1 reply; 13+ messages in thread
From: Alain Carlucci @ 2022-12-26  0:07 UTC (permalink / raw)
  To: linux-input; +Cc: jikos

Hello,

Today I connected a partially broken DS4 via USB and got a kernel
panic with a division by zero in the hid-sony driver.

The issue is caused by sens_denom=0 in the sensor calibration data,
which triggers a division by zero when dualshock4_parse_report() is
invoked, the division happens in the mult_frac() macro.

This patch applies a workaround that allows the DS4 to be used even
with a broken sensor: if the denominator sent by the device is zero,
it is replaced with 1 and a warning is emitted.

Signed-off-by: Alain Carlucci <alain.carlucci@gmail.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 13125997a..f8b05cb29 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1714,6 +1714,7 @@ static int dualshock4_get_calibration_data(struct sony_sc *sc)
  	short acc_z_plus, acc_z_minus;
  	int speed_2x;
  	int range_2g;
+	int calib_id;
  
  	/* For Bluetooth we use a different request, which supports CRC.
  	 * Note: in Bluetooth mode feature report 0x02 also changes the state
@@ -1858,6 +1859,23 @@ static int dualshock4_get_calibration_data(struct sony_sc *sc)
  	sc->ds4_calib_data[5].sens_numer = 2*DS4_ACC_RES_PER_G;
  	sc->ds4_calib_data[5].sens_denom = range_2g;
  
+	for (calib_id = 0; calib_id < 6; calib_id++) {
+		/* Ensure there are no denominators equal to zero to prevent
+		 * crashes while dividing by that number.
+		 */
+
+		if (sc->ds4_calib_data[calib_id].sens_denom != 0) {
+			/* Denominator OK, skip this */
+			continue;
+		}
+
+		sc->ds4_calib_data[calib_id].sens_denom = 1;
+
+		hid_warn(sc->hdev,
+			 "DualShock 4 USB dongle: invalid calibration for sensor %d\n",
+			 calib_id);
+	}
+
  err_stop:
  	kfree(buf);
  	return ret;
-- 
2.39.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: sony: Fix division by zero
  2022-12-26  0:07 [PATCH] HID: sony: Fix division by zero Alain Carlucci
@ 2022-12-27 16:17 ` Roderick Colenbrander
  2022-12-28 18:01   ` Alain Carlucci
  0 siblings, 1 reply; 13+ messages in thread
From: Roderick Colenbrander @ 2022-12-27 16:17 UTC (permalink / raw)
  To: Alain Carlucci; +Cc: linux-input, jikos

Hi Alain,

Thanks for sharing your patch. Others have encountered similar issues.
This is the case when the calibration coefficients are incorrect.
These are hard programmed into devices from the factory. It are
typically clone devices, which don't implement all DS4 functionality
properly.

Can you try printing all the variables (gyro_speed_plus,..
acc_z_minus) for your device as decimal numbers from the
get_calibration_data function?

I have been considering to add a little bit of clone support code if
possible to the new hid-playstation as long as it doesn't pollute the
code too much or causes issues.

Thanks,
Roderick

On Sun, Dec 25, 2022 at 4:08 PM Alain Carlucci <alain.carlucci@gmail.com> wrote:
>
> Hello,
>
> Today I connected a partially broken DS4 via USB and got a kernel
> panic with a division by zero in the hid-sony driver.
>
> The issue is caused by sens_denom=0 in the sensor calibration data,
> which triggers a division by zero when dualshock4_parse_report() is
> invoked, the division happens in the mult_frac() macro.
>
> This patch applies a workaround that allows the DS4 to be used even
> with a broken sensor: if the denominator sent by the device is zero,
> it is replaced with 1 and a warning is emitted.
>
> Signed-off-by: Alain Carlucci <alain.carlucci@gmail.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 13125997a..f8b05cb29 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -1714,6 +1714,7 @@ static int dualshock4_get_calibration_data(struct sony_sc *sc)
>         short acc_z_plus, acc_z_minus;
>         int speed_2x;
>         int range_2g;
> +       int calib_id;
>
>         /* For Bluetooth we use a different request, which supports CRC.
>          * Note: in Bluetooth mode feature report 0x02 also changes the state
> @@ -1858,6 +1859,23 @@ static int dualshock4_get_calibration_data(struct sony_sc *sc)
>         sc->ds4_calib_data[5].sens_numer = 2*DS4_ACC_RES_PER_G;
>         sc->ds4_calib_data[5].sens_denom = range_2g;
>
> +       for (calib_id = 0; calib_id < 6; calib_id++) {
> +               /* Ensure there are no denominators equal to zero to prevent
> +                * crashes while dividing by that number.
> +                */
> +
> +               if (sc->ds4_calib_data[calib_id].sens_denom != 0) {
> +                       /* Denominator OK, skip this */
> +                       continue;
> +               }
> +
> +               sc->ds4_calib_data[calib_id].sens_denom = 1;
> +
> +               hid_warn(sc->hdev,
> +                        "DualShock 4 USB dongle: invalid calibration for sensor %d\n",
> +                        calib_id);
> +       }
> +
>   err_stop:
>         kfree(buf);
>         return ret;
> --
> 2.39.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: sony: Fix division by zero
  2022-12-27 16:17 ` Roderick Colenbrander
@ 2022-12-28 18:01   ` Alain Carlucci
       [not found]     ` <CAEc3jaD-Z4F8CQBHKrBV=H1JwO3LhQMxy1rv2k30rCYhkr1CmQ@mail.gmail.com>
  0 siblings, 1 reply; 13+ messages in thread
From: Alain Carlucci @ 2022-12-28 18:01 UTC (permalink / raw)
  To: Roderick Colenbrander; +Cc: linux-input, jikos

Hi Roderick,

On Tue, Dec 27, 2022 at 08:17:15AM -0800, Roderick Colenbrander wrote:
>Thanks for sharing your patch. Others have encountered similar issues.
>This is the case when the calibration coefficients are incorrect.
>These are hard programmed into devices from the factory. It are
>typically clone devices, which don't implement all DS4 functionality
>properly.

I cannot ensure it's an original DS4 but it really seems so.
The board is a JDM-055 with a MT3610N, but instead of a BST-BMI270 (as
reported on a reverse-engineered schematic[1]) it has a "N339(CCF)".
I can't find any datasheet or specification of that chip.
I'll provide few images of the board for reference[2].
Do you think it's a clone?

>Can you try printing all the variables (gyro_speed_plus,..
>acc_z_minus) for your device as decimal numbers from the
>get_calibration_data function?

Sure, here is the output:
gyro_pitch_plus=0 gyro_pitch_minus=0 gyro_yaw_plus=0 gyro_yaw_minus=0
gyro_roll_plus=0 gyro_roll_minus=0 gyro_speed_plus=0
gyro_speed_minus=0 acc_x_plus=0 acc_x_minus=0 acc_y_plus=0
acc_y_minus=0 acc_z_plus=0 acc_z_minus=0 

Probably it has communication issues with the IMU.
This one is a joypad with multiple issues I'm trying to solve one by one.

By the way, do you know if there are tools for linux to test all the
functionalities of the DS4? I would be interested to read IMU values,
test audio, vibration and touchpad.

Thanks,
Alain

[1] https://www.acidmods.com/RDC/DS4/JDM-055/1-982-707-31%20small/JDM-055__(31)_SOME_VALUES.pdf
[2] https://imgur.com/a/tckVWKR

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: sony: Fix division by zero
       [not found]     ` <CAEc3jaD-Z4F8CQBHKrBV=H1JwO3LhQMxy1rv2k30rCYhkr1CmQ@mail.gmail.com>
@ 2022-12-28 21:58       ` Alain Carlucci
  2022-12-29 16:28         ` Roderick Colenbrander
  0 siblings, 1 reply; 13+ messages in thread
From: Alain Carlucci @ 2022-12-28 21:58 UTC (permalink / raw)
  To: Roderick Colenbrander; +Cc: linux-input, Jiri Kosina

On Wed, Dec 28, 2022 at 11:38:03AM -0800, Roderick Colenbrander wrote:
>Hi Alain,
>
>Thanks for the info. Out of curiosity are the reported sensor values in the
>hid report non zero?

Just wrote the print code and unexpectedly.. yes!

Here's a report of the raw_data variable in dualshock4_parse_report()
using printk:

[10793.137066] rd0=4 rd1=1  rd2=17 rd3=707 rd4=-8063 rd5=-1536 
[10793.141121] rd0=4 rd1=0  rd2=17 rd3=698 rd4=-8061 rd5=-1546 
[10793.145101] rd0=4 rd1=-1 rd2=19 rd3=699 rd4=-8062 rd5=-1544 
[10793.149059] rd0=3 rd1=0  rd2=19 rd3=695 rd4=-8052 rd5=-1554 
[10793.153085] rd0=4 rd1=-1 rd2=19 rd3=691 rd4=-8052 rd5=-1547 
[10793.157059] rd0=4 rd1=0  rd2=18 rd3=709 rd4=-8057 rd5=-1551 

>As for testing there are no good apps. Evtest can at least do gamepad and
>touchpad. Unfortunately we didn't make a proper desktop test app. Hopefully
>in the future.
Ok got it, thank you.

Thanks,
Alain

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: sony: Fix division by zero
  2022-12-28 21:58       ` Alain Carlucci
@ 2022-12-29 16:28         ` Roderick Colenbrander
  2022-12-29 19:21           ` Alain Carlucci
  0 siblings, 1 reply; 13+ messages in thread
From: Roderick Colenbrander @ 2022-12-29 16:28 UTC (permalink / raw)
  To: Alain Carlucci; +Cc: linux-input, Jiri Kosina

[-- Attachment #1: Type: text/plain, Size: 1945 bytes --]

Hi Alain,

Give this patch a try. It is against hid-playstation, which as of 6.2
supports DS4. Let me know if it works. You can see the sensors working
through evdev on the motion sensors device.

In regards to your DS4. I am not sure if yours is an official one or
not. There are many official DS4 revisions (probably 10+ different
PCBs) and the clones go to great length. Considering the sensors do
seem to work, it does feel odd that the data is zero. Either something
cleared the calibration data (would be strange) or it might be a
clone. If you want to dig deeper, you can play around with
dualshock4_get_calibration_data in hid-playstation. The particular
report (though not fully documented in the driver) does contain a lot
of device info (firmware info, manufacturing dates, various strings).
You can probably find the details online. Some data there might be
weird or not populated.

Thanks,
Roderick

On Wed, Dec 28, 2022 at 1:58 PM Alain Carlucci <alain.carlucci@gmail.com> wrote:
>
> On Wed, Dec 28, 2022 at 11:38:03AM -0800, Roderick Colenbrander wrote:
> >Hi Alain,
> >
> >Thanks for the info. Out of curiosity are the reported sensor values in the
> >hid report non zero?
>
> Just wrote the print code and unexpectedly.. yes!
>
> Here's a report of the raw_data variable in dualshock4_parse_report()
> using printk:
>
> [10793.137066] rd0=4 rd1=1  rd2=17 rd3=707 rd4=-8063 rd5=-1536
> [10793.141121] rd0=4 rd1=0  rd2=17 rd3=698 rd4=-8061 rd5=-1546
> [10793.145101] rd0=4 rd1=-1 rd2=19 rd3=699 rd4=-8062 rd5=-1544
> [10793.149059] rd0=3 rd1=0  rd2=19 rd3=695 rd4=-8052 rd5=-1554
> [10793.153085] rd0=4 rd1=-1 rd2=19 rd3=691 rd4=-8052 rd5=-1547
> [10793.157059] rd0=4 rd1=0  rd2=18 rd3=709 rd4=-8057 rd5=-1551
>
> >As for testing there are no good apps. Evtest can at least do gamepad and
> >touchpad. Unfortunately we didn't make a proper desktop test app. Hopefully
> >in the future.
> Ok got it, thank you.
>
> Thanks,
> Alain

[-- Attachment #2: 0002-HID-playstation-sanity-check-DS4-calibration-data.patch --]
[-- Type: text/x-patch, Size: 3064 bytes --]

From 6e2d8f9fd243bcb8cc3de19212f64fcd8701fca0 Mon Sep 17 00:00:00 2001
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Date: Thu, 29 Dec 2022 08:19:28 -0800
Subject: [PATCH 2/2] HID: playstation: sanity check DS4 calibration data.

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

The devices affected appear to be clone devices, which don't
implement all reports properly and don't populate proper calibration
data.

This patch prevents the crashes by essentially disabling calibration
when invalid values are detected.
---
 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..bdc32c818dc7 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,23 @@ 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 for clone devices,
+	 * which poorly implement support. Some devices appear to return zeros
+	 * for all data, while some hardcode all the same values and for some
+	 * use the wrong sign (the delta is then 0).
+	 * When we detect something fishy, just disable the calibration logic
+	 * altogether as nothing can be trusted for that axis.
+	 */
+	for (i = 0; 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_RES_PER_DEG_S;
+			ds4->gyro_calib_data[i].sens_denom = 1;
+		}
+	}
+
 	/*
 	 * Set accelerometer calibration and normalization parameters.
 	 * Data values will be normalized to 1/DS4_ACC_RES_PER_G g.
@@ -1851,6 +1869,19 @@ 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 for clone devices,
+	 * which poorly implement support.
+	 */
+	for (i = 0; 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_RES_PER_G;
+			ds4->accel_calib_data[i].sens_denom = 1;
+		}
+	}
+
 err_free:
 	kfree(buf);
 	return ret;
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: sony: Fix division by zero
  2022-12-29 16:28         ` Roderick Colenbrander
@ 2022-12-29 19:21           ` Alain Carlucci
  2022-12-29 20:56             ` Roderick Colenbrander
  0 siblings, 1 reply; 13+ messages in thread
From: Alain Carlucci @ 2022-12-29 19:21 UTC (permalink / raw)
  To: Roderick Colenbrander; +Cc: linux-input, Jiri Kosina

Hi Roderick,

>Give this patch a try. It is against hid-playstation, which as of 6.2
>supports DS4. Let me know if it works. You can see the sensors working
>through evdev on the motion sensors device.

Thank you for the patch, just tried. I think there is a typo in the
condition of the for-loop that sanitizes the input.
Instead of `; i < ARRAY_SIZE()` it's written `; ARRAY_SIZE()`, 
which always evaluates to true. The loop then overflows the array and
crashes the kernel.

By the way, the DualSense code has similar calibration code and also
there the input is not sanitized.
I think it's quite easy to create a fake DualSense device with
an Arduino that emulates the protocol up to calib_denom=0, just to
exploit that and crash every linux machine is plugged in. Or even
worst, exploit that via bluetooth.

>If you want to dig deeper, you can play around with
>dualshock4_get_calibration_data in hid-playstation. The particular
>report (though not fully documented in the driver) does contain a lot
>of device info (firmware info, manufacturing dates, various strings).
>You can probably find the details online. Some data there might be
>weird or not populated.

Thank you! Just discovered that this strange DS4 (CUH-ZCT2E) replies
all zeros both to HID request 0x02 (get calibration data) and as the
BT address (request 0x12, Pairing Info), which explains why BT is not
working.

I tried to request version info (0xa3), the response seems the same as
another fully-working and original CUH-ZCT2E:
"""
Build Date: Sep 21 2018 04:50:51
HW Version: 0100.b008
SW Version: 00000001.a00a
SW Series:  2010
Code Size:  0002a000
"""

Anyway, I have seen that request 0xA2 puts the DS4 in an alternate
boot mode, probably DFU mode, where the device reboots as 054c:0856
and waits for data, which seems totally undocumented online. 
Do you know anything about this mode?
It would be amazing to be able to reflash the original firmware,

Thanks,
Alain

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: sony: Fix division by zero
  2022-12-29 19:21           ` Alain Carlucci
@ 2022-12-29 20:56             ` Roderick Colenbrander
  2022-12-30 18:17               ` Alain
       [not found]               ` <CAME7zCKPjFbE6nSSoQOVK=BnFG0YAvMgHjAmHKTXcxk3Weuo+w@mail.gmail.com>
  0 siblings, 2 replies; 13+ messages in thread
From: Roderick Colenbrander @ 2022-12-29 20:56 UTC (permalink / raw)
  To: Alain Carlucci; +Cc: linux-input, Jiri Kosina

Hi Alain,

On Thu, Dec 29, 2022 at 11:21 AM Alain Carlucci
<alain.carlucci@gmail.com> wrote:
>
> Hi Roderick,
>
> >Give this patch a try. It is against hid-playstation, which as of 6.2
> >supports DS4. Let me know if it works. You can see the sensors working
> >through evdev on the motion sensors device.
>
> Thank you for the patch, just tried. I think there is a typo in the
> condition of the for-loop that sanitizes the input.
> Instead of `; i < ARRAY_SIZE()` it's written `; ARRAY_SIZE()`,
> which always evaluates to true. The loop then overflows the array and
> crashes the kernel.

Ooh oops. It was a quick patch I wrote without testing.

> By the way, the DualSense code has similar calibration code and also
> there the input is not sanitized.
> I think it's quite easy to create a fake DualSense device with
> an Arduino that emulates the protocol up to calib_denom=0, just to
> exploit that and crash every linux machine is plugged in. Or even
> worst, exploit that via bluetooth.

You are right it probably won't hurt to duplicate the logic there.

> >If you want to dig deeper, you can play around with
> >dualshock4_get_calibration_data in hid-playstation. The particular
> >report (though not fully documented in the driver) does contain a lot
> >of device info (firmware info, manufacturing dates, various strings).
> >You can probably find the details online. Some data there might be
> >weird or not populated.
>
> Thank you! Just discovered that this strange DS4 (CUH-ZCT2E) replies
> all zeros both to HID request 0x02 (get calibration data) and as the
> BT address (request 0x12, Pairing Info), which explains why BT is not
> working.

There is definitely something weird with your device. Something must
have gotten wiped somehow.

> I tried to request version info (0xa3), the response seems the same as
> another fully-working and original CUH-ZCT2E:
> """
> Build Date: Sep 21 2018 04:50:51
> HW Version: 0100.b008
> SW Version: 00000001.a00a
> SW Series:  2010
> Code Size:  0002a000
> """
>
> Anyway, I have seen that request 0xA2 puts the DS4 in an alternate
> boot mode, probably DFU mode, where the device reboots as 054c:0856
> and waits for data, which seems totally undocumented online.
> Do you know anything about this mode?
> It would be amazing to be able to reflash the original firmware,

Unfortunately I can't disclose any of that information. I can say that
on DS4 it wasn't common to update firmware (as opposed to DualSense)
while out in the wild. Some of these requests and others are probably
used to initiate firmware programming and programming of MAC address,
calibration data and other settings. It is probably quite involved and
hard to get right without bricking your device.

> Thanks,
> Alain

Thanks,
Roderick

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: sony: Fix division by zero
  2022-12-29 20:56             ` Roderick Colenbrander
@ 2022-12-30 18:17               ` Alain
       [not found]               ` <CAME7zCKPjFbE6nSSoQOVK=BnFG0YAvMgHjAmHKTXcxk3Weuo+w@mail.gmail.com>
  1 sibling, 0 replies; 13+ messages in thread
From: Alain @ 2022-12-30 18:17 UTC (permalink / raw)
  To: Roderick Colenbrander, djogorchock; +Cc: linux-input, Jiri Kosina

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

Hi Roderick,
Hi Daniel,

Thank you for all the suggestions, Roderick.
I fixed the typo in your patch, backported to hid-sony and tested both of them.
They fix the issue and the DS4 can be used even without calibration data.
I cannot test if everything works well with a DualSense because I do
not own one.

Hi Daniel, I've seen that in the hid-nintendo driver, the calibration
denominator is not sanitized if it's zero.
If I'm not mistaken, a device that disguises itself as a joycon with
denominator zero can crash the kernel
when this is used in the mult_frac(). I had this behaviour with the
hid-sony driver.
You can find attached the patch that should solve the problem.

Thanks,
Alain

[-- Attachment #2: 0003-HID-nintendo-sanity-check-for-denominators.patch --]
[-- Type: text/x-patch, Size: 1309 bytes --]

From 5f7aa1612a10123b9630701d29e39f8e55a74b5e Mon Sep 17 00:00:00 2001
From: Alain Carlucci <carpikemail@gmail.com>
Date: Fri, 30 Dec 2022 18:53:53 +0100
Subject: [PATCH 3/3] HID: nintendo: sanity check for denominators

This patch prevents a crash if the device reports denominator equal to
zero. In that case, it is substituted with one, so that the driver does
not do a division by zero.
---
 drivers/hid/hid-nintendo.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 5bfc0c450..f050754e2 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -866,6 +866,19 @@ static void joycon_calc_imu_cal_divisors(struct joycon_ctlr *ctlr)
 						ctlr->accel_cal.offset[i];
 		ctlr->imu_cal_gyro_divisor[i] = ctlr->gyro_cal.scale[i] -
 						ctlr->gyro_cal.offset[i];
+
+
+		/* Sanity check accelerometer calibration data. */
+		if (ctlr->imu_cal_accel_divisor[i] == 0) {
+			hid_warn(ctlr->hdev, "Invalid divisor for accelerometer %d.", i);
+			ctlr->imu_cal_accel_divisor[i] = 1;
+		}
+
+		/* Sanity check gyroscope calibration data. */
+		if (ctlr->imu_cal_gyro_divisor[i] == 0) {
+			hid_warn(ctlr->hdev, "Invalid divisor for gyroscope %d.", i);
+			ctlr->imu_cal_gyro_divisor[i] = 1;
+		}
 	}
 }
 
-- 
2.39.0


[-- Attachment #3: 0001-HID-playstation-sanity-check-calibration-data.patch --]
[-- Type: text/x-patch, Size: 4975 bytes --]

From d751911f23aca413b2bf831ea301219c533081e6 Mon Sep 17 00:00:00 2001
From: Alain Carlucci <carpikemail@gmail.com>
Date: Fri, 30 Dec 2022 18:47:54 +0100
Subject: [PATCH 1/3] HID: playstation: sanity check calibration data.

This patch prevents a division by zero if the the device reports invalid
calibration parameters. In that case, fallback to default values.
---
 drivers/hid/hid-playstation.c | 61 +++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index f399bf0d3..a249f3bb0 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -954,6 +954,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 +1006,23 @@ 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 for clone devices,
+	 * which poorly implement support. Some devices appear to return zeros
+	 * for all data, while some hardcode all the same values and for some
+	 * use the wrong sign (the delta is then 0).
+	 * When we detect something fishy, just disable the calibration logic
+	 * altogether as nothing can be trusted for that axis.
+	 */
+	for (i = 0; i < ARRAY_SIZE(ds->gyro_calib_data); i++) {
+		if (ds->gyro_calib_data[i].sens_denom == 0) {
+			hid_warn(ds->base.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_RES_PER_DEG_S;
+			ds->gyro_calib_data[i].sens_denom = 1;
+		}
+	}
+
 	/*
 	 * Set accelerometer calibration and normalization parameters.
 	 * Data values will be normalized to 1/DS_ACC_RES_PER_G g.
@@ -1027,6 +1045,18 @@ 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 for clone devices,
+	 * which poorly implement support.
+	 */
+	for (i = 0; i < ARRAY_SIZE(ds->accel_calib_data); i++) {
+		if (ds->accel_calib_data[i].sens_denom == 0) {
+			hid_warn(ds->base.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_RES_PER_G;
+			ds->accel_calib_data[i].sens_denom = 1;
+		}
+	}
 err_free:
 	kfree(buf);
 	return ret;
@@ -1737,6 +1767,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) {
@@ -1830,6 +1861,23 @@ 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 for clone devices,
+	 * which poorly implement support. Some devices appear to return zeros
+	 * for all data, while some hardcode all the same values and for some
+	 * use the wrong sign (the delta is then 0).
+	 * When we detect something fishy, just disable the calibration logic
+	 * altogether as nothing can be trusted for that axis.
+	 */
+	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_RES_PER_DEG_S;
+			ds4->gyro_calib_data[i].sens_denom = 1;
+		}
+	}
+
 	/*
 	 * Set accelerometer calibration and normalization parameters.
 	 * Data values will be normalized to 1/DS4_ACC_RES_PER_G g.
@@ -1852,6 +1900,19 @@ 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 for clone devices,
+	 * which poorly implement support.
+	 */
+	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_RES_PER_G;
+			ds4->accel_calib_data[i].sens_denom = 1;
+		}
+	}
+
 err_free:
 	kfree(buf);
 	return ret;
-- 
2.39.0


[-- Attachment #4: 0002-HID-sony-sanity-check-DS4-calibration-data.patch --]
[-- Type: text/x-patch, Size: 2086 bytes --]

From dc92f0be1352ba5a27239f451e5f0161e48f14e6 Mon Sep 17 00:00:00 2001
From: Alain Carlucci <carpikemail@gmail.com>
Date: Fri, 30 Dec 2022 18:52:06 +0100
Subject: [PATCH 2/3] HID: sony: sanity check DS4 calibration data

This patch prevents a crash when the DS4 reports invalid calibration
data for the IMUs. It checks if the denominator is zero, and in that
case fallback to a default value.
---
 drivers/hid/hid-sony.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 13125997a..d25e84e35 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1714,6 +1714,7 @@ static int dualshock4_get_calibration_data(struct sony_sc *sc)
 	short acc_z_plus, acc_z_minus;
 	int speed_2x;
 	int range_2g;
+	int i;
 
 	/* For Bluetooth we use a different request, which supports CRC.
 	 * Note: in Bluetooth mode feature report 0x02 also changes the state
@@ -1858,6 +1859,30 @@ static int dualshock4_get_calibration_data(struct sony_sc *sc)
 	sc->ds4_calib_data[5].sens_numer = 2*DS4_ACC_RES_PER_G;
 	sc->ds4_calib_data[5].sens_denom = range_2g;
 
+	/* Sanity check gyro calibration data. This is needed for clone devices,
+	 * which poorly implement support. Some devices appear to return zeros
+	 * for all data, while some hardcode all the same values and for some
+	 * use the wrong sign (the delta is then 0).
+	 * When we detect something fishy, just disable the calibration logic
+	 * altogether as nothing can be trusted for that axis.
+	 */
+	for (i = 0; i < 6; i++) {
+		if (sc->ds4_calib_data[i].sens_denom == 0) {
+			hid_warn(sc->hdev, "Invalid calibration data for axis (%d), disabling calibration.",
+					sc->ds4_calib_data[i].abs_code);
+
+			if (i < 3) {
+				/* Gyroscope */
+				sc->ds4_calib_data[i].sens_numer = DS4_GYRO_RES_PER_DEG_S;
+			} else {
+				/* Accelerometer */
+				sc->ds4_calib_data[i].sens_numer = DS4_ACC_RES_PER_G;
+			}
+			sc->ds4_calib_data[i].bias = 0;
+			sc->ds4_calib_data[i].sens_denom = 1;
+		}
+	}
+
 err_stop:
 	kfree(buf);
 	return ret;
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: sony: Fix division by zero
       [not found]               ` <CAME7zCKPjFbE6nSSoQOVK=BnFG0YAvMgHjAmHKTXcxk3Weuo+w@mail.gmail.com>
@ 2022-12-30 21:53                 ` Roderick Colenbrander
  2023-01-04 20:47                   ` Roderick Colenbrander
  0 siblings, 1 reply; 13+ messages in thread
From: Roderick Colenbrander @ 2022-12-30 21:53 UTC (permalink / raw)
  To: Alain; +Cc: linux-input, Jiri Kosina, djogorchock

Hi Alain,

Thanks for testing. I added a similar patch to my hid-playstation tree
this morning (hid-sony will go away soon).

I'm not entirely happy with the patches yet and need to do some
thinking. The issue is the value range, which is not correct. For the
accelerometer the numerator and denominator need to be 1 to match the
proper range. (It just happens that the scaling factors are the way
they are I think.)

The gyroscope is the issue and I'm not entirely sure what the
numerator needs to be. If I print the coefficients using one of my
dualshocks, the ratio of numerator to denominator is around 60. And
then I'm hunted by some comment from someone in the community..
https://dsremap.readthedocs.io/en/latest/reverse.html#use-calibration

"Note:
There’s a 2 factor in the Linux driver, i.e. max_speed and min_speed
are added instead of averaged. Either there’s something I don’t get,
or the factor is taken care of in the resolution constant, or it’s a
bug.
"

I need to do some thinking on whether the current code is right (even
if it isn't, it can't be changed as it would break software). Then
what the factor needs to be.

Thanks,
Roderick

On Fri, Dec 30, 2022 at 10:12 AM Alain <alain.carlucci@gmail.com> wrote:
>
> Hi Roderick,
> Hi Daniel,
>
> Thank you for all the suggestions, Roderick.
> I fixed the typo in your patch, backported to hid-sony and tested both of them.
> They fix the issue and the DS4 can be used even without calibration data.
> I cannot test if everything works well with a DualSense because I do not own one.
>
> Hi Daniel, I've seen that in the hid-nintendo driver, the calibration denominator is not sanitized if it's zero.
> If I'm not mistaken, a device that disguises itself as a joycon with denominator zero can crash the kernel
> when this is used in the mult_frac(). I had this behaviour with the hid-sony driver.
> You can find attached the patch that should solve the problem.
>
> Thanks,
> Alain
>
>
> Il giorno gio 29 dic 2022 alle ore 21:56 Roderick Colenbrander <thunderbird2k@gmail.com> ha scritto:
>>
>> Hi Alain,
>>
>> On Thu, Dec 29, 2022 at 11:21 AM Alain Carlucci
>> <alain.carlucci@gmail.com> wrote:
>> >
>> > Hi Roderick,
>> >
>> > >Give this patch a try. It is against hid-playstation, which as of 6.2
>> > >supports DS4. Let me know if it works. You can see the sensors working
>> > >through evdev on the motion sensors device.
>> >
>> > Thank you for the patch, just tried. I think there is a typo in the
>> > condition of the for-loop that sanitizes the input.
>> > Instead of `; i < ARRAY_SIZE()` it's written `; ARRAY_SIZE()`,
>> > which always evaluates to true. The loop then overflows the array and
>> > crashes the kernel.
>>
>> Ooh oops. It was a quick patch I wrote without testing.
>>
>> > By the way, the DualSense code has similar calibration code and also
>> > there the input is not sanitized.
>> > I think it's quite easy to create a fake DualSense device with
>> > an Arduino that emulates the protocol up to calib_denom=0, just to
>> > exploit that and crash every linux machine is plugged in. Or even
>> > worst, exploit that via bluetooth.
>>
>> You are right it probably won't hurt to duplicate the logic there.
>>
>> > >If you want to dig deeper, you can play around with
>> > >dualshock4_get_calibration_data in hid-playstation. The particular
>> > >report (though not fully documented in the driver) does contain a lot
>> > >of device info (firmware info, manufacturing dates, various strings).
>> > >You can probably find the details online. Some data there might be
>> > >weird or not populated.
>> >
>> > Thank you! Just discovered that this strange DS4 (CUH-ZCT2E) replies
>> > all zeros both to HID request 0x02 (get calibration data) and as the
>> > BT address (request 0x12, Pairing Info), which explains why BT is not
>> > working.
>>
>> There is definitely something weird with your device. Something must
>> have gotten wiped somehow.
>>
>> > I tried to request version info (0xa3), the response seems the same as
>> > another fully-working and original CUH-ZCT2E:
>> > """
>> > Build Date: Sep 21 2018 04:50:51
>> > HW Version: 0100.b008
>> > SW Version: 00000001.a00a
>> > SW Series:  2010
>> > Code Size:  0002a000
>> > """
>> >
>> > Anyway, I have seen that request 0xA2 puts the DS4 in an alternate
>> > boot mode, probably DFU mode, where the device reboots as 054c:0856
>> > and waits for data, which seems totally undocumented online.
>> > Do you know anything about this mode?
>> > It would be amazing to be able to reflash the original firmware,
>>
>> Unfortunately I can't disclose any of that information. I can say that
>> on DS4 it wasn't common to update firmware (as opposed to DualSense)
>> while out in the wild. Some of these requests and others are probably
>> used to initiate firmware programming and programming of MAC address,
>> calibration data and other settings. It is probably quite involved and
>> hard to get right without bricking your device.
>>
>> > Thanks,
>> > Alain
>>
>> Thanks,
>> Roderick

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: sony: Fix division by zero
  2022-12-30 21:53                 ` Roderick Colenbrander
@ 2023-01-04 20:47                   ` Roderick Colenbrander
  2023-01-05 10:44                     ` Alain Carlucci
  2023-01-05 18:23                     ` Alain Carlucci
  0 siblings, 2 replies; 13+ messages in thread
From: Roderick Colenbrander @ 2023-01-04 20:47 UTC (permalink / raw)
  To: Alain; +Cc: linux-input, Jiri Kosina, djogorchock

Hi Alain,

Just a brief update. I'm still looking at this and I'm not yet sure
what to do. It looks like there is something wrong with the value
ranges of the gyroscope. I need to dig deep in my memory how this code
came around. Some parts of the sensors code were developed and tested
by a team member, who left. For testing he used some internal test 3D
application in which you center some object. There is some validation
there on the values. However, there is just something strange.

The uncalibrated gyroscope values are 16-bit signed, but the hardware
limits are +/- 2000 degree per second. To have enough precision during
calibration the value range is extended (resolution per axis is set to
1024, so 1024 corresponds to 1 degree per second).

When I heavily shake a DS4 in evtest, I can easily reach values like
15 million or more. However, the maximum value we have set on the
evdev node to about 2M. In other words there is something wrong. I
need to look closely at other internal code.

I really hate this as if true, the DS4 unit tests in hid-tools (and
Android) are broken too.

Thanks,
Roderick

On Fri, Dec 30, 2022 at 1:53 PM Roderick Colenbrander
<thunderbird2k@gmail.com> wrote:
>
> Hi Alain,
>
> Thanks for testing. I added a similar patch to my hid-playstation tree
> this morning (hid-sony will go away soon).
>
> I'm not entirely happy with the patches yet and need to do some
> thinking. The issue is the value range, which is not correct. For the
> accelerometer the numerator and denominator need to be 1 to match the
> proper range. (It just happens that the scaling factors are the way
> they are I think.)
>
> The gyroscope is the issue and I'm not entirely sure what the
> numerator needs to be. If I print the coefficients using one of my
> dualshocks, the ratio of numerator to denominator is around 60. And
> then I'm hunted by some comment from someone in the community..
> https://dsremap.readthedocs.io/en/latest/reverse.html#use-calibration
>
> "Note:
> There’s a 2 factor in the Linux driver, i.e. max_speed and min_speed
> are added instead of averaged. Either there’s something I don’t get,
> or the factor is taken care of in the resolution constant, or it’s a
> bug.
> "
>
> I need to do some thinking on whether the current code is right (even
> if it isn't, it can't be changed as it would break software). Then
> what the factor needs to be.
>
> Thanks,
> Roderick
>
> On Fri, Dec 30, 2022 at 10:12 AM Alain <alain.carlucci@gmail.com> wrote:
> >
> > Hi Roderick,
> > Hi Daniel,
> >
> > Thank you for all the suggestions, Roderick.
> > I fixed the typo in your patch, backported to hid-sony and tested both of them.
> > They fix the issue and the DS4 can be used even without calibration data.
> > I cannot test if everything works well with a DualSense because I do not own one.
> >
> > Hi Daniel, I've seen that in the hid-nintendo driver, the calibration denominator is not sanitized if it's zero.
> > If I'm not mistaken, a device that disguises itself as a joycon with denominator zero can crash the kernel
> > when this is used in the mult_frac(). I had this behaviour with the hid-sony driver.
> > You can find attached the patch that should solve the problem.
> >
> > Thanks,
> > Alain
> >
> >
> > Il giorno gio 29 dic 2022 alle ore 21:56 Roderick Colenbrander <thunderbird2k@gmail.com> ha scritto:
> >>
> >> Hi Alain,
> >>
> >> On Thu, Dec 29, 2022 at 11:21 AM Alain Carlucci
> >> <alain.carlucci@gmail.com> wrote:
> >> >
> >> > Hi Roderick,
> >> >
> >> > >Give this patch a try. It is against hid-playstation, which as of 6.2
> >> > >supports DS4. Let me know if it works. You can see the sensors working
> >> > >through evdev on the motion sensors device.
> >> >
> >> > Thank you for the patch, just tried. I think there is a typo in the
> >> > condition of the for-loop that sanitizes the input.
> >> > Instead of `; i < ARRAY_SIZE()` it's written `; ARRAY_SIZE()`,
> >> > which always evaluates to true. The loop then overflows the array and
> >> > crashes the kernel.
> >>
> >> Ooh oops. It was a quick patch I wrote without testing.
> >>
> >> > By the way, the DualSense code has similar calibration code and also
> >> > there the input is not sanitized.
> >> > I think it's quite easy to create a fake DualSense device with
> >> > an Arduino that emulates the protocol up to calib_denom=0, just to
> >> > exploit that and crash every linux machine is plugged in. Or even
> >> > worst, exploit that via bluetooth.
> >>
> >> You are right it probably won't hurt to duplicate the logic there.
> >>
> >> > >If you want to dig deeper, you can play around with
> >> > >dualshock4_get_calibration_data in hid-playstation. The particular
> >> > >report (though not fully documented in the driver) does contain a lot
> >> > >of device info (firmware info, manufacturing dates, various strings).
> >> > >You can probably find the details online. Some data there might be
> >> > >weird or not populated.
> >> >
> >> > Thank you! Just discovered that this strange DS4 (CUH-ZCT2E) replies
> >> > all zeros both to HID request 0x02 (get calibration data) and as the
> >> > BT address (request 0x12, Pairing Info), which explains why BT is not
> >> > working.
> >>
> >> There is definitely something weird with your device. Something must
> >> have gotten wiped somehow.
> >>
> >> > I tried to request version info (0xa3), the response seems the same as
> >> > another fully-working and original CUH-ZCT2E:
> >> > """
> >> > Build Date: Sep 21 2018 04:50:51
> >> > HW Version: 0100.b008
> >> > SW Version: 00000001.a00a
> >> > SW Series:  2010
> >> > Code Size:  0002a000
> >> > """
> >> >
> >> > Anyway, I have seen that request 0xA2 puts the DS4 in an alternate
> >> > boot mode, probably DFU mode, where the device reboots as 054c:0856
> >> > and waits for data, which seems totally undocumented online.
> >> > Do you know anything about this mode?
> >> > It would be amazing to be able to reflash the original firmware,
> >>
> >> Unfortunately I can't disclose any of that information. I can say that
> >> on DS4 it wasn't common to update firmware (as opposed to DualSense)
> >> while out in the wild. Some of these requests and others are probably
> >> used to initiate firmware programming and programming of MAC address,
> >> calibration data and other settings. It is probably quite involved and
> >> hard to get right without bricking your device.
> >>
> >> > Thanks,
> >> > Alain
> >>
> >> Thanks,
> >> Roderick

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: sony: Fix division by zero
  2023-01-04 20:47                   ` Roderick Colenbrander
@ 2023-01-05 10:44                     ` Alain Carlucci
  2023-01-05 18:23                     ` Alain Carlucci
  1 sibling, 0 replies; 13+ messages in thread
From: Alain Carlucci @ 2023-01-05 10:44 UTC (permalink / raw)
  To: Roderick Colenbrander; +Cc: linux-input, Jiri Kosina, djogorchock

Hi Roderick,

sorry for the late response, I've been busy in these days. I'll reply
to both emails in this one.

>Thanks for testing. I added a similar patch to my hid-playstation tree
>this morning (hid-sony will go away soon).
Ok perfect! Does hid-sony will continue to support old Dualshock
controllers or they will be all merged into hid-playstation?

>I'm not entirely happy with the patches yet and need to do some
>thinking. The issue is the value range, which is not correct. For the
>accelerometer the numerator and denominator need to be 1 to match the
>proper range. (It just happens that the scaling factors are the way
>they are I think.)

>The uncalibrated gyroscope values are 16-bit signed, but the hardware
>limits are +/- 2000 degree per second. To have enough precision during
>calibration the value range is extended (resolution per axis is set to
>1024, so 1024 corresponds to 1 degree per second).

Can you tell me a bit more about what is the expected calibrated
output for both gyro and accelerometer? 
Like unit of measurement (I hope deg/s and m/s^2) and data type?
Is "1024 -> 1dg/s" a mapping used only while calibrating or is the
expected conversion during normal usage?

>When I heavily shake a DS4 in evtest, I can easily reach values like
>15 million or more. However, the maximum value we have set on the
>evdev node to about 2M. In other words there is something wrong. I
>need to look closely at other internal code.
Definitely seems there is an issue.
Can you suggest me a tool to test the gyro/accel using linux?

Thanks,
Alain

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: sony: Fix division by zero
  2023-01-04 20:47                   ` Roderick Colenbrander
  2023-01-05 10:44                     ` Alain Carlucci
@ 2023-01-05 18:23                     ` Alain Carlucci
  2023-01-05 23:29                       ` Roderick Colenbrander
  1 sibling, 1 reply; 13+ messages in thread
From: Alain Carlucci @ 2023-01-05 18:23 UTC (permalink / raw)
  To: Roderick Colenbrander; +Cc: linux-input, Jiri Kosina, djogorchock

Hi Roderick,

I just tried to see with evtest the values of gyro/accel after
changing the driver so that follows the suggestion on the dsremap
website: dividing by two speed_2x:
speed_2x = (gyro_speed_plus + gyro_speed_minus) >> 1;

The DS4 shows values no higher than 600000 (post-calibration) while 
heavily shaking the joystick. For the record, the calibration is:

gyro_pitch_plus: 8848   gyro_pitch_minus: -8853
gyro_yaw_plus: 8833     gyro_yaw_minus: -8827
gyro_roll_plus: 8856    gyro_roll_minus: -8841
gyro_speed_plus: 540    gyro_speed_minus: 540
acc_x_plus: 8107        acc_x_minus: -8107
acc_y_plus: 8259        acc_y_minus: -8259
acc_z_plus: 8187        acc_z_minus: -8186

This is an example of the output:

Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 128610
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 95747
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 61321
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 28864
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 874
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -27802
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -54949
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -82064
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -110398
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -138107
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -170345
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -205239
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -242320
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -281525
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -318043
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -356748
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -394453
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -430628
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -465428
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -496105
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -526469
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -551897
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -554865
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -522127
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -450933
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -323041
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -180404
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -44859
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 71006
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 148353
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 202209
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 242757
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 274183
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 298456
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 316106
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 331569
Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 338942

Thanks,
Alain

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: sony: Fix division by zero
  2023-01-05 18:23                     ` Alain Carlucci
@ 2023-01-05 23:29                       ` Roderick Colenbrander
  0 siblings, 0 replies; 13+ messages in thread
From: Roderick Colenbrander @ 2023-01-05 23:29 UTC (permalink / raw)
  To: Alain Carlucci; +Cc: linux-input, Jiri Kosina, djogorchock

Hi Alain,

I'm technically still on vacation, but this issue annoyed me a lot. I
had a look at other internal code and I'm relieved the code is
correct. (Well there is one tiny little issue around calibration,
which I will fix, but in practice only causes a tiny, tiny error).

I don't know why I thought it was wrong. Likely I had worked on a
patched kernel with some logic changed which caused these high values.

In any case I mentioned how the ratio was around 60 on my DS4. It
didn't make sense to me, but it is basically: 2000 / 32768 * 1024
(62.5). The 2000 is the range of the sensor, 32768 max value of 16-bit
signed, 1024 the scaling factor we use in the driver.

So basically:+    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;
+        }
+    }

The same for accelerometer.

I will clean up the patch, add some other patches and send them out shortly.

Thanks,
Roderick

On Thu, Jan 5, 2023 at 10:24 AM Alain Carlucci <alain.carlucci@gmail.com> wrote:
>
> Hi Roderick,
>
> I just tried to see with evtest the values of gyro/accel after
> changing the driver so that follows the suggestion on the dsremap
> website: dividing by two speed_2x:
> speed_2x = (gyro_speed_plus + gyro_speed_minus) >> 1;
>
> The DS4 shows values no higher than 600000 (post-calibration) while
> heavily shaking the joystick. For the record, the calibration is:
>
> gyro_pitch_plus: 8848   gyro_pitch_minus: -8853
> gyro_yaw_plus: 8833     gyro_yaw_minus: -8827
> gyro_roll_plus: 8856    gyro_roll_minus: -8841
> gyro_speed_plus: 540    gyro_speed_minus: 540
> acc_x_plus: 8107        acc_x_minus: -8107
> acc_y_plus: 8259        acc_y_minus: -8259
> acc_z_plus: 8187        acc_z_minus: -8186
>
> This is an example of the output:
>
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 128610
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 95747
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 61321
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 28864
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 874
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -27802
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -54949
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -82064
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -110398
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -138107
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -170345
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -205239
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -242320
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -281525
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -318043
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -356748
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -394453
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -430628
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -465428
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -496105
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -526469
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -551897
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -554865
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -522127
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -450933
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -323041
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -180404
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value -44859
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 71006
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 148353
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 202209
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 242757
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 274183
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 298456
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 316106
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 331569
> Event: time [...], type 3 (EV_ABS), code 3 (ABS_RX), value 338942
>
> Thanks,
> Alain

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-01-05 23:29 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-26  0:07 [PATCH] HID: sony: Fix division by zero Alain Carlucci
2022-12-27 16:17 ` Roderick Colenbrander
2022-12-28 18:01   ` Alain Carlucci
     [not found]     ` <CAEc3jaD-Z4F8CQBHKrBV=H1JwO3LhQMxy1rv2k30rCYhkr1CmQ@mail.gmail.com>
2022-12-28 21:58       ` Alain Carlucci
2022-12-29 16:28         ` Roderick Colenbrander
2022-12-29 19:21           ` Alain Carlucci
2022-12-29 20:56             ` Roderick Colenbrander
2022-12-30 18:17               ` Alain
     [not found]               ` <CAME7zCKPjFbE6nSSoQOVK=BnFG0YAvMgHjAmHKTXcxk3Weuo+w@mail.gmail.com>
2022-12-30 21:53                 ` Roderick Colenbrander
2023-01-04 20:47                   ` Roderick Colenbrander
2023-01-05 10:44                     ` Alain Carlucci
2023-01-05 18:23                     ` Alain Carlucci
2023-01-05 23:29                       ` Roderick Colenbrander

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.