| Message ID | E1YW7sv-0003mA-JP@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Thursday, March 12, 2015 06:31:05 PM Russell King wrote: > Synchronise the PM domain status with runtime PM's status after a > platform device has been probed. This augments the solution in commit > 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach > completes"). > > The above commit added a call to power up the PM domain when a device > attaches to the domain in order to match the behaviour required by > drivers that make no use of runtime PM. The assumption is that the > device driver will cause a runtime PM transition, which will synchronise > the PM domain state with the runtime PM state. > > However, by default, runtime PM will assume that the device is initially > suspended, and some drivers may make use of this should they not need to > touch the hardware during probe. > > In order to allow such drivers, trigger the PM domain code to check > whether the PM domain can be suspended after the probe function, undoing > the effect of the power-on prior to the probe. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> That works for me. Kevin, Ulf? > --- > drivers/base/platform.c | 2 ++ > drivers/base/power/common.c | 15 +++++++++++++++ > drivers/base/power/domain.c | 23 +++++++++++++++++++++++ > include/linux/pm.h | 1 + > include/linux/pm_domain.h | 4 ++++ > 5 files changed, 45 insertions(+) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 9421fed40905..552d1affc060 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev) > ret = drv->probe(dev); > if (ret) > dev_pm_domain_detach(_dev, true); > + else > + dev_pm_domain_sync(_dev); > } > > if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) { > diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c > index b0f138806bbc..8c739a14d3c7 100644 > --- a/drivers/base/power/common.c > +++ b/drivers/base/power/common.c > @@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) > dev->pm_domain->detach(dev, power_off); > } > EXPORT_SYMBOL_GPL(dev_pm_domain_detach); > + > +/** > + * dev_pm_domain_sync - synchronise the PM domain state with its devices > + * @dev: device corresponding with domain > + * > + * Synchronise the PM domain state with the recently probed device, which > + * may be in a variety of PM states. This ensures that a device which > + * enables runtime PM in suspended state, and never transitions to active > + * in its probe handler is properly suspended after the probe. > + */ > +void dev_pm_domain_sync(struct device *dev) > +{ > + if (dev->pm_domain && dev->pm_domain->sync) > + dev->pm_domain->sync(dev); > +} > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 11a1023fa64a..13ae3355dff7 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off) > genpd_queue_power_off_work(pd); > } > > +static void genpd_dev_pm_sync(struct device *dev) > +{ > + struct generic_pm_domain *pd = NULL, *gpd; > + > + if (!dev->pm_domain) > + return; > + > + mutex_lock(&gpd_list_lock); > + list_for_each_entry(gpd, &gpd_list, gpd_list_node) { > + if (&gpd->domain == dev->pm_domain) { > + pd = gpd; > + break; > + } > + } > + mutex_unlock(&gpd_list_lock); > + > + if (!pd) > + return; > + > + genpd_queue_power_off_work(pd); > +} > + > /** > * genpd_dev_pm_attach - Attach a device to its PM domain using DT. > * @dev: Device to attach. > @@ -2223,6 +2245,7 @@ int genpd_dev_pm_attach(struct device *dev) > } > > dev->pm_domain->detach = genpd_dev_pm_detach; > + dev->pm_domain->sync = genpd_dev_pm_sync; > pm_genpd_poweron(pd); > > return 0; > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 8b5976364619..676ca4055239 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -607,6 +607,7 @@ extern int dev_pm_put_subsys_data(struct device *dev); > struct dev_pm_domain { > struct dev_pm_ops ops; > void (*detach)(struct device *dev, bool power_off); > + void (*sync)(struct device *dev); > }; > > /* > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index a9edab2c787a..8d58b30e23ac 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -319,12 +319,16 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np, > #ifdef CONFIG_PM > extern int dev_pm_domain_attach(struct device *dev, bool power_on); > extern void dev_pm_domain_detach(struct device *dev, bool power_off); > +void dev_pm_domain_sync(struct device *dev); > #else > static inline int dev_pm_domain_attach(struct device *dev, bool power_on) > { > return -ENODEV; > } > static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {} > +static inline void dev_pm_domain_sync(struct device *dev) > +{ > +} > #endif > > #endif /* _LINUX_PM_DOMAIN_H */ >
On 12 March 2015 at 19:31, Russell King <rmk+kernel@arm.linux.org.uk> wrote: > Synchronise the PM domain status with runtime PM's status after a > platform device has been probed. This augments the solution in commit > 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach > completes"). > > The above commit added a call to power up the PM domain when a device > attaches to the domain in order to match the behaviour required by > drivers that make no use of runtime PM. The assumption is that the > device driver will cause a runtime PM transition, which will synchronise > the PM domain state with the runtime PM state. > > However, by default, runtime PM will assume that the device is initially > suspended, and some drivers may make use of this should they not need to > touch the hardware during probe. > > In order to allow such drivers, trigger the PM domain code to check > whether the PM domain can be suspended after the probe function, undoing > the effect of the power-on prior to the probe. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > drivers/base/platform.c | 2 ++ Don't we need this for more buses than the platform bus? > drivers/base/power/common.c | 15 +++++++++++++++ > drivers/base/power/domain.c | 23 +++++++++++++++++++++++ > include/linux/pm.h | 1 + > include/linux/pm_domain.h | 4 ++++ > 5 files changed, 45 insertions(+) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 9421fed40905..552d1affc060 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev) > ret = drv->probe(dev); > if (ret) > dev_pm_domain_detach(_dev, true); > + else > + dev_pm_domain_sync(_dev); > } > > if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) { > diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c > index b0f138806bbc..8c739a14d3c7 100644 > --- a/drivers/base/power/common.c > +++ b/drivers/base/power/common.c > @@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) > dev->pm_domain->detach(dev, power_off); > } > EXPORT_SYMBOL_GPL(dev_pm_domain_detach); > + > +/** > + * dev_pm_domain_sync - synchronise the PM domain state with its devices > + * @dev: device corresponding with domain > + * > + * Synchronise the PM domain state with the recently probed device, which > + * may be in a variety of PM states. This ensures that a device which > + * enables runtime PM in suspended state, and never transitions to active > + * in its probe handler is properly suspended after the probe. > + */ > +void dev_pm_domain_sync(struct device *dev) > +{ > + if (dev->pm_domain && dev->pm_domain->sync) > + dev->pm_domain->sync(dev); > +} This is more of a taste and flavour comment, regarding the design approach. To address the issue which @subject patch does, and together with the original problem, which was about making sure a PM domain stays powered during probe, that to me seems like a perfect match for a get/put API. The current solution from commit 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach completes"), is to me just a hacky workaround (which obviously wasn't the proper solution) . $subject patch follows that approach. What do you think of using a get/put approach instead, that would also give us nice symmetry of the API. As you know I have patches available for this, I am happy to post them if needed. > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 11a1023fa64a..13ae3355dff7 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off) > genpd_queue_power_off_work(pd); > } > > +static void genpd_dev_pm_sync(struct device *dev) > +{ > + struct generic_pm_domain *pd = NULL, *gpd; > + > + if (!dev->pm_domain) > + return; > + > + mutex_lock(&gpd_list_lock); > + list_for_each_entry(gpd, &gpd_list, gpd_list_node) { > + if (&gpd->domain == dev->pm_domain) { > + pd = gpd; > + break; > + } > + } > + mutex_unlock(&gpd_list_lock); Walking through the gpd list seems a bit "heavy". Can't we just expect the caller to have a valid generic_pm_domain pointer for its device? > + > + if (!pd) > + return; > + > + genpd_queue_power_off_work(pd); > +} > + > /** > * genpd_dev_pm_attach - Attach a device to its PM domain using DT. > * @dev: Device to attach. > @@ -2223,6 +2245,7 @@ int genpd_dev_pm_attach(struct device *dev) > } > > dev->pm_domain->detach = genpd_dev_pm_detach; > + dev->pm_domain->sync = genpd_dev_pm_sync; > pm_genpd_poweron(pd); > > return 0; > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 8b5976364619..676ca4055239 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -607,6 +607,7 @@ extern int dev_pm_put_subsys_data(struct device *dev); > struct dev_pm_domain { > struct dev_pm_ops ops; > void (*detach)(struct device *dev, bool power_off); > + void (*sync)(struct device *dev); > }; > > /* > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index a9edab2c787a..8d58b30e23ac 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -319,12 +319,16 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np, > #ifdef CONFIG_PM > extern int dev_pm_domain_attach(struct device *dev, bool power_on); > extern void dev_pm_domain_detach(struct device *dev, bool power_off); > +void dev_pm_domain_sync(struct device *dev); > #else > static inline int dev_pm_domain_attach(struct device *dev, bool power_on) > { > return -ENODEV; > } > static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {} > +static inline void dev_pm_domain_sync(struct device *dev) > +{ > +} > #endif > > #endif /* _LINUX_PM_DOMAIN_H */ > -- > 1.8.3.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 13, 2015 at 10:30:59AM +0100, Ulf Hansson wrote: > On 12 March 2015 at 19:31, Russell King <rmk+kernel@arm.linux.org.uk> wrote: > > Synchronise the PM domain status with runtime PM's status after a > > platform device has been probed. This augments the solution in commit > > 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach > > completes"). > > > > The above commit added a call to power up the PM domain when a device > > attaches to the domain in order to match the behaviour required by > > drivers that make no use of runtime PM. The assumption is that the > > device driver will cause a runtime PM transition, which will synchronise > > the PM domain state with the runtime PM state. > > > > However, by default, runtime PM will assume that the device is initially > > suspended, and some drivers may make use of this should they not need to > > touch the hardware during probe. > > > > In order to allow such drivers, trigger the PM domain code to check > > whether the PM domain can be suspended after the probe function, undoing > > the effect of the power-on prior to the probe. > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > --- > > drivers/base/platform.c | 2 ++ > > Don't we need this for more buses than the platform bus? As you very well know, only the platform bus does this automatic binding of a PM domain based on OF - something that you yourself were involved in adding (your sign-off is on the patch adding it, so I assume that you reviewed that patch as thoroughly as you seem to be reviewing mine) which is the cause of my problems. > > drivers/base/power/common.c | 15 +++++++++++++++ > > drivers/base/power/domain.c | 23 +++++++++++++++++++++++ > > include/linux/pm.h | 1 + > > include/linux/pm_domain.h | 4 ++++ > > 5 files changed, 45 insertions(+) > > > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > > index 9421fed40905..552d1affc060 100644 > > --- a/drivers/base/platform.c > > +++ b/drivers/base/platform.c > > @@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev) > > ret = drv->probe(dev); > > if (ret) > > dev_pm_domain_detach(_dev, true); > > + else > > + dev_pm_domain_sync(_dev); > > } > > > > if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) { > > diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c > > index b0f138806bbc..8c739a14d3c7 100644 > > --- a/drivers/base/power/common.c > > +++ b/drivers/base/power/common.c > > @@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) > > dev->pm_domain->detach(dev, power_off); > > } > > EXPORT_SYMBOL_GPL(dev_pm_domain_detach); > > + > > +/** > > + * dev_pm_domain_sync - synchronise the PM domain state with its devices > > + * @dev: device corresponding with domain > > + * > > + * Synchronise the PM domain state with the recently probed device, which > > + * may be in a variety of PM states. This ensures that a device which > > + * enables runtime PM in suspended state, and never transitions to active > > + * in its probe handler is properly suspended after the probe. > > + */ > > +void dev_pm_domain_sync(struct device *dev) > > +{ > > + if (dev->pm_domain && dev->pm_domain->sync) > > + dev->pm_domain->sync(dev); > > +} > > This is more of a taste and flavour comment, regarding the design approach. > > To address the issue which @subject patch does, and together with the > original problem, which was about making sure a PM domain stays > powered during probe, that to me seems like a perfect match for a > get/put API. > > The current solution from commit 2ed127697eb1 ("PM / Domains: Power on > the PM domain right after attach completes"), is to me just a hacky > workaround (which obviously wasn't the proper solution) . $subject > patch follows that approach. > > What do you think of using a get/put approach instead, that would also > give us nice symmetry of the API. As you know I have patches available > for this, I am happy to post them if needed. What I think you're proposing is nothing less than a total rewrite of the PM domain code. If that's what it's going to take to get this stuff in, then I'm just not interested in persuing this anymore, sorry. I don't have the time and effort for that - something that people well know when they send me emails that go unanswered... I'm just trying to fix the problem which you created - and this is the way which was discussed and settled upon. If you _now_ want a different approach, that's up to _you_ to implement. Stop wasting my time. > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 11a1023fa64a..13ae3355dff7 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off) > > genpd_queue_power_off_work(pd); > > } > > > > +static void genpd_dev_pm_sync(struct device *dev) > > +{ > > + struct generic_pm_domain *pd = NULL, *gpd; > > + > > + if (!dev->pm_domain) > > + return; > > + > > + mutex_lock(&gpd_list_lock); > > + list_for_each_entry(gpd, &gpd_list, gpd_list_node) { > > + if (&gpd->domain == dev->pm_domain) { > > + pd = gpd; > > + break; > > + } > > + } > > + mutex_unlock(&gpd_list_lock); > > Walking through the gpd list seems a bit "heavy". Can't we just expect > the caller to have a valid generic_pm_domain pointer for its device? No you can't. See the second patch in this series. dev->pm_domain can contain other stuff which isn't a generic_pm_domain. generic_pm_domain is just one instance of a pm_domain implementation. Others exist. I would have assumed you would know these details as you have decided to co-maintain this code, or if not, you'd be prepared to audit the kernel to find out what might be in dev->pm_domain so that you have due diligence before commenting on something you clearly know nothing about... What I find even _more_ unacceptable is that you are question this, but you didn't question the exact same code which was part of Tomasz Figa's patch to add the OF-based PM domain code. On the face of it, this strikes of double standards - somehow I have to justify my code more than other people do.
On 13 March 2015 at 11:14, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, Mar 13, 2015 at 10:30:59AM +0100, Ulf Hansson wrote: >> On 12 March 2015 at 19:31, Russell King <rmk+kernel@arm.linux.org.uk> wrote: >> > Synchronise the PM domain status with runtime PM's status after a >> > platform device has been probed. This augments the solution in commit >> > 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach >> > completes"). >> > >> > The above commit added a call to power up the PM domain when a device >> > attaches to the domain in order to match the behaviour required by >> > drivers that make no use of runtime PM. The assumption is that the >> > device driver will cause a runtime PM transition, which will synchronise >> > the PM domain state with the runtime PM state. >> > >> > However, by default, runtime PM will assume that the device is initially >> > suspended, and some drivers may make use of this should they not need to >> > touch the hardware during probe. >> > >> > In order to allow such drivers, trigger the PM domain code to check >> > whether the PM domain can be suspended after the probe function, undoing >> > the effect of the power-on prior to the probe. >> > >> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> >> > --- >> > drivers/base/platform.c | 2 ++ >> >> Don't we need this for more buses than the platform bus? > > As you very well know, only the platform bus does this automatic binding > of a PM domain based on OF - something that you yourself were involved in > adding (your sign-off is on the patch adding it, so I assume that you > reviewed that patch as thoroughly as you seem to be reviewing mine) which > is the cause of my problems. Besides the platform bus, we also have spi, sdio, i2c, amba. I was thinking that those should suffer some that same issue you are trying to fix, right? > >> > drivers/base/power/common.c | 15 +++++++++++++++ >> > drivers/base/power/domain.c | 23 +++++++++++++++++++++++ >> > include/linux/pm.h | 1 + >> > include/linux/pm_domain.h | 4 ++++ >> > 5 files changed, 45 insertions(+) >> > >> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c >> > index 9421fed40905..552d1affc060 100644 >> > --- a/drivers/base/platform.c >> > +++ b/drivers/base/platform.c >> > @@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev) >> > ret = drv->probe(dev); >> > if (ret) >> > dev_pm_domain_detach(_dev, true); >> > + else >> > + dev_pm_domain_sync(_dev); >> > } >> > >> > if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) { >> > diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c >> > index b0f138806bbc..8c739a14d3c7 100644 >> > --- a/drivers/base/power/common.c >> > +++ b/drivers/base/power/common.c >> > @@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) >> > dev->pm_domain->detach(dev, power_off); >> > } >> > EXPORT_SYMBOL_GPL(dev_pm_domain_detach); >> > + >> > +/** >> > + * dev_pm_domain_sync - synchronise the PM domain state with its devices >> > + * @dev: device corresponding with domain >> > + * >> > + * Synchronise the PM domain state with the recently probed device, which >> > + * may be in a variety of PM states. This ensures that a device which >> > + * enables runtime PM in suspended state, and never transitions to active >> > + * in its probe handler is properly suspended after the probe. >> > + */ >> > +void dev_pm_domain_sync(struct device *dev) >> > +{ >> > + if (dev->pm_domain && dev->pm_domain->sync) >> > + dev->pm_domain->sync(dev); >> > +} >> >> This is more of a taste and flavour comment, regarding the design approach. >> >> To address the issue which @subject patch does, and together with the >> original problem, which was about making sure a PM domain stays >> powered during probe, that to me seems like a perfect match for a >> get/put API. >> >> The current solution from commit 2ed127697eb1 ("PM / Domains: Power on >> the PM domain right after attach completes"), is to me just a hacky >> workaround (which obviously wasn't the proper solution) . $subject >> patch follows that approach. >> >> What do you think of using a get/put approach instead, that would also >> give us nice symmetry of the API. As you know I have patches available >> for this, I am happy to post them if needed. > > What I think you're proposing is nothing less than a total rewrite of the > PM domain code. No, that's not what I proposed. Sorry if I was unclear. Instead of the >sync() API+callback, my proposual was to add a ->get() and a ->put() API+callback. The APIs will have to be called from the buses probe functions, to make sure the PM domain stays powered during probe and gets gated if possible when leaving probe. That will then make it possible to revert 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach completes") I will (re)post these patches, since it's easier to discuss/look at code directly. > > If that's what it's going to take to get this stuff in, then I'm just not > interested in persuing this anymore, sorry. I don't have the time and > effort for that - something that people well know when they send me emails > that go unanswered... > > I'm just trying to fix the problem which you created - and this is the way > which was discussed and settled upon. If you _now_ want a different > approach, that's up to _you_ to implement. Stop wasting my time. > >> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> > index 11a1023fa64a..13ae3355dff7 100644 >> > --- a/drivers/base/power/domain.c >> > +++ b/drivers/base/power/domain.c >> > @@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off) >> > genpd_queue_power_off_work(pd); >> > } >> > >> > +static void genpd_dev_pm_sync(struct device *dev) >> > +{ >> > + struct generic_pm_domain *pd = NULL, *gpd; >> > + >> > + if (!dev->pm_domain) >> > + return; >> > + >> > + mutex_lock(&gpd_list_lock); >> > + list_for_each_entry(gpd, &gpd_list, gpd_list_node) { >> > + if (&gpd->domain == dev->pm_domain) { >> > + pd = gpd; >> > + break; >> > + } >> > + } >> > + mutex_unlock(&gpd_list_lock); >> >> Walking through the gpd list seems a bit "heavy". Can't we just expect >> the caller to have a valid generic_pm_domain pointer for its device? > > No you can't. See the second patch in this series. dev->pm_domain can > contain other stuff which isn't a generic_pm_domain. generic_pm_domain > is just one instance of a pm_domain implementation. Others exist. > > I would have assumed you would know these details as you have decided to > co-maintain this code, or if not, you'd be prepared to audit the kernel > to find out what might be in dev->pm_domain so that you have due diligence > before commenting on something you clearly know nothing about... > > What I find even _more_ unacceptable is that you are question this, but > you didn't question the exact same code which was part of Tomasz Figa's > patch to add the OF-based PM domain code. On the face of it, this > strikes of double standards - somehow I have to justify my code more > than other people do. I am really sorry if I made you upset Russell, I was trying to ask for you most valuable feedback, but apparently I failed miserably. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 13, 2015 at 10:30:59AM +0100, Ulf Hansson wrote: > On 12 March 2015 at 19:31, Russell King <rmk+kernel@arm.linux.org.uk> wrote: > > Synchronise the PM domain status with runtime PM's status after a > > platform device has been probed. This augments the solution in commit > > 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach > > completes"). > > > > The above commit added a call to power up the PM domain when a device > > attaches to the domain in order to match the behaviour required by > > drivers that make no use of runtime PM. The assumption is that the > > device driver will cause a runtime PM transition, which will synchronise > > the PM domain state with the runtime PM state. > > > > However, by default, runtime PM will assume that the device is initially > > suspended, and some drivers may make use of this should they not need to > > touch the hardware during probe. > > > > In order to allow such drivers, trigger the PM domain code to check > > whether the PM domain can be suspended after the probe function, undoing > > the effect of the power-on prior to the probe. > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > --- > > drivers/base/platform.c | 2 ++ > > Don't we need this for more buses than the platform bus? It's worth noting that I won't be fixing SDIO: sdio_acpi_set_handle(func); ret = device_add(&func->dev); if (ret == 0) { sdio_func_set_present(func); dev_pm_domain_attach(&func->dev, false); } which is inherently racy - the driver can be probed in device_add(). What happened to "setup stuff, then publish"...
Russell King <rmk+kernel@arm.linux.org.uk> writes: > Synchronise the PM domain status with runtime PM's status after a > platform device has been probed. This augments the solution in commit > 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach > completes"). > > The above commit added a call to power up the PM domain when a device > attaches to the domain in order to match the behaviour required by > drivers that make no use of runtime PM. The assumption is that the > device driver will cause a runtime PM transition, which will synchronise > the PM domain state with the runtime PM state. > > However, by default, runtime PM will assume that the device is initially > suspended, and some drivers may make use of this should they not need to > touch the hardware during probe. > > In order to allow such drivers, trigger the PM domain code to check > whether the PM domain can be suspended after the probe function, undoing > the effect of the power-on prior to the probe. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> I think this is a good fix to the existing problem. One minor nit on a comment below, otherwise Acked-by: Kevin Hilman <khilman@linaro.org> [...] > + > +/** > + * dev_pm_domain_sync - synchronise the PM domain state with its devices > + * @dev: device corresponding with domain > + * > + * Synchronise the PM domain state with the recently probed device, which > + * may be in a variety of PM states. This ensures that a device which > + * enables runtime PM in suspended state, and never transitions to active > + * in its probe handler is properly suspended after the probe. > + */ Tt's not the *device* tha needs to be properly suspended after the probe (since it's already/still runtime suspended), but the pm_domain that would be potentially powered down. Hence, I'd reword the last sentence slightly: This ensures that a device which enables runtime PM in suspended state, and never transitions to active in its probe handler gives an opportunity for the PM domain to be powered down after the probe. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 9421fed40905..552d1affc060 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -512,6 +512,8 @@ static int platform_drv_probe(struct device *_dev) ret = drv->probe(dev); if (ret) dev_pm_domain_detach(_dev, true); + else + dev_pm_domain_sync(_dev); } if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) { diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c index b0f138806bbc..8c739a14d3c7 100644 --- a/drivers/base/power/common.c +++ b/drivers/base/power/common.c @@ -134,3 +134,18 @@ void dev_pm_domain_detach(struct device *dev, bool power_off) dev->pm_domain->detach(dev, power_off); } EXPORT_SYMBOL_GPL(dev_pm_domain_detach); + +/** + * dev_pm_domain_sync - synchronise the PM domain state with its devices + * @dev: device corresponding with domain + * + * Synchronise the PM domain state with the recently probed device, which + * may be in a variety of PM states. This ensures that a device which + * enables runtime PM in suspended state, and never transitions to active + * in its probe handler is properly suspended after the probe. + */ +void dev_pm_domain_sync(struct device *dev) +{ + if (dev->pm_domain && dev->pm_domain->sync) + dev->pm_domain->sync(dev); +} diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 11a1023fa64a..13ae3355dff7 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -2157,6 +2157,28 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off) genpd_queue_power_off_work(pd); } +static void genpd_dev_pm_sync(struct device *dev) +{ + struct generic_pm_domain *pd = NULL, *gpd; + + if (!dev->pm_domain) + return; + + mutex_lock(&gpd_list_lock); + list_for_each_entry(gpd, &gpd_list, gpd_list_node) { + if (&gpd->domain == dev->pm_domain) { + pd = gpd; + break; + } + } + mutex_unlock(&gpd_list_lock); + + if (!pd) + return; + + genpd_queue_power_off_work(pd); +} + /** * genpd_dev_pm_attach - Attach a device to its PM domain using DT. * @dev: Device to attach. @@ -2223,6 +2245,7 @@ int genpd_dev_pm_attach(struct device *dev) } dev->pm_domain->detach = genpd_dev_pm_detach; + dev->pm_domain->sync = genpd_dev_pm_sync; pm_genpd_poweron(pd); return 0; diff --git a/include/linux/pm.h b/include/linux/pm.h index 8b5976364619..676ca4055239 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -607,6 +607,7 @@ extern int dev_pm_put_subsys_data(struct device *dev); struct dev_pm_domain { struct dev_pm_ops ops; void (*detach)(struct device *dev, bool power_off); + void (*sync)(struct device *dev); }; /* diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index a9edab2c787a..8d58b30e23ac 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -319,12 +319,16 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np, #ifdef CONFIG_PM extern int dev_pm_domain_attach(struct device *dev, bool power_on); extern void dev_pm_domain_detach(struct device *dev, bool power_off); +void dev_pm_domain_sync(struct device *dev); #else static inline int dev_pm_domain_attach(struct device *dev, bool power_on) { return -ENODEV; } static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {} +static inline void dev_pm_domain_sync(struct device *dev) +{ +} #endif #endif /* _LINUX_PM_DOMAIN_H */
Synchronise the PM domain status with runtime PM's status after a platform device has been probed. This augments the solution in commit 2ed127697eb1 ("PM / Domains: Power on the PM domain right after attach completes"). The above commit added a call to power up the PM domain when a device attaches to the domain in order to match the behaviour required by drivers that make no use of runtime PM. The assumption is that the device driver will cause a runtime PM transition, which will synchronise the PM domain state with the runtime PM state. However, by default, runtime PM will assume that the device is initially suspended, and some drivers may make use of this should they not need to touch the hardware during probe. In order to allow such drivers, trigger the PM domain code to check whether the PM domain can be suspended after the probe function, undoing the effect of the power-on prior to the probe. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/base/platform.c | 2 ++ drivers/base/power/common.c | 15 +++++++++++++++ drivers/base/power/domain.c | 23 +++++++++++++++++++++++ include/linux/pm.h | 1 + include/linux/pm_domain.h | 4 ++++ 5 files changed, 45 insertions(+)