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

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 [off-list ref] wrote:

Hi


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

quoted

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 != ...) {
  ...
}

quoted

+             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?

quoted

+
+     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)?

quoted

+                     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