Date:   Tue, 11 Dec 2018 20:24:40 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Wolfram Sang <wsa+renesas@...g-engineering.com>,
        linux-i2c@...r.kernel.org
Cc:     linux-renesas-soc@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        bcm-kernel-feedback-list@...adcom.com,
        linux-kernel@...r.kernel.org, linux-samsung-soc@...r.kernel.org
Subject: Re: [RFC/RFT 00/10] i2c: move handling of suspended adapters to the
 core

Hi Wolfram,

On 10-12-18 22:02, Wolfram Sang wrote:
> Finally, here is the implementation Hans and I agreed on. Plus, all potential
> users I could spot already converted. Renesas R-Car driver was added on top.
> This series was tested on a Renesas Lager board (R-Car H2). I had to hack some
> error cases into the code to verify the workings. I couldn't create an error
> case otherwise, this is why further testing with more complex setups is very
> welcome.
> 
> For my taste, there is still too much boilerplate code for drivers left. Plus,
> it is also too easy to put it too early or too late. But I haven't come up with
> a better idea yet. And it is time to get this somehow forward.
> 
> Please comment, review, test... a branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/core-pm-helpers

So I've been testing this patch-set on some Intel devices with
an i2c-designware controller, combined with a patch to make the
suspend/resume methods of the controller call i2c_mark_adapter_suspended()

The results were ... interesting:

Take 1:
4.20-rc6 + your branch + i2c-designw-patch (+ some unrelated patches which are in my personal tree).
*i2c transfers no longer work*, because the controller runtime suspends and the pm_runtime_get_sync
to undo this is called from the driver's master_xfer function and the
is_suspended check happens before this.

Take 2:
Kernel from 1 with a patch added (attached) to make the core call
pm_runtime_get_sync from __i2c_transfer() if a flag is set +
i2c-designware changes to set this flag
*i2c transfers still do not work*, because __i2c_transfer is
called with the bus-lock held and the pm_runtime_get_sync calls
the adapters resume callback which calls i2c_mark_adapter_suspended()
which tries to take the bus-lock again -> deadlock

Take 3:
Kernel from 2 with the i2c-designware suspend/callback patches
modified to directly set adapter.is_suspended. To avoid the deadlock.
Ok, this works.  Note I've not tested reverting one of my recent
ACPI suspend ordering patches yet, which should actually trigger the
WARN_ON, I've ran out of steam just getting things to work with
the patches present.

I'm attaching 2 patches as I'm using them in Take 3.

Note that the i2c-sprd driver changes in your patchset also get close
to triggering this problem, but they dodge the bullet because there
are separate system-wide and runtime suspend handlers and only the
system-wide handlers call the new i2c_mark_adapter_suspended() function.

As I explain in the commit message of the first attached patch
simply always using split handlers is not really an option because
of adapter drivers using DPM_FLAG_SMART_PREPARE or
DPM_FLAG_SMART_SUSPEND.

So I'm afraid that this is way messier then I would like it to be,
we could go with my flag solution combined with not protecting
the is_suspended flag under the bus lock. That would make the
WARN_ON slightly racy, so we might fail to trigger the warning
under some rare circumstances, reverting to the old behavior of
continuing with the transfer on a suspended adapter, but I don't
think that is all that bad.

Regards,

Hans



> Wolfram Sang (10):
>    i2c: add 'is_suspended' flag for i2c adapters
>    i2c: reject new transfers when adapters are suspended
>    i2c: synquacer: remove unused is_suspended flag
>    i2c: brcmstb: use core helper to mark adapter suspended
>    i2c: zx2967: use core helper to mark adapter suspended
>    i2c: sprd: don't use pdev as variable name for struct device *
>    i2c: sprd: use core helper to mark adapter suspended
>    i2c: exynos5: use core helper to mark adapter suspended
>    i2c: s3c2410: use core helper to mark adapter suspended
>    i2c: rcar: add suspend/resume support
> 
>   drivers/i2c/busses/i2c-brcmstb.c   | 13 ++-----------
>   drivers/i2c/busses/i2c-exynos5.c   | 11 ++---------
>   drivers/i2c/busses/i2c-rcar.c      | 25 +++++++++++++++++++++++++
>   drivers/i2c/busses/i2c-s3c2410.c   |  8 ++------
>   drivers/i2c/busses/i2c-sprd.c      | 34 ++++++++++++----------------------
>   drivers/i2c/busses/i2c-synquacer.c |  5 -----
>   drivers/i2c/busses/i2c-zx2967.c    |  8 ++------
>   drivers/i2c/i2c-core-base.c        |  3 +++
>   include/linux/i2c.h                |  9 +++++++++
>   9 files changed, 57 insertions(+), 59 deletions(-)
> 

View attachment "0001-i2c-add-I2C_AQ_RUNTIME_PM-adapter-quirk-flag.patch" of type "text/x-patch" (3246 bytes)

View attachment "0002-i2c-designware-Set-is_suspended-flag-when-suspended.patch" of type "text/x-patch" (3806 bytes)

Powered by blists - more mailing lists