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

Commit Message

Roderick Colenbrander Dec. 19, 2020, 6:23 a.m. UTC

From: Roderick Colenbrander <[e-mail]>

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 <[e-mail]>
---
 drivers/hid/hid-playstation.c | 81 +++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

Comments

Barnabás Pőcze Dec. 27, 2020, 5:06 p.m. UTC | #1

Hi


2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:

> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index a55375ac79a9..2f989da906f3 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> [...]
> +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 snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->fw_version);

`sysfs_emit()` is preferred over *printf().


> +}
> +
> +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 snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->hw_version);

Same here.


> +}
> [...]
> +static int dualsense_get_firmware_info(struct dualsense *ds)
> +{
> +	uint8_t *buf;
> +	int ret = 0;

Is there any reason it needs to be initialized?


> +
> +	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;
> +
> +	ds->base.hw_version = get_unaligned_le32(&buf[24]);
> +	ds->base.fw_version = get_unaligned_le32(&buf[28]);

Shouldn't the size of the reply be checked?


> +
> +err_free:
> +	kfree(buf);
> +	return ret;
> +}
> +
>  static int dualsense_get_mac_address(struct dualsense *ds)
>  {
>  	uint8_t *buf;
> @@ -1172,6 +1233,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((struct ps_device *)ds);
>  	if (ret < 0)
>  		return ERR_PTR(ret);
> @@ -1241,6 +1308,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 (struct ps_device *)ds;
>
>  err:
> @@ -1295,6 +1368,12 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		goto err_close;
>  	}
>
> +	ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);

It's a minor thing, but I think `device_{add,remove}_group()` would be better
here in the sense that it expresses the fact that the group is added to a device,
not just any object better.


> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to register sysfs nodes.\n");
> +		goto err_close;
> +	}
> +
>  	return ret;
>
>  err_close:
> @@ -1313,6 +1392,8 @@ static void ps_remove(struct hid_device *hdev)
>
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);
> +
> +	sysfs_remove_group(&hdev->dev.kobj, &ps_device_attribute_group);
>  }
>
>  static const struct hid_device_id ps_devices[] = {
> --
> 2.26.2


Regards,
Barnabás Pőcze

Roderick Colenbrander Dec. 27, 2020, 10:21 p.m. UTC | #2

Hi Barnabás,

Thanks for you great feedback.

On Sun, Dec 27, 2020 at 9:06 AM Barnabás Pőcze <[e-mail]> wrote:
>
> Hi
>
>
> 2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:
>
> > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > index a55375ac79a9..2f989da906f3 100644
> > --- a/drivers/hid/hid-playstation.c
> > +++ b/drivers/hid/hid-playstation.c
> > [...]
> > +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 snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->fw_version);
>
> `sysfs_emit()` is preferred over *printf().

Thanks, I wasn't aware of its existence. Looks like it was added
recently, but yeah it is a lot nicer.

>
> > +}
> > +
> > +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 snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->hw_version);
>
> Same here.
>
>
> > +}
> > [...]
> > +static int dualsense_get_firmware_info(struct dualsense *ds)
> > +{
> > +     uint8_t *buf;
> > +     int ret = 0;
>
> Is there any reason it needs to be initialized?
>
>
> > +
> > +     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;
> > +
> > +     ds->base.hw_version = get_unaligned_le32(&buf[24]);
> > +     ds->base.fw_version = get_unaligned_le32(&buf[28]);
>
> Shouldn't the size of the reply be checked?

Good point, I added a check and returning -EINVAL now in case of a
size mismatch.

>
> > +
> > +err_free:
> > +     kfree(buf);
> > +     return ret;
> > +}
> > +
> >  static int dualsense_get_mac_address(struct dualsense *ds)
> >  {
> >       uint8_t *buf;
> > @@ -1172,6 +1233,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((struct ps_device *)ds);
> >       if (ret < 0)
> >               return ERR_PTR(ret);
> > @@ -1241,6 +1308,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 (struct ps_device *)ds;
> >
> >  err:
> > @@ -1295,6 +1368,12 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >               goto err_close;
> >       }
> >
> > +     ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);
>
> It's a minor thing, but I think `device_{add,remove}_group()` would be better
> here in the sense that it expresses the fact that the group is added to a device,
> not just any object better.

Agreed, that's nicer I wasn't aware of it. I try to follow what other
hid drivers do and they all used the kobj directly, which honestly
felt nasty. Will change it to this.

>
> > +     if (ret < 0) {
> > +             hid_err(hdev, "Failed to register sysfs nodes.\n");
> > +             goto err_close;
> > +     }
> > +
> >       return ret;
> >
> >  err_close:
> > @@ -1313,6 +1392,8 @@ static void ps_remove(struct hid_device *hdev)
> >
> >       hid_hw_close(hdev);
> >       hid_hw_stop(hdev);
> > +
> > +     sysfs_remove_group(&hdev->dev.kobj, &ps_device_attribute_group);
> >  }
> >
> >  static const struct hid_device_id ps_devices[] = {
> > --
> > 2.26.2
>
>
> Regards,
> Barnabás Pőcze

Thanks,
Roderick

Roderick Colenbrander Dec. 27, 2020, 10:27 p.m. UTC | #3

On Sun, Dec 27, 2020 at 2:21 PM Roderick Colenbrander
<[e-mail]> wrote:
>
> Hi Barnabás,
>
> Thanks for you great feedback.
>
> On Sun, Dec 27, 2020 at 9:06 AM Barnabás Pőcze <[e-mail]> wrote:
> >
> > Hi
> >
> >
> > 2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:
> >
> > > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > > index a55375ac79a9..2f989da906f3 100644
> > > --- a/drivers/hid/hid-playstation.c
> > > +++ b/drivers/hid/hid-playstation.c
> > > [...]
> > > +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 snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->fw_version);
> >
> > `sysfs_emit()` is preferred over *printf().
>
> Thanks, I wasn't aware of its existence. Looks like it was added
> recently, but yeah it is a lot nicer.
>
> >
> > > +}
> > > +
> > > +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 snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->hw_version);
> >
> > Same here.
> >
> >
> > > +}
> > > [...]
> > > +static int dualsense_get_firmware_info(struct dualsense *ds)
> > > +{
> > > +     uint8_t *buf;
> > > +     int ret = 0;
> >
> > Is there any reason it needs to be initialized?
> >
> >
> > > +
> > > +     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;
> > > +
> > > +     ds->base.hw_version = get_unaligned_le32(&buf[24]);
> > > +     ds->base.fw_version = get_unaligned_le32(&buf[28]);
> >
> > Shouldn't the size of the reply be checked?
>
> Good point, I added a check and returning -EINVAL now in case of a
> size mismatch.
>
> >
> > > +
> > > +err_free:
> > > +     kfree(buf);
> > > +     return ret;
> > > +}
> > > +
> > >  static int dualsense_get_mac_address(struct dualsense *ds)
> > >  {
> > >       uint8_t *buf;
> > > @@ -1172,6 +1233,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((struct ps_device *)ds);
> > >       if (ret < 0)
> > >               return ERR_PTR(ret);
> > > @@ -1241,6 +1308,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 (struct ps_device *)ds;
> > >
> > >  err:
> > > @@ -1295,6 +1368,12 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > >               goto err_close;
> > >       }
> > >
> > > +     ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);
> >
> > It's a minor thing, but I think `device_{add,remove}_group()` would be better
> > here in the sense that it expresses the fact that the group is added to a device,
> > not just any object better.
>
> Agreed, that's nicer I wasn't aware of it. I try to follow what other
> hid drivers do and they all used the kobj directly, which honestly
> felt nasty. Will change it to this.
>

Actually devm_device_add_group seems to be even nicer. Surprisingly it
isn't widely used yet.

