Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 7 Feb 2022 18:50:40 -0800 (PST)
From:   Hugh Dickins <hughd@google.com>
To:     Dmitry Torokhov <dmitry.torokhov@gmail.com>
cc:     Rajat Jain <rajatja@google.com>, Wolfram Sang <wsa@kernel.org>,
        Hugh Dickins <hughd@google.com>,
        Derek Basehore <dbasehore@chromium.org>,
        Jarkko Nikula <jarkko.nikula@linux.intel.com>,
        Thorsten Leemhuis <regressions@leemhuis.info>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-i2c <linux-i2c@vger.kernel.org>,
        "loic.poulain@linaro.org" <loic.poulain@linaro.org>,
        Andrew Duggan <aduggan@synaptics.com>,
        vincent.huang@tw.synaptics.com, cheiny@synaptics.com,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        linux-input <linux-input@vger.kernel.org>
Subject: Re: 5.17-rc regression: rmi4 clients cannot deal with asynchronous
 suspend? (was: X1 Carbon touchpad not resumed)
In-Reply-To: <YgHTYrODoo2ou49J@google.com>
Message-ID: <b76771e5-b8e-54c9-2474-d5a73d236cba@google.com>
References: <89456fcd-a113-4c82-4b10-a9bcaefac68f@google.com> <YgF/0QGFN4SppLKg@shikoro> <CACK8Z6Etj-gq1VKpkUBstiXEETekPWG9p9gKBtuFaZF05pQEvQ@mail.gmail.com> <CACK8Z6FUsceYgBoaAtN8o4m9HpZaBZMt0Nqtvw0a1Z3EuD_nWg@mail.gmail.com> <YgHTYrODoo2ou49J@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 7 Feb 2022, Dmitry Torokhov wrote:
> On Mon, Feb 07, 2022 at 01:41:36PM -0800, Rajat Jain wrote:
> > +linux-input@vger.kernel.org
> > 
> > On Mon, Feb 7, 2022 at 1:09 PM Rajat Jain <rajatja@google.com> wrote:
> > >
> > > +Rafael (for any inputs on asynchronous suspend / resume)
> > > +Dmitry Torokhov (since no other maintainer of rmi4 in MAINTAINERS file)
> > > +loic.poulain@linaro.org (who fixed RMI device hierarchy recently)
> > > + Some Synaptics folks (from recent commits - Vincent Huang, Andrew
> > > Duggan, Cheiny)
> > >
> > > On Mon, Feb 7, 2022 at 12:23 PM Wolfram Sang <wsa@kernel.org> wrote:
> > > >
> > > > Hello Hugh,
> > > >
> > > > > Bisection led to 172d931910e1db800f4e71e8ed92281b6f8c6ee2
> > > > > ("i2c: enable async suspend/resume on i2c client devices")
> > > > > and reverting that fixes it for me.
> > > >
> > > > Thank you for the report plus bisection and sorry for the regression!
> > >
> > > +1, Thanks for the bisection, and apologies for the inconveniences.
> > >
> > > The problem here seems to be that for some reason, some devices (all
> > > connected to rmi4 adapter) failed to resume, but only when
> > > asynchronous suspend is enabled (by 172d931910e1):
> > >
> > > [   79.221064] rmi4_smbus 6-002c: failed to get SMBus version number!
> > > [   79.265074] rmi4_physical rmi4-00: rmi_driver_reset_handler: Failed
> > > to read current IRQ mask.
> > > [   79.308330] rmi4_f01 rmi4-00.fn01: Failed to restore normal operation: -6.
> > > [   79.308335] rmi4_f01 rmi4-00.fn01: Resume failed with code -6.
> > > [   79.308339] rmi4_physical rmi4-00: Failed to suspend functions: -6
> > > [   79.308342] rmi4_smbus 6-002c: Failed to resume device: -6
> > > [   79.351967] rmi4_physical rmi4-00: Failed to read irqs, code=-6
> > >
> > > A resume failure that only shows up during asynchronous resume,
> > > typically means that the device is dependent on some other device to
> > > resume first, but this dependency is NOT established in a parent child
> > > relationship (which is wrong and needs to be fixed, perhaps using
> > > device_add_link()). Thus the kernel may be resuming these devices
> > > without first resuming some other device that these devices need to
> > > depend on.
> > >
> > > TBH, I'm not sure how to fix this. The only hint I see is that all of
> > > these devices seem to be attached to rmi4 device so perhaps something
> > > there? I see 6e4860410b828f recently fixed device hierarchy for rmi4,
> > > and so seemingly should have fixed this very issue (as also seen in
> > > commit message)?
> > >
> > > >
> > > > I will wait a few days if people come up with a fix. If not, I will
> > > > revert the offending commit.
> > >
> > > While I'll be sad because this means no i2c-client can now resume in
> > > parallel and increases resume latency by a *LOT* (hundreds of ms on
> > > all Linux systems), I understand that this needs to be done unless
> > > someone comes up with a fix.
> 
> There is intricate dance happening switching touchpad from legacy PS/2
> into RMI mode, with touchpad being dependent not only on SMbus
> controller, but also on i8042 keyboard controller and its PS/2 port (or
> rather their emulation by the system firmware).
> 
> I wonder if we could apply a little bit more targeted patch:
> 
> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
> index 2407ea43de59..3901d06d38ca 100644
> --- a/drivers/input/rmi4/rmi_smbus.c
> +++ b/drivers/input/rmi4/rmi_smbus.c
> @@ -335,6 +335,7 @@ static int rmi_smb_probe(struct i2c_client *client,
>  		return error;
>  	}
>  
> +	device_disable_async_suspend(&client->dev);
>  	return 0;
>  }
>  
> 
> ... and if that works then we cant try to establish proper dependencies
> via device links later.
> 
> Hugh, could you please try this out and see if it helps?

Yes, that works nicely, thanks Dmitry.

By the way, my memory's been jogged by "rmi4" and the discussion above:
I had a similar-ish problem with it a year ago, discussed with PM guys,

https://lore.kernel.org/linux-pm/alpine.LSU.2.11.2101102010200.25762@eggly.anvils/

I'm not saying you have to read through that thread, but you may find
some relevance in it - Saravana concluded rmi4 driver isn't capturing
parent/child relationship correctly (at that time, anyway).

Hugh