Re: [RFC PATCH 0/3] input: gpio-keys - fix pm ordering

[Saravana Kannan]

On Wed, May 3, 2023 at 3:21 PM Doug Berger <opendmb@xxxxxxxxx> wrote:
>
> 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.

Thanks. It definitely made it easier for me to get to the root of the
problem/omission.

> 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.

For older versions, if they have device link support, I'd recommend
creating device links and letting that take care of it.

Also, if you use the right GPIO APIs, at least on newer kernels Linus
W was looking into creating device links automatically from the GPIO
framework level.

So maybe you can backport some variant of that into the older kernels
and leave it to fw_devlink on the newer ones.

-Saravana

> 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
>