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@xxxxxxxxxxxxxx> 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@xxxxxxxx
