On 5/2/2023 7:18 PM, Saravana Kannan wrote:
On Mon, May 1, 2023 at 1:40 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
On Thu, Apr 27, 2023 at 3:18 PM Doug Berger <opendmb@xxxxxxxxx> wrote:
Commit 52cdbdd49853 ("driver core: correct device's shutdown
order") allowed for proper sequencing of the gpio-keys device
shutdown callbacks by moving the device to the end of the
devices_kset list at probe which was delayed by child
dependencies.
However, commit 722e5f2b1eec ("driver core: Partially revert
"driver core: correct device's shutdown order"") removed this
portion of that commit causing a reversion in the gpio-keys
behavior which can prevent waking from shutdown.
This RFC is an attempt to find a better solution for properly
creating gpio-keys device links to ensure its suspend/resume and
shutdown services are invoked before those of its suppliers.
The first patch here is pretty brute force but simple and has
the advantage that it should be easily backportable to the
versions where the regression first occurred.
We really shouldn't be calling device_pm_move_to_tail() in drivers
because device link uses device_pm_move_to_tail() for ordering too.
And it becomes a "race" between device links and when the driver calls
device_pm_move_to_tail() and I'm not sure we'll get the same ordering
every time.
The second patch is perhaps better in spirit though still a bit
inelegant, but it can only be backported to versions of the
kernel that contain the commit in its 'Fixes:' tag. That isn't
really a valid 'Fixes:' tag since that commit did not cause the
regression, but it does represent how far the patch could be
backported.
Both commits shouldn't really exist in the same kernel so the
third patch reverts the first in an attempt to make that clear
(though it may be a source of confusion for some).
Hopefully someone with a better understanding of device links
will see a less intrusive way to automatically capture these
dependencies for parent device drivers that implement the
functions of child node devices.
Can you give a summary of the issue on a real system? I took a look at
the two commits you've referenced above and it's not clear what's
still broken in the 6.3+
But I'd think that just teaching fw_devlink about some property should
be sufficient. If you are facing a real issue, have you made sure you
have fw_devlink=on (this is the default unless you turned it off in
the commandline when it had issues in the past).
I took a closer look at how gpio-keys work and I can see why
fw_devlink doesn't pick up the GPIO dependencies. It's because the
gpio dependencies are listed under child "key-x" device nodes under
the main "gpio-keys" device tree node. fw_devlink doesn't consider
dependencies under child nodes as mandatory dependencies of the parent
node.
The main reason for this was because of how fw_devlink used to work.
But I might be able to change fw_devlink to pick this up
automatically. I need to think a bit more about this because in some
cases, ignoring those dependencies is the right thing to do. Give me a
few weeks to think through and experiment with this on my end.
Thank you for taking a deeper look at gpio-keys, and for getting through
the gobblety-gook description in my cover-letter ;).
Yes, the dependencies of children are not automatically inherited by
their parents and it is not clear to me whether or not that should
change, but it definitely creates a problem for the sequencing of
gpio-keys device callbacks.
I initially prepared the second patch as a way to explicitly create
device links specifically for the gpio-keys device from these child
dependencies as a work around for the fw_devlinks being dropped. I don't
really consider this a viable patch which is why I made it an RFC, but I
hoped it would highlight the issue.
However, the regression actually occurs in v4.18 and fw_devlink isn't
introduced until v5.7 so it is desirable to think about solutions that
could be backported to older versions. This is why I provided the first
patch for discussion. Again, it is not a desirable solution just an
illustration what could be easily backported to restore the gpio-keys
behavior prior to commit 722e5f2b1eec ("driver core: Partially revert
"driver core: correct device's shutdown order"") without affecting other
devices.
Thanks again for your willingness to take the time to think this through,
Doug
-Saravana
