Re: [PATCH] i2c: let I2C masters ignore their children for PM

Ulf Hansson
2016-04-11T12:53:32Z

On 11 April 2016 at 13:38, Linus Walleij <linus.walleij@linaro.org> wrote:
> When using a certain I2C device with runtime PM enabled on
> a certain I2C bus adaper the following happens:
>
> struct amba_device *foo
> \
> struct i2c_adapter *bar
> \
> struct i2c_client *baz
>
> The AMBA device foo has its device PM struct set to ignore
> children with pm_suspend_ignore_children(&foo->dev, true).
> This makes runtime PM work just fine locally in the driver:
> the fact that devices on the bus are suspended or resumed
> individually does not affect its operation, and the hardware
> does not power up unless transferring messages.
>
> However this child ignorance property is not inherited into
> the struct i2c_adapter *bar.
>
> On system suspend things will work fine.
>
> On system resume the following annoying phenomenon occurs:
>
> - In the pm_runtime_force_resume() path of
> struct i2c_client *baz, pm_runtime_set_active(&baz->dev); is
> eventually called.
>
> - This becomes __pm_runtime_set_status(&baz->dev, RPM_ACTIVE);
>
> - __pm_runtime_set_status() detects that RPM state is changed,
> and checks whether the parent is:
> not active (RPM_ACTIVE) and not ignoring its children
> If this happens it concludes something is wrong, because
> a parent that is not ignoring its children must be active
> before any children activate.
>
> - Since the struct i2c_adapter *bar does not ignore
> its children, the PM core thinks that it must indeed go
> online before its children, the check bails out with
> -EBUSY, i.e. the i2c_client *baz thinks it can't work
> because it's parent is not online, and it respects its
> parent.
>
> - In the driver the .resume() callback returns -EBUSY from
> the runtime_force_resume() call as per above. This leaves
> the device in a suspended state, leading to bad behaviour
> later when the device is used. The following debug
> print is made with an extra printg patch but illustrates
> the problem:
>
> [ 17.040832] bh1780 2-0029: parent (i2c-2) is not active
> parent->power.ignore_children = 0
> [ 17.040832] bh1780 2-0029: pm_runtime_force_resume:
> pm_runtime_set_active() failed (-16)
> [ 17.040863] dpm_run_callback():
> pm_runtime_force_resume+0x0/0x88 returns -16
> [ 17.040863] PM: Device 2-0029 failed to resume: error -16
>
> Fix this by letting all struct i2c_adapter:s ignore their
> children: i2c children have no business doing keeping
> their parents awake: they are completely autonomous
> devices that just use their parent to talk, a usecase
> which must be power managed in the host on a per-message
> basis.
>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Change subject from
> "let I2C masters inherit suspend child ignorance"
> to
> "let I2C masters ignore their children for PM"
> - Use the big hammer and do the sensible thing:
> mark all i2c adapters as ignoring their children
> when it comes to power management.
> ---
> drivers/i2c/i2c-core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 0f2f8484e8ec..5e114342dcbc 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1566,6 +1566,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>
> pm_runtime_no_callbacks(&adap->dev);
> pm_runtime_enable(&adap->dev);
> + pm_suspend_ignore_children(&adap->dev, true);

I guess it doesn't matter much, but I would set this before calling
pm_runtime_enable().

>
> #ifdef CONFIG_I2C_COMPAT
> res = class_compat_create_link(i2c_adapter_compat_class, &adap->dev,
> --
> 2.4.3
>

Besides the nitpick, feel free to add my:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe