[PATCH 0/3] HID: sony: various DS4 improvements

[PATCH 0/3] HID: sony: various DS4 improvements

[Roderick Colenbrander]

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

Hi,

This patch series provides a number of independent DS4 driver
improvements.

The first patch improves DS4 battery level and battery status
reporting. The current implementation interpreted the value
ranges and status levels incorrectly.

The second patch fixes a DS4 dongle kernel crash (bug 206785).
The specific problem is related to Steam, which implements
its own user-space DS4 driver using hidraw. It collides during
DS4 dongle hotplug, causing 'out of order HID feature reports'.
The driver didn't expect this and this led to a kernel crash
later on due to interpreting data incorrectly. The workaround
is checking if the right data was returned and retrying.
Though, I really really dislike this type of fix. Long-term
some solution is needed to perhaps prevent hidraw and evdev
drivers to step on each other's toes. For now this fixes
the current problem.

The last patch fixes sysfs cleanup issues encountered using
the DS4 dongle.

Thanks,
Roderick

Roderick Colenbrander (3):
  HID: sony: Report more accurate DS4 power status.
  HID: sony: Workaround for DS4 dongle hotplug kernel crash.
  HID: sony: Don't use fw_version/hw_version for sysfs cleanup.

 drivers/hid/hid-sony.c | 135 ++++++++++++++++++++++++++---------------
 1 file changed, 86 insertions(+), 49 deletions(-)

-- 
2.26.2

[PATCH 1/3] HID: sony: Report more accurate DS4 power status.

[Roderick Colenbrander]

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

This patch moves the power supply status logic to DS3/DS4 parse_report,
to allow for more accurate DS4 status reporting.

The DS4 power status code was actually incorrect, but reported okay'ish
data in most cases. There was a different interpretation of the battery_info
values 0-10 or 0-9 depending on cable state. It added +1 to extend the range
to 0-10. This is actually incorrect, there is no different range. Values
higher than 11 actually indicates 'full' and 14/15 'error' for which we
reported 100% and a valid power state.

