[PATCH 0/3] phy: core: Re-work runtime PM deployment and fix an issue

[PATCH 0/3] phy: core: Re-work runtime PM deployment and fix an issue

[Ulf Hansson]

The intend of this series is to simplify the runtime PM deployment in the phy
core, but while doing that fixing a related problem for a Renesas SoC making use
of a phy. This issue was raised by Geert Uytterhoeven and Yoshihiro Shimoda, due
to a changed behaviour in the runtime PM core.

The commit in question is f8817f61e821 (PM / runtime: Drop children check from
__pm_runtime_set_status()). However, it turned out that the problem existed
already prior this change, only that the error was printed instead of giving a
WARN splat.

More details about the changes are available in each change log. For additional
background, please have look at the link below.

https://patchwork.kernel.org/patch/10086651/

Ulf Hansson (3):
  phy: core: Move runtime PM reference counting to the parent device
  phy: core: Drop unused runtime PM APIs
  phy: core: Update the runtime PM section in the docs to reflect
    changes

 Documentation/phy.txt   | 28 +++++++-------
 drivers/phy/phy-core.c  | 99 +++++++------------------------------------------
 include/linux/phy/phy.h | 45 ----------------------
 3 files changed, 28 insertions(+), 144 deletions(-)

-- 
2.7.4

[PATCH 1/3] phy: core: Move runtime PM reference counting to the parent device

[Ulf Hansson]

The runtime PM deployment in the phy core is a bit unnecessary complicated
and the main reason is because it operates on the phy device, which is
created by the phy core and assigned as a child device of the phy provider
device.

Let's simplify the code, by replacing the existing calls to
phy_pm_runtime_get_sync() and phy_pm_runtime_put(), with regular calls to
pm_runtime_get_sync() and pm_runtime_put(). While doing that, let's also
change to give the phy provider device as the parameter to the runtime PM
calls. This together with adding error paths, that allows the phy
provider device to be runtime PM disabled, enables further clean up the
code. More precisely, we can simply avoid to enable runtime PM for the phy
device altogether, so let's do that as well.

More importantly, this change also fixes an issue for system suspend.
Especially in those cases when the phy provider device gets put into a low
power state via calling the pm_runtime_force_suspend() helper, as is the
case for a Renesas SoC, which has the phy provider device attached to the
generic PM domain.

The problem in this case, is that pm_runtime_force_suspend() expects the
child device of the provider device to be runtime suspended, else this will
trigger a WARN splat (correctly) when runtime PM gets re-enabled at system
resume.

In the current case, even if phy_power_off() triggers a pm_runtime_put()
during system suspend the phy device (child) doesn't get runtime suspended,
because that is prevented in the system suspend phases. However, by
avoiding to enable runtime PM, this problem goes away.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/phy/phy-core.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index b4964b0..9fa3f13 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -222,10 +222,10 @@ int phy_init(struct phy *phy)
 	if (!phy)
 		return 0;
 
-	ret = phy_pm_runtime_get_sync(phy);
-	if (ret < 0 && ret != -ENOTSUPP)
+	ret = pm_runtime_get_sync(phy->dev.parent);
+	if (ret < 0 && ret != -EACCES)
 		return ret;
-	ret = 0; /* Override possible ret == -ENOTSUPP */
+	ret = 0;
 
 	mutex_lock(&phy->mutex);
 	if (phy->init_count == 0 && phy->ops->init) {
@@ -239,7 +239,7 @@ int phy_init(struct phy *phy)
 
 out:
 	mutex_unlock(&phy->mutex);
-	phy_pm_runtime_put(phy);
+	pm_runtime_put(phy->dev.parent);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(phy_init);
@@ -251,10 +251,10 @@ int phy_exit(struct phy *phy)
 	if (!phy)
 		return 0;
 
-	ret = phy_pm_runtime_get_sync(phy);
-	if (ret < 0 && ret != -ENOTSUPP)
+	ret = pm_runtime_get_sync(phy->dev.parent);
+	if (ret < 0 && ret != -EACCES)
 		return ret;
-	ret = 0; /* Override possible ret == -ENOTSUPP */
+	ret = 0;
 
 	mutex_lock(&phy->mutex);
 	if (phy->init_count == 1 && phy->ops->exit) {
@@ -268,7 +268,7 @@ int phy_exit(struct phy *phy)
 
 out:
 	mutex_unlock(&phy->mutex);
-	phy_pm_runtime_put(phy);
+	pm_runtime_put(phy->dev.parent);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(phy_exit);
@@ -286,11 +286,10 @@ int phy_power_on(struct phy *phy)
 			goto out;
 	}
 
-	ret = phy_pm_runtime_get_sync(phy);
-	if (ret < 0 && ret != -ENOTSUPP)
+	ret = pm_runtime_get_sync(phy->dev.parent);
+	if (ret < 0 && ret != -EACCES)
 		goto err_pm_sync;
-
-	ret = 0; /* Override possible ret == -ENOTSUPP */
+	ret = 0;
 
 	mutex_lock(&phy->mutex);
 	if (phy->power_count == 0 && phy->ops->power_on) {
@@ -306,7 +305,7 @@ int phy_power_on(struct phy *phy)
 
 err_pwr_on:
 	mutex_unlock(&phy->mutex);
-	phy_pm_runtime_put_sync(phy);
+	pm_runtime_put(phy->dev.parent);
 err_pm_sync:
 	if (phy->pwr)
 		regulator_disable(phy->pwr);
@@ -333,7 +332,7 @@ int phy_power_off(struct phy *phy)
 	}
 	--phy->power_count;
 	mutex_unlock(&phy->mutex);
-	phy_pm_runtime_put(phy);
+	pm_runtime_put(phy->dev.parent);
 
 	if (phy->pwr)
 		regulator_disable(phy->pwr);
@@ -774,11 +773,6 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
 	if (ret)
 		goto put_dev;
 
-	if (pm_runtime_enabled(dev)) {
-		pm_runtime_enable(&phy->dev);
-		pm_runtime_no_callbacks(&phy->dev);
-	}
-
 	return phy;
 
 put_dev:
@@ -831,7 +825,6 @@ EXPORT_SYMBOL_GPL(devm_phy_create);
  */
 void phy_destroy(struct phy *phy)
 {
-	pm_runtime_disable(&phy->dev);
 	device_unregister(&phy->dev);
 }
 EXPORT_SYMBOL_GPL(phy_destroy);
-- 
2.7.4

[PATCH 2/3] phy: core: Drop unused runtime PM APIs

[Ulf Hansson]

The phy core already deploys runtime PM management, so there seems to be no
reason for having a separate option of controlling runtime PM for phys via
the phy runtime PM APIs.

Moreover, since previous changes moved the runtime PM reference counting
onto the parent provider device, which also avoid to enabled runtime PM for
the phy child device, the APIs becomes NOOP. Therefore, let's remove them
altogether.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/phy/phy-core.c  | 66 -------------------------------------------------
 include/linux/phy/phy.h | 45 ---------------------------------
 2 files changed, 111 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 9fa3f13..60818e1 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -149,72 +149,6 @@ static struct phy_provider *of_phy_provider_lookup(struct device_node *node)
 	return ERR_PTR(-EPROBE_DEFER);
 }
 
