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

[Roderick Colenbrander <roderick@gaikai.com>]

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