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

[Sonny Rao <sonnyrao-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]

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