diff mbox series

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

Message ID 20210102223109.996781-14-roderick@gaikai.com
State Superseded
Headers show
Series None | 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




-- 
Roderick Colenbrander
Senior Manager of Hardware & Systems Engineering
Sony Interactive Entertainment LLC
roderick.colenbrander@sony.com
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: