From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754781AbdLTJCW (ORCPT ); Wed, 20 Dec 2017 04:02:22 -0500 Received: from lelnx194.ext.ti.com ([198.47.27.80]:23386 "EHLO lelnx194.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752886AbdLTJCN (ORCPT ); Wed, 20 Dec 2017 04:02:13 -0500 Subject: Re: [PATCH 1/3] phy: core: Move runtime PM reference counting to the parent device To: Ulf Hansson References: <1513718551-28624-1-git-send-email-ulf.hansson@linaro.org> <1513718551-28624-2-git-send-email-ulf.hansson@linaro.org> CC: "linux-kernel@vger.kernel.org" , "Rafael J . Wysocki" , "linux-pm@vger.kernel.org" , Yoshihiro Shimoda , Geert Uytterhoeven , Linux-Renesas From: Kishon Vijay Abraham I Message-ID: <0c0cf0ba-e729-a1d8-ac0a-38c8eb13a5c6@ti.com> Date: Wed, 20 Dec 2017 14:32:01 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ulf, On Wednesday 20 December 2017 02:05 PM, Ulf Hansson wrote: > On 20 December 2017 at 07:42, Kishon Vijay Abraham I 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 >>> --- >>> 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