-int phy_pm_runtime_get(struct phy *phy)
-{
-	int ret;
-
-	if (!pm_runtime_enabled(&phy->dev))
-		return -ENOTSUPP;
-
-	ret = pm_runtime_get(&phy->dev);
-	if (ret < 0 && ret != -EINPROGRESS)
-		pm_runtime_put_noidle(&phy->dev);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(phy_pm_runtime_get);
-
-int phy_pm_runtime_get_sync(struct phy *phy)
-{
-	int ret;
-
-	if (!pm_runtime_enabled(&phy->dev))
-		return -ENOTSUPP;
-
-	ret = pm_runtime_get_sync(&phy->dev);
-	if (ret < 0)
-		pm_runtime_put_sync(&phy->dev);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(phy_pm_runtime_get_sync);
-
-int phy_pm_runtime_put(struct phy *phy)
-{
-	if (!pm_runtime_enabled(&phy->dev))
-		return -ENOTSUPP;
-
-	return pm_runtime_put(&phy->dev);
-}
-EXPORT_SYMBOL_GPL(phy_pm_runtime_put);
-
-int phy_pm_runtime_put_sync(struct phy *phy)
-{
-	if (!pm_runtime_enabled(&phy->dev))
-		return -ENOTSUPP;
-
-	return pm_runtime_put_sync(&phy->dev);
-}
-EXPORT_SYMBOL_GPL(phy_pm_runtime_put_sync);
-
-void phy_pm_runtime_allow(struct phy *phy)
-{
-	if (!pm_runtime_enabled(&phy->dev))
-		return;
-
-	pm_runtime_allow(&phy->dev);
-}
-EXPORT_SYMBOL_GPL(phy_pm_runtime_allow);
-
-void phy_pm_runtime_forbid(struct phy *phy)
-{
-	if (!pm_runtime_enabled(&phy->dev))
-		return;
-
-	pm_runtime_forbid(&phy->dev);
-}
-EXPORT_SYMBOL_GPL(phy_pm_runtime_forbid);
-
 int phy_init(struct phy *phy)
 {
 	int ret;
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 4f8423a..29871aaa 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -17,7 +17,6 @@
 #include <linux/err.h>
 #include <linux/of.h>
 #include <linux/device.h>
-#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 
 struct phy;
@@ -133,12 +132,6 @@ static inline void *phy_get_drvdata(struct phy *phy)
 }
 
 #if IS_ENABLED(CONFIG_GENERIC_PHY)
-int phy_pm_runtime_get(struct phy *phy);
-int phy_pm_runtime_get_sync(struct phy *phy);
-int phy_pm_runtime_put(struct phy *phy);
-int phy_pm_runtime_put_sync(struct phy *phy);
-void phy_pm_runtime_allow(struct phy *phy);
-void phy_pm_runtime_forbid(struct phy *phy);
 int phy_init(struct phy *phy);
 int phy_exit(struct phy *phy);
 int phy_power_on(struct phy *phy);
@@ -187,44 +180,6 @@ void devm_of_phy_provider_unregister(struct device *dev,
 int phy_create_lookup(struct phy *phy, const char *con_id, const char *dev_id);
 void phy_remove_lookup(struct phy *phy, const char *con_id, const char *dev_id);
 #else
-static inline int phy_pm_runtime_get(struct phy *phy)
-{
-	if (!phy)
-		return 0;
-	return -ENOSYS;
-}
-
-static inline int phy_pm_runtime_get_sync(struct phy *phy)
-{
-	if (!phy)
-		return 0;
-	return -ENOSYS;
-}
-
-static inline int phy_pm_runtime_put(struct phy *phy)
-{
-	if (!phy)
-		return 0;
-	return -ENOSYS;
-}
-
-static inline int phy_pm_runtime_put_sync(struct phy *phy)
-{
-	if (!phy)
-		return 0;
-	return -ENOSYS;
-}
-
-static inline void phy_pm_runtime_allow(struct phy *phy)
-{
-	return;
-}
-
-static inline void phy_pm_runtime_forbid(struct phy *phy)
-{
-	return;
-}
-
 static inline int phy_init(struct phy *phy)
 {
 	if (!phy)
-- 
2.7.4

[PATCH 3/3] phy: core: Update the runtime PM section in the docs to reflect changes

[Ulf Hansson]

Let's update and clarify he phy documentation, to reflect the latest
changes around the runtime PM deployment in the phy core.

Cc: Jonathan Corbet <corbet@lwn.net>
Cc: linux-doc@vger.kernel.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 Documentation/phy.txt | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/Documentation/phy.txt b/Documentation/phy.txt
index 457c3e0..6d9e3d3 100644
--- a/Documentation/phy.txt
+++ b/Documentation/phy.txt
@@ -160,19 +160,21 @@ associated with this PHY.
 PM Runtime
 ==========
 
-This subsystem is pm runtime enabled. So while creating the PHY,
-pm_runtime_enable of the phy device created by this subsystem is called and
-while destroying the PHY, pm_runtime_disable is called. Note that the phy
-device created by this subsystem will be a child of the device that calls
-phy_create (PHY provider device).
-
-So pm_runtime_get_sync of the phy_device created by this subsystem will invoke
-pm_runtime_get_sync of PHY provider device because of parent-child relationship.
-It should also be noted that phy_power_on and phy_power_off performs
-phy_pm_runtime_get_sync and phy_pm_runtime_put respectively.
-There are exported APIs like phy_pm_runtime_get, phy_pm_runtime_get_sync,
-phy_pm_runtime_put, phy_pm_runtime_put_sync, phy_pm_runtime_allow and
-phy_pm_runtime_forbid for performing PM operations.
+This subsystem deploys runtime PM support. More precisely, calls to
+pm_runtime_get_sync() and to pm_runtime_put() surrounds calls to the phy
+provider callbacks, ->init|exit(), in phy_init|exit(). At phy_power_on(), the
+runtime PM usage count is raised again, via pm_runtime_get_sync(). The usage
+count remain raised, until the internal phy power on count reaches zero in
+phy_power_off(), at which point pm_runtime_put() is called to restore the
+runtime PM usage count. In this way, the device is guranteed to stay runtime
+resumed as long as the phy is powered on.
+
+In regards to the runtime PM deployment in the phy core, it should also be
+noted that it's deployed for the phy provider device, which is the parent of
+the phy child device. In other words, the phy device created by the phy core
+remains runtime PM disabled. Of course, whether runtime PM is really used or
+not, depends on whether the phy provider driver has enabled runtime PM for its
+provider device.
 
 PHY Mappings
 ============
-- 
2.7.4

Re: [PATCH 1/3] phy: core: Move runtime PM reference counting to the parent device

[Kishon Vijay Abraham I]

Hi Ulf,

On Wednesday 20 December 2017 02:52 AM, Ulf Hansson wrote:
> The runtime PM deployment in the phy core is a bit unnecessary complicated
> and the main reason is because it operates on the phy device, which is
> created by the phy core and assigned as a child device of the phy provider
> device.
> 
> Let's simplify the code, by replacing the existing calls to
> phy_pm_runtime_get_sync() and phy_pm_runtime_put(), with regular calls to
> pm_runtime_get_sync() and pm_runtime_put(). While doing that, let's also
> change to give the phy provider device as the parameter to the runtime PM
> calls. This together with adding error paths, that allows the phy
> provider device to be runtime PM disabled, enables further clean up the
> code. More precisely, we can simply avoid to enable runtime PM for the phy
> device altogether, so let's do that as well.
> 
> More importantly, this change also fixes an issue for system suspend.
> Especially in those cases when the phy provider device gets put into a low
> power state via calling the pm_runtime_force_suspend() helper, as is the
> case for a Renesas SoC, which has the phy provider device attached to the
> generic PM domain.
> 
> The problem in this case, is that pm_runtime_force_suspend() expects the
> child device of the provider device to be runtime suspended, else this will
> trigger a WARN splat (correctly) when runtime PM gets re-enabled at system
> resume.
> 
> In the current case, even if phy_power_off() triggers a pm_runtime_put()
> during system suspend the phy device (child) doesn't get runtime suspended,
> because that is prevented in the system suspend phases. However, by
> avoiding to enable runtime PM, this problem goes away.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/phy/phy-core.c | 33 +++++++++++++--------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index b4964b0..9fa3f13 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -222,10 +222,10 @@ int phy_init(struct phy *phy)
>  	if (!phy)
>  		return 0;
>  
> -	ret = phy_pm_runtime_get_sync(phy);
> -	if (ret < 0 && ret != -ENOTSUPP)
> +	ret = pm_runtime_get_sync(phy->dev.parent);

Won't this make phy-core manage pm_runtime of phy_provider even though the
phy_provider might not intend it?

Thanks
Kishon

Re: [PATCH 1/3] phy: core: Move runtime PM reference counting to the parent device

[Kishon Vijay Abraham I]

Hi Ulf,

On Wednesday 20 December 2017 02:52 AM, Ulf Hansson wrote:
> The runtime PM deployment in the phy core is a bit unnecessary complicated
> and the main reason is because it operates on the phy device, which is
> created by the phy core and assigned as a child device of the phy provider
> device.
> 
> Let's simplify the code, by replacing the existing calls to
> phy_pm_runtime_get_sync() and phy_pm_runtime_put(), with regular calls to
> pm_runtime_get_sync() and pm_runtime_put(). While doing that, let's also
> change to give the phy provider device as the parameter to the runtime PM
> calls. This together with adding error paths, that allows the phy
> provider device to be runtime PM disabled, enables further clean up the
> code. More precisely, we can simply avoid to enable runtime PM for the phy
> device altogether, so let's do that as well.
> 
> More importantly, this change also fixes an issue for system suspend.
> Especially in those cases when the phy provider device gets put into a low
> power state via calling the pm_runtime_force_suspend() helper, as is the
> case for a Renesas SoC, which has the phy provider device attached to the
> generic PM domain.
> 
> The problem in this case, is that pm_runtime_force_suspend() expects the
> child device of the provider device to be runtime suspended, else this will
> trigger a WARN splat (correctly) when runtime PM gets re-enabled at system
> resume.
> 
> In the current case, even if phy_power_off() triggers a pm_runtime_put()
> during system suspend the phy device (child) doesn't get runtime suspended,
> because that is prevented in the system suspend phases. However, by
> avoiding to enable runtime PM, this problem goes away.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/phy/phy-core.c | 33 +++++++++++++--------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index b4964b0..9fa3f13 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -222,10 +222,10 @@ int phy_init(struct phy *phy)
>  	if (!phy)
>  		return 0;
>  
> -	ret = phy_pm_runtime_get_sync(phy);
> -	if (ret < 0 && ret != -ENOTSUPP)
> +	ret = pm_runtime_get_sync(phy->dev.parent);

Won't this make phy-core manage pm_runtime of phy_provider even though the
phy_provider might not intend it?

Thanks
Kishon

Re: [PATCH 1/3] phy: core: Move runtime PM reference counting to the parent device

[Ulf Hansson]

On 20 December 2017 at 07:42, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> Hi Ulf,
>
> On Wednesday 20 December 2017 02:52 AM, Ulf Hansson wrote:
>> The runtime PM deployment in the phy core is a bit unnecessary complicated
>> and the main reason is because it operates on the phy device, which is
>> created by the phy core and assigned as a child device of the phy provider
>> device.
>>
>> Let's simplify the code, by replacing the existing calls to
>> phy_pm_runtime_get_sync() and phy_pm_runtime_put(), with regular calls to
>> pm_runtime_get_sync() and pm_runtime_put(). While doing that, let's also
>> change to give the phy provider device as the parameter to the runtime PM
>> calls. This together with adding error paths, that allows the phy
>> provider device to be runtime PM disabled, enables further clean up the
>> code. More precisely, we can simply avoid to enable runtime PM for the phy
>> device altogether, so let's do that as well.
>>
>> More importantly, this change also fixes an issue for system suspend.
>> Especially in those cases when the phy provider device gets put into a low
>> power state via calling the pm_runtime_force_suspend() helper, as is the
>> case for a Renesas SoC, which has the phy provider device attached to the
>> generic PM domain.
>>
>> The problem in this case, is that pm_runtime_force_suspend() expects the
>> child device of the provider device to be runtime suspended, else this will
>> trigger a WARN splat (correctly) when runtime PM gets re-enabled at system
>> resume.
>>
>> In the current case, even if phy_power_off() triggers a pm_runtime_put()
>> during system suspend the phy device (child) doesn't get runtime suspended,
>> because that is prevented in the system suspend phases. However, by
>> avoiding to enable runtime PM, this problem goes away.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/phy/phy-core.c | 33 +++++++++++++--------------------
>>  1 file changed, 13 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>> index b4964b0..9fa3f13 100644
>> --- a/drivers/phy/phy-core.c
>> +++ b/drivers/phy/phy-core.c
>> @@ -222,10 +222,10 @@ int phy_init(struct phy *phy)
>>       if (!phy)
>>               return 0;
>>
>> -     ret = phy_pm_runtime_get_sync(phy);
>> -     if (ret < 0 && ret != -ENOTSUPP)
>> +     ret = pm_runtime_get_sync(phy->dev.parent);
>
> Won't this make phy-core manage pm_runtime of phy_provider even though the
> phy_provider might not intend it?

No it shouldn't.

There are two cases to consider around this.

1) CONFIG_PM is unset. In this case pm_runtime_get_sync() will return
1, which is treated as succeeds by the error path.

2) CONFIG_PM is set, but the phy provider don't use runtime PM, thus
it hasn't called pm_runtime_enable() for its device. In this case,
pm_runtime_get_sync() returns -EACCES, which is also treated as
success by the error path.

Does it make sense?

Kind regards
Uffe

Re: [PATCH 1/3] phy: core: Move runtime PM reference counting to the parent device

[Kishon Vijay Abraham I]

Hi Ulf,

On Wednesday 20 December 2017 02:05 PM, Ulf Hansson wrote:
> On 20 December 2017 at 07:42, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> Hi Ulf,
>>
>> On Wednesday 20 December 2017 02:52 AM, Ulf Hansson wrote:
>>> The runtime PM deployment in the phy core is a bit unnecessary complicated
>>> and the main reason is because it operates on the phy device, which is
>>> created by the phy core and assigned as a child device of the phy provider
>>> device.
>>>
>>> Let's simplify the code, by replacing the existing calls to
>>> phy_pm_runtime_get_sync() and phy_pm_runtime_put(), with regular calls to
>>> pm_runtime_get_sync() and pm_runtime_put(). While doing that, let's also
>>> change to give the phy provider device as the parameter to the runtime PM
>>> calls. This together with adding error paths, that allows the phy
>>> provider device to be runtime PM disabled, enables further clean up the
>>> code. More precisely, we can simply avoid to enable runtime PM for the phy
>>> device altogether, so let's do that as well.
>>>
>>> More importantly, this change also fixes an issue for system suspend.
>>> Especially in those cases when the phy provider device gets put into a low
>>> power state via calling the pm_runtime_force_suspend() helper, as is the
>>> case for a Renesas SoC, which has the phy provider device attached to the
>>> generic PM domain.
>>>
>>> The problem in this case, is that pm_runtime_force_suspend() expects the
>>> child device of the provider device to be runtime suspended, else this will
>>> trigger a WARN splat (correctly) when runtime PM gets re-enabled at system
>>> resume.
>>>
>>> In the current case, even if phy_power_off() triggers a pm_runtime_put()
>>> during system suspend the phy device (child) doesn't get runtime suspended,
>>> because that is prevented in the system suspend phases. However, by
>>> avoiding to enable runtime PM, this problem goes away.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/phy/phy-core.c | 33 +++++++++++++--------------------
>>>  1 file changed, 13 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>> index b4964b0..9fa3f13 100644
>>> --- a/drivers/phy/phy-core.c
>>> +++ b/drivers/phy/phy-core.c
>>> @@ -222,10 +222,10 @@ int phy_init(struct phy *phy)
>>>       if (!phy)
>>>               return 0;
>>>
>>> -     ret = phy_pm_runtime_get_sync(phy);
>>> -     if (ret < 0 && ret != -ENOTSUPP)
>>> +     ret = pm_runtime_get_sync(phy->dev.parent);
>>
>> Won't this make phy-core manage pm_runtime of phy_provider even though the
>> phy_provider might not intend it?
> 
> No it shouldn't.
> 
> There are two cases to consider around this.
> 
> 1) CONFIG_PM is unset. In this case pm_runtime_get_sync() will return
> 1, which is treated as succeeds by the error path.
> 
> 2) CONFIG_PM is set, but the phy provider don't use runtime PM, thus
> it hasn't called pm_runtime_enable() for its device. In this case,
> pm_runtime_get_sync() returns -EACCES, which is also treated as
> success by the error path.

There can be a case where the phy_provider uses runtime PM but doesn't want
phy-core to manage it.

Thanks
Kishon

Re: [PATCH 1/3] phy: core: Move runtime PM reference counting to the parent device

[Ulf Hansson]

On 20 December 2017 at 10:02, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> Hi Ulf,
>
> On Wednesday 20 December 2017 02:05 PM, Ulf Hansson wrote:
>> On 20 December 2017 at 07:42, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>> Hi Ulf,
>>>
>>> On Wednesday 20 December 2017 02:52 AM, Ulf Hansson wrote:
>>>> The runtime PM deployment in the phy core is a bit unnecessary complicated
>>>> and the main reason is because it operates on the phy device, which is
>>>> created by the phy core and assigned as a child device of the phy provider
>>>> device.
>>>>
>>>> Let's simplify the code, by replacing the existing calls to
>>>> phy_pm_runtime_get_sync() and phy_pm_runtime_put(), with regular calls to
>>>> pm_runtime_get_sync() and pm_runtime_put(). While doing that, let's also
>>>> change to give the phy provider device as the parameter to the runtime PM
>>>> calls. This together with adding error paths, that allows the phy
>>>> provider device to be runtime PM disabled, enables further clean up the
>>>> code. More precisely, we can simply avoid to enable runtime PM for the phy
>>>> device altogether, so let's do that as well.
>>>>
>>>> More importantly, this change also fixes an issue for system suspend.
>>>> Especially in those cases when the phy provider device gets put into a low
>>>> power state via calling the pm_runtime_force_suspend() helper, as is the
>>>> case for a Renesas SoC, which has the phy provider device attached to the
>>>> generic PM domain.
>>>>
>>>> The problem in this case, is that pm_runtime_force_suspend() expects the
>>>> child device of the provider device to be runtime suspended, else this will
>>>> trigger a WARN splat (correctly) when runtime PM gets re-enabled at system
>>>> resume.
>>>>
>>>> In the current case, even if phy_power_off() triggers a pm_runtime_put()
>>>> during system suspend the phy device (child) doesn't get runtime suspended,
>>>> because that is prevented in the system suspend phases. However, by
>>>> avoiding to enable runtime PM, this problem goes away.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> ---
>>>>  drivers/phy/phy-core.c | 33 +++++++++++++--------------------
>>>>  1 file changed, 13 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>>> index b4964b0..9fa3f13 100644
>>>> --- a/drivers/phy/phy-core.c
>>>> +++ b/drivers/phy/phy-core.c
>>>> @@ -222,10 +222,10 @@ int phy_init(struct phy *phy)
>>>>       if (!phy)
>>>>               return 0;
>>>>
>>>> -     ret = phy_pm_runtime_get_sync(phy);
>>>> -     if (ret < 0 && ret != -ENOTSUPP)
>>>> +     ret = pm_runtime_get_sync(phy->dev.parent);
>>>
>>> Won't this make phy-core manage pm_runtime of phy_provider even though the
>>> phy_provider might not intend it?
>>
>> No it shouldn't.
>>
>> There are two cases to consider around this.
>>
>> 1) CONFIG_PM is unset. In this case pm_runtime_get_sync() will return
>> 1, which is treated as succeeds by the error path.
>>
>> 2) CONFIG_PM is set, but the phy provider don't use runtime PM, thus
>> it hasn't called pm_runtime_enable() for its device. In this case,
>> pm_runtime_get_sync() returns -EACCES, which is also treated as
>> success by the error path.
>
> There can be a case where the phy_provider uses runtime PM but doesn't want
> phy-core to manage it.

Ah, so you mean there are cases when the provider driver calls
pm_runtime_enable() *after* it calls phy_create()/dev_phy_create()
instead of before?

I am not really sure I understand *why* a provider driver wants to do
that though, do you have more details?
I mean, even if the phy core handles runtime PM, additional management
can be done on top in the phy provider, there is nothing preventing
that, but I guess that isn't sufficient?

Kind regards
Uffe

Re: [PATCH 1/3] phy: core: Move runtime PM reference counting to the parent device

[Ulf Hansson]

On 20 December 2017 at 13:08, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 20 December 2017 at 10:02, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> Hi Ulf,
>>
>> On Wednesday 20 December 2017 02:05 PM, Ulf Hansson wrote:
>>> On 20 December 2017 at 07:42, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>> Hi Ulf,
>>>>
>>>> On Wednesday 20 December 2017 02:52 AM, Ulf Hansson wrote:
>>>>> The runtime PM deployment in the phy core is a bit unnecessary complicated
>>>>> and the main reason is because it operates on the phy device, which is
>>>>> created by the phy core and assigned as a child device of the phy provider
>>>>> device.
>>>>>
>>>>> Let's simplify the code, by replacing the existing calls to
>>>>> phy_pm_runtime_get_sync() and phy_pm_runtime_put(), with regular calls to
>>>>> pm_runtime_get_sync() and pm_runtime_put(). While doing that, let's also
>>>>> change to give the phy provider device as the parameter to the runtime PM
>>>>> calls. This together with adding error paths, that allows the phy
>>>>> provider device to be runtime PM disabled, enables further clean up the
>>>>> code. More precisely, we can simply avoid to enable runtime PM for the phy
>>>>> device altogether, so let's do that as well.
>>>>>
>>>>> More importantly, this change also fixes an issue for system suspend.
>>>>> Especially in those cases when the phy provider device gets put into a low
>>>>> power state via calling the pm_runtime_force_suspend() helper, as is the
>>>>> case for a Renesas SoC, which has the phy provider device attached to the
>>>>> generic PM domain.
>>>>>
>>>>> The problem in this case, is that pm_runtime_force_suspend() expects the
>>>>> child device of the provider device to be runtime suspended, else this will
>>>>> trigger a WARN splat (correctly) when runtime PM gets re-enabled at system
>>>>> resume.
>>>>>
>>>>> In the current case, even if phy_power_off() triggers a pm_runtime_put()
>>>>> during system suspend the phy device (child) doesn't get runtime suspended,
>>>>> because that is prevented in the system suspend phases. However, by
>>>>> avoiding to enable runtime PM, this problem goes away.
>>>>>
>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>> ---
>>>>>  drivers/phy/phy-core.c | 33 +++++++++++++--------------------
>>>>>  1 file changed, 13 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>>>> index b4964b0..9fa3f13 100644
>>>>> --- a/drivers/phy/phy-core.c
>>>>> +++ b/drivers/phy/phy-core.c
>>>>> @@ -222,10 +222,10 @@ int phy_init(struct phy *phy)
>>>>>       if (!phy)
>>>>>               return 0;
>>>>>
>>>>> -     ret = phy_pm_runtime_get_sync(phy);
>>>>> -     if (ret < 0 && ret != -ENOTSUPP)
>>>>> +     ret = pm_runtime_get_sync(phy->dev.parent);
>>>>
>>>> Won't this make phy-core manage pm_runtime of phy_provider even though the
>>>> phy_provider might not intend it?
>>>
>>> No it shouldn't.
>>>
>>> There are two cases to consider around this.
>>>
>>> 1) CONFIG_PM is unset. In this case pm_runtime_get_sync() will return
>>> 1, which is treated as succeeds by the error path.
>>>
>>> 2) CONFIG_PM is set, but the phy provider don't use runtime PM, thus
>>> it hasn't called pm_runtime_enable() for its device. In this case,
>>> pm_runtime_get_sync() returns -EACCES, which is also treated as
>>> success by the error path.
>>
>> There can be a case where the phy_provider uses runtime PM but doesn't want
>> phy-core to manage it.
>
> Ah, so you mean there are cases when the provider driver calls
> pm_runtime_enable() *after* it calls phy_create()/dev_phy_create()
> instead of before?

Okay so I found an example, thanks for pointing it out!

drivers/phy/ti/phy-twl4030-usb.c

>
> I am not really sure I understand *why* a provider driver wants to do
> that though, do you have more details?
> I mean, even if the phy core handles runtime PM, additional management
> can be done on top in the phy provider, there is nothing preventing
> that, but I guess that isn't sufficient?

In the above example, I guess the reason is related to the use of
usb_add_phy_dev().

I must say, the code in drivers/phy/ti/phy-twl4030-usb.c dealing with
these things looks a bit fragile, from a runtime PM point view.
However that's different story. :-)

Let me re-spin this, taking care of the problem you pointed out!

Kind regards
Uffe