From: Kevin Hilman <khilman@kernel.org> To: Ulf Hansson <ulf.hansson@linaro.org>, "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>, linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, Geert Uytterhoeven <geert+renesas@glider.be>, Alan Stern <stern@rowland.harvard.edu>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Tomasz Figa <tomasz.figa@gmail.com>, Simon Horman <horms@verge.net.au>, Magnus Damm <magnus.damm@gmail.com>, Ben Dooks <ben-linux@fluff.org>, Kukjin Kim <kgene.kim@samsung.com>, Philipp Zabel <philipp.zabel@gmail.com>, Mark Brown <broonie@kernel.org>, Wolfram Sang <wsa@the-dreams.de>, Russell King <linux@arm.linux.org.uk>, Dmitry Torokhov <dmitry.torokhov@gmail.com>, Jack Dai <jack.dai@rock-chips.com>, Jinkun Hong <jinkun.hong@rock-chips.com> Subject: Re: [PATCH v2 0/4] PM / Domains: Fix race conditions during boot Date: Thu, 02 Oct 2014 18:14:42 -0700 [thread overview] Message-ID: <7h38b6rm2l.fsf@deeprootsystems.com> (raw) In-Reply-To: <1412174494-15346-1-git-send-email-ulf.hansson@linaro.org> (Ulf Hansson's message of "Wed, 1 Oct 2014 16:41:30 +0200") Ulf, Rafael, Ulf Hansson <ulf.hansson@linaro.org> writes: > When there are more than one device in a PM domain these will obviously > be probed at different times. Depending on timing and the implemented > support for runtime PM in a driver/subsystem, genpd may be advised to > power off a PM domain after a successful probe sequence. > > Ideally we should have relied on the driver/subsystem, through runtime > PM, to bring their device's PM domain into powered state prior doing > probing if such requirement exist. I think I've stumbled on a related problem, or maybe the same one. Even if platform-specific init code has initialized a device with pm_runtime_set_active(), it seems that the genpd domain can still power off before before all of its devices are probed. This is because pm_genpd_poweroff() requires there to be a driver when it's checking if a device is pm_runtime_suspended() which will not be the case if the driver has not been probed yet. Consider this case: There are several devices in the domain that haven't been probed yet (dev->driver == NULL), but have been marked with pm_runtime_set_active() + _get_noresume(), so pm_runtime_suspended() == false. Then, one of devices is in the domain is probed, and during the probe it does a _get_sync(), sets some stuff up, and then does _put_sync(). After the probe, because of the _put_sync(), the genpd ->runtime_suspend() will be triggered, causing it to attempt a _genpd_poweroff(). Since the rest of the devices in the domain haven't (yet) been probed, their dev->driver pointers are all still NULL, so the pm_runtime_suspended() check will not be attempted for them. The result is that the genpd will poweroff after the first device is probed, but before the others have had a chance to probe, which is not exactly desired behavior for a genpd that has been initialized as powered on. With the hack below[1], I'm able to avoid that problem, but am not completely sure yet if this is safe in general. Rafael, do you remember why that check for dev->driver is needed? Without digging deeper (which I'll do tomorrow), seems to me that checking pm_runtime_suspended() on devices without drivers is a reasonable thing to do since they can be initailzed by platform code before they are probed. If you think this is OK, I'll cook up a real patch with a changelog. Ulf, I'm not sure if this is the same problem you're having, but do you think this would solve your problem if the drivers are properly initialized? Kevin [1] diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 568bf3172bef..17b0d9466d98 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -471,7 +471,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd) if (stat > PM_QOS_FLAGS_NONE) return -EBUSY; - if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev) + if ((!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)) not_suspended++; }
WARNING: multiple messages have this Message-ID (diff)
From: khilman@kernel.org (Kevin Hilman) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2 0/4] PM / Domains: Fix race conditions during boot Date: Thu, 02 Oct 2014 18:14:42 -0700 [thread overview] Message-ID: <7h38b6rm2l.fsf@deeprootsystems.com> (raw) In-Reply-To: <1412174494-15346-1-git-send-email-ulf.hansson@linaro.org> (Ulf Hansson's message of "Wed, 1 Oct 2014 16:41:30 +0200") Ulf, Rafael, Ulf Hansson <ulf.hansson@linaro.org> writes: > When there are more than one device in a PM domain these will obviously > be probed at different times. Depending on timing and the implemented > support for runtime PM in a driver/subsystem, genpd may be advised to > power off a PM domain after a successful probe sequence. > > Ideally we should have relied on the driver/subsystem, through runtime > PM, to bring their device's PM domain into powered state prior doing > probing if such requirement exist. I think I've stumbled on a related problem, or maybe the same one. Even if platform-specific init code has initialized a device with pm_runtime_set_active(), it seems that the genpd domain can still power off before before all of its devices are probed. This is because pm_genpd_poweroff() requires there to be a driver when it's checking if a device is pm_runtime_suspended() which will not be the case if the driver has not been probed yet. Consider this case: There are several devices in the domain that haven't been probed yet (dev->driver == NULL), but have been marked with pm_runtime_set_active() + _get_noresume(), so pm_runtime_suspended() == false. Then, one of devices is in the domain is probed, and during the probe it does a _get_sync(), sets some stuff up, and then does _put_sync(). After the probe, because of the _put_sync(), the genpd ->runtime_suspend() will be triggered, causing it to attempt a _genpd_poweroff(). Since the rest of the devices in the domain haven't (yet) been probed, their dev->driver pointers are all still NULL, so the pm_runtime_suspended() check will not be attempted for them. The result is that the genpd will poweroff after the first device is probed, but before the others have had a chance to probe, which is not exactly desired behavior for a genpd that has been initialized as powered on. With the hack below[1], I'm able to avoid that problem, but am not completely sure yet if this is safe in general. Rafael, do you remember why that check for dev->driver is needed? Without digging deeper (which I'll do tomorrow), seems to me that checking pm_runtime_suspended() on devices without drivers is a reasonable thing to do since they can be initailzed by platform code before they are probed. If you think this is OK, I'll cook up a real patch with a changelog. Ulf, I'm not sure if this is the same problem you're having, but do you think this would solve your problem if the drivers are properly initialized? Kevin [1] diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 568bf3172bef..17b0d9466d98 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -471,7 +471,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd) if (stat > PM_QOS_FLAGS_NONE) return -EBUSY; - if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev) + if ((!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)) not_suspended++; }
next prev parent reply other threads:[~2014-10-03 1:14 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-10-01 14:41 [PATCH v2 0/4] PM / Domains: Fix race conditions during boot Ulf Hansson 2014-10-01 14:41 ` Ulf Hansson 2014-10-01 14:41 ` [PATCH v2 1/4] PM / Domains: Remove pm_genpd_dev_need_restore() API Ulf Hansson 2014-10-01 14:41 ` Ulf Hansson 2014-10-01 16:36 ` Sylwester Nawrocki 2014-10-01 16:36 ` Sylwester Nawrocki 2014-10-02 9:09 ` Ulf Hansson 2014-10-02 9:09 ` Ulf Hansson 2014-10-02 12:00 ` Sylwester Nawrocki 2014-10-02 12:00 ` Sylwester Nawrocki 2014-10-02 13:30 ` Ulf Hansson 2014-10-02 13:30 ` Ulf Hansson 2014-10-02 15:54 ` Sylwester Nawrocki 2014-10-02 15:54 ` Sylwester Nawrocki 2014-10-03 10:36 ` Ulf Hansson 2014-10-03 10:36 ` Ulf Hansson 2014-11-06 15:57 ` Sylwester Nawrocki 2014-11-06 15:57 ` Sylwester Nawrocki 2014-11-06 19:05 ` Ulf Hansson 2014-11-06 19:05 ` Ulf Hansson 2014-10-01 14:41 ` [PATCH v2 2/4] ARM: exynos: Ensure PM domains are powered at initialization Ulf Hansson 2014-10-01 14:41 ` Ulf Hansson 2014-10-01 16:18 ` Sylwester Nawrocki 2014-10-01 16:18 ` Sylwester Nawrocki 2014-10-01 19:50 ` Rafael J. Wysocki 2014-10-01 19:50 ` Rafael J. Wysocki 2014-10-02 9:42 ` Ulf Hansson 2014-10-02 9:42 ` Ulf Hansson 2014-10-02 9:55 ` Ulf Hansson 2014-10-02 9:55 ` Ulf Hansson 2014-10-01 14:41 ` [PATCH v2 3/4] PM / Domains: Expect PM domains being " Ulf Hansson 2014-10-01 14:41 ` Ulf Hansson 2014-10-01 23:50 ` Simon Horman 2014-10-01 23:50 ` Simon Horman 2014-10-01 14:41 ` [PATCH v2 4/4] PM / Domains: Enforce PM domains to stay powered during boot Ulf Hansson 2014-10-01 14:41 ` Ulf Hansson 2014-10-03 1:14 ` Kevin Hilman [this message] 2014-10-03 1:14 ` [PATCH v2 0/4] PM / Domains: Fix race conditions " Kevin Hilman 2014-10-03 9:47 ` Ulf Hansson 2014-10-03 9:47 ` Ulf Hansson 2014-10-03 15:10 ` Kevin Hilman 2014-10-03 15:10 ` Kevin Hilman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=7h38b6rm2l.fsf@deeprootsystems.com \ --to=khilman@kernel.org \ --cc=ben-linux@fluff.org \ --cc=broonie@kernel.org \ --cc=dmitry.torokhov@gmail.com \ --cc=geert+renesas@glider.be \ --cc=gregkh@linuxfoundation.org \ --cc=horms@verge.net.au \ --cc=jack.dai@rock-chips.com \ --cc=jinkun.hong@rock-chips.com \ --cc=kgene.kim@samsung.com \ --cc=len.brown@intel.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-pm@vger.kernel.org \ --cc=linux-samsung-soc@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=magnus.damm@gmail.com \ --cc=pavel@ucw.cz \ --cc=philipp.zabel@gmail.com \ --cc=rjw@rjwysocki.net \ --cc=stern@rowland.harvard.edu \ --cc=tomasz.figa@gmail.com \ --cc=ulf.hansson@linaro.org \ --cc=wsa@the-dreams.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.