Moving the status logic to parse_report avoids having to pass more state
to the generic sony_battery_get_property and simplifies the code.

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 2f073f536070..81d526a5d003 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -562,9 +562,8 @@ struct sony_sc {
 	u8 hotplug_worker_initialized;
 	u8 state_worker_initialized;
 	u8 defer_initialization;
-	u8 cable_state;
-	u8 battery_charging;
 	u8 battery_capacity;
+	int battery_status;
 	u8 led_state[MAX_LEDS];
 	u8 led_delay_on[MAX_LEDS];
 	u8 led_delay_off[MAX_LEDS];
@@ -892,7 +891,8 @@ static void sixaxis_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	static const u8 sixaxis_battery_capacity[] = { 0, 1, 25, 50, 75, 100 };
 	unsigned long flags;
 	int offset;
-	u8 cable_state, battery_capacity, battery_charging;
+	u8 battery_capacity;
+	int battery_status;
 
 	/*
 	 * The sixaxis is charging if the battery value is 0xee
@@ -904,19 +904,16 @@ static void sixaxis_parse_report(struct sony_sc *sc, u8 *rd, int size)
 
 	if (rd[offset] >= 0xee) {
 		battery_capacity = 100;
-		battery_charging = !(rd[offset] & 0x01);
-		cable_state = 1;
+		battery_status = (rd[offset] & 0x01) ? POWER_SUPPLY_STATUS_FULL : POWER_SUPPLY_STATUS_CHARGING;
 	} else {
 		u8 index = rd[offset] <= 5 ? rd[offset] : 5;
 		battery_capacity = sixaxis_battery_capacity[index];
-		battery_charging = 0;
-		cable_state = 0;
+		battery_status = POWER_SUPPLY_STATUS_DISCHARGING;
 	}
 
 	spin_lock_irqsave(&sc->lock, flags);
-	sc->cable_state = cable_state;
 	sc->battery_capacity = battery_capacity;
-	sc->battery_charging = battery_charging;
+	sc->battery_status = battery_status;
 	spin_unlock_irqrestore(&sc->lock, flags);
 
 	if (sc->quirks & SIXAXIS_CONTROLLER) {
@@ -944,7 +941,8 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	struct input_dev *input_dev = hidinput->input;
 	unsigned long flags;
 	int n, m, offset, num_touch_data, max_touch_data;
-	u8 cable_state, battery_capacity, battery_charging;
+	u8 cable_state, battery_capacity;
+	int battery_status;
 	u16 timestamp;
 
 	/* When using Bluetooth the header is 2 bytes longer, so skip these. */
@@ -1049,29 +1047,52 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	 */
 	offset = data_offset + DS4_INPUT_REPORT_BATTERY_OFFSET;
 	cable_state = (rd[offset] >> 4) & 0x01;
-	battery_capacity = rd[offset] & 0x0F;
 
 	/*
-	 * When a USB power source is connected the battery level ranges from
-	 * 0 to 10, and when running on battery power it ranges from 0 to 9.
-	 * A battery level above 10 when plugged in means charge completed.
+	 * Interpretation of the battery_capacity data depends on the cable state.
+	 * When no cable is connected (bit4 is 0):
+	 * - 0:10: percentage in units of 10%.
+	 * When a cable is plugged in:
+	 * - 0-10: percentage in units of 10%.
+	 * - 11: battery is full
+	 * - 14: not charging due to Voltage or temperature error
+	 * - 15: charge error
 	 */
-	if (!cable_state || battery_capacity > 10)
-		battery_charging = 0;
-	else
-		battery_charging = 1;
+	if (cable_state) {
+		u8 battery_data = rd[offset] & 0xf;
+
+		if (battery_data < 10) {
+			/* Take the mid-point for each battery capacity value,
+			 * because on the hardware side 0 = 0-9%, 1=10-19%, etc.
+			 * This matches official platform behavior, which does
+			 * the same.
+			 */
+			battery_capacity = battery_data * 10 + 5;
+			battery_status = POWER_SUPPLY_STATUS_CHARGING;
+		} else if (battery_data == 10) {
+			battery_capacity = 100;
+			battery_status = POWER_SUPPLY_STATUS_CHARGING;
+		} else if (battery_data == 11) {
+			battery_capacity = 100;
+			battery_status = POWER_SUPPLY_STATUS_FULL;
+		} else { /* 14, 15 and undefined values */
+			battery_capacity = 0;
+			battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
+		}
+	} else {
+		u8 battery_data = rd[offset] & 0xf;
 
-	if (!cable_state)
-		battery_capacity++;
-	if (battery_capacity > 10)
-		battery_capacity = 10;
+		if (battery_data < 10)
+			battery_capacity = battery_data * 10 + 5;
+		else /* 10 */
+			battery_capacity = 100;
 
-	battery_capacity *= 10;
+		battery_status = POWER_SUPPLY_STATUS_DISCHARGING;
+	}
 
 	spin_lock_irqsave(&sc->lock, flags);
-	sc->cable_state = cable_state;
 	sc->battery_capacity = battery_capacity;
-	sc->battery_charging = battery_charging;
+	sc->battery_status = battery_status;
 	spin_unlock_irqrestore(&sc->lock, flags);
 
 	/*
@@ -2300,12 +2321,12 @@ static int sony_battery_get_property(struct power_supply *psy,
 	struct sony_sc *sc = power_supply_get_drvdata(psy);
 	unsigned long flags;
 	int ret = 0;
-	u8 battery_charging, battery_capacity, cable_state;
+	u8 battery_capacity;
+	int battery_status;
 
 	spin_lock_irqsave(&sc->lock, flags);
-	battery_charging = sc->battery_charging;
 	battery_capacity = sc->battery_capacity;
-	cable_state = sc->cable_state;
+	battery_status = sc->battery_status;
 	spin_unlock_irqrestore(&sc->lock, flags);
 
 	switch (psp) {
@@ -2319,13 +2340,7 @@ static int sony_battery_get_property(struct power_supply *psy,
 		val->intval = battery_capacity;
 		break;
 	case POWER_SUPPLY_PROP_STATUS:
-		if (battery_charging)
-			val->intval = POWER_SUPPLY_STATUS_CHARGING;
-		else
-			if (battery_capacity == 100 && cable_state)
-				val->intval = POWER_SUPPLY_STATUS_FULL;
-			else
-				val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+		val->intval = battery_status;
 		break;
 	default:
 		ret = -EINVAL;
-- 
2.26.2

[PATCH 2/3] HID: sony: Workaround for DS4 dongle hotplug kernel crash.

[Roderick Colenbrander]

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

The hid-sony driver has custom DS4 connect/disconnect logic for the
DS4 dongle, which is a USB dongle acting as a proxy to Bluetooth
connected DS4.

The connect/disconnect logic works fine generally, however not in
conjunction with Steam. Steam implements its own DS4 driver using
hidraw. Both hid-sony and Steam are issuing their own HID requests
and are racing each other during DS4 dongle connect/disconnect
resulting in a kernel crash in hid-sony.

The problem is that upon a DS4 connect to the dongle, hid-sony kicks
of 'ds4_get_calibration_data' from within its dongle hotplug code.
The calibration code issues raw HID feature report for reportID 0x02.
When Steam is running, it issues a feature report for reportID 0x12
typically just prior to hid-sony requesting feature reportID 0x02.
The result is that 'ds4_get_calibration_data' receives the data Steam
requested as that's the HID report returing first. Currently this
results in it processing invalid data, which ultimately results in a
divide by zero upon a future 'dualshock4_parse_report'.

The solution for now is to check within 'ds4_get_calibration_data' to
check if we received data for the feature report we issued and if not
retry. This fixes bug 206785.

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 81d526a5d003..83a94ddbfa4e 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1618,16 +1618,38 @@ static int dualshock4_get_calibration_data(struct sony_sc *sc)
 	 * of the controller, so that it sends input reports of type 0x11.
 	 */
 	if (sc->quirks & (DUALSHOCK4_CONTROLLER_USB | DUALSHOCK4_DONGLE)) {
+		int retries;
+
 		buf = kmalloc(DS4_FEATURE_REPORT_0x02_SIZE, GFP_KERNEL);
 		if (!buf)
 			return -ENOMEM;
 
-		ret = hid_hw_raw_request(sc->hdev, 0x02, buf,
-					 DS4_FEATURE_REPORT_0x02_SIZE,
-					 HID_FEATURE_REPORT,
-					 HID_REQ_GET_REPORT);
-		if (ret < 0)
-			goto err_stop;
+		/* We should normally receive the feature report data we asked
+		 * for, but hidraw applications such as Steam can issue feature
+		 * reports as well. In particular for Dongle reconnects, Steam
+		 * and this function are competing resulting in often receiving
+		 * data for a different HID report, so retry a few times.
+		 */
+		for (retries = 0; retries < 3; retries++) {
+			ret = hid_hw_raw_request(sc->hdev, 0x02, buf,
+						 DS4_FEATURE_REPORT_0x02_SIZE,
+						 HID_FEATURE_REPORT,
+						 HID_REQ_GET_REPORT);
+			if (ret < 0)
+				goto err_stop;
+
+			if (buf[0] != 0x02) {
+				if (retries < 2) {
+					hid_warn(sc->hdev, "Retrying DualShock 4 get calibration report (0x02) request\n");
+					continue;
+				} else {
+					ret = -EILSEQ;
+					goto err_stop;
+				}
+			} else {
+				break;
+			}
+		}
 	} else {
 		u8 bthdr = 0xA3;
 		u32 crc;
-- 
2.26.2

[PATCH 3/3] HID: sony: Don't use fw_version/hw_version for sysfs cleanup.

[Roderick Colenbrander]

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

The DS4 dongle reports fw_version and hw_version as 0 when no actual
DS4 is connected to it. This prevents cleaning up sysfs nodes upon
device remove.

This patch decouples sysfs cleanup from the fw_version and hw_version
values by introducing boolean values.

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

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 83a94ddbfa4e..124ed4806c78 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -550,7 +550,9 @@ struct sony_sc {
 	struct power_supply_desc battery_desc;
 	int device_id;
 	unsigned fw_version;
+	bool fw_version_created;
 	unsigned hw_version;
+	bool hw_version_created;
 	u8 *output_report_dmabuf;
 
 #ifdef CONFIG_SONY_FF
@@ -2760,19 +2762,17 @@ static int sony_input_configured(struct hid_device *hdev,
 
 		ret = device_create_file(&sc->hdev->dev, &dev_attr_firmware_version);
 		if (ret) {
-			/* Make zero for cleanup reasons of sysfs entries. */
-			sc->fw_version = 0;
-			sc->hw_version = 0;
 			hid_err(sc->hdev, "can't create sysfs firmware_version attribute err: %d\n", ret);
 			goto err_stop;
 		}
+		sc->fw_version_created = true;
 
 		ret = device_create_file(&sc->hdev->dev, &dev_attr_hardware_version);
 		if (ret) {
-			sc->hw_version = 0;
 			hid_err(sc->hdev, "can't create sysfs hardware_version attribute err: %d\n", ret);
 			goto err_stop;
 		}
+		sc->hw_version_created = true;
 
 		/*
 		 * The Dualshock 4 touchpad supports 2 touches and has a
@@ -2864,9 +2864,9 @@ static int sony_input_configured(struct hid_device *hdev,
 	 */
 	if (sc->ds4_bt_poll_interval)
 		device_remove_file(&sc->hdev->dev, &dev_attr_bt_poll_interval);
-	if (sc->fw_version)
+	if (sc->fw_version_created)
 		device_remove_file(&sc->hdev->dev, &dev_attr_firmware_version);
-	if (sc->hw_version)
+	if (sc->hw_version_created)
 		device_remove_file(&sc->hdev->dev, &dev_attr_hardware_version);
 	sony_cancel_work_sync(sc);
 	sony_remove_dev_list(sc);
@@ -2951,10 +2951,10 @@ static void sony_remove(struct hid_device *hdev)
 	if (sc->quirks & DUALSHOCK4_CONTROLLER_BT)
 		device_remove_file(&sc->hdev->dev, &dev_attr_bt_poll_interval);
 
-	if (sc->fw_version)
+	if (sc->fw_version_created)
 		device_remove_file(&sc->hdev->dev, &dev_attr_firmware_version);
 
-	if (sc->hw_version)
+	if (sc->hw_version_created)
 		device_remove_file(&sc->hdev->dev, &dev_attr_hardware_version);
 
 	sony_cancel_work_sync(sc);
-- 
2.26.2

Re: [PATCH 0/3] HID: sony: various DS4 improvements

[Jiri Kosina]

On Mon, 9 Nov 2020, Roderick Colenbrander wrote:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> Hi,
> 
> This patch series provides a number of independent DS4 driver
> improvements.
> 
> The first patch improves DS4 battery level and battery status
> reporting. The current implementation interpreted the value
> ranges and status levels incorrectly.
> 
> The second patch fixes a DS4 dongle kernel crash (bug 206785).
> The specific problem is related to Steam, which implements
> its own user-space DS4 driver using hidraw. It collides during
> DS4 dongle hotplug, causing 'out of order HID feature reports'.
> The driver didn't expect this and this led to a kernel crash
> later on due to interpreting data incorrectly. The workaround
> is checking if the right data was returned and retrying.
> Though, I really really dislike this type of fix. Long-term
> some solution is needed to perhaps prevent hidraw and evdev
> drivers to step on each other's toes. For now this fixes
> the current problem.
> 
> The last patch fixes sysfs cleanup issues encountered using
> the DS4 dongle.

Hi Roderick,

I've applied the kernel crash fix to for-5.10/upstream-fixes and remaining 
two to for-5.11/sony.

Thanks,

-- 
Jiri Kosina
SUSE Labs