* Re: [PATCH v3] PM: add statistics debugfs file for suspend to ram
From: Rafael J. Wysocki @ 2011-08-09 20:02 UTC (permalink / raw)
To: yanmin_zhang
Cc: Brown, Len, Liu, ShuoX, Greg KH, linux-kernel@vger.kernel.org,
linux-pm@lists.linux-foundation.org
In-Reply-To: <1312868292.1743.15.camel@ymzhang>
On Tuesday, August 09, 2011, Yanmin Zhang wrote:
> On Tue, 2011-08-09 at 13:49 +0900, MyungJoo Ham wrote:
> > On Tue, Aug 9, 2011 at 11:27 AM, Liu, ShuoX <shuox.liu@intel.com> wrote:
> > > From: ShuoX Liu <shuox.liu@intel.com>
> > >
> > > Record S3 failure time about each reason and the latest two failed
> > > devices' names in S3 progress.
> > > We can check it through 'suspend_stats' entry in debugfs.
> > >
> > > The motivation of the patch:
> > >
> > > We are enabling power features on Medfield. Comparing with PC/notebook,
> > > a mobile enters/exits suspend-2-ram (we call it s3 on Medfield) far
> > > more frequently. If it can't enter suspend-2-ram in time, the power
> > > might be used up soon.
> > >
> > > We often find sometimes, a device suspend fails. Then, system retries
> > > s3 over and over again. As display is off, testers and developers
> > > don't know what happens.
> > >
> > > Some testers and developers complain they don't know if system
> > > tries suspend-2-ram, and what device fails to suspend. They need
> > > such info for a quick check. The patch adds suspend_stats under
> > > debugfs for users to check suspend to RAM statistics quickly.
> > >
> > > If not using this patch, we have other methods to get info about
> > > what device fails. One is to turn on CONFIG_PM_DEBUG, but users
> > > would get too much info and testers need recompile the system.
> > >
> > > In addition, dynamic debug is another good tool to dump debug info.
> > > But it still doesn't match our utilization scenario closely.
> > > 1) user need write a user space parser to process the syslog output;
> > > 2) Our testing scenario is we leave the mobile for at least hours.
> > > Then, check its status. No serial console available during the
> > > testing. One is because console would be suspended, and the other
> > > is serial console connecting with spi or HSU devices would consume
> > > power. These devices are powered off at suspend-2-ram.
> > >
> > > Thank Greg and Rafael for their great comments.
> > >
> > > Change Log:
> > > V3: Change some programming styles.
> > > V2:
> > > 1) Change the sysfs entry to debugfs.
> > > 2) Add some explanation in document file.
> > >
> > > Contribution:
> > > yanmin_zhang@linux.intel.com
> > >
> > > Signed-off-by: ShuoX Liu <shuox.liu@intel.com>
> > > ---
> > > Documentation/power/basic-pm-debugging.txt | 19 +++++++++
> > > drivers/base/power/main.c | 35 ++++++++++++++--
> > > include/linux/suspend.h | 16 +++++++
> > > kernel/power/main.c | 59 ++++++++++++++++++++++++++++
> > > kernel/power/suspend.c | 15 ++++++-
> > > 5 files changed, 136 insertions(+), 8 deletions(-)
> > >
> >
> > This patch looks very helpful. Great! :)
> >
> > Anyway, how about including the error value induced by the device
> > along with the device name?
> > For example,
> >
> > failed_devs:
> > last_failed: alarm -5
> > adc -10
> >
> > or
> >
> > failed_devs:
> > last_failed: alarm
> > adc
> > last_errorno: -5
> > -10
> >
>
> > Or, even, we may consider including the failed step:
> > failed_devs:
> > last_failed: alarm
> > adc
> > last_errorno: -5
> > -10
> > last_step: suspend
> > suspend_noirq
> >
> It's a good idea! We would change it to output like above format.
OK, I'm expecting a new version of the patch.
Thanks,
Rafael
^ permalink raw reply
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image.
From: Rafael J. Wysocki @ 2011-08-09 19:56 UTC (permalink / raw)
To: Martin Schwidefsky; +Cc: linux-s390, linux-pm, Jiri Slaby, linux-kernel
In-Reply-To: <20110809174524.45cd496d@mschwide>
On Tuesday, August 09, 2011, Martin Schwidefsky wrote:
> On Fri, 29 Jul 2011 00:01:14 +0200
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>
> > Sorry for the extreme delay.
>
> No big deal, I have been busy with other things anyway.
>
> > Having reconsidered things I think the code under the #ifdef above
> > should really go to arch/s390.
>
> Ok, that is reasonable.
>
> > Now, for the purpose of exporting the headers I'd introduce
> > CONFIG_ARCH_SAVE_PAGE_KEYS and make S390 do
> >
> > select ARCH_SAVE_PAGE_KEYS if HIBERNATION
> >
> > and I'd put a #ifdef depending on that into include/linux/suspend.h.
> >
> > Apart from this, I have only one complaint, which is that the kerneldoc
> > comments should follow the standard (the other comments in snapshot.c don't,
> > but that's a matter for a separate patch).
>
> Sounds good. I will come up with new patches for this and resend them
> for review. Might be one or two weeks though, currently conferencing in
> Orlando..
OK, thanks!
^ permalink raw reply
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image.
From: Martin Schwidefsky @ 2011-08-09 15:45 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-s390, linux-pm, Jiri Slaby, linux-kernel
In-Reply-To: <201107290001.14465.rjw@sisk.pl>
On Fri, 29 Jul 2011 00:01:14 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> Sorry for the extreme delay.
No big deal, I have been busy with other things anyway.
> Having reconsidered things I think the code under the #ifdef above
> should really go to arch/s390.
Ok, that is reasonable.
> Now, for the purpose of exporting the headers I'd introduce
> CONFIG_ARCH_SAVE_PAGE_KEYS and make S390 do
>
> select ARCH_SAVE_PAGE_KEYS if HIBERNATION
>
> and I'd put a #ifdef depending on that into include/linux/suspend.h.
>
> Apart from this, I have only one complaint, which is that the kerneldoc
> comments should follow the standard (the other comments in snapshot.c don't,
> but that's a matter for a separate patch).
Sounds good. I will come up with new patches for this and resend them
for review. Might be one or two weeks though, currently conferencing in
Orlando..
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
^ permalink raw reply
* Re: Crash when suspending Lenovo T510 laptop (2.6.39.3)
From: Alan Stern @ 2011-08-09 14:23 UTC (permalink / raw)
To: Jesper Juhl; +Cc: Len Brown, linux-pm, linux-kernel
In-Reply-To: <alpine.LNX.2.00.1108091426090.17636@swampdragon.chaosbits.net>
On Tue, 9 Aug 2011, Jesper Juhl wrote:
> Just happened again, this time with 3.0.1 :
>
> [30377.372749] ------------[ cut here ]------------
> [30377.372779] WARNING: at drivers/net/e1000e/netdev.c:5235 __e1000_shutdown+0x43f/0x4c0 [e1000e]()
> [30377.372783] Hardware name: 4384GJG
> [30377.372786] Modules linked in: vboxnetadp vboxnetflt vboxdrv appletalk ipx p8022 psnap llc p8023 ipv6 snd_hda_codec_hdmi snd_hda_codec_conexant uvcvideo videodev media v4l2_compat_ioctl32 snd_hda_intel snd_hda_codec snd_hwdep snd_pcm cpufreq_ondemand serio_raw arc4 snd_timer snd_page_alloc psmouse i915 drm_kms_helper sg msr drm acpi_cpufreq iwlagn i2c_algo_bit mac80211 freq_table video evdev e1000e firewire_ohci i2c_i801 battery mxm_wmi i2c_core cfg80211 mei(C) tpm_tis firewire_core iTCO_wdt ac iTCO_vendor_support tpm intel_agp crc_itu_t sdhci_pci sdhci mmc_core intel_ips intel_gtt tpm_bios thermal pcspkr wmi mperf processor button thinkpad_acpi rfkill snd soundcore nvram ext4 mbcache jbd2 crc16 sr_mod cdrom sd_mod usbhid hid ahci libahci libata ehci_hcd scsi_mod usbcore [last unloade
d: vboxdrv]
> [30377.372876] Pid: 17558, comm: kworker/1:0 Tainted: G C 3.0-ARCH #1
> [30377.372879] Call Trace:
> [30377.372895] [<ffffffff8105c7bf>] warn_slowpath_common+0x7f/0xc0
> [30377.372898] [<ffffffff8105c81a>] warn_slowpath_null+0x1a/0x20
> [30377.372902] [<ffffffffa02603cf>] __e1000_shutdown+0x43f/0x4c0 [e1000e]
> [30377.372906] [<ffffffffa0260489>] e1000_runtime_suspend+0x39/0x50 [e1000e]
> [30377.372911] [<ffffffff81240b9d>] pci_pm_runtime_suspend+0x4d/0x100
It seems to me that this is a bug in the e1000e driver. Therefore you
should CC: the maintainer for that driver; that's who is most likely to
be able to fix it.
Alan Stern
^ permalink raw reply
* Re: Crash when suspending Lenovo T510 laptop (2.6.39.3)
From: Jesper Juhl @ 2011-08-09 12:27 UTC (permalink / raw)
To: Alan Stern; +Cc: Len Brown, linux-pm, linux-kernel
In-Reply-To: <alpine.LNX.2.00.1108061049510.4163@swampdragon.chaosbits.net>
On Sat, 6 Aug 2011, Jesper Juhl wrote:
> On Fri, 5 Aug 2011, Alan Stern wrote:
>
> > On Fri, 5 Aug 2011, Rafael J. Wysocki wrote:
> >
> > > > You're right, sorry. System suspend was not involved, so the problem
> > > > was triggerend by runtime suspend alone, resulting from unplugging the
> > > > Ethernet cable.
> > > >
> > > > It looks like the crash started in pci_disable_msi().
> > >
> > > OTOH, I'm not sure how pci_legacy_suspend_late() was called.
> > > In theory it is only called by the system suspend code and it surely
> > > is not called for e1000e.
> >
> > The ? indicates that pci_legacy_suspend_late() wasn't necessarily on
> > the task's call stack; it may simply have been a leftover pointer from
> > some other operation, sitting somewhere in the middle of the stack
> > page.
> >
>
> Not sure if it helps, but I've been trying to reproduce the crash and
> although I've not succeeded doing that yet, I've got something else.
>
> While trying to reproduce by unplugging/replugging ethernet and
> resuming/suspending a lot I found the following in dmesg after one of the
> resumes - the system seems to be working fine though and network works
> as well, so it's not the same complete crash but it looks
> potentially related :
>
> [ 6564.921192] e1000e 0000:00:19.0: restoring config space at offset 0x1 (was 0x100103, writing 0x100107)
> [ 6564.921222] ehci_hcd 0000:00:1a.0: restoring config space at offset 0xf (was 0x400, writing 0x40b)
> [ 6564.921240] ehci_hcd 0000:00:1a.0: restoring config space at offset 0x4 (was 0x0, writing 0xf2828000)
> [ 6564.921247] ehci_hcd 0000:00:1a.0: restoring config space at offset 0x1 (was 0x2900000, writing 0x2900102)
> [ 6564.937545] ehci_hcd 0000:00:1a.0: power state changed by ACPI to D0
> [ 6564.937554] ehci_hcd 0000:00:1a.0: power state changed by ACPI to D0
> [ 6564.937708] pcieport 0000:00:1c.1: restoring config space at offset 0x7 (was 0xf0, writing 0x200000f0)
> [ 6564.937805] pcieport 0000:00:1c.4: restoring config space at offset 0xf (was 0x100, writing 0x4010b)
> [ 6564.937815] pcieport 0000:00:1c.4: restoring config space at offset 0x9 (was 0x10001, writing 0x1fff1)
> [ 6564.937820] pcieport 0000:00:1c.4: restoring config space at offset 0x8 (was 0x0, writing 0xf250f250)
> [ 6564.937824] pcieport 0000:00:1c.4: restoring config space at offset 0x7 (was 0x20000000, writing 0x200000f0)
> [ 6564.937832] pcieport 0000:00:1c.4: restoring config space at offset 0x3 (was 0x810000, writing 0x810010)
> [ 6564.937838] pcieport 0000:00:1c.4: restoring config space at offset 0x1 (was 0x100000, writing 0x100107)
> [ 6564.937872] ehci_hcd 0000:00:1d.0: restoring config space at offset 0xf (was 0x400, writing 0x40b)
> [ 6564.937890] ehci_hcd 0000:00:1d.0: restoring config space at offset 0x4 (was 0x0, writing 0xf2828400)
> [ 6564.937897] ehci_hcd 0000:00:1d.0: restoring config space at offset 0x1 (was 0x2900000, writing 0x2900102)
> [ 6564.937915] ehci_hcd 0000:00:1d.0: power state changed by ACPI to D0
> [ 6564.937918] ehci_hcd 0000:00:1d.0: power state changed by ACPI to D0
> [ 6564.938019] ahci 0000:00:1f.2: restoring config space at offset 0x1 (was 0x2b00007, writing 0x2b00407)
> [ 6564.938085] intel ips 0000:00:1f.6: restoring config space at offset 0xf (was 0x400, writing 0x40b)
> [ 6564.938107] intel ips 0000:00:1f.6: restoring config space at offset 0x1 (was 0x100000, writing 0x100002)
> [ 6564.938674] PM: early resume of devices complete after 17.784 msecs[ 6564.939048] sdhci-pci 0000:0d:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
> [ 6564.939053] iwlagn 0000:03:00.0: RF_KILL bit toggled to disable radio.
> [ 6564.939118] e1000e 0000:00:19.0: irq 41 for MSI/MSI-X
> [ 6564.942219] ehci_hcd 0000:00:1a.0: power state changed by ACPI to D0
> [ 6564.942229] pci 0000:00:1e.0: setting latency timer to 64
> [ 6564.942236] ehci_hcd 0000:00:1d.0: power state changed by ACPI to D0
> [ 6564.942246] HDA Intel 0000:00:1b.0: PCI INT B -> GSI 17 (level, low) -> IRQ 17
> [ 6564.942253] HDA Intel 0000:00:1b.0: setting latency timer to 64
> [ 6564.942261] ehci_hcd 0000:00:1a.0: power state changed by ACPI to D0
> [ 6564.942265] ahci 0000:00:1f.2: setting latency timer to 64
> [ 6564.942276] ehci_hcd 0000:00:1a.0: PCI INT D -> GSI 23 (level, low) -> IRQ 23
> [ 6564.942344] ehci_hcd 0000:00:1a.0: setting latency timer to 64
> [ 6564.942360] HDA Intel 0000:00:1b.0: irq 44 for MSI/MSI-X
> [ 6564.942436] ehci_hcd 0000:00:1d.0: power state changed by ACPI to D0
> [ 6564.942444] ehci_hcd 0000:00:1d.0: PCI INT D -> GSI 19 (level, low) -> IRQ 19
> [ 6564.942452] ehci_hcd 0000:00:1d.0: setting latency timer to 64
> [ 6564.942456] sd 0:0:0:0: [sda] Starting disk
> [ 6564.942458] i915 0000:00:02.0: power state changed by ACPI to D0
> [ 6564.942463] i915 0000:00:02.0: power state changed by ACPI to D0
> [ 6564.942466] i915 0000:00:02.0: setting latency timer to 64
> [ 6564.997449] firewire_ohci 0000:0d:00.3: irq 45 for MSI/MSI-X
> [ 6564.997785] firewire_core: skipped bus generations, destroying all nodes
> [ 6565.123881] usb 2-1.1: reset full speed USB device number 3 using ehci_hcd
> [ 6565.256738] ata6: SATA link down (SStatus 0 SControl 300)
> [ 6565.256782] ata2: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> [ 6565.259348] ata2.00: ACPI cmd e3/00:1f:00:00:00:a0 (IDLE) succeeded
> [ 6565.259563] ata2.00: ACPI cmd e3/00:02:00:00:00:a0 (IDLE) succeeded
> [ 6565.263747] ata2.00: ACPI cmd e3/00:1f:00:00:00:a0 (IDLE) succeeded
> [ 6565.263990] ata2.00: ACPI cmd e3/00:02:00:00:00:a0 (IDLE) succeeded
> [ 6565.265544] ata2.00: configured for UDMA/66
> [ 6565.276723] ata5: SATA link down (SStatus 0 SControl 300)
> [ 6565.439619] sdhci-pci 0000:0d:00.0: Will use DMA mode even though HW doesn't fully claim to support it.
> [ 6565.439631] sdhci-pci 0000:0d:00.0: setting latency timer to 64
> [ 6565.463009] usb 1-1.2: reset low speed USB device number 3 using ehci_hcd
> [ 6565.496163] firewire_core: rediscovered device fw0
> [ 6565.802321] usb 1-1.3: reset full speed USB device number 4 using ehci_hcd
> [ 6565.951782] usb 1-1.6: reset high speed USB device number 6 using ehci_hcd
> [ 6566.014871] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
> [ 6566.021871] ata1.00: ACPI cmd ef/02:00:00:00:00:a0 (SET FEATURES) succeeded
> [ 6566.021877] ata1.00: ACPI cmd f5/00:00:00:00:00:a0 (SECURITY FREEZE LOCK) filtered out
> [ 6566.022178] ata1.00: ACPI cmd ef/5f:00:00:00:00:a0 (SET FEATURES) succeeded
> [ 6566.022183] ata1.00: ACPI cmd ef/10:03:00:00:00:a0 (SET FEATURES) filtered out
> [ 6566.024813] ata1.00: ACPI cmd ef/02:00:00:00:00:a0 (SET FEATURES) succeeded
> [ 6566.024819] ata1.00: ACPI cmd f5/00:00:00:00:00:a0 (SECURITY FREEZE LOCK) filtered out
> [ 6566.025080] ata1.00: ACPI cmd ef/5f:00:00:00:00:a0 (SET FEATURES) succeeded
> [ 6566.025085] ata1.00: ACPI cmd ef/10:03:00:00:00:a0 (SET FEATURES) filtered out
> [ 6566.026327] ata1.00: configured for UDMA/100
> [ 6566.104911] usb 2-1.1.4: reset full speed USB device number 4 using ehci_hcd
> [ 6566.190354] PM: resume of devices complete after 1254.565 msecs
> [ 6566.191321] PM: Finishing wakeup.
> [ 6566.191323] Restarting tasks ... done.
> [ 6566.199706] video LNXVIDEO:00: Restoring backlight state
> [ 6566.661832] e1000e: eth0 NIC Link is Up 100 Mbps Full Duplex, Flow Control: Rx
> [ 6566.661839] e1000e 0000:00:19.0: eth0: 10/100 speed: disabling TSO
> [ 6566.662160] ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> [ 6568.525731] e1000e 0000:00:19.0: PME# enabled
> [ 6568.551863] e1000e 0000:00:19.0: BAR 0: set to [mem 0xf2600000-0xf261ffff] (PCI address [0xf2600000-0xf261ffff])
> [ 6568.551876] e1000e 0000:00:19.0: BAR 1: set to [mem 0xf2625000-0xf2625fff] (PCI address [0xf2625000-0xf2625fff])
> [ 6568.551886] e1000e 0000:00:19.0: BAR 2: set to [io 0x1820-0x183f] (PCI address [0x1820-0x183f])
> [ 6568.551909] e1000e 0000:00:19.0: restoring config space at offset 0xf (was 0x100, writing 0x10b)
> [ 6568.551936] e1000e 0000:00:19.0: restoring config space at offset 0x1 (was 0x100000, writing 0x100107)
> [ 6568.551999] e1000e 0000:00:19.0: PME# disabled
> [ 6568.645183] e1000e 0000:00:19.0: irq 41 for MSI/MSI-X
> [ 6568.698271] e1000e 0000:00:19.0: irq 41 for MSI/MSI-X
> [ 6568.699163] ADDRCONF(NETDEV_UP): eth0: link is not ready
> [ 6568.814811] e1000e 0000:00:19.0: PME# enabled
> [ 6568.844535] e1000e 0000:00:19.0: BAR 0: set to [mem 0xf2600000-0xf261ffff] (PCI address [0xf2600000-0xf261ffff])
> [ 6568.844548] e1000e 0000:00:19.0: BAR 1: set to [mem 0xf2625000-0xf2625fff] (PCI address [0xf2625000-0xf2625fff])
> [ 6568.844558] e1000e 0000:00:19.0: BAR 2: set to [io 0x1820-0x183f] (PCI address [0x1820-0x183f])
> [ 6568.844580] e1000e 0000:00:19.0: restoring config space at offset 0xf (was 0x100, writing 0x10b)
> [ 6568.844607] e1000e 0000:00:19.0: restoring config space at offset 0x1 (was 0x100000, writing 0x100507)
> [ 6568.844668] e1000e 0000:00:19.0: PME# disabled
> [ 6568.937809] e1000e 0000:00:19.0: irq 41 for MSI/MSI-X
> [ 6568.990871] e1000e 0000:00:19.0: irq 41 for MSI/MSI-X
> [ 6568.991756] ADDRCONF(NETDEV_UP): eth0: link is not ready
> [ 6569.991674] ------------[ cut here ]------------
> [ 6569.991694] WARNING: at drivers/net/e1000e/netdev.c:5221 __e1000_shutdown+0x43f/0x4c0 [e1000e]()
> [ 6569.991698] Hardware name: 4384GJG
> [ 6569.991699] Modules linked in: snd_seq snd_seq_device aesni_intel cryptd aes_x86_64 aes_generic ipv6 uvcvideo videodev media v4l2_compat_ioctl32 usbhid hid snd_hda_codec_hdmi btusb bluetooth snd_hda_codec_conexant arc4 ecb snd_hda_intel snd_hda_codec snd_hwdep e1000e i915 snd_pcm iwlagn ehci_hcd snd_timer drm_kms_helper usbcore snd_page_alloc sdhci_pci sdhci pcspkr mmc_core sg drm vboxnetadp vboxnetflt evdev tpm_tis i2c_algo_bit mac80211 serio_raw psmouse iTCO_wdt vboxdrv firewire_ohci firewire_core i2c_i801 crc_itu_t i2c_core iTCO_vendor_support cfg80211 battery thermal intel_ips wmi tpm intel_agp video ac tpm_bios button intel_gtt cpufreq_ondemand msr acpi_cpufreq freq_table processor mperf thinkpad_acpi rfkill snd soundcore nvram ext4 mbcache jbd2 crc16 sr_mod sd_mod cdrom ahci lib
ahci libata scsi_mod
> [ 6569.991778] Pid: 7847, comm: kworker/1:0 Not tainted 2.6.39-ARCH #1
> [ 6569.991781] Call Trace:
> [ 6569.991792] [<ffffffff8105b90f>] warn_slowpath_common+0x7f/0xc0
> [ 6569.991797] [<ffffffff8105b96a>] warn_slowpath_null+0x1a/0x20
> [ 6569.991804] [<ffffffffa064df7f>] __e1000_shutdown+0x43f/0x4c0 [e1000e]
> [ 6569.991816] [<ffffffff81012fc9>] ? sched_clock+0x9/0x10
> [ 6569.991819] [<ffffffffa064e039>] e1000_runtime_suspend+0x39/0x50 [e1000e]
> [ 6569.991823] [<ffffffff8100a6cf>] ? __switch_to+0xbf/0x2f0
> [ 6569.991828] [<ffffffff812392dd>] pci_pm_runtime_suspend+0x4d/0x100
> [ 6569.991830] [<ffffffff81239290>] ? pci_legacy_suspend_late+0x100/0x100
> [ 6569.991834] [<ffffffff812e7d52>] rpm_callback+0x42/0x80
> [ 6569.991837] [<ffffffff813e4faf>] ? schedule+0x33f/0xad0
> [ 6569.991839] [<ffffffff812e82fa>] rpm_suspend+0x1da/0x3f0
> [ 6569.991843] [<ffffffff8107701c>] ? queue_delayed_work_on+0xbc/0x150
> [ 6569.991846] [<ffffffff812e9380>] ? pm_schedule_suspend+0xe0/0xe0
> [ 6569.991848] [<ffffffff812e941a>] pm_runtime_work+0x9a/0xc0
> [ 6569.991850] [<ffffffff81077f6e>] process_one_work+0x11e/0x4c0
> [ 6569.991852] [<ffffffff810788ff>] worker_thread+0x15f/0x350
> [ 6569.991854] [<ffffffff810787a0>] ? manage_workers.isra.29+0x230/0x230
> [ 6569.991857] [<ffffffff8107d6ec>] kthread+0x8c/0xa0
> [ 6569.991860] [<ffffffff813e9fe4>] kernel_thread_helper+0x4/0x10
> [ 6569.991862] [<ffffffff8107d660>] ? kthread_worker_fn+0x190/0x190
> [ 6569.991864] [<ffffffff813e9fe0>] ? gs_change+0x13/0x13
> [ 6569.991865] ---[ end trace c54917cde5791882 ]---
> [ 6570.221032] e1000e 0000:00:19.0: PME# enabled
> [ 6570.464118] EXT4-fs (sda3): re-mounted. Opts: commit=0
> [ 6570.466279] EXT4-fs (sda1): re-mounted. Opts: commit=0
> [ 6570.644945] EXT4-fs (sda4): re-mounted. Opts: commit=0
> [ 6571.823780] e1000e 0000:00:19.0: BAR 0: set to [mem 0xf2600000-0xf261ffff] (PCI address [0xf2600000-0xf261ffff])
> [ 6571.823791] e1000e 0000:00:19.0: BAR 1: set to [mem 0xf2625000-0xf2625fff] (PCI address [0xf2625000-0xf2625fff])
> [ 6571.823800] e1000e 0000:00:19.0: BAR 2: set to [io 0x1820-0x183f] (PCI address [0x1820-0x183f])
> [ 6571.823821] e1000e 0000:00:19.0: restoring config space at offset 0xf (was 0x100, writing 0x10b)
> [ 6571.823847] e1000e 0000:00:19.0: restoring config space at offset 0x1 (was 0x100000, writing 0x100107)
> [ 6571.823885] e1000e 0000:00:19.0: PME# disabled
> [ 6571.823965] e1000e 0000:00:19.0: irq 41 for MSI/MSI-X
> [ 6571.826445] e1000e 0000:00:19.0: eth0: PHY Wakeup cause - Link Status Change
> [ 6573.521494] e1000e: eth0 NIC Link is Up 100 Mbps Full Duplex, Flow Control: Rx
> [ 6573.521500] e1000e 0000:00:19.0: eth0: 10/100 speed: disabling TSO
> [ 6573.521814] ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
>
> The kernel is still the same 2.6.39.3
>
> Hope that helps.
>
> I'll keep trying to reproduce the crash - will probably build a 3.0.1 with
> lots of debug options enabled and try to reproduce with that.
>
>
Just happened again, this time with 3.0.1 :
[30377.372749] ------------[ cut here ]------------
[30377.372779] WARNING: at drivers/net/e1000e/netdev.c:5235 __e1000_shutdown+0x43f/0x4c0 [e1000e]()
[30377.372783] Hardware name: 4384GJG
[30377.372786] Modules linked in: vboxnetadp vboxnetflt vboxdrv appletalk ipx p8022 psnap llc p8023 ipv6 snd_hda_codec_hdmi snd_hda_codec_conexant uvcvideo videodev media v4l2_compat_ioctl32 snd_hda_intel snd_hda_codec snd_hwdep snd_pcm cpufreq_ondemand serio_raw arc4 snd_timer snd_page_alloc psmouse i915 drm_kms_helper sg msr drm acpi_cpufreq iwlagn i2c_algo_bit mac80211 freq_table video evdev e1000e firewire_ohci i2c_i801 battery mxm_wmi i2c_core cfg80211 mei(C) tpm_tis firewire_core iTCO_wdt ac iTCO_vendor_support tpm intel_agp crc_itu_t sdhci_pci sdhci mmc_core intel_ips intel_gtt tpm_bios thermal pcspkr wmi mperf processor button thinkpad_acpi rfkill snd soundcore nvram ext4 mbcache jbd2 crc16 sr_mod cdrom sd_mod usbhid hid ahci libahci libata ehci_hcd scsi_mod usbcore [last unloaded:
vboxdrv]
[30377.372876] Pid: 17558, comm: kworker/1:0 Tainted: G C 3.0-ARCH #1
[30377.372879] Call Trace:
[30377.372895] [<ffffffff8105c7bf>] warn_slowpath_common+0x7f/0xc0
[30377.372898] [<ffffffff8105c81a>] warn_slowpath_null+0x1a/0x20
[30377.372902] [<ffffffffa02603cf>] __e1000_shutdown+0x43f/0x4c0 [e1000e]
[30377.372906] [<ffffffffa0260489>] e1000_runtime_suspend+0x39/0x50 [e1000e]
[30377.372911] [<ffffffff81240b9d>] pci_pm_runtime_suspend+0x4d/0x100
[30377.372913] [<ffffffff81240b50>] ? pci_legacy_suspend_late+0x100/0x100
[30377.372918] [<ffffffff812f26e2>] rpm_callback+0x42/0x80
[30377.372920] [<ffffffff812f2c7c>] rpm_suspend+0x1cc/0x3e0
[30377.372922] [<ffffffff812f3c80>] ? pm_schedule_suspend+0xe0/0xe0
[30377.372925] [<ffffffff812f3d1a>] pm_runtime_work+0x9a/0xc0
[30377.372929] [<ffffffff81079576>] process_one_work+0x116/0x4d0
[30377.372932] [<ffffffff81079f1f>] worker_thread+0x15f/0x350
[30377.372934] [<ffffffff81079dc0>] ? manage_workers.isra.29+0x230/0x230
[30377.372936] [<ffffffff8107ed2c>] kthread+0x8c/0xa0
[30377.372940] [<ffffffff813f4ea4>] kernel_thread_helper+0x4/0x10
[30377.372942] [<ffffffff8107eca0>] ? kthread_worker_fn+0x190/0x190
[30377.372947] [<ffffffff813f4ea0>] ? gs_change+0x13/0x13
[30377.372949] ---[ end trace 91bf31e3c70d5224 ]---
[30377.672896] e1000e 0000:00:19.0: PME# enabled
[30379.316301] e1000e 0000:00:19.0: BAR 0: set to [mem 0xf2600000-0xf261ffff] (PCI address [0xf2600000-0xf261ffff])
[30379.316308] e1000e 0000:00:19.0: BAR 1: set to [mem 0xf2625000-0xf2625fff] (PCI address [0xf2625000-0xf2625fff])
[30379.316313] e1000e 0000:00:19.0: BAR 2: set to [io 0x1820-0x183f] (PCI address [0x1820-0x183f])
[30379.316337] e1000e 0000:00:19.0: restoring config space at offset 0xf (was 0x100, writing 0x10b)
[30379.316358] e1000e 0000:00:19.0: restoring config space at offset 0x1 (was 0x100000, writing 0x100107)
[30379.316391] e1000e 0000:00:19.0: PME# disabled
[30379.316463] e1000e 0000:00:19.0: irq 41 for MSI/MSI-X
[30379.318956] e1000e 0000:00:19.0: eth0: PHY Wakeup cause - Link Status Change
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply
* Re: [PATCH v3] PM: add statistics debugfs file for suspend to ram
From: Yanmin Zhang @ 2011-08-09 5:38 UTC (permalink / raw)
To: MyungJoo Ham
Cc: Liu, ShuoX, linux-pm@lists.linux-foundation.org, Greg KH,
linux-kernel@vger.kernel.org, Brown, Len
In-Reply-To: <CAJ0PZbQ4Bk7Up9texX2hU8NP583da7UP9Hk3GRHE6-VKsg69kA@mail.gmail.com>
On Tue, 2011-08-09 at 13:49 +0900, MyungJoo Ham wrote:
> On Tue, Aug 9, 2011 at 11:27 AM, Liu, ShuoX <shuox.liu@intel.com> wrote:
> > From: ShuoX Liu <shuox.liu@intel.com>
> >
> > Record S3 failure time about each reason and the latest two failed
> > devices' names in S3 progress.
> > We can check it through 'suspend_stats' entry in debugfs.
> >
> > The motivation of the patch:
> >
> > We are enabling power features on Medfield. Comparing with PC/notebook,
> > a mobile enters/exits suspend-2-ram (we call it s3 on Medfield) far
> > more frequently. If it can't enter suspend-2-ram in time, the power
> > might be used up soon.
> >
> > We often find sometimes, a device suspend fails. Then, system retries
> > s3 over and over again. As display is off, testers and developers
> > don't know what happens.
> >
> > Some testers and developers complain they don't know if system
> > tries suspend-2-ram, and what device fails to suspend. They need
> > such info for a quick check. The patch adds suspend_stats under
> > debugfs for users to check suspend to RAM statistics quickly.
> >
> > If not using this patch, we have other methods to get info about
> > what device fails. One is to turn on CONFIG_PM_DEBUG, but users
> > would get too much info and testers need recompile the system.
> >
> > In addition, dynamic debug is another good tool to dump debug info.
> > But it still doesn't match our utilization scenario closely.
> > 1) user need write a user space parser to process the syslog output;
> > 2) Our testing scenario is we leave the mobile for at least hours.
> > Then, check its status. No serial console available during the
> > testing. One is because console would be suspended, and the other
> > is serial console connecting with spi or HSU devices would consume
> > power. These devices are powered off at suspend-2-ram.
> >
> > Thank Greg and Rafael for their great comments.
> >
> > Change Log:
> > V3: Change some programming styles.
> > V2:
> > 1) Change the sysfs entry to debugfs.
> > 2) Add some explanation in document file.
> >
> > Contribution:
> > yanmin_zhang@linux.intel.com
> >
> > Signed-off-by: ShuoX Liu <shuox.liu@intel.com>
> > ---
> > Documentation/power/basic-pm-debugging.txt | 19 +++++++++
> > drivers/base/power/main.c | 35 ++++++++++++++--
> > include/linux/suspend.h | 16 +++++++
> > kernel/power/main.c | 59 ++++++++++++++++++++++++++++
> > kernel/power/suspend.c | 15 ++++++-
> > 5 files changed, 136 insertions(+), 8 deletions(-)
> >
>
> This patch looks very helpful. Great! :)
>
> Anyway, how about including the error value induced by the device
> along with the device name?
> For example,
>
> failed_devs:
> last_failed: alarm -5
> adc -10
>
> or
>
> failed_devs:
> last_failed: alarm
> adc
> last_errorno: -5
> -10
>
> Or, even, we may consider including the failed step:
> failed_devs:
> last_failed: alarm
> adc
> last_errorno: -5
> -10
> last_step: suspend
> suspend_noirq
>
It's a good idea! We would change it to output like above format.
Thanks MyunJoo.
^ permalink raw reply
* Re: [PATCH v4 0/3] DEVFREQ, DVFS framework for non-CPU devices
From: MyungJoo Ham @ 2011-08-09 5:27 UTC (permalink / raw)
To: Turquette, Mike
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, Thomas Gleixner,
linux-pm
In-Reply-To: <CAJOA=zOi9HzM7M72C+QS6iLcYD_xDeQkEZ3fYpVo_v4f4mfAyQ@mail.gmail.com>
On Tue, Aug 9, 2011 at 4:13 AM, Turquette, Mike <mturquette@ti.com> wrote:
> On Thu, Aug 4, 2011 at 11:18 PM, MyungJoo Ham <myungjoo.ham@gmail.com> wrote:
>> On Fri, Aug 5, 2011 at 6:59 AM, Turquette, Mike <mturquette@ti.com> wrote:
>>> On Thu, Aug 4, 2011 at 1:15 AM, MyungJoo Ham <myungjoo.ham@gmail.com> wrote:
>>>>
>>>> Ok.. I see.
>>>>
>>>> Now, I can agree with you that tickle is subset of QoS request.
>>>>
>>>> As long as we have QoS request feature on devices with either OPP or
>>>> DEVFREQ, tickling is not needed.
>>>>
>>>> I'll remove tickle in the next revision (along with some bugfixes for
>>>> bugs found recently).
>>>>
>>>>
>>>> Anyway, it appears that clock-rate-wise QoS request may be dealt at
>>>> OPP so that the OPPs meeting the QoS requests w/ frequency or voltage
>>>> specifications are enabled and returned with opp_find_* functions.
>>>> Maybe we will need to separate enable/disable by
>>>> opp_enable()/opp_disable() from enable/disable by QoS requests so that
>>>> the two may have different semantics. Then, QoS requests cannot
>>>> override opp_disable and opp_enable cannot override QoS requests. This
>>>> way, any clock-setting code properly based on OPP (including any
>>>> customized devfreq governors) cannot violate QoS requests.
>>>
>>> If devfreq used the QoS API in it's ->target() call then this would
>>> not be an issue, and further illustrates the idea of devfreq simply
>>> being a policy into a proper QoS API.
>>>
>>> Regards,
>>> Mike
>>>
>>
>> Yes, if we choose an OPP that meets the PM-QoS requests before calling
>> ->target() in devfreq_do(), there wouldn't be an issue.
>>
>> However, if a device is using DEVFREQ, it also means the device has
>> OPPs (mandatory for DEVFREQ). If the device is using PM QoS as well as
>> OPP, I guess the correctly implemented device driver needs to call
>> opp_enable() and opp_disable() according to PM QoS's update_target()
>> calls through the PM QoS notifier cb.
>>
>> Then, for such drivers, DEVFREQ automatically meets PM QoS requests
>> without any modification; as long as QoS meeting OPPs are enabled at
>> the device driver's PM QoS callback, there is no QoS issue.
>>
>> Therefore, now, it appears that neither OPP or DEVFREQ should be
>> allowed to directly touch PM QoS APIs, but, the device driver should
>> do so with notifier by simply calling opp-enable/disable if the
>> frequency is the key QoS "actuator".
>>
>> If we are going to let DEVFREQ handle its corresponding devices' PM
>> QoS APIs, it would mean that both device driver and its DEVFREQ codes
>> will be handling PM QoS API duplicated (or worse, inconsistently).
>
> I'm not getting too bogged down with the OPP specifics because I don't
> know if that interface is going to be used in the future, and I don't
> think that devfreq will need to know about OPPs once the DVFS QoS API
> exists. In that case, devfreq can just requests clock frequencies
> through the QoS API. I view devfreq's usage of the OPP library as
> temporary.
>
> The real issue here is that we don't want some weird feedback loop
> with device QoS requests and devfreq targets stepping on each other.
>
> One way to handle this is to partition QoS use in drivers away from
> devfreq usage. For example, if a GPU supports performance counters
> and can introspect its own usage, then it is a perfect candidate for
> devfreq; on the flip-side device drivers should *not* be allowed to
> put performance/qos constraints on this particular GPU, since we
> assume that the performance counters/devfreq governor will handle the
> whole job for us. Since this centralizes the decision-making for the
> GPU it is safe for the devfreq->target() call to use the QoS APIs for
> controlling the GPU, since no one else will. This avoids the feedback
> loop.
>
> On the other hand, if the GPU does not support performance counters
> then it should not use devfreq at all and rely 100% of QoS constraints
> from various sources: the GPU driver might request a high OPP every
> time work comes in, coupled with a timeout; if a QoS knob is exported
> to userspace then some OpenGL library might hold constraints through
> it; or some other kernel driver (video-related?) needs to use the GPU
> then it can hold a constraint through the QoS API.
>
> So there is a clear partition of QoS API usage between devices that
> support performance counters and ones that do not. We want to avoid a
> feedback loop here.
Such weird feedback loop may happen if the result frequency ranges
from QoS are inconsistent with those from DEVFREQ. However, if DEVFREQ
provides frequencies that do not violate QoS requirements, there would
be no such feedback loop. If they are consistent (DEVFREQ never gives
frequencies that do NOT satisfy QoS), DEVFREQ is transparent to PM
QoS.
For now, with OPPs, DEVFREQ will always select frequencies that
satisfy QoS requirements if the driver implementation is correct.
However, as you mentioned, if we assume that OPP is going to disappear
in the future kernel, then we will somehow need to design an interface
for device drivers to provide lists of available frequencies to
DEVFREQ and those lists are going to be runtime-update-able and where
QoS requirements are applied. As with the current DEVFREQ
implementation, if DEVFREQ chooses frequencies only from those enabled
by the driver (meeting the QoS requirements), things are same with the
cases with OPPs. The reason why QoS requirements are being interpreted
at device drivers, not at DEVFREQ, is that the device driver itself is
the only entity that can interpret QoS requests to frequencies
(latency/#ops/... --> Hz).
And, for the another point, making PM QoS disabled (or ignored?) for
devices with DEVFREQ, I guess that should be left for each device
driver's decision. Even when DVFS is used for a device, there are
cases where QoS requests are needed as well because DEVFREQ is based
on polling which incurs response time that is not acceptable for some
cases: e.g., a sudden user input at idle in a system with high-speed
displays. Besides, there could be another type of PM QoS requirements:
"under xxx MHz" in order to restrict (or as a response to) the
temperature or power consumption rate, which should affect DEVFREQ's
behavior. For battery-powered devices (laptops will want to do so as
well at userspace), I guess such features will be soon needed; at
least, we are working on it.
Cheers!
MyungJoo
>
> Regards,
> Mike
>
>> Cheers,
>> MyungJoo
>>
>>>> How about this concept of getting QoS requests associated with clock rates?
>>>>
>>>>
>>>>
>>>> Cheers!
>>>> MyungJoo.
>>>> --
>>>> MyungJoo Ham, Ph.D.
>>>> Mobile Software Platform Lab,
>>>> Digital Media and Communications (DMC) Business
>>>> Samsung Electronics
>>>> cell: 82-10-6714-2858
>>>> _______________________________________________
>>>> linux-pm mailing list
>>>> linux-pm@lists.linux-foundation.org
>>>> https://lists.linux-foundation.org/mailman/listinfo/linux-pm
>>>>
>>>
>>
>>
>>
>> --
>> MyungJoo Ham, Ph.D.
>> Mobile Software Platform Lab,
>> Digital Media and Communications (DMC) Business
>> Samsung Electronics
>> cell: 82-10-6714-2858
>>
>
--
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
^ permalink raw reply
* Re: [PATCH v3] PM: add statistics debugfs file for suspend to ram
From: MyungJoo Ham @ 2011-08-09 4:49 UTC (permalink / raw)
To: Liu, ShuoX
Cc: Brown, Len, linux-pm@lists.linux-foundation.org, Greg KH,
Yanmin_zhang@linux.intel.com, linux-kernel@vger.kernel.org
In-Reply-To: <6E3BC7F7C9A4BF4286DD4C043110F30B5B8057C306@shsmsx502.ccr.corp.intel.com>
On Tue, Aug 9, 2011 at 11:27 AM, Liu, ShuoX <shuox.liu@intel.com> wrote:
> From: ShuoX Liu <shuox.liu@intel.com>
>
> Record S3 failure time about each reason and the latest two failed
> devices' names in S3 progress.
> We can check it through 'suspend_stats' entry in debugfs.
>
> The motivation of the patch:
>
> We are enabling power features on Medfield. Comparing with PC/notebook,
> a mobile enters/exits suspend-2-ram (we call it s3 on Medfield) far
> more frequently. If it can't enter suspend-2-ram in time, the power
> might be used up soon.
>
> We often find sometimes, a device suspend fails. Then, system retries
> s3 over and over again. As display is off, testers and developers
> don't know what happens.
>
> Some testers and developers complain they don't know if system
> tries suspend-2-ram, and what device fails to suspend. They need
> such info for a quick check. The patch adds suspend_stats under
> debugfs for users to check suspend to RAM statistics quickly.
>
> If not using this patch, we have other methods to get info about
> what device fails. One is to turn on CONFIG_PM_DEBUG, but users
> would get too much info and testers need recompile the system.
>
> In addition, dynamic debug is another good tool to dump debug info.
> But it still doesn't match our utilization scenario closely.
> 1) user need write a user space parser to process the syslog output;
> 2) Our testing scenario is we leave the mobile for at least hours.
> Then, check its status. No serial console available during the
> testing. One is because console would be suspended, and the other
> is serial console connecting with spi or HSU devices would consume
> power. These devices are powered off at suspend-2-ram.
>
> Thank Greg and Rafael for their great comments.
>
> Change Log:
> V3: Change some programming styles.
> V2:
> 1) Change the sysfs entry to debugfs.
> 2) Add some explanation in document file.
>
> Contribution:
> yanmin_zhang@linux.intel.com
>
> Signed-off-by: ShuoX Liu <shuox.liu@intel.com>
> ---
> Documentation/power/basic-pm-debugging.txt | 19 +++++++++
> drivers/base/power/main.c | 35 ++++++++++++++--
> include/linux/suspend.h | 16 +++++++
> kernel/power/main.c | 59 ++++++++++++++++++++++++++++
> kernel/power/suspend.c | 15 ++++++-
> 5 files changed, 136 insertions(+), 8 deletions(-)
>
This patch looks very helpful. Great! :)
Anyway, how about including the error value induced by the device
along with the device name?
For example,
failed_devs:
last_failed: alarm -5
adc -10
or
failed_devs:
last_failed: alarm
adc
last_errorno: -5
-10
Or, even, we may consider including the failed step:
failed_devs:
last_failed: alarm
adc
last_errorno: -5
-10
last_step: suspend
suspend_noirq
Cheers!
MyungJoo
> diff --git a/Documentation/power/basic-pm-debugging.txt b/Documentation/power/basic-pm-debugging.txt
> index ddd7817..acbd3e0 100644
> --- a/Documentation/power/basic-pm-debugging.txt
> +++ b/Documentation/power/basic-pm-debugging.txt
> @@ -201,3 +201,22 @@ case, you may be able to search for failing drivers by following the procedure
> analogous to the one described in section 1. If you find some failing drivers,
> you will have to unload them every time before an STR transition (ie. before
> you run s2ram), and please report the problems with them.
> +
> +There is a debugfs entry which shows the suspend to RAM statistics. Here is an
> +example of its output.
> + # mount -t debugfs none /sys/kernel/debug
> + # cat /sys/kernel/debug/suspend_stats
> + success: 20
> + fail: 5
> + failed_freeze: 0
> + failed_prepare: 0
> + failed_suspend: 5
> + failed_suspend_noirq: 0
> + failed_resume: 0
> + failed_resume_noirq: 0
> + failed_devs:
> + last_failed: alarm
> + adc
> +Field success means the success number of suspend to RAM, and field fail means
> +the failure number. Others are the failure number of different steps of suspend
> +to RAM. suspend_stats just lists the last 2 failed devices.
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index a854591..111f86c 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -46,6 +46,7 @@ LIST_HEAD(dpm_prepared_list);
> LIST_HEAD(dpm_suspended_list);
> LIST_HEAD(dpm_noirq_list);
>
> +struct suspend_stats suspend_stats;
> static DEFINE_MUTEX(dpm_list_mtx);
> static pm_message_t pm_transition;
>
> @@ -180,6 +181,15 @@ static void initcall_debug_report(struct device *dev, ktime_t calltime,
> }
> }
>
> +static void dpm_save_dev_name(const char *name)
> +{
> + strlcpy(suspend_stats.failed_devs[suspend_stats.last_failed],
> + name,
> + sizeof(suspend_stats.failed_devs[0]));
> + suspend_stats.last_failed++;
> + suspend_stats.last_failed %= REC_FAILED_DEV_NUM;
> +}
> +
> /**
> * dpm_wait - Wait for a PM operation to complete.
> * @dev: Device to wait for.
> @@ -464,8 +474,11 @@ void dpm_resume_noirq(pm_message_t state)
> mutex_unlock(&dpm_list_mtx);
>
> error = device_resume_noirq(dev, state);
> - if (error)
> + if (error) {
> + suspend_stats.failed_resume_noirq++;
> + dpm_save_dev_name(dev_name(dev));
> pm_dev_err(dev, state, " early", error);
> + }
>
> mutex_lock(&dpm_list_mtx);
> put_device(dev);
> @@ -626,8 +639,11 @@ void dpm_resume(pm_message_t state)
> mutex_unlock(&dpm_list_mtx);
>
> error = device_resume(dev, state, false);
> - if (error)
> + if (error) {
> + suspend_stats.failed_resume++;
> + dpm_save_dev_name(dev_name(dev));
> pm_dev_err(dev, state, "", error);
> + }
>
> mutex_lock(&dpm_list_mtx);
> }
> @@ -802,6 +818,8 @@ int dpm_suspend_noirq(pm_message_t state)
> mutex_lock(&dpm_list_mtx);
> if (error) {
> pm_dev_err(dev, state, " late", error);
> + suspend_stats.failed_suspend_noirq++;
> + dpm_save_dev_name(dev_name(dev));
> put_device(dev);
> break;
> }
> @@ -923,8 +941,10 @@ static void async_suspend(void *data, async_cookie_t cookie)
> int error;
>
> error = __device_suspend(dev, pm_transition, true);
> - if (error)
> + if (error) {
> + dpm_save_dev_name(dev_name(dev));
> pm_dev_err(dev, pm_transition, " async", error);
> + }
>
> put_device(dev);
> }
> @@ -967,6 +987,7 @@ int dpm_suspend(pm_message_t state)
> mutex_lock(&dpm_list_mtx);
> if (error) {
> pm_dev_err(dev, state, "", error);
> + dpm_save_dev_name(dev_name(dev));
> put_device(dev);
> break;
> }
> @@ -980,7 +1001,9 @@ int dpm_suspend(pm_message_t state)
> async_synchronize_full();
> if (!error)
> error = async_error;
> - if (!error)
> + if (error)
> + suspend_stats.failed_suspend++;
> + else
> dpm_show_time(starttime, state, NULL);
> return error;
> }
> @@ -1088,7 +1111,9 @@ int dpm_suspend_start(pm_message_t state)
> int error;
>
> error = dpm_prepare(state);
> - if (!error)
> + if (error)
> + suspend_stats.failed_prepare++;
> + else
> error = dpm_suspend(state);
> return error;
> }
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 6bbcef2..6a8ff23 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -34,6 +34,22 @@ typedef int __bitwise suspend_state_t;
> #define PM_SUSPEND_MEM ((__force suspend_state_t) 3)
> #define PM_SUSPEND_MAX ((__force suspend_state_t) 4)
>
> +struct suspend_stats {
> + int success;
> + int fail;
> + int failed_freeze;
> + int failed_prepare;
> + int failed_suspend;
> + int failed_suspend_noirq;
> + int failed_resume;
> + int failed_resume_noirq;
> +#define REC_FAILED_DEV_NUM 2
> + char failed_devs[REC_FAILED_DEV_NUM][40];
> + int last_failed;
> +};
> +
> +extern struct suspend_stats suspend_stats;
> +
> /**
> * struct platform_suspend_ops - Callbacks for managing platform dependent
> * system sleep states.
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 6c601f8..0977f5d 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -12,6 +12,8 @@
> #include <linux/string.h>
> #include <linux/resume-trace.h>
> #include <linux/workqueue.h>
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
>
> #include "power.h"
>
> @@ -133,6 +135,59 @@ power_attr(pm_test);
>
> #endif /* CONFIG_PM_SLEEP */
>
> +#ifdef CONFIG_DEBUG_FS
> +static int suspend_stats_show(struct seq_file *s, void *unused)
> +{
> + int i, index, last_index;
> +
> + last_index = suspend_stats.last_failed + REC_FAILED_DEV_NUM - 1;
> + last_index %= REC_FAILED_DEV_NUM;
> + seq_printf(s, "%s: %d\n%s: %d\n%s: %d\n%s: %d\n"
> + "%s: %d\n%s: %d\n%s: %d\n%s: %d\n",
> + "success", suspend_stats.success,
> + "fail", suspend_stats.fail,
> + "failed_freeze", suspend_stats.failed_freeze,
> + "failed_prepare", suspend_stats.failed_prepare,
> + "failed_suspend", suspend_stats.failed_suspend,
> + "failed_suspend_noirq",
> + suspend_stats.failed_suspend_noirq,
> + "failed_resume", suspend_stats.failed_resume,
> + "failed_resume_noirq",
> + suspend_stats.failed_resume_noirq);
> + seq_printf(s, "failed_devs:\n last_failed:\t%s\n",
> + suspend_stats.failed_devs[last_index]);
> + for (i = 1; i < REC_FAILED_DEV_NUM; i++) {
> + index = last_index + REC_FAILED_DEV_NUM - i;
> + index %= REC_FAILED_DEV_NUM;
> + seq_printf(s, "\t\t%s\n",
> + suspend_stats.failed_devs[index]);
> + }
> +
> + return 0;
> +}
> +
> +static int suspend_stats_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, suspend_stats_show, NULL);
> +}
> +
> +static const struct file_operations suspend_stats_operations = {
> + .open = suspend_stats_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> +};
> +
> +static int __init pm_debugfs_init(void)
> +{
> + debugfs_create_file("suspend_stats", S_IFREG | S_IRUGO,
> + NULL, NULL, &suspend_stats_operations);
> + return 0;
> +}
> +
> +late_initcall(pm_debugfs_init);
> +#endif /* CONFIG_DEBUG_FS */
> +
> struct kobject *power_kobj;
>
> /**
> @@ -194,6 +249,10 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
> }
> if (state < PM_SUSPEND_MAX && *s)
> error = enter_state(state);
> + if (error)
> + suspend_stats.fail++;
> + else
> + suspend_stats.success++;
> #endif
>
> Exit:
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index b6b71ad..e5197a0 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -104,7 +104,9 @@ static int suspend_prepare(void)
> goto Finish;
>
> error = suspend_freeze_processes();
> - if (!error)
> + if (error)
> + suspend_stats.failed_freeze++;
> + else
> return 0;
>
> suspend_thaw_processes();
> @@ -315,8 +317,15 @@ int enter_state(suspend_state_t state)
> */
> int pm_suspend(suspend_state_t state)
> {
> - if (state > PM_SUSPEND_ON && state <= PM_SUSPEND_MAX)
> - return enter_state(state);
> + int ret;
> + if (state > PM_SUSPEND_ON && state <= PM_SUSPEND_MAX) {
> + ret = enter_state(state);
> + if (ret)
> + suspend_stats.fail++;
> + else
> + suspend_stats.success++;
> + return ret;
> + }
> return -EINVAL;
> }
> EXPORT_SYMBOL(pm_suspend);
> --
> 1.7.1
>
>
>
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm
>
--
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
^ permalink raw reply
* Re: [Announcement] The PM tree has been renamed
From: Stephen Rothwell @ 2011-08-09 4:20 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux PM mailing list, LKML
In-Reply-To: <201108082357.12169.rjw@sisk.pl>
[-- Attachment #1.1: Type: text/plain, Size: 558 bytes --]
Hi Rafael,
On Mon, 8 Aug 2011 23:57:12 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>
> I've just renamed the PM tree at kernel.org to linux-pm (from suspend-2.6),
> so please use
>
> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
>
> to access it. The old one is going to be around until necessary.
OK, I have updated my records. I have also renamed the "suspend" tree to
be "pm". Thanks for letting me know.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #1.2: Type: application/pgp-signature, Size: 490 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* [PATCH v3] PM: add statistics debugfs file for suspend to ram
From: Liu, ShuoX @ 2011-08-09 2:27 UTC (permalink / raw)
To: linux-pm@lists.linux-foundation.org
Cc: Brown, Len, Yanmin_zhang@linux.intel.com, Greg KH,
linux-kernel@vger.kernel.org
From: ShuoX Liu <shuox.liu@intel.com>
Record S3 failure time about each reason and the latest two failed
devices' names in S3 progress.
We can check it through 'suspend_stats' entry in debugfs.
The motivation of the patch:
We are enabling power features on Medfield. Comparing with PC/notebook,
a mobile enters/exits suspend-2-ram (we call it s3 on Medfield) far
more frequently. If it can't enter suspend-2-ram in time, the power
might be used up soon.
We often find sometimes, a device suspend fails. Then, system retries
s3 over and over again. As display is off, testers and developers
don't know what happens.
Some testers and developers complain they don't know if system
tries suspend-2-ram, and what device fails to suspend. They need
such info for a quick check. The patch adds suspend_stats under
debugfs for users to check suspend to RAM statistics quickly.
If not using this patch, we have other methods to get info about
what device fails. One is to turn on CONFIG_PM_DEBUG, but users
would get too much info and testers need recompile the system.
In addition, dynamic debug is another good tool to dump debug info.
But it still doesn't match our utilization scenario closely.
1) user need write a user space parser to process the syslog output;
2) Our testing scenario is we leave the mobile for at least hours.
Then, check its status. No serial console available during the
testing. One is because console would be suspended, and the other
is serial console connecting with spi or HSU devices would consume
power. These devices are powered off at suspend-2-ram.
Thank Greg and Rafael for their great comments.
Change Log:
V3: Change some programming styles.
V2:
1) Change the sysfs entry to debugfs.
2) Add some explanation in document file.
Contribution:
yanmin_zhang@linux.intel.com
Signed-off-by: ShuoX Liu <shuox.liu@intel.com>
---
Documentation/power/basic-pm-debugging.txt | 19 +++++++++
drivers/base/power/main.c | 35 ++++++++++++++--
include/linux/suspend.h | 16 +++++++
kernel/power/main.c | 59 ++++++++++++++++++++++++++++
kernel/power/suspend.c | 15 ++++++-
5 files changed, 136 insertions(+), 8 deletions(-)
diff --git a/Documentation/power/basic-pm-debugging.txt b/Documentation/power/basic-pm-debugging.txt
index ddd7817..acbd3e0 100644
--- a/Documentation/power/basic-pm-debugging.txt
+++ b/Documentation/power/basic-pm-debugging.txt
@@ -201,3 +201,22 @@ case, you may be able to search for failing drivers by following the procedure
analogous to the one described in section 1. If you find some failing drivers,
you will have to unload them every time before an STR transition (ie. before
you run s2ram), and please report the problems with them.
+
+There is a debugfs entry which shows the suspend to RAM statistics. Here is an
+example of its output.
+ # mount -t debugfs none /sys/kernel/debug
+ # cat /sys/kernel/debug/suspend_stats
+ success: 20
+ fail: 5
+ failed_freeze: 0
+ failed_prepare: 0
+ failed_suspend: 5
+ failed_suspend_noirq: 0
+ failed_resume: 0
+ failed_resume_noirq: 0
+ failed_devs:
+ last_failed: alarm
+ adc
+Field success means the success number of suspend to RAM, and field fail means
+the failure number. Others are the failure number of different steps of suspend
+to RAM. suspend_stats just lists the last 2 failed devices.
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index a854591..111f86c 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -46,6 +46,7 @@ LIST_HEAD(dpm_prepared_list);
LIST_HEAD(dpm_suspended_list);
LIST_HEAD(dpm_noirq_list);
+struct suspend_stats suspend_stats;
static DEFINE_MUTEX(dpm_list_mtx);
static pm_message_t pm_transition;
@@ -180,6 +181,15 @@ static void initcall_debug_report(struct device *dev, ktime_t calltime,
}
}
+static void dpm_save_dev_name(const char *name)
+{
+ strlcpy(suspend_stats.failed_devs[suspend_stats.last_failed],
+ name,
+ sizeof(suspend_stats.failed_devs[0]));
+ suspend_stats.last_failed++;
+ suspend_stats.last_failed %= REC_FAILED_DEV_NUM;
+}
+
/**
* dpm_wait - Wait for a PM operation to complete.
* @dev: Device to wait for.
@@ -464,8 +474,11 @@ void dpm_resume_noirq(pm_message_t state)
mutex_unlock(&dpm_list_mtx);
error = device_resume_noirq(dev, state);
- if (error)
+ if (error) {
+ suspend_stats.failed_resume_noirq++;
+ dpm_save_dev_name(dev_name(dev));
pm_dev_err(dev, state, " early", error);
+ }
mutex_lock(&dpm_list_mtx);
put_device(dev);
@@ -626,8 +639,11 @@ void dpm_resume(pm_message_t state)
mutex_unlock(&dpm_list_mtx);
error = device_resume(dev, state, false);
- if (error)
+ if (error) {
+ suspend_stats.failed_resume++;
+ dpm_save_dev_name(dev_name(dev));
pm_dev_err(dev, state, "", error);
+ }
mutex_lock(&dpm_list_mtx);
}
@@ -802,6 +818,8 @@ int dpm_suspend_noirq(pm_message_t state)
mutex_lock(&dpm_list_mtx);
if (error) {
pm_dev_err(dev, state, " late", error);
+ suspend_stats.failed_suspend_noirq++;
+ dpm_save_dev_name(dev_name(dev));
put_device(dev);
break;
}
@@ -923,8 +941,10 @@ static void async_suspend(void *data, async_cookie_t cookie)
int error;
error = __device_suspend(dev, pm_transition, true);
- if (error)
+ if (error) {
+ dpm_save_dev_name(dev_name(dev));
pm_dev_err(dev, pm_transition, " async", error);
+ }
put_device(dev);
}
@@ -967,6 +987,7 @@ int dpm_suspend(pm_message_t state)
mutex_lock(&dpm_list_mtx);
if (error) {
pm_dev_err(dev, state, "", error);
+ dpm_save_dev_name(dev_name(dev));
put_device(dev);
break;
}
@@ -980,7 +1001,9 @@ int dpm_suspend(pm_message_t state)
async_synchronize_full();
if (!error)
error = async_error;
- if (!error)
+ if (error)
+ suspend_stats.failed_suspend++;
+ else
dpm_show_time(starttime, state, NULL);
return error;
}
@@ -1088,7 +1111,9 @@ int dpm_suspend_start(pm_message_t state)
int error;
error = dpm_prepare(state);
- if (!error)
+ if (error)
+ suspend_stats.failed_prepare++;
+ else
error = dpm_suspend(state);
return error;
}
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 6bbcef2..6a8ff23 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -34,6 +34,22 @@ typedef int __bitwise suspend_state_t;
#define PM_SUSPEND_MEM ((__force suspend_state_t) 3)
#define PM_SUSPEND_MAX ((__force suspend_state_t) 4)
+struct suspend_stats {
+ int success;
+ int fail;
+ int failed_freeze;
+ int failed_prepare;
+ int failed_suspend;
+ int failed_suspend_noirq;
+ int failed_resume;
+ int failed_resume_noirq;
+#define REC_FAILED_DEV_NUM 2
+ char failed_devs[REC_FAILED_DEV_NUM][40];
+ int last_failed;
+};
+
+extern struct suspend_stats suspend_stats;
+
/**
* struct platform_suspend_ops - Callbacks for managing platform dependent
* system sleep states.
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 6c601f8..0977f5d 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -12,6 +12,8 @@
#include <linux/string.h>
#include <linux/resume-trace.h>
#include <linux/workqueue.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
#include "power.h"
@@ -133,6 +135,59 @@ power_attr(pm_test);
#endif /* CONFIG_PM_SLEEP */
+#ifdef CONFIG_DEBUG_FS
+static int suspend_stats_show(struct seq_file *s, void *unused)
+{
+ int i, index, last_index;
+
+ last_index = suspend_stats.last_failed + REC_FAILED_DEV_NUM - 1;
+ last_index %= REC_FAILED_DEV_NUM;
+ seq_printf(s, "%s: %d\n%s: %d\n%s: %d\n%s: %d\n"
+ "%s: %d\n%s: %d\n%s: %d\n%s: %d\n",
+ "success", suspend_stats.success,
+ "fail", suspend_stats.fail,
+ "failed_freeze", suspend_stats.failed_freeze,
+ "failed_prepare", suspend_stats.failed_prepare,
+ "failed_suspend", suspend_stats.failed_suspend,
+ "failed_suspend_noirq",
+ suspend_stats.failed_suspend_noirq,
+ "failed_resume", suspend_stats.failed_resume,
+ "failed_resume_noirq",
+ suspend_stats.failed_resume_noirq);
+ seq_printf(s, "failed_devs:\n last_failed:\t%s\n",
+ suspend_stats.failed_devs[last_index]);
+ for (i = 1; i < REC_FAILED_DEV_NUM; i++) {
+ index = last_index + REC_FAILED_DEV_NUM - i;
+ index %= REC_FAILED_DEV_NUM;
+ seq_printf(s, "\t\t%s\n",
+ suspend_stats.failed_devs[index]);
+ }
+
+ return 0;
+}
+
+static int suspend_stats_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, suspend_stats_show, NULL);
+}
+
+static const struct file_operations suspend_stats_operations = {
+ .open = suspend_stats_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static int __init pm_debugfs_init(void)
+{
+ debugfs_create_file("suspend_stats", S_IFREG | S_IRUGO,
+ NULL, NULL, &suspend_stats_operations);
+ return 0;
+}
+
+late_initcall(pm_debugfs_init);
+#endif /* CONFIG_DEBUG_FS */
+
struct kobject *power_kobj;
/**
@@ -194,6 +249,10 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
}
if (state < PM_SUSPEND_MAX && *s)
error = enter_state(state);
+ if (error)
+ suspend_stats.fail++;
+ else
+ suspend_stats.success++;
#endif
Exit:
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index b6b71ad..e5197a0 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -104,7 +104,9 @@ static int suspend_prepare(void)
goto Finish;
error = suspend_freeze_processes();
- if (!error)
+ if (error)
+ suspend_stats.failed_freeze++;
+ else
return 0;
suspend_thaw_processes();
@@ -315,8 +317,15 @@ int enter_state(suspend_state_t state)
*/
int pm_suspend(suspend_state_t state)
{
- if (state > PM_SUSPEND_ON && state <= PM_SUSPEND_MAX)
- return enter_state(state);
+ int ret;
+ if (state > PM_SUSPEND_ON && state <= PM_SUSPEND_MAX) {
+ ret = enter_state(state);
+ if (ret)
+ suspend_stats.fail++;
+ else
+ suspend_stats.success++;
+ return ret;
+ }
return -EINVAL;
}
EXPORT_SYMBOL(pm_suspend);
--
1.7.1
^ permalink raw reply related
* [PATCH 4/4] PM: Move clock-related definitions and headers to separate file
From: Rafael J. Wysocki @ 2011-08-08 22:33 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: Greg KH, LKML, linux-sh
In-Reply-To: <201108090028.12698.rjw@sisk.pl>
From: Rafael J. Wysocki <rjw@sisk.pl>
Since the PM clock management code in drivers/base/power/clock_ops.c
is used for both runtime PM and system suspend/hibernation, the
definitions of data structures and headers related to it should not
be located in include/linux/pm_rumtime.h. Move them to a separate
header file.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
arch/arm/mach-omap1/pm_bus.c | 1
arch/arm/mach-shmobile/board-ap4evb.c | 1
arch/arm/mach-shmobile/board-mackerel.c | 2
arch/arm/mach-shmobile/pm-sh7372.c | 2
arch/arm/mach-shmobile/pm_runtime.c | 1
drivers/base/power/clock_ops.c | 2
drivers/base/power/common.c | 3 -
include/linux/pm_clock.h | 70 ++++++++++++++++++++++++++++++++
include/linux/pm_runtime.h | 56 -------------------------
9 files changed, 77 insertions(+), 61 deletions(-)
Index: linux-2.6/drivers/base/power/common.c
===================================================================
--- linux-2.6.orig/drivers/base/power/common.c
+++ linux-2.6/drivers/base/power/common.c
@@ -9,8 +9,7 @@
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/slab.h>
-#include <linux/device.h>
-#include <linux/pm_runtime.h>
+#include <linux/pm_clock.h>
/**
* dev_pm_get_subsys_data - Create or refcount power.subsys_data for device.
Index: linux-2.6/include/linux/pm_clock.h
===================================================================
--- /dev/null
+++ linux-2.6/include/linux/pm_clock.h
@@ -0,0 +1,70 @@
+/*
+ * pm_clock.h - Definitions and headers related to device clocks.
+ *
+ * Copyright (C) 2011 Rafael J. Wysocki <rjw@sisk.pl>, Renesas Electronics Corp.
+ *
+ * This file is released under the GPLv2.
+ */
+
+#ifndef _LINUX_PM_CLOCK_H
+#define _LINUX_PM_CLOCK_H
+
+#include <linux/device.h>
+
+struct pm_clk_notifier_block {
+ struct notifier_block nb;
+ struct dev_pm_domain *pm_domain;
+ char *con_ids[];
+};
+
+#ifdef CONFIG_PM_CLK
+static inline bool pm_clk_no_clocks(struct device *dev)
+{
+ return dev && dev->power.subsys_data
+ && list_empty(&dev->power.subsys_data->clock_list);
+}
+
+extern void pm_clk_init(struct device *dev);
+extern int pm_clk_create(struct device *dev);
+extern void pm_clk_destroy(struct device *dev);
+extern int pm_clk_add(struct device *dev, const char *con_id);
+extern void pm_clk_remove(struct device *dev, const char *con_id);
+extern int pm_clk_suspend(struct device *dev);
+extern int pm_clk_resume(struct device *dev);
+#else
+static inline bool pm_clk_no_clocks(struct device *dev)
+{
+ return true;
+}
+static inline void pm_clk_init(struct device *dev)
+{
+}
+static inline int pm_clk_create(struct device *dev)
+{
+ return -EINVAL;
+}
+static inline void pm_clk_destroy(struct device *dev)
+{
+}
+static inline int pm_clk_add(struct device *dev, const char *con_id)
+{
+ return -EINVAL;
+}
+static inline void pm_clk_remove(struct device *dev, const char *con_id)
+{
+}
+#define pm_clk_suspend NULL
+#define pm_clk_resume NULL
+#endif
+
+#ifdef CONFIG_HAVE_CLK
+extern void pm_clk_add_notifier(struct bus_type *bus,
+ struct pm_clk_notifier_block *clknb);
+#else
+static inline void pm_clk_add_notifier(struct bus_type *bus,
+ struct pm_clk_notifier_block *clknb)
+{
+}
+#endif
+
+#endif
Index: linux-2.6/include/linux/pm_runtime.h
===================================================================
--- linux-2.6.orig/include/linux/pm_runtime.h
+++ linux-2.6/include/linux/pm_runtime.h
@@ -251,60 +251,4 @@ static inline void pm_runtime_dont_use_a
__pm_runtime_use_autosuspend(dev, false);
}
-struct pm_clk_notifier_block {
- struct notifier_block nb;
- struct dev_pm_domain *pm_domain;
- char *con_ids[];
-};
-
-#ifdef CONFIG_PM_CLK
-static inline bool pm_clk_no_clocks(struct device *dev)
-{
- return dev && dev->power.subsys_data
- && list_empty(&dev->power.subsys_data->clock_list);
-}
-
-extern void pm_clk_init(struct device *dev);
-extern int pm_clk_create(struct device *dev);
-extern void pm_clk_destroy(struct device *dev);
-extern int pm_clk_add(struct device *dev, const char *con_id);
-extern void pm_clk_remove(struct device *dev, const char *con_id);
-extern int pm_clk_suspend(struct device *dev);
-extern int pm_clk_resume(struct device *dev);
-#else
-static inline bool pm_clk_no_clocks(struct device *dev)
-{
- return true;
-}
-static inline void pm_clk_init(struct device *dev)
-{
-}
-static inline int pm_clk_create(struct device *dev)
-{
- return -EINVAL;
-}
-static inline void pm_clk_destroy(struct device *dev)
-{
-}
-static inline int pm_clk_add(struct device *dev, const char *con_id)
-{
- return -EINVAL;
-}
-static inline void pm_clk_remove(struct device *dev, const char *con_id)
-{
-}
-#define pm_clk_suspend NULL
-#define pm_clk_resume NULL
-#endif
-
-#ifdef CONFIG_HAVE_CLK
-extern void pm_clk_add_notifier(struct bus_type *bus,
- struct pm_clk_notifier_block *clknb);
-#else
-static inline void pm_clk_add_notifier(struct bus_type *bus,
- struct pm_clk_notifier_block *clknb)
-{
-}
-#endif
-
#endif
Index: linux-2.6/arch/arm/mach-shmobile/pm-sh7372.c
===================================================================
--- linux-2.6.orig/arch/arm/mach-shmobile/pm-sh7372.c
+++ linux-2.6/arch/arm/mach-shmobile/pm-sh7372.c
@@ -15,7 +15,7 @@
#include <linux/list.h>
#include <linux/err.h>
#include <linux/slab.h>
-#include <linux/pm_runtime.h>
+#include <linux/pm_clock.h>
#include <linux/platform_device.h>
#include <linux/delay.h>
#include <asm/system.h>
Index: linux-2.6/drivers/base/power/clock_ops.c
===================================================================
--- linux-2.6.orig/drivers/base/power/clock_ops.c
+++ linux-2.6/drivers/base/power/clock_ops.c
@@ -10,7 +10,7 @@
#include <linux/kernel.h>
#include <linux/io.h>
#include <linux/pm.h>
-#include <linux/pm_runtime.h>
+#include <linux/pm_clock.h>
#include <linux/clk.h>
#include <linux/slab.h>
#include <linux/err.h>
Index: linux-2.6/arch/arm/mach-shmobile/pm_runtime.c
===================================================================
--- linux-2.6.orig/arch/arm/mach-shmobile/pm_runtime.c
+++ linux-2.6/arch/arm/mach-shmobile/pm_runtime.c
@@ -15,6 +15,7 @@
#include <linux/io.h>
#include <linux/pm_runtime.h>
#include <linux/pm_domain.h>
+#include <linux/pm_clock.h>
#include <linux/platform_device.h>
#include <linux/clk.h>
#include <linux/sh_clk.h>
Index: linux-2.6/arch/arm/mach-shmobile/board-mackerel.c
===================================================================
--- linux-2.6.orig/arch/arm/mach-shmobile/board-mackerel.c
+++ linux-2.6/arch/arm/mach-shmobile/board-mackerel.c
@@ -39,7 +39,7 @@
#include <linux/mtd/mtd.h>
#include <linux/mtd/partitions.h>
#include <linux/mtd/physmap.h>
-#include <linux/pm_runtime.h>
+#include <linux/pm_clock.h>
#include <linux/smsc911x.h>
#include <linux/sh_intc.h>
#include <linux/tca6416_keypad.h>
Index: linux-2.6/arch/arm/mach-shmobile/board-ap4evb.c
===================================================================
--- linux-2.6.orig/arch/arm/mach-shmobile/board-ap4evb.c
+++ linux-2.6/arch/arm/mach-shmobile/board-ap4evb.c
@@ -42,6 +42,7 @@
#include <linux/leds.h>
#include <linux/input/sh_keysc.h>
#include <linux/usb/r8a66597.h>
+#include <linux/pm_clock.h>
#include <media/sh_mobile_ceu.h>
#include <media/sh_mobile_csi2.h>
Index: linux-2.6/arch/arm/mach-omap1/pm_bus.c
===================================================================
--- linux-2.6.orig/arch/arm/mach-omap1/pm_bus.c
+++ linux-2.6/arch/arm/mach-omap1/pm_bus.c
@@ -13,6 +13,7 @@
#include <linux/kernel.h>
#include <linux/io.h>
#include <linux/pm_runtime.h>
+#include <linux/pm_clock.h>
#include <linux/platform_device.h>
#include <linux/mutex.h>
#include <linux/clk.h>
^ permalink raw reply
* [PATCH 3/4] PM / Domains: Use power.sybsys_data to reduce overhead
From: Rafael J. Wysocki @ 2011-08-08 22:32 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: Greg KH, LKML, linux-sh
In-Reply-To: <201108090028.12698.rjw@sisk.pl>
From: Rafael J. Wysocki <rjw@sisk.pl>
Currently pm_genpd_runtime_resume() has to walk the list of devices
from the device's PM domain to find the corresponding device list
object containing the need_restore field to check if the driver's
.runtime_resume() callback should be executed for the device.
This is suboptimal and can be simplified by using power.sybsys_data
to store device information used by the generic PM domains code.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
arch/arm/mach-shmobile/pm-sh7372.c | 6 --
drivers/base/power/domain.c | 87 ++++++++++++-------------------------
include/linux/pm.h | 9 +++
include/linux/pm_domain.h | 6 --
include/linux/pm_runtime.h | 10 ++++
5 files changed, 51 insertions(+), 67 deletions(-)
Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -422,12 +422,21 @@ enum rpm_request {
struct wakeup_source;
+struct pm_domain_data {
+ struct list_head list_node;
+ struct device *dev;
+ bool need_restore;
+};
+
struct pm_subsys_data {
struct mutex lock;
unsigned int refcount;
#ifdef CONFIG_PM_CLK
struct list_head clock_list;
#endif
+#ifdef CONFIG_PM_GENERIC_DOMAINS
+ struct pm_domain_data domain_data;
+#endif
};
struct dev_pm_info {
Index: linux-2.6/include/linux/pm_domain.h
===================================================================
--- linux-2.6.orig/include/linux/pm_domain.h
+++ linux-2.6/include/linux/pm_domain.h
@@ -61,12 +61,6 @@ struct gpd_link {
struct list_head slave_node;
};
-struct dev_list_entry {
- struct list_head node;
- struct device *dev;
- bool need_restore;
-};
-
#ifdef CONFIG_PM_GENERIC_DOMAINS
extern int pm_genpd_add_device(struct generic_pm_domain *genpd,
struct device *dev);
Index: linux-2.6/drivers/base/power/domain.c
===================================================================
--- linux-2.6.orig/drivers/base/power/domain.c
+++ linux-2.6/drivers/base/power/domain.c
@@ -181,18 +181,18 @@ int pm_genpd_poweron(struct generic_pm_d
/**
* __pm_genpd_save_device - Save the pre-suspend state of a device.
- * @dle: Device list entry of the device to save the state of.
+ * @pdd: Domain data of the device to save the state of.
* @genpd: PM domain the device belongs to.
*/
-static int __pm_genpd_save_device(struct dev_list_entry *dle,
+static int __pm_genpd_save_device(struct pm_domain_data *pdd,
struct generic_pm_domain *genpd)
__releases(&genpd->lock) __acquires(&genpd->lock)
{
- struct device *dev = dle->dev;
+ struct device *dev = pdd->dev;
struct device_driver *drv = dev->driver;
int ret = 0;
- if (dle->need_restore)
+ if (pdd->need_restore)
return 0;
mutex_unlock(&genpd->lock);
@@ -210,24 +210,24 @@ static int __pm_genpd_save_device(struct
mutex_lock(&genpd->lock);
if (!ret)
- dle->need_restore = true;
+ pdd->need_restore = true;
return ret;
}
/**
* __pm_genpd_restore_device - Restore the pre-suspend state of a device.
- * @dle: Device list entry of the device to restore the state of.
+ * @pdd: Domain data of the device to restore the state of.
* @genpd: PM domain the device belongs to.
*/
-static void __pm_genpd_restore_device(struct dev_list_entry *dle,
+static void __pm_genpd_restore_device(struct pm_domain_data *pdd,
struct generic_pm_domain *genpd)
__releases(&genpd->lock) __acquires(&genpd->lock)
{
- struct device *dev = dle->dev;
+ struct device *dev = pdd->dev;
struct device_driver *drv = dev->driver;
- if (!dle->need_restore)
+ if (!pdd->need_restore)
return;
mutex_unlock(&genpd->lock);
@@ -244,7 +244,7 @@ static void __pm_genpd_restore_device(st
mutex_lock(&genpd->lock);
- dle->need_restore = false;
+ pdd->need_restore = false;
}
/**
@@ -286,7 +286,7 @@ void genpd_queue_power_off_work(struct g
static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
__releases(&genpd->lock) __acquires(&genpd->lock)
{
- struct dev_list_entry *dle;
+ struct pm_domain_data *pdd;
struct gpd_link *link;
unsigned int not_suspended;
int ret = 0;
@@ -308,8 +308,8 @@ static int pm_genpd_poweroff(struct gene
return -EBUSY;
not_suspended = 0;
- list_for_each_entry(dle, &genpd->dev_list, node)
- if (dle->dev->driver && !pm_runtime_suspended(dle->dev))
+ list_for_each_entry(pdd, &genpd->dev_list, list_node)
+ if (pdd->dev->driver && !pm_runtime_suspended(pdd->dev))
not_suspended++;
if (not_suspended > genpd->in_progress)
@@ -332,9 +332,9 @@ static int pm_genpd_poweroff(struct gene
genpd->status = GPD_STATE_BUSY;
genpd->poweroff_task = current;
- list_for_each_entry_reverse(dle, &genpd->dev_list, node) {
+ list_for_each_entry_reverse(pdd, &genpd->dev_list, list_node) {
ret = atomic_read(&genpd->sd_count) == 0 ?
- __pm_genpd_save_device(dle, genpd) : -EBUSY;
+ __pm_genpd_save_device(pdd, genpd) : -EBUSY;
if (genpd_abort_poweroff(genpd))
goto out;
@@ -433,24 +433,6 @@ static int pm_genpd_runtime_suspend(stru
}
/**
- * __pm_genpd_runtime_resume - Resume a device belonging to I/O PM domain.
- * @dev: Device to resume.
- * @genpd: PM domain the device belongs to.
- */
-static void __pm_genpd_runtime_resume(struct device *dev,
- struct generic_pm_domain *genpd)
-{
- struct dev_list_entry *dle;
-
- list_for_each_entry(dle, &genpd->dev_list, node) {
- if (dle->dev == dev) {
- __pm_genpd_restore_device(dle, genpd);
- break;
- }
- }
-}
-
-/**
* pm_genpd_runtime_resume - Resume a device belonging to I/O PM domain.
* @dev: Device to resume.
*
@@ -495,7 +477,7 @@ static int pm_genpd_runtime_resume(struc
mutex_lock(&genpd->lock);
}
finish_wait(&genpd->status_wait_queue, &wait);
- __pm_genpd_runtime_resume(dev, genpd);
+ __pm_genpd_restore_device(&dev->power.subsys_data->domain_data, genpd);
genpd->resume_count--;
genpd_set_active(genpd);
wake_up_all(&genpd->status_wait_queue);
@@ -510,8 +492,6 @@ static int pm_genpd_runtime_resume(struc
#else
static inline void genpd_power_off_work_fn(struct work_struct *work) {}
-static inline void __pm_genpd_runtime_resume(struct device *dev,
- struct generic_pm_domain *genpd) {}
#define pm_genpd_runtime_suspend NULL
#define pm_genpd_runtime_resume NULL
@@ -1068,7 +1048,7 @@ static void pm_genpd_complete(struct dev
*/
int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
{
- struct dev_list_entry *dle;
+ struct pm_domain_data *pdd;
int ret = 0;
dev_dbg(dev, "%s()\n", __func__);
@@ -1088,26 +1068,20 @@ int pm_genpd_add_device(struct generic_p
goto out;
}
- list_for_each_entry(dle, &genpd->dev_list, node)
- if (dle->dev == dev) {
+ list_for_each_entry(pdd, &genpd->dev_list, list_node)
+ if (pdd->dev == dev) {
ret = -EINVAL;
goto out;
}
- dle = kzalloc(sizeof(*dle), GFP_KERNEL);
- if (!dle) {
- ret = -ENOMEM;
- goto out;
- }
-
- dle->dev = dev;
- dle->need_restore = false;
- list_add_tail(&dle->node, &genpd->dev_list);
genpd->device_count++;
- spin_lock_irq(&dev->power.lock);
dev->pm_domain = &genpd->domain;
- spin_unlock_irq(&dev->power.lock);
+ dev_pm_get_subsys_data(dev);
+ pdd = &dev->power.subsys_data->domain_data;
+ pdd->dev = dev;
+ pdd->need_restore = false;
+ list_add_tail(&pdd->list_node, &genpd->dev_list);
out:
genpd_release_lock(genpd);
@@ -1123,7 +1097,7 @@ int pm_genpd_add_device(struct generic_p
int pm_genpd_remove_device(struct generic_pm_domain *genpd,
struct device *dev)
{
- struct dev_list_entry *dle;
+ struct pm_domain_data *pdd;
int ret = -EINVAL;
dev_dbg(dev, "%s()\n", __func__);
@@ -1138,17 +1112,16 @@ int pm_genpd_remove_device(struct generi
goto out;
}
- list_for_each_entry(dle, &genpd->dev_list, node) {
- if (dle->dev != dev)
+ list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+ if (pdd->dev != dev)
continue;
- spin_lock_irq(&dev->power.lock);
+ list_del_init(&pdd->list_node);
+ pdd->dev = NULL;
+ dev_pm_put_subsys_data(dev);
dev->pm_domain = NULL;
- spin_unlock_irq(&dev->power.lock);
genpd->device_count--;
- list_del(&dle->node);
- kfree(dle);
ret = 0;
break;
Index: linux-2.6/arch/arm/mach-shmobile/pm-sh7372.c
===================================================================
--- linux-2.6.orig/arch/arm/mach-shmobile/pm-sh7372.c
+++ linux-2.6/arch/arm/mach-shmobile/pm-sh7372.c
@@ -114,11 +114,9 @@ void sh7372_add_device_to_domain(struct
{
struct device *dev = &pdev->dev;
- if (!dev->power.subsys_data) {
- pm_clk_create(dev);
- pm_clk_add(dev, NULL);
- }
pm_genpd_add_device(&sh7372_pd->genpd, dev);
+ if (pm_clk_no_clocks(dev))
+ pm_clk_add(dev, NULL);
}
struct sh7372_pm_domain sh7372_a4lc = {
Index: linux-2.6/include/linux/pm_runtime.h
===================================================================
--- linux-2.6.orig/include/linux/pm_runtime.h
+++ linux-2.6/include/linux/pm_runtime.h
@@ -258,6 +258,12 @@ struct pm_clk_notifier_block {
};
#ifdef CONFIG_PM_CLK
+static inline bool pm_clk_no_clocks(struct device *dev)
+{
+ return dev && dev->power.subsys_data
+ && list_empty(&dev->power.subsys_data->clock_list);
+}
+
extern void pm_clk_init(struct device *dev);
extern int pm_clk_create(struct device *dev);
extern void pm_clk_destroy(struct device *dev);
@@ -266,6 +272,10 @@ extern void pm_clk_remove(struct device
extern int pm_clk_suspend(struct device *dev);
extern int pm_clk_resume(struct device *dev);
#else
+static inline bool pm_clk_no_clocks(struct device *dev)
+{
+ return true;
+}
static inline void pm_clk_init(struct device *dev)
{
}
^ permalink raw reply
* [PATCH 2/4] PM: Reference counting of power.subsys_data
From: Rafael J. Wysocki @ 2011-08-08 22:31 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: Greg KH, LKML, linux-sh
In-Reply-To: <201108090028.12698.rjw@sisk.pl>
From: Rafael J. Wysocki <rjw@sisk.pl>
Since the power.subsys_data device field will be used by multiple
filesystems, introduce a reference counting mechanism for it to avoid
freeing it prematurely or changing its value at a wrong time.
Make the PM clocks management code that currently is the only user of
power.subsys_data use the new reference counting.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
drivers/base/power/Makefile | 2
drivers/base/power/clock_ops.c | 24 ++---------
drivers/base/power/common.c | 86 +++++++++++++++++++++++++++++++++++++++++
include/linux/pm.h | 3 +
4 files changed, 95 insertions(+), 20 deletions(-)
Index: linux-2.6/drivers/base/power/common.c
===================================================================
--- /dev/null
+++ linux-2.6/drivers/base/power/common.c
@@ -0,0 +1,86 @@
+/*
+ * drivers/base/power/common.c - Common device power management code.
+ *
+ * Copyright (C) 2011 Rafael J. Wysocki <rjw@sisk.pl>, Renesas Electronics Corp.
+ *
+ * This file is released under the GPLv2.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/pm_runtime.h>
+
+/**
+ * dev_pm_get_subsys_data - Create or refcount power.subsys_data for device.
+ * @dev: Device to handle.
+ *
+ * If power.subsys_data is NULL, point it to a new object, otherwise increment
+ * its reference counter. Return 1 if a new object has been created, otherwise
+ * return 0 or error code.
+ */
+int dev_pm_get_subsys_data(struct device *dev)
+{
+ struct pm_subsys_data *psd;
+ int ret = 0;
+
+ psd = kzalloc(sizeof(*psd), GFP_KERNEL);
+ if (!psd)
+ return -ENOMEM;
+
+ spin_lock_irq(&dev->power.lock);
+
+ if (dev->power.subsys_data) {
+ dev->power.subsys_data->refcount++;
+ } else {
+ mutex_init(&psd->lock);
+ psd->refcount = 1;
+ dev->power.subsys_data = psd;
+ pm_clk_init(dev);
+ psd = NULL;
+ ret = 1;
+ }
+
+ spin_unlock_irq(&dev->power.lock);
+
+ /* kfree() verifies that its argument is nonzero. */
+ kfree(psd);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_get_subsys_data);
+
+/**
+ * dev_pm_put_subsys_data - Drop reference to power.subsys_data.
+ * @dev: Device to handle.
+ *
+ * If the reference counter of power.subsys_data is zero after dropping the
+ * reference, power.subsys_data is removed. Return 1 if that happens or 0
+ * otherwise.
+ */
+int dev_pm_put_subsys_data(struct device *dev)
+{
+ struct pm_subsys_data *psd;
+ int ret = 0;
+
+ spin_lock_irq(&dev->power.lock);
+
+ psd = dev_to_psd(dev);
+ if (!psd) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (--psd->refcount == 0) {
+ dev->power.subsys_data = NULL;
+ kfree(psd);
+ ret = 1;
+ }
+
+ out:
+ spin_unlock_irq(&dev->power.lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_put_subsys_data);
Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -424,6 +424,7 @@ struct wakeup_source;
struct pm_subsys_data {
struct mutex lock;
+ unsigned int refcount;
#ifdef CONFIG_PM_CLK
struct list_head clock_list;
#endif
@@ -474,6 +475,8 @@ struct dev_pm_info {
};
extern void update_pm_runtime_accounting(struct device *dev);
+extern int dev_pm_get_subsys_data(struct device *dev);
+extern int dev_pm_put_subsys_data(struct device *dev);
/*
* Power domains provide callbacks that are executed during system suspend,
Index: linux-2.6/drivers/base/power/Makefile
===================================================================
--- linux-2.6.orig/drivers/base/power/Makefile
+++ linux-2.6/drivers/base/power/Makefile
@@ -1,4 +1,4 @@
-obj-$(CONFIG_PM) += sysfs.o generic_ops.o
+obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o
obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o
obj-$(CONFIG_PM_RUNTIME) += runtime.o
obj-$(CONFIG_PM_TRACE_RTC) += trace.o
Index: linux-2.6/drivers/base/power/clock_ops.c
===================================================================
--- linux-2.6.orig/drivers/base/power/clock_ops.c
+++ linux-2.6/drivers/base/power/clock_ops.c
@@ -140,12 +140,8 @@ void pm_clk_remove(struct device *dev, c
void pm_clk_init(struct device *dev)
{
struct pm_subsys_data *psd = dev_to_psd(dev);
-
- if (!psd)
- return;
-
- INIT_LIST_HEAD(&psd->clock_list);
- mutex_init(&psd->lock);
+ if (psd)
+ INIT_LIST_HEAD(&psd->clock_list);
}
/**
@@ -157,16 +153,8 @@ void pm_clk_init(struct device *dev)
*/
int pm_clk_create(struct device *dev)
{
- struct pm_subsys_data *psd;
-
- psd = kzalloc(sizeof(*psd), GFP_KERNEL);
- if (!psd) {
- dev_err(dev, "Not enough memory for PM clock data.\n");
- return -ENOMEM;
- }
- dev->power.subsys_data = psd;
- pm_clk_init(dev);
- return 0;
+ int ret = dev_pm_get_subsys_data(dev);
+ return ret < 0 ? ret : 0;
}
/**
@@ -185,8 +173,6 @@ void pm_clk_destroy(struct device *dev)
if (!psd)
return;
- dev->power.subsys_data = NULL;
-
mutex_lock(&psd->lock);
list_for_each_entry_safe_reverse(ce, c, &psd->clock_list, node)
@@ -194,7 +180,7 @@ void pm_clk_destroy(struct device *dev)
mutex_unlock(&psd->lock);
- kfree(psd);
+ dev_pm_put_subsys_data(dev);
}
#endif /* CONFIG_PM */
^ permalink raw reply
* [PATCH 1/4] PM: Introduce struct pm_subsys_data
From: Rafael J. Wysocki @ 2011-08-08 22:30 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: Greg KH, LKML, linux-sh
In-Reply-To: <201108090028.12698.rjw@sisk.pl>
From: Rafael J. Wysocki <rjw@sisk.pl>
Introduce struct pm_subsys_data that may be subclassed by subsystems
to store subsystem-specific information related to the device. Move
the clock management fields accessed through the power.subsys_data
pointer in struct device to the new strucutre.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
arch/arm/mach-shmobile/pm-sh7372.c | 2
drivers/base/power/clock_ops.c | 122 +++++++++++++++++++------------------
include/linux/device.h | 5 +
include/linux/pm.h | 10 ++-
include/linux/pm_runtime.h | 8 +-
5 files changed, 85 insertions(+), 62 deletions(-)
Index: linux-2.6/drivers/base/power/clock_ops.c
===================================================================
--- linux-2.6.orig/drivers/base/power/clock_ops.c
+++ linux-2.6/drivers/base/power/clock_ops.c
@@ -17,11 +17,6 @@
#ifdef CONFIG_PM
-struct pm_clk_data {
- struct list_head clock_list;
- struct mutex lock;
-};
-
enum pce_status {
PCE_STATUS_NONE = 0,
PCE_STATUS_ACQUIRED,
@@ -36,11 +31,6 @@ struct pm_clock_entry {
enum pce_status status;
};
-static struct pm_clk_data *__to_pcd(struct device *dev)
-{
- return dev ? dev->power.subsys_data : NULL;
-}
-
/**
* pm_clk_add - Start using a device clock for power management.
* @dev: Device whose clock is going to be used for power management.
@@ -51,10 +41,10 @@ static struct pm_clk_data *__to_pcd(stru
*/
int pm_clk_add(struct device *dev, const char *con_id)
{
- struct pm_clk_data *pcd = __to_pcd(dev);
+ struct pm_subsys_data *psd = dev_to_psd(dev);
struct pm_clock_entry *ce;
- if (!pcd)
+ if (!psd)
return -EINVAL;
ce = kzalloc(sizeof(*ce), GFP_KERNEL);
@@ -73,9 +63,9 @@ int pm_clk_add(struct device *dev, const
}
}
- mutex_lock(&pcd->lock);
- list_add_tail(&ce->node, &pcd->clock_list);
- mutex_unlock(&pcd->lock);
+ mutex_lock(&psd->lock);
+ list_add_tail(&ce->node, &psd->clock_list);
+ mutex_unlock(&psd->lock);
return 0;
}
@@ -117,15 +107,15 @@ static void __pm_clk_remove(struct pm_cl
*/
void pm_clk_remove(struct device *dev, const char *con_id)
{
- struct pm_clk_data *pcd = __to_pcd(dev);
+ struct pm_subsys_data *psd = dev_to_psd(dev);
struct pm_clock_entry *ce;
- if (!pcd)
+ if (!psd)
return;
- mutex_lock(&pcd->lock);
+ mutex_lock(&psd->lock);
- list_for_each_entry(ce, &pcd->clock_list, node) {
+ list_for_each_entry(ce, &psd->clock_list, node) {
if (!con_id && !ce->con_id) {
__pm_clk_remove(ce);
break;
@@ -137,29 +127,45 @@ void pm_clk_remove(struct device *dev, c
}
}
- mutex_unlock(&pcd->lock);
+ mutex_unlock(&psd->lock);
}
/**
* pm_clk_init - Initialize a device's list of power management clocks.
* @dev: Device to initialize the list of PM clocks for.
*
- * Allocate a struct pm_clk_data object, initialize its lock member and
- * make the @dev's power.subsys_data field point to it.
+ * Initialize the lock and clock_list members of the device's pm_subsys_data
+ * object.
*/
-int pm_clk_init(struct device *dev)
+void pm_clk_init(struct device *dev)
{
- struct pm_clk_data *pcd;
+ struct pm_subsys_data *psd = dev_to_psd(dev);
+
+ if (!psd)
+ return;
- pcd = kzalloc(sizeof(*pcd), GFP_KERNEL);
- if (!pcd) {
+ INIT_LIST_HEAD(&psd->clock_list);
+ mutex_init(&psd->lock);
+}
+
+/**
+ * pm_clk_create - Create and initialize a device's list of PM clocks.
+ * @dev: Device to create and initialize the list of PM clocks for.
+ *
+ * Allocate a struct pm_subsys_data object, initialize its lock and clock_list
+ * members and make the @dev's power.subsys_data field point to it.
+ */
+int pm_clk_create(struct device *dev)
+{
+ struct pm_subsys_data *psd;
+
+ psd = kzalloc(sizeof(*psd), GFP_KERNEL);
+ if (!psd) {
dev_err(dev, "Not enough memory for PM clock data.\n");
return -ENOMEM;
}
-
- INIT_LIST_HEAD(&pcd->clock_list);
- mutex_init(&pcd->lock);
- dev->power.subsys_data = pcd;
+ dev->power.subsys_data = psd;
+ pm_clk_init(dev);
return 0;
}
@@ -168,27 +174,27 @@ int pm_clk_init(struct device *dev)
* @dev: Device to destroy the list of PM clocks for.
*
* Clear the @dev's power.subsys_data field, remove the list of clock entries
- * from the struct pm_clk_data object pointed to by it before and free
+ * from the struct pm_subsys_data object pointed to by it before and free
* that object.
*/
void pm_clk_destroy(struct device *dev)
{
- struct pm_clk_data *pcd = __to_pcd(dev);
+ struct pm_subsys_data *psd = dev_to_psd(dev);
struct pm_clock_entry *ce, *c;
- if (!pcd)
+ if (!psd)
return;
dev->power.subsys_data = NULL;
- mutex_lock(&pcd->lock);
+ mutex_lock(&psd->lock);
- list_for_each_entry_safe_reverse(ce, c, &pcd->clock_list, node)
+ list_for_each_entry_safe_reverse(ce, c, &psd->clock_list, node)
__pm_clk_remove(ce);
- mutex_unlock(&pcd->lock);
+ mutex_unlock(&psd->lock);
- kfree(pcd);
+ kfree(psd);
}
#endif /* CONFIG_PM */
@@ -218,17 +224,17 @@ static void pm_clk_acquire(struct device
*/
int pm_clk_suspend(struct device *dev)
{
- struct pm_clk_data *pcd = __to_pcd(dev);
+ struct pm_subsys_data *psd = dev_to_psd(dev);
struct pm_clock_entry *ce;
dev_dbg(dev, "%s()\n", __func__);
- if (!pcd)
+ if (!psd)
return 0;
- mutex_lock(&pcd->lock);
+ mutex_lock(&psd->lock);
- list_for_each_entry_reverse(ce, &pcd->clock_list, node) {
+ list_for_each_entry_reverse(ce, &psd->clock_list, node) {
if (ce->status == PCE_STATUS_NONE)
pm_clk_acquire(dev, ce);
@@ -238,7 +244,7 @@ int pm_clk_suspend(struct device *dev)
}
}
- mutex_unlock(&pcd->lock);
+ mutex_unlock(&psd->lock);
return 0;
}
@@ -249,17 +255,17 @@ int pm_clk_suspend(struct device *dev)
*/
int pm_clk_resume(struct device *dev)
{
- struct pm_clk_data *pcd = __to_pcd(dev);
+ struct pm_subsys_data *psd = dev_to_psd(dev);
struct pm_clock_entry *ce;
dev_dbg(dev, "%s()\n", __func__);
- if (!pcd)
+ if (!psd)
return 0;
- mutex_lock(&pcd->lock);
+ mutex_lock(&psd->lock);
- list_for_each_entry(ce, &pcd->clock_list, node) {
+ list_for_each_entry(ce, &psd->clock_list, node) {
if (ce->status == PCE_STATUS_NONE)
pm_clk_acquire(dev, ce);
@@ -269,7 +275,7 @@ int pm_clk_resume(struct device *dev)
}
}
- mutex_unlock(&pcd->lock);
+ mutex_unlock(&psd->lock);
return 0;
}
@@ -307,7 +313,7 @@ static int pm_clk_notify(struct notifier
if (dev->pm_domain)
break;
- error = pm_clk_init(dev);
+ error = pm_clk_create(dev);
if (error)
break;
@@ -342,21 +348,21 @@ static int pm_clk_notify(struct notifier
*/
int pm_clk_suspend(struct device *dev)
{
- struct pm_clk_data *pcd = __to_pcd(dev);
+ struct pm_subsys_data *psd = dev_to_psd(dev);
struct pm_clock_entry *ce;
dev_dbg(dev, "%s()\n", __func__);
/* If there is no driver, the clocks are already disabled. */
- if (!pcd || !dev->driver)
+ if (!psd || !dev->driver)
return 0;
- mutex_lock(&pcd->lock);
+ mutex_lock(&psd->lock);
- list_for_each_entry_reverse(ce, &pcd->clock_list, node)
+ list_for_each_entry_reverse(ce, &psd->clock_list, node)
clk_disable(ce->clk);
- mutex_unlock(&pcd->lock);
+ mutex_unlock(&psd->lock);
return 0;
}
@@ -367,21 +373,21 @@ int pm_clk_suspend(struct device *dev)
*/
int pm_clk_resume(struct device *dev)
{
- struct pm_clk_data *pcd = __to_pcd(dev);
+ struct pm_subsys_data *psd = dev_to_psd(dev);
struct pm_clock_entry *ce;
dev_dbg(dev, "%s()\n", __func__);
/* If there is no driver, the clocks should remain disabled. */
- if (!pcd || !dev->driver)
+ if (!psd || !dev->driver)
return 0;
- mutex_lock(&pcd->lock);
+ mutex_lock(&psd->lock);
- list_for_each_entry(ce, &pcd->clock_list, node)
+ list_for_each_entry(ce, &psd->clock_list, node)
clk_enable(ce->clk);
- mutex_unlock(&pcd->lock);
+ mutex_unlock(&psd->lock);
return 0;
}
Index: linux-2.6/include/linux/device.h
===================================================================
--- linux-2.6.orig/include/linux/device.h
+++ linux-2.6/include/linux/device.h
@@ -636,6 +636,11 @@ static inline void set_dev_node(struct d
}
#endif
+static inline struct pm_subsys_data *dev_to_psd(struct device *dev)
+{
+ return dev ? dev->power.subsys_data : NULL;
+}
+
static inline unsigned int dev_get_uevent_suppress(const struct device *dev)
{
return dev->kobj.uevent_suppress;
Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -24,6 +24,7 @@
#include <linux/list.h>
#include <linux/workqueue.h>
#include <linux/spinlock.h>
+#include <linux/mutex.h>
#include <linux/wait.h>
#include <linux/timer.h>
#include <linux/completion.h>
@@ -421,6 +422,13 @@ enum rpm_request {
struct wakeup_source;
+struct pm_subsys_data {
+ struct mutex lock;
+#ifdef CONFIG_PM_CLK
+ struct list_head clock_list;
+#endif
+};
+
struct dev_pm_info {
pm_message_t power_state;
unsigned int can_wakeup:1;
@@ -462,7 +470,7 @@ struct dev_pm_info {
unsigned long suspended_jiffies;
unsigned long accounting_timestamp;
#endif
- void *subsys_data; /* Owned by the subsystem. */
+ struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */
};
extern void update_pm_runtime_accounting(struct device *dev);
Index: linux-2.6/arch/arm/mach-shmobile/pm-sh7372.c
===================================================================
--- linux-2.6.orig/arch/arm/mach-shmobile/pm-sh7372.c
+++ linux-2.6/arch/arm/mach-shmobile/pm-sh7372.c
@@ -115,7 +115,7 @@ void sh7372_add_device_to_domain(struct
struct device *dev = &pdev->dev;
if (!dev->power.subsys_data) {
- pm_clk_init(dev);
+ pm_clk_create(dev);
pm_clk_add(dev, NULL);
}
pm_genpd_add_device(&sh7372_pd->genpd, dev);
Index: linux-2.6/include/linux/pm_runtime.h
===================================================================
--- linux-2.6.orig/include/linux/pm_runtime.h
+++ linux-2.6/include/linux/pm_runtime.h
@@ -258,14 +258,18 @@ struct pm_clk_notifier_block {
};
#ifdef CONFIG_PM_CLK
-extern int pm_clk_init(struct device *dev);
+extern void pm_clk_init(struct device *dev);
+extern int pm_clk_create(struct device *dev);
extern void pm_clk_destroy(struct device *dev);
extern int pm_clk_add(struct device *dev, const char *con_id);
extern void pm_clk_remove(struct device *dev, const char *con_id);
extern int pm_clk_suspend(struct device *dev);
extern int pm_clk_resume(struct device *dev);
#else
-static inline int pm_clk_init(struct device *dev)
+static inline void pm_clk_init(struct device *dev)
+{
+}
+static inline int pm_clk_create(struct device *dev)
{
return -EINVAL;
}
^ permalink raw reply
* [PATCH 0/4] PM: Make power.subsys_data be more useful
From: Rafael J. Wysocki @ 2011-08-08 22:28 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: Greg KH, LKML, linux-sh
Hi,
The following patchset reworks the power.subsys_data field of
struct device so that it can be used not only for clock PM
management.
[1/4] - Introduce struct pm_subsys_data to be point to by power.subsys_data.
[2/4] - Add reference counting for power.subsys_data.
[3/4] - Optimize generic PM domains code by making it use power.subsys_data.
[4/4] - Move PM clock management headers and definitions to a separate file.
The patchset is on top of the pm-domains branch of the linux-pm tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git pm-domains
It has been tested on an ARM shmobile Mackerel board.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] PM: add statistics sysfs file for suspend to ram
From: Rafael J. Wysocki @ 2011-08-08 22:09 UTC (permalink / raw)
To: Pavel Machek
Cc: Liu, ShuoX, yanmin_zhang, Greg KH,
linux-pm@lists.linux-foundation.org, Brown, Len
In-Reply-To: <20110808215452.GB22709@elf.ucw.cz>
On Monday, August 08, 2011, Pavel Machek wrote:
> Hi!
>
> > > > > > Thanks for the nice pointer. I checked dynamic debug. It's really a good debug tool.
> > > > > > With the dynamic debug:
> > > > > > 1) user need write a user space parser to process the syslog output;
> > > > > > 2) Our testing scenario is we leave the mobile for at least hours. Then, check its status.
> > > > > > No serial console available during the testing. One is because console would be suspended,
> > > > > > and the other is serial console connecting with spi or HSU devices would consume power. These
> > > > > > devices are powered off at suspend-2-ram.
> ...
> > Not in the case described by Yanmin.
>
> Really? I see the description above.
>
> Yes, they'd need to set up syslog to only log >= KERN_ERR, then parse
> the (small) results. Even hours worth of suspends should not cause
> _that_ many errors.
>
> Serial console is irrelevant. You need live machine to dump dmesg, but
> again, you need live machine to access debugfs, too.
This sounds like substantial overhead to collect statistics that we can
collect at a much lower cost in the kernel.
The patch isn't very intrusive and rather straightforward.
Thanks,
Rafael
^ permalink raw reply
* [Announcement] The PM tree has been renamed
From: Rafael J. Wysocki @ 2011-08-08 21:57 UTC (permalink / raw)
To: Linux PM mailing list; +Cc: Stephen Rothwell, LKML
Hi,
I've just renamed the PM tree at kernel.org to linux-pm (from suspend-2.6),
so please use
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
to access it. The old one is going to be around until necessary.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] PM: add statistics sysfs file for suspend to ram
From: Pavel Machek @ 2011-08-08 21:54 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Liu, ShuoX, yanmin_zhang, Greg KH,
linux-pm@lists.linux-foundation.org, Brown, Len
In-Reply-To: <201108082314.23860.rjw@sisk.pl>
Hi!
> > > > > Thanks for the nice pointer. I checked dynamic debug. It's really a good debug tool.
> > > > > With the dynamic debug:
> > > > > 1) user need write a user space parser to process the syslog output;
> > > > > 2) Our testing scenario is we leave the mobile for at least hours. Then, check its status.
> > > > > No serial console available during the testing. One is because console would be suspended,
> > > > > and the other is serial console connecting with spi or HSU devices would consume power. These
> > > > > devices are powered off at suspend-2-ram.
...
> Not in the case described by Yanmin.
Really? I see the description above.
Yes, they'd need to set up syslog to only log >= KERN_ERR, then parse
the (small) results. Even hours worth of suspends should not cause
_that_ many errors.
Serial console is irrelevant. You need live machine to dump dmesg, but
again, you need live machine to access debugfs, too.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH v5 0/3] DEVFREQ, DVFS Framework for Non-CPU Devices.
From: Greg KH @ 2011-08-08 21:35 UTC (permalink / raw)
To: MyungJoo Ham; +Cc: Len Brown, Kyungmin Park, linux-pm, Thomas Gleixner
In-Reply-To: <CAJ0PZbSgVujLSW9Ea_iWHZVbq2_H3EWGJg_1Zh5rUfvG3sxgsw@mail.gmail.com>
On Mon, Aug 08, 2011 at 06:53:31PM +0900, MyungJoo Ham wrote:
> Hello all,
>
>
> I've got one issue requesting some comments: whether to create devfreq
> directory or not.
>
> For now, we only have four basic governors and one not-yet-posted
> devfreq user for DEVFREQ; thus, it seems not necessary for just now.
>
> However, like CPUFREQ, DEVFREQ may have several users later and we may
> want to gather these files to one location: /drivers/devfreq or
> somewhere else in /drivers/*. For example, Exynos4210 Memory DEVFREQ
> user is located at arch/arm/mach-exynos4/devfreq_bus.c and with a
> separated directory, we may have it at
> drivers/devfreq/exynos4_memory_bus.c.
>
> How do you think about separating DEVFREQ directory? Should I do so or
> wait until we have several DEVFREQ users?
A new subdir is fine, it's not an issue, and seems to make sense here,
as you point out.
greg k-h
^ permalink raw reply
* Re: [PATCH 02/11] PM: extend PM QoS with per-device wake-up constraints
From: Rafael J. Wysocki @ 2011-08-08 21:31 UTC (permalink / raw)
To: Mark Brown; +Cc: Linux PM mailing list, linux-omap, Jean Pihet, markgross
In-Reply-To: <20110807024712.GA5887@opensource.wolfsonmicro.com>
On Sunday, August 07, 2011, Mark Brown wrote:
> On Sat, Aug 06, 2011 at 09:46:20PM +0200, Rafael J. Wysocki wrote:
> > On Saturday, August 06, 2011, Mark Brown wrote:
>
> > > I wouldn't say we've got to rely on userspace here - it seems like we
> > > ought to be able to use DMI or other system data to identify the
> > > affected systems and activate the workarounds.
>
> > That's only practical on systems where the kernel can be rebuilt.
> > Moreover, if that affects individual devices, using DMI-based blacklists
> > is not really practical at all.
>
> OK, so this does sound like there's probably a genuine issue on PCs due
> to limitations in the environment. Is it possible to expose these
> things to userspace in a way that's limited to the affected platforms?
Well, in principle we could make it depend in CONFIG_ACPI or something like
this, but I'm not sure that will be well-received. :-)
> > > You're right that it doesn't stop the kernel doing anything, the concern
> > > is that people just won't bother making the kernel work properly and
> > > will just do their power management in userspace and not bother fixing
> > > the kernel.
>
> > I wouldn't call it "fixing the kernel". I'd rather say "putting workarounds
> > into the kernel", which I'm not sure is the right thing to at least in some
> > cases.
>
> That does sound like a fair characterization for PCs. For embedded
> systems where we have a *much* better knowledge of the hardware we're
> running on you're just working with the basics of what the hardware
> needs to run - the hardware needs whatever it needs and no matter what
> you think of the quality of the hardware there's an expectation that the
> kernel is ging to be able to work.
In the particular case in question, though, there's some freedom. Namely,
the hardware will work for many different QoS constraints and it's not
precisely known in principle and upfront which one would be optimal for
a given workload.
> > > Punting to userspace seems like it is creating the
> > > expectation that we can't make the kernel work and isn't great from a
> > > usability perspective since users shouldn't really be worrying about bus
> > > performance or so on, it's not something that's visible at the level
> > > applications work at.
>
> > However, platform builders may want to fine tuned things and I'm not sure
> > we should require them to patch the kernel for this purpose.
>
> As I've said it's not the fine tuning that I'm worried about, it's the
> specific mechanism that's being suggested. Being able to tune things in
> a way that's relevant to the device being tuned seems entirely sensible.
Do you know any better mechanism consistent accross all devices?
Please be specific. :-)
> > > I'm not sure I can see a lot of cases where you'd have root access and
> > > not be able to do kernel updates if required? Having stuff in debugfs
> > > for diagnostics doesn't strike me as a problem if that's the issue.
>
> > Root access doesn't necessarily mean you have all of the requisite
> > tools (like compilers etc.) and installing them isn't always trivial
> > (think of systems like phones etc.), let alone building the kernel from
> > sources (where you don't necessarily know the original .config used for
> > building your device's kernel).
>
> Phones are exactly the sort of case I'm primarily concerned with here.
>
> Realistically if you're in a position where you need to work at this
> very low level on an embedded device you can replace the entire firmware
> on the device. We don't want to end up in the situation where we've got
> kernel support for a device and the only way to get it to actually run
> sensibly is to install the silicon vendor's proprietary userspace, and
> we especially don't want to end up in the situation where that userspace
> is using standard and supported kernel interfaces to do its thing.
Well, the wakelocks example shows clearly that preventing certain interfaces
from being merged into mainline doesn't actually prevent people from using
them in actual products. I claim it's way better if a vendor uses its
proprietary user space with the mainline kernel than if that vendor patches
the kernel and _then_ uses its proprietary user space with it. Your
argumentation seems to suggest that we encourage the latter.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] PM: add statistics sysfs file for suspend to ram
From: Rafael J. Wysocki @ 2011-08-08 21:14 UTC (permalink / raw)
To: Pavel Machek
Cc: Liu, ShuoX, yanmin_zhang, Greg KH,
linux-pm@lists.linux-foundation.org, Brown, Len
In-Reply-To: <20110808210537.GA22709@elf.ucw.cz>
On Monday, August 08, 2011, Pavel Machek wrote:
> On Mon 2011-08-08 14:01:04, Greg KH wrote:
> > On Mon, Aug 08, 2011 at 10:37:03PM +0200, Rafael J. Wysocki wrote:
> > > On Monday, August 08, 2011, Yanmin Zhang wrote:
> > > > On Fri, 2011-08-05 at 21:20 +0200, Rafael J. Wysocki wrote:
> > > > > On Friday, August 05, 2011, Yanmin Zhang wrote:
> > > > > > On Fri, 2011-08-05 at 09:57 +0800, Liu, ShuoX wrote:
> > > > > > > > On Thursday, August 04, 2011, Greg KH wrote:
> > > > > > > > > On Thu, Aug 04, 2011 at 01:13:38PM +0800, Liu, ShuoX wrote:
> > > > > > > > > > >From a906b0b5b4ff3414ceb9fc7a69a3d7c9d66e46b1 Mon Sep 17
> > > > > > > > 00:00:00 2001
> > > > > > > > > > From: ShuoX Liu <shuox.liu@intel.com>
> > > > > > > > > > Date: Thu, 28 Jul 2011 10:54:22 +0800
> > > > > > > > > > Subject: [PATCH] PM: add statistics sysfs file for suspend to ram.
> > > > > > > > >
> > > > > > > > > What's this stuff here for? That's not needed (hint, I would have to
> > > > > > > > > edit it out by hand to be able to apply this patch.)
> > > > > > > > >
> > > > > > > > > Thanks for resending a version that passes checkpatch.pl and could be
> > > > > > > > > applied, but all of my previous comments still stand. This patch, as
> > > > > > > > > is, is totally unacceptable.
> > > > > > > >
> > > > > > > > Agreed, plus I'd like to know the motivation behind it. That is, we have
> > > > > > > > quite a few debug facilities in that code, so why are they insufficient?
> > > > > > Thanks Greg, Rafael. We are changing the patch based on your comments.
> > > > > >
> > > > > > >
> > > > > > > Some explanation from Yanmin,
> > > > > > > "We are enabling power features on Medfield. Some testers and developers
> > > > > > > complain they don't know if system tries suspend-2-ram, and what device
> > > > > > > fails to suspend. They need such info for a quick check. If turning on
> > > > > > > CONFIG_PM_DEBUG, we get too much info and testers need recompile
> > > > > > > the system.
> > > > > > Comparing with PC/notebook, a mobile enters/exits suspend-2-ram (we call it s3 on
> > > > > > Medfield) far more frequently. If it can't enter suspend-2-ram in time, the power
> > > > > > might be used up soon.
> > > > > >
> > > > > > We often find sometimes, a device suspend fails. Then, system retries s3 over
> > > > > > and over again. As display is off, testers/developers don't know what happens.
> > > > > > Teh system
> > > > > >
> > > > > > With the patch, we could know what the bad device is.
> > > > > >
> > > > > > The patch doesn't hurt performance as it's just statistics collector.
> > > > > >
> > > > > > CONFIG_PM_DEBUG is very useful for finer investigation about what happens behind. What
> > > > > > we provide by the patch is to analyze the issues quickly, even by an ordinary tester.
> > > > >
> > > > > Well, what about using dynamic debug?
> > > > Thanks for the nice pointer. I checked dynamic debug. It's really a good debug tool.
> > > > With the dynamic debug:
> > > > 1) user need write a user space parser to process the syslog output;
> > > > 2) Our testing scenario is we leave the mobile for at least hours. Then, check its status.
> > > > No serial console available during the testing. One is because console would be suspended,
> > > > and the other is serial console connecting with spi or HSU devices would consume power. These
> > > > devices are powered off at suspend-2-ram.
> > > >
> > > > Below is an example output of the statistics from my mobile (we are changing the output
> > > > from sysfs to debugfs now):
> > > > #adb shell cat /sys/power/suspend_stats
> > > > success: 5
> > > > fail: 1
> > > > failed_freeze: 0
> > > > failed_prepare: 0
> > > > failed_suspend: 1
> > > > failed_suspend_noirq: 0
> > > > failed_resume: 0
> > > > failed_resume_noirq: 0
> > > > failed_devs:
> > > > last_failed: alarm
> > >
> > > OK, I see. Greg, what do you think?
> >
> > Files in debugfs like this are fine with me. If you think the
> > information is valid, and useful to people, I have no objection to the
> > patch.
>
> Dunno. Just parsing relevant part of dmesg after each suspend should
> be enough.
Not in the case described by Yanmin.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] PM / Freezer: Freeze filesystems while freezing processes (v2)
From: Rafael J. Wysocki @ 2011-08-08 21:11 UTC (permalink / raw)
To: Dave Chinner
Cc: Christoph, Theodore Ts'o, LKML, xfs, Christoph Hellwig,
linux-fsdevel, Linux PM mailing list, linux-ext4
In-Reply-To: <20110807001446.GI3162@dastard>
On Sunday, August 07, 2011, Dave Chinner wrote:
> On Sat, Aug 06, 2011 at 11:17:18PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > Freeze all filesystems during the freezing of tasks by calling
> > freeze_bdev() for each of them and thaw them during the thawing
> > of tasks with the help of thaw_bdev().
> >
> > This is needed by hibernation, because some filesystems (e.g. XFS)
> > deadlock with the preallocation of memory used by it if the memory
> > pressure caused by it is too heavy.
> >
> > The additional benefit of this change is that, if something goes
> > wrong after filesystems have been frozen, they will stay in a
> > consistent state and journal replays won't be necessary (e.g. after
> > a failing suspend or resume). In particular, this should help to
> > solve a long-standing issue that in some cases during resume from
> > hibernation the boot loader causes the journal to be replied for the
> > filesystem containing the kernel image and initrd causing it to
> > become inconsistent with the information stored in the hibernation
> > image.
> >
> > This change is based on earlier work by Nigel Cunningham.
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >
> > OK, so nobody except for Pavel appears to have any comments, so I assume
> > that everyone except for Pavel is fine with the approach, interestingly enough.
> >
> > I've removed the MS_FROZEN Pavel complained about from freeze_filesystems()
> > and added comments explaining why lockdep_off/on() are used.
> >
> > Thanks,
> > Rafael
> >
> > ---
> > fs/block_dev.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/fs.h | 6 +++++
> > kernel/power/process.c | 7 +++++-
> > 3 files changed, 68 insertions(+), 1 deletion(-)
> >
> > Index: linux-2.6/include/linux/fs.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/fs.h
> > +++ linux-2.6/include/linux/fs.h
> > @@ -211,6 +211,7 @@ struct inodes_stat_t {
> > #define MS_KERNMOUNT (1<<22) /* this is a kern_mount call */
> > #define MS_I_VERSION (1<<23) /* Update inode I_version field */
> > #define MS_STRICTATIME (1<<24) /* Always perform atime updates */
> > +#define MS_FROZEN (1<<25) /* bdev has been frozen */
> > #define MS_NOSEC (1<<28)
> > #define MS_BORN (1<<29)
> > #define MS_ACTIVE (1<<30)
> > @@ -2047,6 +2048,8 @@ extern struct super_block *freeze_bdev(s
> > extern void emergency_thaw_all(void);
> > extern int thaw_bdev(struct block_device *bdev, struct super_block *sb);
> > extern int fsync_bdev(struct block_device *);
> > +extern void freeze_filesystems(void);
> > +extern void thaw_filesystems(void);
> > #else
> > static inline void bd_forget(struct inode *inode) {}
> > static inline int sync_blockdev(struct block_device *bdev) { return 0; }
> > @@ -2061,6 +2064,9 @@ static inline int thaw_bdev(struct block
> > {
> > return 0;
> > }
> > +
> > +static inline void freeze_filesystems(void) {}
> > +static inline void thaw_filesystems(void) {}
> > #endif
> > extern int sync_filesystem(struct super_block *);
> > extern const struct file_operations def_blk_fops;
> > Index: linux-2.6/fs/block_dev.c
> > ===================================================================
> > --- linux-2.6.orig/fs/block_dev.c
> > +++ linux-2.6/fs/block_dev.c
> > @@ -314,6 +314,62 @@ out:
> > }
> > EXPORT_SYMBOL(thaw_bdev);
> >
> > +/**
> > + * freeze_filesystems - Force all filesystems into a consistent state.
> > + */
> > +void freeze_filesystems(void)
> > +{
> > + struct super_block *sb;
> > +
> > + /*
> > + * This is necessary, because some filesystems (e.g. ext3) lock
> > + * mutexes in their .freeze_fs() callbacks and leave them locked for
> > + * their .unfreeze_fs() callbacks to unlock. This is done under
> > + * bdev->bd_fsfreeze_mutex, which is then released, but it makes
> > + * lockdep think something may be wrong when freeze_bdev() attempts
> > + * to acquire bdev->bd_fsfreeze_mutex for the next filesystem.
> > + */
> > + lockdep_off();
>
> I thought those problems were fixed. If they aren't, then they most
> certainly need to be because holding mutexes over system calls is a
> bug.
>
> Well, well:
>
> [252182.603134] ================================================
> [252182.604832] [ BUG: lock held when returning to user space! ]
> [252182.606086] ------------------------------------------------
> [252182.607400] xfs_io/4917 is leaving the kernel with locks still held!
> [252182.608905] 1 lock held by xfs_io/4917:
> [252182.609739] #0: (&journal->j_barrier){+.+...}, at: [<ffffffff812a2aaf>] journal_lock_updates+0xef/0x100
>
> <sigh>
>
> Looks like the problem was fixed for ext4, but not ext3. Please
> report this to the ext3/4 list and get it fixed, don't work around
> it here.
OK, but I guess I'll have to post a patch to fix this myself so that
anyone notices. :-)
> > + /*
> > + * Freeze in reverse order so filesystems depending on others are
> > + * frozen in the right order (eg. loopback on ext3).
> > + */
> > + list_for_each_entry_reverse(sb, &super_blocks, s_list) {
> > + if (!sb->s_root || !sb->s_bdev ||
> > + (sb->s_frozen == SB_FREEZE_TRANS) ||
> > + (sb->s_flags & MS_RDONLY))
> > + continue;
> > +
> > + freeze_bdev(sb->s_bdev);
> > + sb->s_flags |= MS_FROZEN;
> > + }
>
> AFAIK, that won't work for btrfs - you have to call freeze_super()
> directly for btrfs because it has a special relationship with
> sb->s_bdev. And besides, all freeze_bdev does is get an active
> reference on the superblock and call freeze_super().
OK, so do you mean I should call freeze_super() rather than freeze_bdev()?
> Also, that's traversing the list of superblock with locking and
> dereferencing the superblock without properly checking that the
> superblock is not being torn down. You should probably use
> iterate_supers (or at least copy the code), with a function that
> drops the s_umount read lock befor calling freeze_super() and then
> picks it back up afterwards.
Hmm, I'll try that, but I doubt I'll get it right first time. :-)
> > +
> > + lockdep_on();
> > +}
> > +
> > +/**
> > + * thaw_filesystems - Make all filesystems active again.
> > + */
> > +void thaw_filesystems(void)
> > +{
> > + struct super_block *sb;
> > +
> > + /*
> > + * This is necessary for the same reason as in freeze_filesystems()
> > + * above.
> > + */
> > + lockdep_off();
> > +
> > + list_for_each_entry(sb, &super_blocks, s_list)
> > + if (sb->s_flags & MS_FROZEN) {
> > + sb->s_flags &= ~MS_FROZEN;
> > + thaw_bdev(sb->s_bdev, sb);
> > + }
>
> And once again, iterate_supers() is what you want here.
OK
> And you should only call thaw_bdev() as it needs to do checks other
> than checking MS_FROZEN
Hmm, I'm not really sure what you mean?
> e.g. the above will unfreeze filesystems that
> were already frozen at the time a suspend occurs, and that could
> lead to corruption depending on why the filesystem was frozen...
>
> Also, you still need to check for a valid sb->s_bdev here, otherwise
> <splat>.
I see.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] PM: add statistics sysfs file for suspend to ram
From: Pavel Machek @ 2011-08-08 21:05 UTC (permalink / raw)
To: Greg KH
Cc: Liu, ShuoX, yanmin_zhang, linux-pm@lists.linux-foundation.org,
Brown, Len
In-Reply-To: <20110808210104.GA2926@suse.de>
On Mon 2011-08-08 14:01:04, Greg KH wrote:
> On Mon, Aug 08, 2011 at 10:37:03PM +0200, Rafael J. Wysocki wrote:
> > On Monday, August 08, 2011, Yanmin Zhang wrote:
> > > On Fri, 2011-08-05 at 21:20 +0200, Rafael J. Wysocki wrote:
> > > > On Friday, August 05, 2011, Yanmin Zhang wrote:
> > > > > On Fri, 2011-08-05 at 09:57 +0800, Liu, ShuoX wrote:
> > > > > > > On Thursday, August 04, 2011, Greg KH wrote:
> > > > > > > > On Thu, Aug 04, 2011 at 01:13:38PM +0800, Liu, ShuoX wrote:
> > > > > > > > > >From a906b0b5b4ff3414ceb9fc7a69a3d7c9d66e46b1 Mon Sep 17
> > > > > > > 00:00:00 2001
> > > > > > > > > From: ShuoX Liu <shuox.liu@intel.com>
> > > > > > > > > Date: Thu, 28 Jul 2011 10:54:22 +0800
> > > > > > > > > Subject: [PATCH] PM: add statistics sysfs file for suspend to ram.
> > > > > > > >
> > > > > > > > What's this stuff here for? That's not needed (hint, I would have to
> > > > > > > > edit it out by hand to be able to apply this patch.)
> > > > > > > >
> > > > > > > > Thanks for resending a version that passes checkpatch.pl and could be
> > > > > > > > applied, but all of my previous comments still stand. This patch, as
> > > > > > > > is, is totally unacceptable.
> > > > > > >
> > > > > > > Agreed, plus I'd like to know the motivation behind it. That is, we have
> > > > > > > quite a few debug facilities in that code, so why are they insufficient?
> > > > > Thanks Greg, Rafael. We are changing the patch based on your comments.
> > > > >
> > > > > >
> > > > > > Some explanation from Yanmin,
> > > > > > "We are enabling power features on Medfield. Some testers and developers
> > > > > > complain they don't know if system tries suspend-2-ram, and what device
> > > > > > fails to suspend. They need such info for a quick check. If turning on
> > > > > > CONFIG_PM_DEBUG, we get too much info and testers need recompile
> > > > > > the system.
> > > > > Comparing with PC/notebook, a mobile enters/exits suspend-2-ram (we call it s3 on
> > > > > Medfield) far more frequently. If it can't enter suspend-2-ram in time, the power
> > > > > might be used up soon.
> > > > >
> > > > > We often find sometimes, a device suspend fails. Then, system retries s3 over
> > > > > and over again. As display is off, testers/developers don't know what happens.
> > > > > Teh system
> > > > >
> > > > > With the patch, we could know what the bad device is.
> > > > >
> > > > > The patch doesn't hurt performance as it's just statistics collector.
> > > > >
> > > > > CONFIG_PM_DEBUG is very useful for finer investigation about what happens behind. What
> > > > > we provide by the patch is to analyze the issues quickly, even by an ordinary tester.
> > > >
> > > > Well, what about using dynamic debug?
> > > Thanks for the nice pointer. I checked dynamic debug. It's really a good debug tool.
> > > With the dynamic debug:
> > > 1) user need write a user space parser to process the syslog output;
> > > 2) Our testing scenario is we leave the mobile for at least hours. Then, check its status.
> > > No serial console available during the testing. One is because console would be suspended,
> > > and the other is serial console connecting with spi or HSU devices would consume power. These
> > > devices are powered off at suspend-2-ram.
> > >
> > > Below is an example output of the statistics from my mobile (we are changing the output
> > > from sysfs to debugfs now):
> > > #adb shell cat /sys/power/suspend_stats
> > > success: 5
> > > fail: 1
> > > failed_freeze: 0
> > > failed_prepare: 0
> > > failed_suspend: 1
> > > failed_suspend_noirq: 0
> > > failed_resume: 0
> > > failed_resume_noirq: 0
> > > failed_devs:
> > > last_failed: alarm
> >
> > OK, I see. Greg, what do you think?
>
> Files in debugfs like this are fine with me. If you think the
> information is valid, and useful to people, I have no objection to the
> patch.
Dunno. Just parsing relevant part of dmesg after each suspend should
be enough.
Actually, kernel messages should probably be adjusted so that
filtering for KERN_ERR will get exactly the relevant info... ?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH] PM: add statistics sysfs file for suspend to ram
From: Greg KH @ 2011-08-08 21:01 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Liu, ShuoX, yanmin_zhang, linux-pm@lists.linux-foundation.org,
Brown, Len
In-Reply-To: <201108082237.03559.rjw@sisk.pl>
On Mon, Aug 08, 2011 at 10:37:03PM +0200, Rafael J. Wysocki wrote:
> On Monday, August 08, 2011, Yanmin Zhang wrote:
> > On Fri, 2011-08-05 at 21:20 +0200, Rafael J. Wysocki wrote:
> > > On Friday, August 05, 2011, Yanmin Zhang wrote:
> > > > On Fri, 2011-08-05 at 09:57 +0800, Liu, ShuoX wrote:
> > > > > > On Thursday, August 04, 2011, Greg KH wrote:
> > > > > > > On Thu, Aug 04, 2011 at 01:13:38PM +0800, Liu, ShuoX wrote:
> > > > > > > > >From a906b0b5b4ff3414ceb9fc7a69a3d7c9d66e46b1 Mon Sep 17
> > > > > > 00:00:00 2001
> > > > > > > > From: ShuoX Liu <shuox.liu@intel.com>
> > > > > > > > Date: Thu, 28 Jul 2011 10:54:22 +0800
> > > > > > > > Subject: [PATCH] PM: add statistics sysfs file for suspend to ram.
> > > > > > >
> > > > > > > What's this stuff here for? That's not needed (hint, I would have to
> > > > > > > edit it out by hand to be able to apply this patch.)
> > > > > > >
> > > > > > > Thanks for resending a version that passes checkpatch.pl and could be
> > > > > > > applied, but all of my previous comments still stand. This patch, as
> > > > > > > is, is totally unacceptable.
> > > > > >
> > > > > > Agreed, plus I'd like to know the motivation behind it. That is, we have
> > > > > > quite a few debug facilities in that code, so why are they insufficient?
> > > > Thanks Greg, Rafael. We are changing the patch based on your comments.
> > > >
> > > > >
> > > > > Some explanation from Yanmin,
> > > > > "We are enabling power features on Medfield. Some testers and developers
> > > > > complain they don't know if system tries suspend-2-ram, and what device
> > > > > fails to suspend. They need such info for a quick check. If turning on
> > > > > CONFIG_PM_DEBUG, we get too much info and testers need recompile
> > > > > the system.
> > > > Comparing with PC/notebook, a mobile enters/exits suspend-2-ram (we call it s3 on
> > > > Medfield) far more frequently. If it can't enter suspend-2-ram in time, the power
> > > > might be used up soon.
> > > >
> > > > We often find sometimes, a device suspend fails. Then, system retries s3 over
> > > > and over again. As display is off, testers/developers don't know what happens.
> > > > Teh system
> > > >
> > > > With the patch, we could know what the bad device is.
> > > >
> > > > The patch doesn't hurt performance as it's just statistics collector.
> > > >
> > > > CONFIG_PM_DEBUG is very useful for finer investigation about what happens behind. What
> > > > we provide by the patch is to analyze the issues quickly, even by an ordinary tester.
> > >
> > > Well, what about using dynamic debug?
> > Thanks for the nice pointer. I checked dynamic debug. It's really a good debug tool.
> > With the dynamic debug:
> > 1) user need write a user space parser to process the syslog output;
> > 2) Our testing scenario is we leave the mobile for at least hours. Then, check its status.
> > No serial console available during the testing. One is because console would be suspended,
> > and the other is serial console connecting with spi or HSU devices would consume power. These
> > devices are powered off at suspend-2-ram.
> >
> > Below is an example output of the statistics from my mobile (we are changing the output
> > from sysfs to debugfs now):
> > #adb shell cat /sys/power/suspend_stats
> > success: 5
> > fail: 1
> > failed_freeze: 0
> > failed_prepare: 0
> > failed_suspend: 1
> > failed_suspend_noirq: 0
> > failed_resume: 0
> > failed_resume_noirq: 0
> > failed_devs:
> > last_failed: alarm
>
> OK, I see. Greg, what do you think?
Files in debugfs like this are fine with me. If you think the
information is valid, and useful to people, I have no objection to the
patch.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2] PM: add statistics debugfs file for suspend to ram
From: Rafael J. Wysocki @ 2011-08-08 20:46 UTC (permalink / raw)
To: Liu, ShuoX
Cc: Brown, Len, linux-pm@lists.linux-foundation.org, Greg KH,
linux-kernel@vger.kernel.org
In-Reply-To: <6E3BC7F7C9A4BF4286DD4C043110F30B5B8057C04B@shsmsx502.ccr.corp.intel.com>
Hi,
On Monday, August 08, 2011, Liu, ShuoX wrote:
...
> @@ -982,6 +1003,8 @@ int dpm_suspend(pm_message_t state)
> error = async_error;
> if (!error)
> dpm_show_time(starttime, state, NULL);
> + else
> + suspend_stats.failed_suspend++;
> return error;
> }
I'd prefer it to be
if (error)
something;
else
something else;
> @@ -1090,6 +1113,8 @@ int dpm_suspend_start(pm_message_t state)
> error = dpm_prepare(state);
> if (!error)
> error = dpm_suspend(state);
> + else
> + suspend_stats.failed_prepare++;
> return error;
> }
Same here.
> EXPORT_SYMBOL_GPL(dpm_suspend_start);
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 6bbcef2..6a8ff23 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -34,6 +34,22 @@ typedef int __bitwise suspend_state_t;
> #define PM_SUSPEND_MEM ((__force suspend_state_t) 3)
> #define PM_SUSPEND_MAX ((__force suspend_state_t) 4)
>
> +struct suspend_stats {
> + int success;
> + int fail;
> + int failed_freeze;
> + int failed_prepare;
> + int failed_suspend;
> + int failed_suspend_noirq;
> + int failed_resume;
> + int failed_resume_noirq;
> +#define REC_FAILED_DEV_NUM 2
> + char failed_devs[REC_FAILED_DEV_NUM][40];
> + int last_failed;
> +};
> +
> +extern struct suspend_stats suspend_stats;
> +
> /**
> * struct platform_suspend_ops - Callbacks for managing platform dependent
> * system sleep states.
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 6c601f8..0977f5d 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -12,6 +12,8 @@
> #include <linux/string.h>
> #include <linux/resume-trace.h>
> #include <linux/workqueue.h>
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
>
> #include "power.h"
>
> @@ -133,6 +135,59 @@ power_attr(pm_test);
>
> #endif /* CONFIG_PM_SLEEP */
>
> +#ifdef CONFIG_DEBUG_FS
> +static int suspend_stats_show(struct seq_file *s, void *unused)
> +{
> + int i, index, last_index;
> +
> + last_index = suspend_stats.last_failed + REC_FAILED_DEV_NUM - 1;
> + last_index %= REC_FAILED_DEV_NUM;
> + seq_printf(s, "%s: %d\n%s: %d\n%s: %d\n%s: %d\n"
> + "%s: %d\n%s: %d\n%s: %d\n%s: %d\n",
> + "success", suspend_stats.success,
> + "fail", suspend_stats.fail,
> + "failed_freeze", suspend_stats.failed_freeze,
> + "failed_prepare", suspend_stats.failed_prepare,
> + "failed_suspend", suspend_stats.failed_suspend,
> + "failed_suspend_noirq",
> + suspend_stats.failed_suspend_noirq,
> + "failed_resume", suspend_stats.failed_resume,
> + "failed_resume_noirq",
> + suspend_stats.failed_resume_noirq);
> + seq_printf(s, "failed_devs:\n last_failed:\t%s\n",
> + suspend_stats.failed_devs[last_index]);
> + for (i = 1; i < REC_FAILED_DEV_NUM; i++) {
> + index = last_index + REC_FAILED_DEV_NUM - i;
> + index %= REC_FAILED_DEV_NUM;
> + seq_printf(s, "\t\t%s\n",
> + suspend_stats.failed_devs[index]);
> + }
> +
> + return 0;
> +}
> +
> +static int suspend_stats_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, suspend_stats_show, NULL);
> +}
> +
> +static const struct file_operations suspend_stats_operations = {
> + .open = suspend_stats_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> +};
> +
> +static int __init pm_debugfs_init(void)
> +{
> + debugfs_create_file("suspend_stats", S_IFREG | S_IRUGO,
> + NULL, NULL, &suspend_stats_operations);
> + return 0;
> +}
> +
> +late_initcall(pm_debugfs_init);
> +#endif /* CONFIG_DEBUG_FS */
> +
> struct kobject *power_kobj;
>
> /**
> @@ -194,6 +249,10 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
> }
> if (state < PM_SUSPEND_MAX && *s)
> error = enter_state(state);
> + if (error)
> + suspend_stats.fail++;
> + else
> + suspend_stats.success++;
> #endif
>
> Exit:
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index b6b71ad..9bb4281 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -106,6 +106,8 @@ static int suspend_prepare(void)
> error = suspend_freeze_processes();
> if (!error)
> return 0;
> + else
> + suspend_stats.failed_freeze++;
>
And same here.
Apart from the above, I don't have complaints.
Thanks,
Rafael
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox