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;
Message-ID: <597ba853-1fe4-9263-c448-422b0c2a91d8@gmail.com>
Date:   Wed, 3 May 2023 15:20:27 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.1.2
Subject: Re: [RFC PATCH 0/3] input: gpio-keys - fix pm ordering
To:     Saravana Kannan <saravanak@google.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>,
        Florian Fainelli <f.fainelli@gmail.com>,
        linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
        Android Kernel Team <kernel-team@android.com>
References: <20230427221625.116050-1-opendmb@gmail.com>
 <CAGETcx-gDcqY7-_Ud0_rOtgvk7NbzevSa4UCV=NcqiV9zjAv7w@mail.gmail.com>
 <CAGETcx_xjNT1Tp0GeqoddFwGFpv3O33hZZpEiThNg1wwWcfEQw@mail.gmail.com>
Content-Language: en-US
From:   Doug Berger <opendmb@gmail.com>
In-Reply-To: <CAGETcx_xjNT1Tp0GeqoddFwGFpv3O33hZZpEiThNg1wwWcfEQw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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