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

[Doug Berger]

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