diff mbox series

[v2,13/13] HID: playstation: report DualSense hardware and firmware version.

Message ID 20210102223109.996781-14-roderick@gaikai.com (mailing list archive)
State Superseded
Delegated to: Jiri Kosina
Headers show
Series HID: new driver for PS5 'DualSense' controller | expand

Commit Message

Roderick Colenbrander Jan. 2, 2021, 10:31 p.m. UTC 
From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Retrieve DualSense hardware and firmware information using a vendor
specific feature report. Report the data through sysfs and also
report using hid_info as there can be signficant differences between
versions.

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

Comments

Barnabás Pőcze Jan. 7, 2021, 10:26 p.m. UTC | #1 
Hi


2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:

> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index 1a95c81da8a3..8440af6d6cd7 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> [...]
> +static int dualsense_get_firmware_info(struct dualsense *ds)
> +{
> +	uint8_t *buf;
> +	int ret;
> +
> +	buf = kzalloc(DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_FIRMWARE_INFO, buf,
> +			DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, HID_FEATURE_REPORT,
> +			HID_REQ_GET_REPORT);
> +	if (ret < 0)
> +		goto err_free;
> +	else if (ret != DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE) {

As per coding style[1], please either use {} for all branches, or just drop the
`else` and maybe add a new line:

```
if (ret < 0)
  goto ...

if (ret != ...) {
  ...
}
```

> +		hid_err(ds->base.hdev, "failed to retrieve DualSense firmware info\n");
> +		ret = -EINVAL;
> +		goto err_free;
> +	}

Shouldn't the CRC be validated here when using Bluetooth? Or there is none?


> +
> +	ds->base.hw_version = get_unaligned_le32(&buf[24]);
> +	ds->base.fw_version = get_unaligned_le32(&buf[28]);
> +
> +err_free:
> +	kfree(buf);
> +	return ret;
> +}
> +
>  static int dualsense_get_mac_address(struct dualsense *ds)
>  {
>  	uint8_t *buf;
> @@ -1195,6 +1261,12 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  	}
>  	snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
>
> +	ret = dualsense_get_firmware_info(ds);
> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to get firmware info from DualSense\n");
> +		return ERR_PTR(ret);
> +	}
> +
>  	ret = ps_devices_list_add(ps_dev);
>  	if (ret < 0)
>  		return ERR_PTR(ret);
> @@ -1261,6 +1333,12 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  	/* Set player LEDs to our player id. */
>  	dualsense_set_player_leds(ds);
>
> +	/* Reporting hardware and firmware is important as there are frequent updates, which
> +	 * can change behavior.
> +	 */
> +	hid_info(hdev, "Registered DualSense controller hw_version=%x fw_version=%x\n",

Maybe the format could be same as in the device attributes (0x%08x)?


> +			ds->base.hw_version, ds->base.fw_version);
> +
>  	return &ds->base;
>
>  err:
> @@ -1311,6 +1389,12 @@  static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		}
>  	}
>
> +	ret = devm_device_add_group(&hdev->dev, &ps_device_attribute_group);
> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to register sysfs nodes.\n");
> +		goto err_close;
> +	}
> +
>  	return ret;
>
>  err_close:
>


[1]: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces


Regards,
Barnabás Pőcze
Roderick Colenbrander Jan. 9, 2021, 1:35 a.m. UTC | #2 
Hi Barnabás,

