[PATCH 0/3] PM / sleep: let slow buses inherit child ignorance

[PATCH 0/3] PM / sleep: let slow buses inherit child ignorance

[Linus Walleij]

I ran into a problem when using runtime_force_suspend()/resume()
on a target I2C device.

After some root causing I realized this is because slow buses create
struct device-containing masters from hardware devices, that do not
inherit the child ignorance setting, while they should.

Children on a slow message-oriented bus like I2C or SPI do not need
their I2C/SPI (etc) host to be up and running all the time in order
to go into a resumed state.

It is more customary for such busses to go online at the instant
that a driver or child signals that it want to transfer a message
on the bus.

Some drivers (such as drivers/i2c/busses/i2c-nomadik.c) have the
.ignore_children flag set properly in their main device and have
runtime PM running, but run into weird phenomena when their
children want to resume from sleep.

The most details are in the I2C patch [2/3].

The third patch is here to illustrate that the problem is present
in all slow external busses.

Linus Walleij (3):
  PM / sleep: add a helper function to inherit child ignorance
  i2c: let I2C masters inherit suspend child ignorance
  RFC: spi: let SPI masters inherit suspend child ignorance

 drivers/i2c/i2c-core.c | 1 +
 drivers/spi/spi.c      | 1 +
 include/linux/device.h | 7 +++++++
 3 files changed, 9 insertions(+)

-- 
2.4.3

[PATCH 1/3] PM / sleep: add a helper function to inherit child ignorance

[Linus Walleij]

A few subsystems dealing with peripherals on slow buses with
transfer-oriented runtime PM will need to inherit the child
ignorance property of their parent hardware devices.

Especially struct i2c_adapter and struct spi_master needs
this.

This adds a helper function to deal with this and make it
clear what is going on.

Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Mark Brown <broonie@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 include/linux/device.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 002c59728dbe..675c09e62239 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -961,6 +961,13 @@ static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
 	dev->power.ignore_children = enable;
 }
 
+static inline void pm_suspend_inherit_ignore_children(struct device *dev)
+{
+	if (!dev->parent)
+		return;
+	dev->power.ignore_children = dev->parent->power.ignore_children;
+}
+
 static inline void dev_pm_syscore_device(struct device *dev, bool val)
 {
 #ifdef CONFIG_PM_SLEEP
-- 
2.4.3

Re: [PATCH 1/3] PM / sleep: add a helper function to inherit child ignorance

[Linus Walleij]

On Thu, Apr 7, 2016 at 3:20 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> A few subsystems dealing with peripherals on slow buses with
> transfer-oriented runtime PM will need to inherit the child
> ignorance property of their parent hardware devices.
>
> Especially struct i2c_adapter and struct spi_master needs
> this.
>
> This adds a helper function to deal with this and make it
> clear what is going on.
>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Just drop this, I submitted new patches to simply mark all
I2C adapters and SPI hosts as ignoring their children.

Yours,
Linus Walleij

[PATCH 2/3] i2c: let I2C masters inherit suspend child ignorance

[Linus Walleij]

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
is 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 has not inherited the
  child ignorance and thus flags 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 inherit the
child ignorance setting from their parent when registering
an adapter. This way the child will realize it is ignored
and it can proceed to be resumed.

Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Mark Brown <broonie@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Wolfram: if I can convince you and Rafael that this is the
right thing to do, I guess Rafael could eventually merge
this oneliner through his PM tree with your ACK.
---
 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..dd12d4a162ed 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_inherit_ignore_children(&adap->dev);
 
 #ifdef CONFIG_I2C_COMPAT
 	res = class_compat_create_link(i2c_adapter_compat_class, &adap->dev,
-- 
2.4.3

Re: [PATCH 2/3] i2c: let I2C masters inherit suspend child ignorance

[Linus Walleij]

On Thu, Apr 7, 2016 at 3:20 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> @@ -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_inherit_ignore_children(&adap->dev);

If everyone agrees that i2c devices should never require the bus
to be resumed, we can of course use the big hammer and
just:

pm_suspend_ignore_children(&adap->dev, true);

Same for SPI. (And w1, ...)

Thoughts?

Yours,
Linus Walleij

Re: [PATCH 2/3] i2c: let I2C masters inherit suspend child ignorance

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

On Mon, Apr 11, 2016 at 08:53:40AM +0200, Linus Walleij wrote:

> If everyone agrees that i2c devices should never require the bus
> to be resumed, we can of course use the big hammer and
> just:

> pm_suspend_ignore_children(&adap->dev, true);

> Same for SPI. (And w1, ...)

> Thoughts?

That seems better.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 3/3] RFC: spi: let SPI masters inherit suspend child ignorance

[Linus Walleij]

This patch is analogous to the I2C patch preceding it
and the reasoning is the same for all slow busses
where the master device should only wake up when
messages get transferred rather than constantly
babysitting its children.

the master devices need to inherit the child ignorance
from their hardware device (whether AMBA device or
platform device etc) so that the children on the
external bus see that they are actively ignored by their
parent and can runtime_force_resume() from system suspend
without getting an error code.

Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Mark Brown <broonie@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Mark: mostly trying to bring home a point here and fix
it in all places if possible. No SPI master seems to be
ignoring its children for the moment but I have a patch
for the spi-pl022 doing this (ignoring its children)
I just need to find a suitable test target.
---
 drivers/spi/spi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index de2f2f90d799..48affabd170a 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1764,6 +1764,7 @@ struct spi_master *spi_alloc_master(struct device *dev, unsigned size)
 	master->num_chipselect = 1;
 	master->dev.class = &spi_master_class;
 	master->dev.parent = dev;
+	pm_suspend_inherit_ignore_children(&master->dev);
 	spi_master_set_devdata(master, &master[1]);
 
 	return master;
-- 
2.4.3

Re: [PATCH 3/3] RFC: spi: let SPI masters inherit suspend child ignorance

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Thu, Apr 07, 2016 at 03:20:37PM +0200, Linus Walleij wrote:
> This patch is analogous to the I2C patch preceding it

Please write standalone changelogs so people can understand what the
patch is supposed to do from reading the changlog.  They'll have to
follow the change later on when looking at it in git.

> and the reasoning is the same for all slow busses
> where the master device should only wake up when
> messages get transferred rather than constantly
> babysitting its children.

This isn't really about the bus being slow, it's about the bus being
able to usefully pause itself.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]