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

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 pobrn@protonmail.com wrote:

quoted

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

quoted

[...]

quoted

quoted

quoted

-         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