Roderick

Barnabás Pőcze Dec. 27, 2020, 10:37 p.m. UTC | #4

2020. december 27., vasárnap 23:27 keltezéssel, Roderick Colenbrander írta:

> [...]
> > > > -       ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);
> > > >
> > > >
> > >
> > > It's a minor thing, but I think `device_{add,remove}_group()` would be better
> > > here in the sense that it expresses the fact that the group is added to a device,
> > > not just any object better.
> >
> > Agreed, that's nicer I wasn't aware of it. I try to follow what other
> > hid drivers do and they all used the kobj directly, which honestly
> > felt nasty. Will change it to this.
>
> Actually devm_device_add_group seems to be even nicer. Surprisingly it
> isn't widely used yet.
>
> Roderick


Well, indeed, although I believe that shouldn't be used here. Consider
what happens if the hid-playstation module is unloaded. The attributes
of the HID device will not be unregistered, but the backing functions/etc.
are unloaded, so reading/writing them will have undesirable effects - I imagine.
So in either case, you'll need to use `[devm_]device_remove_group()`, and for
that reason I think using the devm_* variant is less efficient.
Please note, that I am not 100% sure this hypothesis is correct, but I'm pretty sure.


Regards,
Barnabás Pőcze

Roderick Colenbrander Dec. 28, 2020, 10:45 p.m. UTC | #5

On Sun, Dec 27, 2020 at 2:38 PM Barnabás Pőcze <[e-mail]> wrote:
>
> 2020. december 27., vasárnap 23:27 keltezéssel, Roderick Colenbrander írta:
>
> > [...]
> > > > > -       ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);
> > > > >
> > > > >
> > > >
> > > > It's a minor thing, but I think `device_{add,remove}_group()` would be better
> > > > here in the sense that it expresses the fact that the group is added to a device,
> > > > not just any object better.
> > >
> > > Agreed, that's nicer I wasn't aware of it. I try to follow what other
> > > hid drivers do and they all used the kobj directly, which honestly
> > > felt nasty. Will change it to this.
> >
> > Actually devm_device_add_group seems to be even nicer. Surprisingly it
> > isn't widely used yet.
> >
> > Roderick
>
>
> Well, indeed, although I believe that shouldn't be used here. Consider
> what happens if the hid-playstation module is unloaded. The attributes
> of the HID device will not be unregistered, but the backing functions/etc.
> are unloaded, so reading/writing them will have undesirable effects - I imagine.
> So in either case, you'll need to use `[devm_]device_remove_group()`, and for
> that reason I think using the devm_* variant is less efficient.
> Please note, that I am not 100% sure this hypothesis is correct, but I'm pretty sure.
>
>
> Regards,
> Barnabás Pőcze

I did some more digging into 'devm_device_add_group' as I was curious.
It is widely used for touchscreen drivers apparently and some other
devices and generally used from 'probe' as you would expect. None of
the drivers I found call devm_device_remove_group. Though, none of the
drivers use HID.

I tried using the call and it seems to work fine even after driver
unloads/reloads without a 'devm_device_remove_group' call. I don't
believe any sysfs entries are kept around (also based on watching the
contents of the sysfs directory for the device). If they were I'm sure
the kernel would have thrown some errors during a future
'devm_device_add_group' call as you know sysfs gets quite unhappy if
you added a duplicate node.

This makes me believe it is getting cleaned up, but I'm not sure how.
I suspect it happens when the HID driver is unregistered
(hid_unregister_driver) from the bus, which follows a bus rescan. When
the driver is removed, device_driver_detach is called, which triggers
a lot of cleanup logic in 'device_driver_release_internal'. I haven't
traced this call, but I think its call 'devres_release_all(dev)' is
what is doing the magic.

Any thoughts?

Regards,
Roderick

Barnabás Pőcze Dec. 29, 2020, 3:10 p.m. UTC | #6

2020. december 28., hétfő 23:45 keltezéssel, Roderick Colenbrander írta:

> On Sun, Dec 27, 2020 at 2:38 PM Barnabás Pőcze [e-mail] wrote:
>
> > 2020.  december 27., vasárnap 23:27 keltezéssel, Roderick Colenbrander írta:
> >
> > > [...]
> > >
> > > > > > -         ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);
> > > > > >
> > > > > >
> > > > >
> > > > > It's a minor thing, but I think `device_{add,remove}_group()` would be better
> > > > > here in the sense that it expresses the fact that the group is added to a device,
> > > > > not just any object better.
> > > >
> > > > Agreed, that's nicer I wasn't aware of it. I try to follow what other
> > > > hid drivers do and they all used the kobj directly, which honestly
> > > > felt nasty. Will change it to this.
> > >
> > > Actually devm_device_add_group seems to be even nicer. Surprisingly it
> > > isn't widely used yet.
> > > Roderick
> >
> > Well, indeed, although I believe that shouldn't be used here. Consider
> > what happens if the hid-playstation module is unloaded. The attributes
> > of the HID device will not be unregistered, but the backing functions/etc.
> > are unloaded, so reading/writing them will have undesirable effects - I imagine.
> > So in either case, you'll need to use `[devm_]device_remove_group()`, and for
> > that reason I think using the devm_* variant is less efficient.
> > Please note, that I am not 100% sure this hypothesis is correct, but I'm pretty sure.
> > Regards,
> > Barnabás Pőcze
>
> I did some more digging into 'devm_device_add_group' as I was curious.
> It is widely used for touchscreen drivers apparently and some other
> devices and generally used from 'probe' as you would expect. None of
> the drivers I found call devm_device_remove_group. Though, none of the
> drivers use HID.
>
> I tried using the call and it seems to work fine even after driver
> unloads/reloads without a 'devm_device_remove_group' call. I don't
> believe any sysfs entries are kept around (also based on watching the
> contents of the sysfs directory for the device). If they were I'm sure
> the kernel would have thrown some errors during a future
> 'devm_device_add_group' call as you know sysfs gets quite unhappy if
> you added a duplicate node.
>
> This makes me believe it is getting cleaned up, but I'm not sure how.
> I suspect it happens when the HID driver is unregistered
> (hid_unregister_driver) from the bus, which follows a bus rescan. When
> the driver is removed, device_driver_detach is called, which triggers
> a lot of cleanup logic in 'device_driver_release_internal'. I haven't
> traced this call, but I think its call 'devres_release_all(dev)' is
> what is doing the magic.
>
> Any thoughts?


I also did some tests and it seems you're right, I was under the impression
that "device managed" resources are tied to the lifetime of the device,
and are not released when the driver unbinds, but this assumptions seems
to be false and device managed resources are, in fact, released when a driver
detaches so sorry for making you take this detour into investigating it.


Regards,
Barnabás Pőcze

Patch

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index a55375ac79a9..2f989da906f3 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -36,6 +36,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);
 };
@@ -63,6 +65,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)
@@ -585,6 +589,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 snprintf(buf, PAGE_SIZE, "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 snprintf(buf, PAGE_SIZE, "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;
@@ -683,6 +721,29 @@  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 = 0;
+
+	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;
+
+	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;
@@ -1172,6 +1233,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((struct ps_device *)ds);
 	if (ret < 0)
 		return ERR_PTR(ret);
@@ -1241,6 +1308,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 (struct ps_device *)ds;
 
 err:
@@ -1295,6 +1368,12 @@  static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto err_close;
 	}
 
+	ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to register sysfs nodes.\n");
+		goto err_close;
+	}
+
 	return ret;
 
 err_close:
@@ -1313,6 +1392,8 @@  static void ps_remove(struct hid_device *hdev)
 
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
+
+	sysfs_remove_group(&hdev->dev.kobj, &ps_device_attribute_group);
 }
 
 static const struct hid_device_id ps_devices[] = {