A couple of places lacked MAC address checks (some of these reports I
didn't have datasheets on). In the end I decided to make a new helper
function as there is so much common nasty code. It also exposed a few
tiny bugs as some reports were an incorrect size (not critical as the
data wasn't used). It is a lot simpler now with more and better
checking.

static int ps_get_report(struct hid_device *hdev, uint8_t report_id,
uint8_t *buf, size_t size)
{
    int ret;

    ret = hid_hw_raw_request(hdev, report_id, buf, size,
HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
    if (ret < 0) {
        hid_err(hdev, "Failed to retrieve feature report %d,
ret=%d\n", report_id, ret);
        return ret;
    }

    if (ret != size) {
        hid_err(hdev, "Invalid byte count transferred, expected %zu
got %d\n", size, ret);
        return -EINVAL;
    }

    if (buf[0] != report_id) {
        hid_err(hdev, "Incorrect reportID received, expected %d got
%d\n", report_id, buf[0]);
        return -EINVAL;
    }

    if (hdev->bus == BUS_BLUETOOTH) {
        /* Last 4 bytes contains crc32. */
        uint8_t crc_offset = size - 4;
        uint32_t report_crc = get_unaligned_le32(&buf[crc_offset]);

        if (!ps_check_crc32(PS_FEATURE_CRC32_SEED, buf, crc_offset,
report_crc)) {
            hid_err(hdev, "CRC check failed for reportID=%d\n", report_id);
            return -EILSEQ;
        }
    }

    return 0;
}

On Thu, Jan 7, 2021 at 2:26 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
>
> > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > index 1a95c81da8a3..8440af6d6cd7 100644
> > --- a/drivers/hid/hid-playstation.c
> > +++ b/drivers/hid/hid-playstation.c
> > [...]
> > +static int dualsense_get_firmware_info(struct dualsense *ds)
> > +{
> > +     uint8_t *buf;
> > +     int ret;
> > +
> > +     buf = kzalloc(DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, GFP_KERNEL);
> > +     if (!buf)
> > +             return -ENOMEM;
> > +
> > +     ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_FIRMWARE_INFO, buf,
> > +                     DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, HID_FEATURE_REPORT,
> > +                     HID_REQ_GET_REPORT);
> > +     if (ret < 0)
> > +             goto err_free;
> > +     else if (ret != DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE) {
>
> As per coding style[1], please either use {} for all branches, or just drop the
> `else` and maybe add a new line:
>
> ```
> if (ret < 0)
>   goto ...
>
> if (ret != ...) {
>   ...
> }
> ```
>
> > +             hid_err(ds->base.hdev, "failed to retrieve DualSense firmware info\n");
> > +             ret = -EINVAL;
> > +             goto err_free;
> > +     }
>
> Shouldn't the CRC be validated here when using Bluetooth? Or there is none?
>
>
> > +
> > +     ds->base.hw_version = get_unaligned_le32(&buf[24]);
> > +     ds->base.fw_version = get_unaligned_le32(&buf[28]);
> > +
> > +err_free:
> > +     kfree(buf);
> > +     return ret;
> > +}
> > +
> >  static int dualsense_get_mac_address(struct dualsense *ds)
> >  {
> >       uint8_t *buf;
> > @@ -1195,6 +1261,12 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
> >       }
> >       snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
> >
> > +     ret = dualsense_get_firmware_info(ds);
> > +     if (ret < 0) {
> > +             hid_err(hdev, "Failed to get firmware info from DualSense\n");
> > +             return ERR_PTR(ret);
> > +     }
> > +
> >       ret = ps_devices_list_add(ps_dev);
> >       if (ret < 0)
> >               return ERR_PTR(ret);
> > @@ -1261,6 +1333,12 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
> >       /* Set player LEDs to our player id. */
> >       dualsense_set_player_leds(ds);
> >
> > +     /* Reporting hardware and firmware is important as there are frequent updates, which
> > +      * can change behavior.
> > +      */
> > +     hid_info(hdev, "Registered DualSense controller hw_version=%x fw_version=%x\n",
>
> Maybe the format could be same as in the device attributes (0x%08x)?
>
>
> > +                     ds->base.hw_version, ds->base.fw_version);
> > +
> >       return &ds->base;
> >
> >  err:
> > @@ -1311,6 +1389,12 @@  static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >               }
> >       }
> >
> > +     ret = devm_device_add_group(&hdev->dev, &ps_device_attribute_group);
> > +     if (ret < 0) {
> > +             hid_err(hdev, "Failed to register sysfs nodes.\n");
> > +             goto err_close;
> > +     }
> > +
> >       return ret;
> >
> >  err_close:
> >
>
>
> [1]: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces
>
>
> Regards,
> Barnabás Pőcze
diff mbox series

Patch

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 1a95c81da8a3..8440af6d6cd7 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -41,6 +41,8 @@  struct ps_device {
 	int battery_status;
 
 	uint8_t mac_address[6];
+	uint32_t hw_version;
+	uint32_t fw_version;
 
 	int (*parse_report)(struct ps_device *dev, struct hid_report *report, u8 *data, int size);
 };
@@ -68,6 +70,8 @@  struct ps_led_info {
 #define DS_FEATURE_REPORT_CALIBRATION_SIZE	41
 #define DS_FEATURE_REPORT_PAIRING_INFO		9
 #define DS_FEATURE_REPORT_PAIRING_INFO_SIZE	19
+#define DS_FEATURE_REPORT_FIRMWARE_INFO		32
+#define DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE	64
 
 /* Button masks for DualSense input report. */
 #define DS_BUTTONS0_HAT_SWITCH	GENMASK(3, 0)
@@ -593,6 +597,40 @@  static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width,
 	return touchpad;
 }
 
+static ssize_t ps_show_firmware_version(struct device *dev,
+				struct device_attribute
+				*attr, char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct ps_device *ps_dev = hid_get_drvdata(hdev);
+
+	return sysfs_emit(buf, "0x%08x\n", ps_dev->fw_version);
+}
+
+static DEVICE_ATTR(firmware_version, 0444, ps_show_firmware_version, NULL);
+
+static ssize_t ps_show_hardware_version(struct device *dev,
+				struct device_attribute
+				*attr, char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct ps_device *ps_dev = hid_get_drvdata(hdev);
+
+	return sysfs_emit(buf, "0x%08x\n", ps_dev->hw_version);
+}
+
+static DEVICE_ATTR(hardware_version, 0444, ps_show_hardware_version, NULL);
+
+static struct attribute *ps_device_attributes[] = {
+	&dev_attr_firmware_version.attr,
+	&dev_attr_hardware_version.attr,
+	NULL
+};
+
+static const struct attribute_group ps_device_attribute_group = {
+	.attrs = ps_device_attributes,
+};
+
 static int dualsense_get_calibration_data(struct dualsense *ds)
 {
 	short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
@@ -696,6 +734,34 @@  static int dualsense_get_calibration_data(struct dualsense *ds)
 	return ret;
 }
 
+static int dualsense_get_firmware_info(struct dualsense *ds)
+{
+	uint8_t *buf;
+	int ret;
+
+	buf = kzalloc(DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_FIRMWARE_INFO, buf,
+			DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, HID_FEATURE_REPORT,
+			HID_REQ_GET_REPORT);
+	if (ret < 0)
+		goto err_free;
+	else if (ret != DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE) {
+		hid_err(ds->base.hdev, "failed to retrieve DualSense firmware info\n");
+		ret = -EINVAL;
+		goto err_free;
+	}
+
+	ds->base.hw_version = get_unaligned_le32(&buf[24]);
+	ds->base.fw_version = get_unaligned_le32(&buf[28]);
+
+err_free:
+	kfree(buf);
+	return ret;
+}
+
 static int dualsense_get_mac_address(struct dualsense *ds)
 {
 	uint8_t *buf;
@@ -1195,6 +1261,12 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
 	}
 	snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
 
+	ret = dualsense_get_firmware_info(ds);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to get firmware info from DualSense\n");
+		return ERR_PTR(ret);
+	}
+
 	ret = ps_devices_list_add(ps_dev);
 	if (ret < 0)
 		return ERR_PTR(ret);
@@ -1261,6 +1333,12 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
 	/* Set player LEDs to our player id. */
 	dualsense_set_player_leds(ds);
 
+	/* Reporting hardware and firmware is important as there are frequent updates, which
+	 * can change behavior.
+	 */
+	hid_info(hdev, "Registered DualSense controller hw_version=%x fw_version=%x\n",
+			ds->base.hw_version, ds->base.fw_version);
+
 	return &ds->base;
 
 err:
@@ -1311,6 +1389,12 @@  static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		}
 	}
 
+	ret = devm_device_add_group(&hdev->dev, &ps_device_attribute_group);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to register sysfs nodes.\n");
+		goto err_close;
+	}
+
 	return ret;
 
 err_close: