Messages in this thread | | | Date | Wed, 3 May 2023 15:20:27 -0700 | Subject | Re: [RFC PATCH 0/3] input: gpio-keys - fix pm ordering | From | Doug Berger <> |
| |
On 5/2/2023 7:18 PM, Saravana Kannan wrote: > On Mon, May 1, 2023 at 1:40 PM Saravana Kannan <saravanak@google.com> wrote: >> >> On Thu, Apr 27, 2023 at 3:18 PM Doug Berger <opendmb@gmail.com> 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
| |