Re: [PATCH v2 0/9] i2c: move handling of suspended adapters to the core

[Hans de Goede]

Hi,

On 26-12-18 12:01, Geert Uytterhoeven wrote:
> Hi Wolfram,
>
> On Sat, Dec 22, 2018 at 9:26 PM Wolfram Sang
> <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> > Here is the new version without specific I2C helpers but using the
> > 'is_suspended' flag from the PM core. I didn't like messing with the
> > flag directly, so I did a helper in patch 1. So far, I like the
> > approach. The diffstat looks nice, and i2c-rcar.c and i2c-sh_mobile.c
> > rejected rightfully too later transfers without further modifications.
> > Tested on a Renesas Lager board (R-Car H2).
> >
> > I dropped a few Tested-by tags because I think this approach is too
> > different from V1 to keep them. I hope you guys can have a look again.
> > Thanks for all the testing, so far!
> >
> > A branch can be found here:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/reject-when-suspended
> >
> > If this series is acceptable, I'd suggest to take it via my i2c tree
> > after rc1. And then I'll provide an immutable branch for the PM tree to
> > pick. Let me know if this works for you.
>
> This breaks resume of the CS2000 clock driver on Salvator-X(S) (and
> presumable ULCB, too):
>
>      Accessing already suspended I2C/SMBus adapter
>      WARNING: CPU: 1 PID: 1186 at drivers/i2c/i2c-core-smbus.c:551
> __i2c_smbus_xfer+0x38/0x66c
>      Modules linked in:
>      CPU: 1 PID: 1186 Comm: s2idle Not tainted
> 4.20.0-salvator-x-08442-ge5992c41ac706409 #264
>      Hardware name: Renesas Salvator-X board based on r8a7795 ES1.x (DT)
>      pstate: 60400005 (nZCv daif +PAN -UAO)
>      pc : __i2c_smbus_xfer+0x38/0x66c
>      lr : __i2c_smbus_xfer+0x38/0x66c
>      sp : ffffff8013413880
>      x29: ffffff8013413880 x28: ffffffc6f78b4820
>      x27: 0000000000000010 x26: ffffff8010cf6178
>      x25: ffffff8013413976 x24: 0000000000000002
>      x23: 0000000000000016 x22: ffffffc6f7872088
>      x21: 0000000000000000 x20: 000000000000004f
>      x19: ffffffc6f7872088 x18: 000000000000000a
>      x17: 0000000000000000 x16: 0000000000000000
>      x15: 0000000000029c80 x14: 0720072007200720
>      x13: 0720072007200720 x12: 0720072007200720
>      x11: 0720072007200720 x10: 0720072007200720
>      x9 : ffffff801100e6d0 x8 : 6461207375424d53
>      x7 : ffffff8011c825c8 x6 : ffffff8011c82000
>      x5 : 0000000000000000 x4 : ffffff8013414000
>      x3 : ffffff80134136e0 x2 : 00000046ee875000
>      x1 : 82c6d7c720d64000 x0 : 0000000000000000
>      Call trace:
>       __i2c_smbus_xfer+0x38/0x66c
>       i2c_smbus_xfer+0x64/0x98
>       i2c_smbus_read_byte_data+0x40/0x6c
>       cs2000_bset.isra.1+0x2c/0x58
>       __cs2000_set_rate.constprop.7+0x8c/0x134
>       cs2000_resume+0x14/0x1c
>       dpm_run_callback+0x15c/0x2d8
>       device_resume_early+0x98/0xec
>       dpm_resume_early+0x3b0/0x454
>       suspend_devices_and_enter+0x7bc/0xbb0
>
> The CS2000 driver uses SET_LATE_SYSTEM_SLEEP_PM_OPS().
>
> Suspend/resume of the BD9571 driver works fine, as that one uses
> SET_SYSTEM_SLEEP_PM_OPS().

Ok, so this is the same thing I noticed, the approach using
the pm-core is_suspend flag only works for adapters which
suspend during the regular suspend phase and as such that
approach cannot work everywhere.

Regards,

Hans