Re: 5.11-rc device reordering breaks ThinkPad rmi4 suspend

[Rafael J. Wysocki]

On Mon, Jan 11, 2021 at 2:43 PM Thierry Reding <treding@xxxxxxxxxx> wrote:
>
> On Sun, Jan 10, 2021 at 08:44:13PM -0800, Hugh Dickins wrote:
> > Hi Rafael,
> >
> > Synaptics RMI4 SMBus touchpad on ThinkPad X1 Carbon (5th generation)
> > fails to suspend when running 5.11-rc kernels: bisected to
> > 5b6164d3465f ("driver core: Reorder devices on successful probe"),
> > and reverting that fixes it.  dmesg.xz attached, but go ahead and ask
> > me to switch on a debug option to extract further info if that may help.
>
> Hi Hugh,
>
> Quoting what I think are the relevant parts of that log:
>
> [   34.373742] printk: Suspending console(s) (use no_console_suspend to debug)
> [   34.429015] rmi4_physical rmi4-00: Failed to read irqs, code=-6
> [   34.474973] rmi4_f01 rmi4-00.fn01: Failed to write sleep mode: -6.
> [   34.474994] rmi4_f01 rmi4-00.fn01: Suspend failed with code -6.
> [   34.475001] rmi4_physical rmi4-00: Failed to suspend functions: -6
> [   34.475105] rmi4_smbus 6-002c: Failed to suspend device: -6
> [   34.475113] PM: dpm_run_callback(): rmi_smb_suspend+0x0/0x3c returns -6
> [   34.475130] PM: Device 6-002c failed to suspend: error -6
> [   34.475187] PM: Some devices failed to suspend, or early wake event detected
> [   34.480324] rmi4_f03 rmi4-00.fn03: rmi_f03_pt_write: Failed to write to F03 TX register (-6).
> [   34.480748] rmi4_f03 rmi4-00.fn03: rmi_f03_pt_write: Failed to write to F03 TX register (-6).
> [   34.481558] rmi4_physical rmi4-00: rmi_driver_clear_irq_bits: Failed to change enabled interrupts!
> [   34.487935] acpi LNXPOWER:02: Turning OFF
> [   34.488707] acpi LNXPOWER:01: Turning OFF
> [   34.489554] rmi4_physical rmi4-00: rmi_driver_set_irq_bits: Failed to change enabled interrupts!
> [   34.489669] psmouse: probe of serio2 failed with error -1
> [   34.489882] OOM killer enabled.
> [   34.489891] Restarting tasks ... done.
> [   34.589183] PM: suspend exit
> [   34.589839] PM: suspend entry (s2idle)
> [   34.605884] Filesystems sync: 0.017 seconds
> [   34.607594] Freezing user space processes ... (elapsed 0.006 seconds) done.
> [   34.613645] OOM killer disabled.
> [   34.613650] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> [   34.615482] printk: Suspending console(s) (use no_console_suspend to debug)
> [   34.653097] rmi4_f01 rmi4-00.fn01: Failed to write sleep mode: -6.
> [   34.653108] rmi4_f01 rmi4-00.fn01: Suspend failed with code -6.
> [   34.653115] rmi4_physical rmi4-00: Failed to suspend functions: -6
> [   34.653123] rmi4_smbus 6-002c: Failed to suspend device: -6
> [   34.653129] PM: dpm_run_callback(): rmi_smb_suspend+0x0/0x3c returns -6
> [   34.653160] PM: Device 6-002c failed to suspend: error -6
> [   34.653174] PM: Some devices failed to suspend, or early wake event detected
> [   34.660515] OOM killer enabled.
> [   34.660524] Restarting tasks ...
> [   34.661456] rmi4_physical rmi4-00: rmi_driver_set_irq_bits: Failed to change enabled interrupts!
> [   34.661591] psmouse: probe of serio2 failed with error -1
> [   34.669469] done.
> [   34.748386] PM: suspend exit
>
> I think what might be happening here is that the offending patch causes
> some devices to be reordered in a way different to how they were ordered
> originally and the rmi4 driver currently depends on that implicit order.

Actually, the only possible case in which the commit in question can
introduce suspend failures like this is when some dependency
information is missing and so the reordering causes the ordering to
change from the (working) implicit one.

> Interestingly one of the bugs that the offending patch fixes is similar
> in the failure mode but for the reverse reason: the implicit order
> causes suspend/resume to fail.

And that happens because some dependency information is missing.

So we have failing cases when dependency information is missing, so
instead of fixing those we have tried to make the core change the
ordering after every successful probe in the hope that this will take
care of the problem without introducing new breakage.

However, it evidently has introduced new breakage and in order to fix
it we need to figure out what dependency information is missing in the
failing cases and put that information in, but we may as well do the
same for the cases that are failing without the offending change.

So why don't we revert the commit in question and do just that?