Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230427221625.116050-1-opendmb@gmail.com> <CAGETcx-gDcqY7-_Ud0_rOtgvk7NbzevSa4UCV=NcqiV9zjAv7w@mail.gmail.com>
In-Reply-To: <CAGETcx-gDcqY7-_Ud0_rOtgvk7NbzevSa4UCV=NcqiV9zjAv7w@mail.gmail.com>
From:   Saravana Kannan <saravanak@google.com>
Date:   Tue, 2 May 2023 19:18:55 -0700
Message-ID: <CAGETcx_xjNT1Tp0GeqoddFwGFpv3O33hZZpEiThNg1wwWcfEQw@mail.gmail.com>
Subject: Re: [RFC PATCH 0/3] input: gpio-keys - fix pm ordering
To:     Doug Berger <opendmb@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        Gergo Koteles <soyer@irl.hu>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Hans de Goede <hdegoede@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Kees Cook <keescook@chromium.org>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
        Android Kernel Team <kernel-team@android.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, May 1, 2023 at 1:40=E2=80=AFPM Saravana Kannan <saravanak@google.co=
m> wrote:
>
> On Thu, Apr 27, 2023 at 3:18=E2=80=AFPM Doug Berger <opendmb@gmail.com> w=
rote:
> >
> > 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=3Don (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