linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Roderick Colenbrander <roderick@gaikai.com>
To: "Barnabás Pőcze" <pobrn@protonmail.com>
Cc: Jiri Kosina <jikos@kernel.org>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	"linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
	Chris Ye <lzye@google.com>,
	Roderick Colenbrander <roderick.colenbrander@sony.com>
Subject: Re: [PATCH 13/13] HID: playstation: report DualSense hardware and firmware version.
Date: Mon, 28 Dec 2020 14:45:35 -0800	[thread overview]
Message-ID: <CANndSK=v53UMTLrV8YSho2FRJ9_ZAWer4SmiOC0RS_pvf-bwVQ@mail.gmail.com> (raw)
In-Reply-To: <3lVD85P15aERBMBIFGdOJT7p4Y8XB0pZREzvTKNJ-OX-Wt7V_pkfwGzC1h_8xboI0UA18rXrh4Ye23kFVeF1udP8FjdGoi3dMlNiE74aCN8=@protonmail.com>

On Sun, Dec 27, 2020 at 2:38 PM Barnabás Pőcze <pobrn@protonmail.com> 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

  reply	other threads:[~2020-12-28 23:12 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-19  6:23 [PATCH 00/13] HID: new driver for PS5 'DualSense' controller Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 01/13] HID: playstation: initial DualSense USB support Roderick Colenbrander
2020-12-27 16:23   ` Barnabás Pőcze
2020-12-27 23:04     ` Roderick Colenbrander
2020-12-29 19:12       ` Barnabás Pőcze
2020-12-31  0:08   ` Barnabás Pőcze
2020-12-31  1:08     ` Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 02/13] HID: playstation: use DualSense MAC address as unique identifier Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 03/13] HID: playstation: add DualSense battery support Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 04/13] HID: playstation: add DualSense touchpad support Roderick Colenbrander
2020-12-26  2:14   ` Samuel Čavoj
2020-12-26 22:27     ` Roderick Colenbrander
2020-12-29 19:49   ` Barnabás Pőcze
2020-12-29 21:44     ` Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 05/13] HID: playstation: add DualSense accelerometer and gyroscope support Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 06/13] HID: playstation: track devices in list Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 07/13] HID: playstation: add DualSense Bluetooth support Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 08/13] HID: playstation: add DualSense classic rumble support Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 09/13] HID: playstation: add DualSense lightbar support Roderick Colenbrander
2020-12-27 14:41   ` Barnabás Pőcze
2020-12-28 21:26     ` Roderick Colenbrander
2020-12-29 18:59       ` Barnabás Pőcze
2020-12-29 19:54         ` Roderick Colenbrander
2020-12-29 20:22           ` Barnabás Pőcze
2020-12-19  6:23 ` [PATCH 10/13] HID: playstation: add microphone mute support for DualSense Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 11/13] HID: playstation: add DualSense player LEDs support Roderick Colenbrander
2020-12-26 19:27   ` Samuel Čavoj
2020-12-26 23:07     ` Roderick Colenbrander
2020-12-27 14:27   ` Barnabás Pőcze
2020-12-28 22:02     ` Roderick Colenbrander
2020-12-29 18:49       ` Barnabás Pőcze
2020-12-19  6:23 ` [PATCH 12/13] HID: playstation: DualSense set LEDs to default player id Roderick Colenbrander
2020-12-27  0:08   ` Samuel Čavoj
2020-12-27 23:07     ` Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 13/13] HID: playstation: report DualSense hardware and firmware version Roderick Colenbrander
2020-12-27 17:06   ` Barnabás Pőcze
2020-12-27 22:21     ` Roderick Colenbrander
2020-12-27 22:27       ` Roderick Colenbrander
2020-12-27 22:37         ` Barnabás Pőcze
2020-12-28 22:45           ` Roderick Colenbrander [this message]
2020-12-29 15:10             ` Barnabás Pőcze
2020-12-19  8:38 ` [PATCH 00/13] HID: new driver for PS5 'DualSense' controller Bastien Nocera
2020-12-19 22:39   ` Roderick.Colenbrander

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CANndSK=v53UMTLrV8YSho2FRJ9_ZAWer4SmiOC0RS_pvf-bwVQ@mail.gmail.com' \
    --to=roderick@gaikai.com \
    --cc=benjamin.tissoires@redhat.com \
    --cc=jikos@kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=lzye@google.com \
    --cc=pobrn@protonmail.com \
    --cc=roderick.colenbrander@sony.com \
    --subject='Re: [PATCH 13/13] HID: playstation: report DualSense hardware and firmware version.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox