[RFC][PATCH] Enable async suspend/resume of i2c devices

[RFC][PATCH] Enable async suspend/resume of i2c devices

[Sonny Rao]

This improves our resume time when we have devices on an i2c bus
that are slow to resume.  In particular we have a light sensor that
adds about 50ms of resume time on one device. We have to enable it
both on the i2c master and i2c client side and then we get fully async
suspend/resume.  I suspect we'll see nice gains on systems with more
i2c devices and will test that out soon.

Signed-off-by: Sonny Rao <sonnyrao-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/i2c/i2c-core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index e5f76a0..bbe7569 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -547,6 +547,7 @@ i2c_new_device(struct i2c_adapter *adap, struct
i2c_board_info const *info)

        dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n",
                client->name, dev_name(&client->dev));
+       device_enable_async_suspend(&client->dev);

        return client;

@@ -857,6 +858,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
                dev_warn(&adap->dev,
                         "Failed to create compatibility class link\n");
 #endif
+       device_enable_async_suspend(&adap->dev);

        /* create pre-declared device nodes */
        if (adap->nr < __i2c_first_dynamic_bus_num)
-- 
1.7.3.1

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Mark Brown]

On Mon, Apr 04, 2011 at 08:47:01PM -0700, Sonny Rao wrote:
> This improves our resume time when we have devices on an i2c bus
> that are slow to resume.  In particular we have a light sensor that
> adds about 50ms of resume time on one device. We have to enable it
> both on the i2c master and i2c client side and then we get fully async
> suspend/resume.  I suspect we'll see nice gains on systems with more
> i2c devices and will test that out soon.
> 
> Signed-off-by: Sonny Rao <sonnyrao-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

It'd probably help if the patch explained why this is safe - my
immediate question is why if it's safe to just unconditionally enable
async suspend for all I2C clients and adaptors it's not safe to do so
for all devices of all types?

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Mark Brown]

On Mon, Apr 04, 2011 at 08:47:01PM -0700, Sonny Rao wrote:
> This improves our resume time when we have devices on an i2c bus
> that are slow to resume.  In particular we have a light sensor that
> adds about 50ms of resume time on one device. We have to enable it
> both on the i2c master and i2c client side and then we get fully async
> suspend/resume.  I suspect we'll see nice gains on systems with more
> i2c devices and will test that out soon.
> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>

It'd probably help if the patch explained why this is safe - my
immediate question is why if it's safe to just unconditionally enable
async suspend for all I2C clients and adaptors it's not safe to do so
for all devices of all types?

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Sonny Rao]

On Tue, Apr 5, 2011 at 10:23 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Mon, Apr 04, 2011 at 08:47:01PM -0700, Sonny Rao wrote:
>> This improves our resume time when we have devices on an i2c bus
>> that are slow to resume.  In particular we have a light sensor that
>> adds about 50ms of resume time on one device. We have to enable it
>> both on the i2c master and i2c client side and then we get fully async
>> suspend/resume.  I suspect we'll see nice gains on systems with more
>> i2c devices and will test that out soon.
>>
>> Signed-off-by: Sonny Rao <sonnyrao-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>
> It'd probably help if the patch explained why this is safe - my
> immediate question is why if it's safe to just unconditionally enable
> async suspend for all I2C clients and adaptors it's not safe to do so
> for all devices of all types?
>

Yeah, I admit that I'm not yet familiar enough with suspend/resume
to give a good answer, and I was wondering the same thing. I was
hoping people on the lists would chime in an say if it weren't safe,
or if there is a better way to get some parallelism for slow devices.

It might help if I gave you a bit more background on how I got to this
point.

We've been looking at suspend/resume performance on a dual
core Atom netbook (four hardware threads), and noticed each
that bringing each logical cpu up was adding about 80ms to our
resume time.

There was some expectation that having more cpus available
to do device resume would make up for the initial cost of bringing
each cpu up, but it turned out that wasn't the case.  Going from one
logical cpu to two made total resume time lower, but the third and
fourth cpus only added time.   So there wasn't enough device
parallelism to take advantage of four cpus.

So we started looking for what devices were going slow and the
top hitter was a light sensor that has to get totally reinitialized upon
resume.  Also, 50ms seems pretty small but in the case of the
that turns out to be about 8% of our time in device resume.

I've also noticed some other devices which aren't asynchronous
taking more than a trivial amount of time.  Especially some of the
ACPI devices like fans, batterys, etc, and was going to go look
at making those async as well if it would help.

I'm open to suggestions on how to tackle this, thanks for the
feedback.

Sonny

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Sonny Rao]

On Tue, Apr 5, 2011 at 10:23 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Apr 04, 2011 at 08:47:01PM -0700, Sonny Rao wrote:
>> This improves our resume time when we have devices on an i2c bus
>> that are slow to resume.  In particular we have a light sensor that
>> adds about 50ms of resume time on one device. We have to enable it
>> both on the i2c master and i2c client side and then we get fully async
>> suspend/resume.  I suspect we'll see nice gains on systems with more
>> i2c devices and will test that out soon.
>>
>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>
> It'd probably help if the patch explained why this is safe - my
> immediate question is why if it's safe to just unconditionally enable
> async suspend for all I2C clients and adaptors it's not safe to do so
> for all devices of all types?
>

Yeah, I admit that I'm not yet familiar enough with suspend/resume
to give a good answer, and I was wondering the same thing. I was
hoping people on the lists would chime in an say if it weren't safe,
or if there is a better way to get some parallelism for slow devices.

It might help if I gave you a bit more background on how I got to this
point.

We've been looking at suspend/resume performance on a dual
core Atom netbook (four hardware threads), and noticed each
that bringing each logical cpu up was adding about 80ms to our
resume time.

There was some expectation that having more cpus available
to do device resume would make up for the initial cost of bringing
each cpu up, but it turned out that wasn't the case.  Going from one
logical cpu to two made total resume time lower, but the third and
fourth cpus only added time.   So there wasn't enough device
parallelism to take advantage of four cpus.

So we started looking for what devices were going slow and the
top hitter was a light sensor that has to get totally reinitialized upon
resume.  Also, 50ms seems pretty small but in the case of the
that turns out to be about 8% of our time in device resume.

I've also noticed some other devices which aren't asynchronous
taking more than a trivial amount of time.  Especially some of the
ACPI devices like fans, batterys, etc, and was going to go look
at making those async as well if it would help.

I'm open to suggestions on how to tackle this, thanks for the
feedback.

Sonny

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Jean Delvare]

On Wed, 6 Apr 2011 06:23:35 +0100, Mark Brown wrote:
> On Mon, Apr 04, 2011 at 08:47:01PM -0700, Sonny Rao wrote:
> > This improves our resume time when we have devices on an i2c bus
> > that are slow to resume.  In particular we have a light sensor that
> > adds about 50ms of resume time on one device. We have to enable it
> > both on the i2c master and i2c client side and then we get fully async
> > suspend/resume.  I suspect we'll see nice gains on systems with more
> > i2c devices and will test that out soon.
> > 
> > Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> 
> It'd probably help if the patch explained why this is safe - my
> immediate question is why if it's safe to just unconditionally enable
> async suspend for all I2C clients and adaptors it's not safe to do so
> for all devices of all types?

I have exactly the same concern. From
Documentation/ABI/testing/sysfs-devices-power:

	It generally is unsafe to permit the asynchronous suspend/resume
	of a device unless it is certain that all of the PM dependencies
	of the device are known to the PM core.  However, for some
	devices this attribute is set to "enabled" by bus type code or
	device drivers and in that cases it should be safe to leave the
	default value.

As I don't see any code being added to guarantee that "all of the PM
dependencies of the device are known to the PM core", I am skeptical
about the general correctness of the proposed change.

On the other hand, a quick grep on the kernel tree shows that the scsi,
usb and pci subsystem enable async suspend unconditionally for all
devices. This seems quite contradictory with the quoted statement
above. Rafael, can you please clarify? Is the attribute description too
alarmist, or are the subsystems too optimistic? ;)

Original post for reference:
http://marc.info/?l=linux-i2c&m=130206741920878&w=2

Sonny, on how many systems did you test it? In particular, did you test
it on several TV cards (which use I2C a lot and sometimes in complex
setups)?

-- 
Jean Delvare

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Sonny Rao]

On Wed, Apr 6, 2011 at 12:52 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Wed, 6 Apr 2011 06:23:35 +0100, Mark Brown wrote:
>> On Mon, Apr 04, 2011 at 08:47:01PM -0700, Sonny Rao wrote:
>> > This improves our resume time when we have devices on an i2c bus
>> > that are slow to resume.  In particular we have a light sensor that
>> > adds about 50ms of resume time on one device. We have to enable it
>> > both on the i2c master and i2c client side and then we get fully async
>> > suspend/resume.  I suspect we'll see nice gains on systems with more
>> > i2c devices and will test that out soon.
>> >
>> > Signed-off-by: Sonny Rao <sonnyrao-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>
>> It'd probably help if the patch explained why this is safe - my
>> immediate question is why if it's safe to just unconditionally enable
>> async suspend for all I2C clients and adaptors it's not safe to do so
>> for all devices of all types?
>
> I have exactly the same concern. From
> Documentation/ABI/testing/sysfs-devices-power:
>
>        It generally is unsafe to permit the asynchronous suspend/resume
>        of a device unless it is certain that all of the PM dependencies
>        of the device are known to the PM core.  However, for some
>        devices this attribute is set to "enabled" by bus type code or
>        device drivers and in that cases it should be safe to leave the
>        default value.
>
> As I don't see any code being added to guarantee that "all of the PM
> dependencies of the device are known to the PM core", I am skeptical
> about the general correctness of the proposed change.
>
> On the other hand, a quick grep on the kernel tree shows that the scsi,
> usb and pci subsystem enable async suspend unconditionally for all
> devices. This seems quite contradictory with the quoted statement
> above. Rafael, can you please clarify? Is the attribute description too
> alarmist, or are the subsystems too optimistic? ;)

Yeah, this is sort of why I made the change at this level.
I originally wrote a patch which just enabled it for the particular
sensor, but it didn't match with the other uses of that function
in the kernel which did things to entire subsystems like you
point out.  So, I re-wrote it in the optimistic hope that this was safe
just like USB/PCI/SCSI seem to be.

> Original post for reference:
> http://marc.info/?l=linux-i2c&m=130206741920878&w=2
>
> Sonny, on how many systems did you test it? In particular, did you test
> it on several TV cards (which use I2C a lot and sometimes in complex
> setups)?

It has only been tested on my x86 system with the light sensor and
lightly tested on an ARM system with about 6-7 i2c devices.  I was hoping
the change would help the ARM systems because they make much
heavier use of i2c than the x86 system.  I haven't tested on anything
like multiple TV cards because I don't have such a setup -- the ARM
boxes with several i2c devices is the most complex user of i2c I have
at the moment.

Sonny

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Sonny Rao]

On Wed, Apr 6, 2011 at 12:52 AM, Jean Delvare <khali@linux-fr.org> wrote:
> On Wed, 6 Apr 2011 06:23:35 +0100, Mark Brown wrote:
>> On Mon, Apr 04, 2011 at 08:47:01PM -0700, Sonny Rao wrote:
>> > This improves our resume time when we have devices on an i2c bus
>> > that are slow to resume.  In particular we have a light sensor that
>> > adds about 50ms of resume time on one device. We have to enable it
>> > both on the i2c master and i2c client side and then we get fully async
>> > suspend/resume.  I suspect we'll see nice gains on systems with more
>> > i2c devices and will test that out soon.
>> >
>> > Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>>
>> It'd probably help if the patch explained why this is safe - my
>> immediate question is why if it's safe to just unconditionally enable
>> async suspend for all I2C clients and adaptors it's not safe to do so
>> for all devices of all types?
>
> I have exactly the same concern. From
> Documentation/ABI/testing/sysfs-devices-power:
>
>        It generally is unsafe to permit the asynchronous suspend/resume
>        of a device unless it is certain that all of the PM dependencies
>        of the device are known to the PM core.  However, for some
>        devices this attribute is set to "enabled" by bus type code or
>        device drivers and in that cases it should be safe to leave the
>        default value.
>
> As I don't see any code being added to guarantee that "all of the PM
> dependencies of the device are known to the PM core", I am skeptical
> about the general correctness of the proposed change.
>
> On the other hand, a quick grep on the kernel tree shows that the scsi,
> usb and pci subsystem enable async suspend unconditionally for all
> devices. This seems quite contradictory with the quoted statement
> above. Rafael, can you please clarify? Is the attribute description too
> alarmist, or are the subsystems too optimistic? ;)

Yeah, this is sort of why I made the change at this level.
I originally wrote a patch which just enabled it for the particular
sensor, but it didn't match with the other uses of that function
in the kernel which did things to entire subsystems like you
point out.  So, I re-wrote it in the optimistic hope that this was safe
just like USB/PCI/SCSI seem to be.

> Original post for reference:
> http://marc.info/?l=linux-i2c&m=130206741920878&w=2
>
> Sonny, on how many systems did you test it? In particular, did you test
> it on several TV cards (which use I2C a lot and sometimes in complex
> setups)?

It has only been tested on my x86 system with the light sensor and
lightly tested on an ARM system with about 6-7 i2c devices.  I was hoping
the change would help the ARM systems because they make much
heavier use of i2c than the x86 system.  I haven't tested on anything
like multiple TV cards because I don't have such a setup -- the ARM
boxes with several i2c devices is the most complex user of i2c I have
at the moment.

Sonny

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Jean Delvare]

On Wed, 6 Apr 2011 01:16:07 -0700, Sonny Rao wrote:
> On Wed, Apr 6, 2011 at 12:52 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > Sonny, on how many systems did you test it? In particular, did you test
> > it on several TV cards (which use I2C a lot and sometimes in complex
> > setups)?
> 
> It has only been tested on my x86 system with the light sensor and
> lightly tested on an ARM system with about 6-7 i2c devices.  I was hoping
> the change would help the ARM systems because they make much
> heavier use of i2c than the x86 system.  I haven't tested on anything
> like multiple TV cards because I don't have such a setup -- the ARM
> boxes with several i2c devices is the most complex user of i2c I have
> at the moment.

I meant several TV card models, individually. I don't expect testing
many at once to reveal any problem that individual testing wouldn't
have.

-- 
Jean Delvare

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Jean Delvare]

On Wed, 6 Apr 2011 01:16:07 -0700, Sonny Rao wrote:
> On Wed, Apr 6, 2011 at 12:52 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > Sonny, on how many systems did you test it? In particular, did you test
> > it on several TV cards (which use I2C a lot and sometimes in complex
> > setups)?
> 
> It has only been tested on my x86 system with the light sensor and
> lightly tested on an ARM system with about 6-7 i2c devices.  I was hoping
> the change would help the ARM systems because they make much
> heavier use of i2c than the x86 system.  I haven't tested on anything
> like multiple TV cards because I don't have such a setup -- the ARM
> boxes with several i2c devices is the most complex user of i2c I have
> at the moment.

I meant several TV card models, individually. I don't expect testing
many at once to reveal any problem that individual testing wouldn't
have.

-- 
Jean Delvare

Re: [linux-pm] [RFC][PATCH] Enable async suspend/resume of i2c devices

[Alan Stern]

On Wed, 6 Apr 2011, Jean Delvare wrote:

> On Wed, 6 Apr 2011 06:23:35 +0100, Mark Brown wrote:
> > On Mon, Apr 04, 2011 at 08:47:01PM -0700, Sonny Rao wrote:
> > > This improves our resume time when we have devices on an i2c bus
> > > that are slow to resume.  In particular we have a light sensor that
> > > adds about 50ms of resume time on one device. We have to enable it
> > > both on the i2c master and i2c client side and then we get fully async
> > > suspend/resume.  I suspect we'll see nice gains on systems with more
> > > i2c devices and will test that out soon.
> > > 
> > > Signed-off-by: Sonny Rao <sonnyrao-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > 
> > It'd probably help if the patch explained why this is safe - my
> > immediate question is why if it's safe to just unconditionally enable
> > async suspend for all I2C clients and adaptors it's not safe to do so
> > for all devices of all types?
> 
> I have exactly the same concern. From
> Documentation/ABI/testing/sysfs-devices-power:
> 
> 	It generally is unsafe to permit the asynchronous suspend/resume
> 	of a device unless it is certain that all of the PM dependencies
> 	of the device are known to the PM core.  However, for some
> 	devices this attribute is set to "enabled" by bus type code or
> 	device drivers and in that cases it should be safe to leave the
> 	default value.
> 
> As I don't see any code being added to guarantee that "all of the PM
> dependencies of the device are known to the PM core", I am skeptical
> about the general correctness of the proposed change.
> 
> On the other hand, a quick grep on the kernel tree shows that the scsi,
> usb and pci subsystem enable async suspend unconditionally for all
> devices. This seems quite contradictory with the quoted statement
> above. Rafael, can you please clarify? Is the attribute description too
> alarmist, or are the subsystems too optimistic? ;)

Neither is the case.  For these subsystems, the PM dependencies _are_ 
known.

The statement in the Documentation file refers to dependencies other 
than the usual parent-child relationships.  PCI and SCSI have no such 
dependencies.  In USB some do exist, but the subsystem code knows about 
them and takes them into account.  Grep for "device_pm_wait_for_dev" in 
the appropriate subtrees and you'll see what I mean.

Now, I have no idea what the situation is with regard to I2C...

Alan Stern

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Alan Stern]

On Wed, 6 Apr 2011, Jean Delvare wrote:

> On Wed, 6 Apr 2011 06:23:35 +0100, Mark Brown wrote:
> > On Mon, Apr 04, 2011 at 08:47:01PM -0700, Sonny Rao wrote:
> > > This improves our resume time when we have devices on an i2c bus
> > > that are slow to resume.  In particular we have a light sensor that
> > > adds about 50ms of resume time on one device. We have to enable it
> > > both on the i2c master and i2c client side and then we get fully async
> > > suspend/resume.  I suspect we'll see nice gains on systems with more
> > > i2c devices and will test that out soon.
> > > 
> > > Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> > 
> > It'd probably help if the patch explained why this is safe - my
> > immediate question is why if it's safe to just unconditionally enable
> > async suspend for all I2C clients and adaptors it's not safe to do so
> > for all devices of all types?
> 
> I have exactly the same concern. From
> Documentation/ABI/testing/sysfs-devices-power:
> 
> 	It generally is unsafe to permit the asynchronous suspend/resume
> 	of a device unless it is certain that all of the PM dependencies
> 	of the device are known to the PM core.  However, for some
> 	devices this attribute is set to "enabled" by bus type code or
> 	device drivers and in that cases it should be safe to leave the
> 	default value.
> 
> As I don't see any code being added to guarantee that "all of the PM
> dependencies of the device are known to the PM core", I am skeptical
> about the general correctness of the proposed change.
> 
> On the other hand, a quick grep on the kernel tree shows that the scsi,
> usb and pci subsystem enable async suspend unconditionally for all
> devices. This seems quite contradictory with the quoted statement
> above. Rafael, can you please clarify? Is the attribute description too
> alarmist, or are the subsystems too optimistic? ;)

Neither is the case.  For these subsystems, the PM dependencies _are_ 
known.

The statement in the Documentation file refers to dependencies other 
than the usual parent-child relationships.  PCI and SCSI have no such 
dependencies.  In USB some do exist, but the subsystem code knows about 
them and takes them into account.  Grep for "device_pm_wait_for_dev" in 
the appropriate subtrees and you'll see what I mean.

Now, I have no idea what the situation is with regard to I2C...

Alan Stern

Re: [linux-pm] [RFC][PATCH] Enable async suspend/resume of i2c devices

[Mark Brown]

On Wed, Apr 06, 2011 at 10:49:17AM -0400, Alan Stern wrote:

> Neither is the case.  For these subsystems, the PM dependencies _are_ 
> known.

...

> Now, I have no idea what the situation is with regard to I2C...

You definitely don't know *anything* about the relationships for I2C,
especially in embedded systems.

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Mark Brown]

On Wed, Apr 06, 2011 at 10:49:17AM -0400, Alan Stern wrote:

> Neither is the case.  For these subsystems, the PM dependencies _are_ 
> known.

...

> Now, I have no idea what the situation is with regard to I2C...

You definitely don't know *anything* about the relationships for I2C,
especially in embedded systems.

Re: [linux-pm] [RFC][PATCH] Enable async suspend/resume of i2c devices

[Sonny Rao]

On Wed, Apr 6, 2011 at 3:31 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Wed, Apr 06, 2011 at 10:49:17AM -0400, Alan Stern wrote:
>
>> Neither is the case.  For these subsystems, the PM dependencies _are_
>> known.
>
> ...
>
>> Now, I have no idea what the situation is with regard to I2C...
>
> You definitely don't know *anything* about the relationships for I2C,
> especially in embedded systems.
>

Would it be okay to enable this on a per-device basis where it is
known to be safe?

I started out doing it like this,  but I didn't like the way the patch looked
because it ended up having to call this function twice -- once on the i2c
master device and once on it's client devices (and actually a third time
because it had another struct dev for the industrial IO device).  So,
that seemed pretty ugly.

Also, it didn't seem to match the usage of device_enable_async_suspend()
in other parts of the kernel where it was used on whole subsystems.

Sonny

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Sonny Rao]

On Wed, Apr 6, 2011 at 3:31 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Apr 06, 2011 at 10:49:17AM -0400, Alan Stern wrote:
>
>> Neither is the case.  For these subsystems, the PM dependencies _are_
>> known.
>
> ...
>
>> Now, I have no idea what the situation is with regard to I2C...
>
> You definitely don't know *anything* about the relationships for I2C,
> especially in embedded systems.
>

Would it be okay to enable this on a per-device basis where it is
known to be safe?

I started out doing it like this,  but I didn't like the way the patch looked
because it ended up having to call this function twice -- once on the i2c
master device and once on it's client devices (and actually a third time
because it had another struct dev for the industrial IO device).  So,
that seemed pretty ugly.

Also, it didn't seem to match the usage of device_enable_async_suspend()
in other parts of the kernel where it was used on whole subsystems.

Sonny

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Rafael J. Wysocki]

On Wednesday, April 06, 2011, Jean Delvare wrote:
> On Wed, 6 Apr 2011 06:23:35 +0100, Mark Brown wrote:
> > On Mon, Apr 04, 2011 at 08:47:01PM -0700, Sonny Rao wrote:
> > > This improves our resume time when we have devices on an i2c bus
> > > that are slow to resume.  In particular we have a light sensor that
> > > adds about 50ms of resume time on one device. We have to enable it
> > > both on the i2c master and i2c client side and then we get fully async
> > > suspend/resume.  I suspect we'll see nice gains on systems with more
> > > i2c devices and will test that out soon.
> > > 
> > > Signed-off-by: Sonny Rao <sonnyrao-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > 
> > It'd probably help if the patch explained why this is safe - my
> > immediate question is why if it's safe to just unconditionally enable
> > async suspend for all I2C clients and adaptors it's not safe to do so
> > for all devices of all types?
> 
> I have exactly the same concern. From
> Documentation/ABI/testing/sysfs-devices-power:
> 
> 	It generally is unsafe to permit the asynchronous suspend/resume
> 	of a device unless it is certain that all of the PM dependencies
> 	of the device are known to the PM core.  However, for some
> 	devices this attribute is set to "enabled" by bus type code or
> 	device drivers and in that cases it should be safe to leave the
> 	default value.
> 
> As I don't see any code being added to guarantee that "all of the PM
> dependencies of the device are known to the PM core", I am skeptical
> about the general correctness of the proposed change.
> 
> On the other hand, a quick grep on the kernel tree shows that the scsi,
> usb and pci subsystem enable async suspend unconditionally for all
> devices. This seems quite contradictory with the quoted statement
> above. Rafael, can you please clarify? Is the attribute description too
> alarmist, or are the subsystems too optimistic? ;)

Well, the short explanation is that enabling async suspend/resume for SCSI,
USB and PCI devices gives us the most visible benefit performance-wise, while
at the same time the PM dependencies for all of them generally follow the
parent-child relationships represented by the device hierarchy (i.e. each of
them has a well-defined parent device that it should not be suspended after
and resumed before).

In the few cases where there are known dependencies reaching beyond the device
hierarchy (e.g. the dependencies between the EHCI and UHCI/OHCI USB
controllers), they have been taken into account directly by the relevant
drivers.

Thanks,
Rafael

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Rafael J. Wysocki]

On Wednesday, April 06, 2011, Jean Delvare wrote:
> On Wed, 6 Apr 2011 06:23:35 +0100, Mark Brown wrote:
> > On Mon, Apr 04, 2011 at 08:47:01PM -0700, Sonny Rao wrote:
> > > This improves our resume time when we have devices on an i2c bus
> > > that are slow to resume.  In particular we have a light sensor that
> > > adds about 50ms of resume time on one device. We have to enable it
> > > both on the i2c master and i2c client side and then we get fully async
> > > suspend/resume.  I suspect we'll see nice gains on systems with more
> > > i2c devices and will test that out soon.
> > > 
> > > Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> > 
> > It'd probably help if the patch explained why this is safe - my
> > immediate question is why if it's safe to just unconditionally enable
> > async suspend for all I2C clients and adaptors it's not safe to do so
> > for all devices of all types?
> 
> I have exactly the same concern. From
> Documentation/ABI/testing/sysfs-devices-power:
> 
> 	It generally is unsafe to permit the asynchronous suspend/resume
> 	of a device unless it is certain that all of the PM dependencies
> 	of the device are known to the PM core.  However, for some
> 	devices this attribute is set to "enabled" by bus type code or
> 	device drivers and in that cases it should be safe to leave the
> 	default value.
> 
> As I don't see any code being added to guarantee that "all of the PM
> dependencies of the device are known to the PM core", I am skeptical
> about the general correctness of the proposed change.
> 
> On the other hand, a quick grep on the kernel tree shows that the scsi,
> usb and pci subsystem enable async suspend unconditionally for all
> devices. This seems quite contradictory with the quoted statement
> above. Rafael, can you please clarify? Is the attribute description too
> alarmist, or are the subsystems too optimistic? ;)

Well, the short explanation is that enabling async suspend/resume for SCSI,
USB and PCI devices gives us the most visible benefit performance-wise, while
at the same time the PM dependencies for all of them generally follow the
parent-child relationships represented by the device hierarchy (i.e. each of
them has a well-defined parent device that it should not be suspended after
and resumed before).

In the few cases where there are known dependencies reaching beyond the device
hierarchy (e.g. the dependencies between the EHCI and UHCI/OHCI USB
controllers), they have been taken into account directly by the relevant
drivers.

Thanks,
Rafael

Re: [linux-pm] [RFC][PATCH] Enable async suspend/resume of i2c devices

[Rafael J. Wysocki]

On Thursday, April 07, 2011, Sonny Rao wrote:
> On Wed, Apr 6, 2011 at 3:31 PM, Mark Brown
> <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> > On Wed, Apr 06, 2011 at 10:49:17AM -0400, Alan Stern wrote:
> >
> >> Neither is the case.  For these subsystems, the PM dependencies _are_
> >> known.
> >
> > ...
> >
> >> Now, I have no idea what the situation is with regard to I2C...
> >
> > You definitely don't know *anything* about the relationships for I2C,
> > especially in embedded systems.
> >
> 
> Would it be okay to enable this on a per-device basis where it is
> known to be safe?

Yes.

> I started out doing it like this,  but I didn't like the way the patch looked
> because it ended up having to call this function twice -- once on the i2c
> master device and once on it's client devices (and actually a third time
> because it had another struct dev for the industrial IO device).  So,
> that seemed pretty ugly.
> 
> Also, it didn't seem to match the usage of device_enable_async_suspend()
> in other parts of the kernel where it was used on whole subsystems.

It is used by subsystems where dependencies between devices are known to
generally follow the structure of the device hierarchy within the kernel
(i.e. they are well-defined buses with well-defined parents and children).

Thanks,
Rafael

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Rafael J. Wysocki]

On Thursday, April 07, 2011, Sonny Rao wrote:
> On Wed, Apr 6, 2011 at 3:31 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
> > On Wed, Apr 06, 2011 at 10:49:17AM -0400, Alan Stern wrote:
> >
> >> Neither is the case.  For these subsystems, the PM dependencies _are_
> >> known.
> >
> > ...
> >
> >> Now, I have no idea what the situation is with regard to I2C...
> >
> > You definitely don't know *anything* about the relationships for I2C,
> > especially in embedded systems.
> >
> 
> Would it be okay to enable this on a per-device basis where it is
> known to be safe?

Yes.

> I started out doing it like this,  but I didn't like the way the patch looked
> because it ended up having to call this function twice -- once on the i2c
> master device and once on it's client devices (and actually a third time
> because it had another struct dev for the industrial IO device).  So,
> that seemed pretty ugly.
> 
> Also, it didn't seem to match the usage of device_enable_async_suspend()
> in other parts of the kernel where it was used on whole subsystems.

It is used by subsystems where dependencies between devices are known to
generally follow the structure of the device hierarchy within the kernel
(i.e. they are well-defined buses with well-defined parents and children).

Thanks,
Rafael

Re: [linux-pm] [RFC][PATCH] Enable async suspend/resume of i2c devices

[Sonny Rao]

On Wed, Apr 6, 2011 at 10:22 PM, Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org> wrote:
> On Thursday, April 07, 2011, Sonny Rao wrote:
>> On Wed, Apr 6, 2011 at 3:31 PM, Mark Brown
>> <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
>> > On Wed, Apr 06, 2011 at 10:49:17AM -0400, Alan Stern wrote:
>> >
>> >> Neither is the case.  For these subsystems, the PM dependencies _are_
>> >> known.
>> >
>> > ...
>> >
>> >> Now, I have no idea what the situation is with regard to I2C...
>> >
>> > You definitely don't know *anything* about the relationships for I2C,
>> > especially in embedded systems.
>> >
>>
>> Would it be okay to enable this on a per-device basis where it is
>> known to be safe?
>
> Yes.

Ok, I'll probably submit a patch for the specific case of the slow
light sensor to the IIO
guys later on, and if I find cases where we get wins on other I2C
heavy platforms
(like ARM netbooks) I'll send those out as device-specific changes as well.

<snip>

> It is used by subsystems where dependencies between devices are known to
> generally follow the structure of the device hierarchy within the kernel
> (i.e. they are well-defined buses with well-defined parents and children).

Ok, thanks for the explanation!

Sonny

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Sonny Rao]

On Wed, Apr 6, 2011 at 10:22 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, April 07, 2011, Sonny Rao wrote:
>> On Wed, Apr 6, 2011 at 3:31 PM, Mark Brown
>> <broonie@opensource.wolfsonmicro.com> wrote:
>> > On Wed, Apr 06, 2011 at 10:49:17AM -0400, Alan Stern wrote:
>> >
>> >> Neither is the case.  For these subsystems, the PM dependencies _are_
>> >> known.
>> >
>> > ...
>> >
>> >> Now, I have no idea what the situation is with regard to I2C...
>> >
>> > You definitely don't know *anything* about the relationships for I2C,
>> > especially in embedded systems.
>> >
>>
>> Would it be okay to enable this on a per-device basis where it is
>> known to be safe?
>
> Yes.

Ok, I'll probably submit a patch for the specific case of the slow
light sensor to the IIO
guys later on, and if I find cases where we get wins on other I2C
heavy platforms
(like ARM netbooks) I'll send those out as device-specific changes as well.

<snip>

> It is used by subsystems where dependencies between devices are known to
> generally follow the structure of the device hierarchy within the kernel
> (i.e. they are well-defined buses with well-defined parents and children).

Ok, thanks for the explanation!

Sonny

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Jean Delvare]

Hi Mark,

On Thu, 7 Apr 2011 07:31:24 +0900, Mark Brown wrote:
> On Wed, Apr 06, 2011 at 10:49:17AM -0400, Alan Stern wrote:
> 
> > Neither is the case.  For these subsystems, the PM dependencies _are_ 
> > known.
> 
> ...
> 
> > Now, I have no idea what the situation is with regard to I2C...
> 
> You definitely don't know *anything* about the relationships for I2C,
> especially in embedded systems.

Can you please elaborate? The i2c subsystem uses a standard
parent-children relationship. It seems fairly similar to USB for
example. The only special case I can think of is with bus multiplexing,
but it would be easy enough to switch off async suspend/resume in this
case.

Sonny, I would expect you to obtain the time saving by only switching
the i2c client device to async suspend/resume. wasn't it the case? i2c
bus device suspend/resume shouldn't matter, as the operation should be
handled by the hardware (e.g. PCI) layer.

Rafael, what happens if a parent is set to async suspend/resume but its
children aren't? Does it disable async altogether for the whole branch,
or does it mean that the branch is processed asynchronously as a whole
but its internal processing is serialized? Or something else?

-- 
Jean Delvare

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Jean Delvare]

Hi Mark,

On Thu, 7 Apr 2011 07:31:24 +0900, Mark Brown wrote:
> On Wed, Apr 06, 2011 at 10:49:17AM -0400, Alan Stern wrote:
> 
> > Neither is the case.  For these subsystems, the PM dependencies _are_ 
> > known.
> 
> ...
> 
> > Now, I have no idea what the situation is with regard to I2C...
> 
> You definitely don't know *anything* about the relationships for I2C,
> especially in embedded systems.

Can you please elaborate? The i2c subsystem uses a standard
parent-children relationship. It seems fairly similar to USB for
example. The only special case I can think of is with bus multiplexing,
but it would be easy enough to switch off async suspend/resume in this
case.

Sonny, I would expect you to obtain the time saving by only switching
the i2c client device to async suspend/resume. wasn't it the case? i2c
bus device suspend/resume shouldn't matter, as the operation should be
handled by the hardware (e.g. PCI) layer.

Rafael, what happens if a parent is set to async suspend/resume but its
children aren't? Does it disable async altogether for the whole branch,
or does it mean that the branch is processed asynchronously as a whole
but its internal processing is serialized? Or something else?

-- 
Jean Delvare

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Mark Brown]

On Thu, Apr 07, 2011 at 09:55:13AM +0200, Jean Delvare wrote:
> On Thu, 7 Apr 2011 07:31:24 +0900, Mark Brown wrote:

> > You definitely don't know *anything* about the relationships for I2C,
> > especially in embedded systems.

> Can you please elaborate? The i2c subsystem uses a standard
> parent-children relationship. It seems fairly similar to USB for
> example. The only special case I can think of is with bus multiplexing,
> but it would be easy enough to switch off async suspend/resume in this
> case.

For I2C itself that's the case but for power and GPIO (which are very
common in conjunction with I2C in the embedded context) we loose the
tree.  This is much less of an issue with buses like PCI where
everything is bundled into the bus.

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Mark Brown]

On Thu, Apr 07, 2011 at 09:55:13AM +0200, Jean Delvare wrote:
> On Thu, 7 Apr 2011 07:31:24 +0900, Mark Brown wrote:

> > You definitely don't know *anything* about the relationships for I2C,
> > especially in embedded systems.

> Can you please elaborate? The i2c subsystem uses a standard
> parent-children relationship. It seems fairly similar to USB for
> example. The only special case I can think of is with bus multiplexing,
> but it would be easy enough to switch off async suspend/resume in this
> case.

For I2C itself that's the case but for power and GPIO (which are very
common in conjunction with I2C in the embedded context) we loose the
tree.  This is much less of an issue with buses like PCI where
everything is bundled into the bus.

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Sonny Rao]

On Thu, Apr 7, 2011 at 12:55 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Mark,
>
> On Thu, 7 Apr 2011 07:31:24 +0900, Mark Brown wrote:
>> On Wed, Apr 06, 2011 at 10:49:17AM -0400, Alan Stern wrote:
>>
>> > Neither is the case.  For these subsystems, the PM dependencies _are_
>> > known.
>>
>> ...
>>
>> > Now, I have no idea what the situation is with regard to I2C...
>>
>> You definitely don't know *anything* about the relationships for I2C,
>> especially in embedded systems.
>
> Can you please elaborate? The i2c subsystem uses a standard
> parent-children relationship. It seems fairly similar to USB for
> example. The only special case I can think of is with bus multiplexing,
> but it would be easy enough to switch off async suspend/resume in this
> case.
>
> Sonny, I would expect you to obtain the time saving by only switching
> the i2c client device to async suspend/resume. wasn't it the case? i2c
> bus device suspend/resume shouldn't matter, as the operation should be
> handled by the hardware (e.g. PCI) layer.

I tried that first, and it looked like the client resume wouldn't start on the
async thread until the kernel resumed the i2c master which was
happening very late for some reason.  Maybe this was just a fluke of
the device ordering, but enabling it for both master and client allowed
the device resume to consistently happen much earlier.

Sonny

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Sonny Rao]

On Thu, Apr 7, 2011 at 12:55 AM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Mark,
>
> On Thu, 7 Apr 2011 07:31:24 +0900, Mark Brown wrote:
>> On Wed, Apr 06, 2011 at 10:49:17AM -0400, Alan Stern wrote:
>>
>> > Neither is the case.  For these subsystems, the PM dependencies _are_
>> > known.
>>
>> ...
>>
>> > Now, I have no idea what the situation is with regard to I2C...
>>
>> You definitely don't know *anything* about the relationships for I2C,
>> especially in embedded systems.
>
> Can you please elaborate? The i2c subsystem uses a standard
> parent-children relationship. It seems fairly similar to USB for
> example. The only special case I can think of is with bus multiplexing,
> but it would be easy enough to switch off async suspend/resume in this
> case.
>
> Sonny, I would expect you to obtain the time saving by only switching
> the i2c client device to async suspend/resume. wasn't it the case? i2c
> bus device suspend/resume shouldn't matter, as the operation should be
> handled by the hardware (e.g. PCI) layer.

I tried that first, and it looked like the client resume wouldn't start on the
async thread until the kernel resumed the i2c master which was
happening very late for some reason.  Maybe this was just a fluke of
the device ordering, but enabling it for both master and client allowed
the device resume to consistently happen much earlier.

Sonny

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Jean Delvare]

On Thu, 7 Apr 2011 09:00:43 +0100, Mark Brown wrote:
> On Thu, Apr 07, 2011 at 09:55:13AM +0200, Jean Delvare wrote:
> > On Thu, 7 Apr 2011 07:31:24 +0900, Mark Brown wrote:
> 
> > > You definitely don't know *anything* about the relationships for I2C,
> > > especially in embedded systems.
> 
> > Can you please elaborate? The i2c subsystem uses a standard
> > parent-children relationship. It seems fairly similar to USB for
> > example. The only special case I can think of is with bus multiplexing,
> > but it would be easy enough to switch off async suspend/resume in this
> > case.
> 
> For I2C itself that's the case but for power and GPIO (which are very
> common in conjunction with I2C in the embedded context) we loose the
> tree.  This is much less of an issue with buses like PCI where
> everything is bundled into the bus.

Out of curiosity, how is this handled in the synchronous suspend/resume
case?

-- 
Jean Delvare

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Jean Delvare]

On Thu, 7 Apr 2011 09:00:43 +0100, Mark Brown wrote:
> On Thu, Apr 07, 2011 at 09:55:13AM +0200, Jean Delvare wrote:
> > On Thu, 7 Apr 2011 07:31:24 +0900, Mark Brown wrote:
> 
> > > You definitely don't know *anything* about the relationships for I2C,
> > > especially in embedded systems.
> 
> > Can you please elaborate? The i2c subsystem uses a standard
> > parent-children relationship. It seems fairly similar to USB for
> > example. The only special case I can think of is with bus multiplexing,
> > but it would be easy enough to switch off async suspend/resume in this
> > case.
> 
> For I2C itself that's the case but for power and GPIO (which are very
> common in conjunction with I2C in the embedded context) we loose the
> tree.  This is much less of an issue with buses like PCI where
> everything is bundled into the bus.

Out of curiosity, how is this handled in the synchronous suspend/resume
case?

-- 
Jean Delvare

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Jean Delvare]

On Thu, 7 Apr 2011 01:19:59 -0700, Sonny Rao wrote:
> On Thu, Apr 7, 2011 at 12:55 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > Sonny, I would expect you to obtain the time saving by only switching
> > the i2c client device to async suspend/resume. wasn't it the case? i2c
> > bus device suspend/resume shouldn't matter, as the operation should be
> > handled by the hardware (e.g. PCI) layer.
> 
> I tried that first, and it looked like the client resume wouldn't start on the
> async thread until the kernel resumed the i2c master which was

This is expected... You need an operational i2c master to talk to the
i2c client, regardless of async or not.

> happening very late for some reason.  Maybe this was just a fluke of
> the device ordering, but enabling it for both master and client allowed
> the device resume to consistently happen much earlier.

OK, that makes sense. I don't know how the synchronous resume decides
in which order it processes the devices, presumably it simply walks the
device tree. If you are unlucky and the i2c master is near the end,
then indeed the asynchronous resume of the i2c client device alone may
save nothing at all. I get it now.

-- 
Jean Delvare

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Jean Delvare]

On Thu, 7 Apr 2011 01:19:59 -0700, Sonny Rao wrote:
> On Thu, Apr 7, 2011 at 12:55 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > Sonny, I would expect you to obtain the time saving by only switching
> > the i2c client device to async suspend/resume. wasn't it the case? i2c
> > bus device suspend/resume shouldn't matter, as the operation should be
> > handled by the hardware (e.g. PCI) layer.
> 
> I tried that first, and it looked like the client resume wouldn't start on the
> async thread until the kernel resumed the i2c master which was

This is expected... You need an operational i2c master to talk to the
i2c client, regardless of async or not.

> happening very late for some reason.  Maybe this was just a fluke of
> the device ordering, but enabling it for both master and client allowed
> the device resume to consistently happen much earlier.

OK, that makes sense. I don't know how the synchronous resume decides
in which order it processes the devices, presumably it simply walks the
device tree. If you are unlucky and the i2c master is near the end,
then indeed the asynchronous resume of the i2c client device alone may
save nothing at all. I get it now.

-- 
Jean Delvare

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Alan Stern]

On Thu, 7 Apr 2011, Jean Delvare wrote:

> On Thu, 7 Apr 2011 09:00:43 +0100, Mark Brown wrote:
> > On Thu, Apr 07, 2011 at 09:55:13AM +0200, Jean Delvare wrote:
> > > On Thu, 7 Apr 2011 07:31:24 +0900, Mark Brown wrote:
> > 
> > > > You definitely don't know *anything* about the relationships for I2C,
> > > > especially in embedded systems.
> > 
> > > Can you please elaborate? The i2c subsystem uses a standard
> > > parent-children relationship. It seems fairly similar to USB for
> > > example. The only special case I can think of is with bus multiplexing,
> > > but it would be easy enough to switch off async suspend/resume in this
> > > case.
> > 
> > For I2C itself that's the case but for power and GPIO (which are very
> > common in conjunction with I2C in the embedded context) we loose the
> > tree.  This is much less of an issue with buses like PCI where
> > everything is bundled into the bus.
> 
> Out of curiosity, how is this handled in the synchronous suspend/resume
> case?

With synchronous resume, devices are resumed in the order they were
originally registered.  Since the power and GPIO controllers must have
been registered before the I2C controller (otherwise the I2C wouldn't
have been accessible when it was registered!), they will be resumed
first.

Alan Stern

Re: [RFC][PATCH] Enable async suspend/resume of i2c devices

[Alan Stern]

On Thu, 7 Apr 2011, Jean Delvare wrote:

> On Thu, 7 Apr 2011 09:00:43 +0100, Mark Brown wrote:
> > On Thu, Apr 07, 2011 at 09:55:13AM +0200, Jean Delvare wrote:
> > > On Thu, 7 Apr 2011 07:31:24 +0900, Mark Brown wrote:
> > 
> > > > You definitely don't know *anything* about the relationships for I2C,
> > > > especially in embedded systems.
> > 
> > > Can you please elaborate? The i2c subsystem uses a standard
> > > parent-children relationship. It seems fairly similar to USB for
> > > example. The only special case I can think of is with bus multiplexing,
> > > but it would be easy enough to switch off async suspend/resume in this
> > > case.
> > 
> > For I2C itself that's the case but for power and GPIO (which are very
> > common in conjunction with I2C in the embedded context) we loose the
> > tree.  This is much less of an issue with buses like PCI where
> > everything is bundled into the bus.
> 
> Out of curiosity, how is this handled in the synchronous suspend/resume
> case?

With synchronous resume, devices are resumed in the order they were
originally registered.  Since the power and GPIO controllers must have
been registered before the I2C controller (otherwise the I2C wouldn't
have been accessible when it was registered!), they will be resumed
first.

Alan Stern