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

[Saravana Kannan]

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.

-Saravana