From patchwork Fri Dec 1 09:22:18 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ulf Hansson X-Patchwork-Id: 10086651 X-Patchwork-Delegate: geert@linux-m68k.org Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 45F686037E for ; Fri, 1 Dec 2017 09:22:25 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3C52629432 for ; Fri, 1 Dec 2017 09:22:25 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2F5AF2A381; Fri, 1 Dec 2017 09:22:25 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5A66C29432 for ; Fri, 1 Dec 2017 09:22:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751236AbdLAJWX (ORCPT ); Fri, 1 Dec 2017 04:22:23 -0500 Received: from mail-io0-f179.google.com ([209.85.223.179]:47069 "EHLO mail-io0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751541AbdLAJWU (ORCPT ); Fri, 1 Dec 2017 04:22:20 -0500 Received: by mail-io0-f179.google.com with SMTP id x129so10548334iod.13 for ; Fri, 01 Dec 2017 01:22:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=CLLNoxZqVRj9KbSVnRjdZQUnlXKDNgAKfvYndBRa4bY=; b=IurGQGkVTmUvZiORiKIsYFhNkAsTAN8rgGUj8DjYzOQ4oc5VWKxFUqn1BKgF0ejJhA Gq0vY2PRAFUdGXg1x8Qk/77KNSSC09+QWZ1aSbio0pQ99dMNASDRwmBb4ZIbFO+29FLL et73nR7IA4isq/uxoh2HBoKDMdsLDyGMK8IhM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=CLLNoxZqVRj9KbSVnRjdZQUnlXKDNgAKfvYndBRa4bY=; b=YtqJ9VUinkwxYg51hqNaP92I6qdp0I1cD8DTe1puntmBYvJ4NDnyCq0tkjy2llGKgd PfbXav5WN5iGpMUzxb3Z6BELLc+2hRd2hqtzneSQQEXAg3w3GslU3GYiCzyQSaUl7VsK an4JFN7v6m9L7Zz391rJqNX3UDQhUJwQKvu1zZVWyYY65KNihzdv8G8OA37t69rS5upM xF63meBk1UsOoV4GTtAkL8m5JzuENX3FvqRhZp1oBpuQhQhBRUCDHAEYBT5yJ/H2MvDy OAqR0Wnjq3tttVuqJPEEi6QMROtLt26TSu9XSZodTx7pwClc655ZeE23UdfJ0s4CzYE1 yb2g== X-Gm-Message-State: AJaThX5bMDvqTa1XopaApx4LtuWHtmhioQSQ4VNPby212IhcbFDHlJz7 mTKlrK/hEH40+NcPPKx6XoN2opE40afqEsWF4xwnNg== X-Google-Smtp-Source: AGs4zMat9OcQnEqeLb33EPGDrlGLtKU/6qimo4v1MEQhopyuSS9Xt1b92eQuuQ/eMdti3FeMq49jMBr3Cbdz1XlqPEo= X-Received: by 10.107.7.169 with SMTP id g41mr12190100ioi.38.1512120140169; Fri, 01 Dec 2017 01:22:20 -0800 (PST) MIME-Version: 1.0 Received: by 10.2.86.149 with HTTP; Fri, 1 Dec 2017 01:22:18 -0800 (PST) In-Reply-To: References: <1713438.irjm9MTSvo@aspire.rjw.lan> From: Ulf Hansson Date: Fri, 1 Dec 2017 10:22:18 +0100 Message-ID: Subject: Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status() To: Yoshihiro Shimoda Cc: Geert Uytterhoeven , "Rafael J. Wysocki" , Linux PM , LKML , Greg Kroah-Hartman , Alan Stern , USB list , Linux-Renesas , Kishon Vijay Abraham I Sender: linux-renesas-soc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-renesas-soc@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP + Kishon On 30 November 2017 at 13:51, Yoshihiro Shimoda wrote: > Hi, > >> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 6:59 PM >> >> On 29 November 2017 at 10:43, Geert Uytterhoeven wrote: >> > Hi Ulf, > >> Okay, so the problem remains no matter which solution for wakeup you >> pick in genpd. > > Yes. Today I could reproduce this issue without usb host driver. > - The renesas_usb3 usb peripheral driver has generic phy handling. > (The peripheral driver uses different generic phy driver (phy-rcar-gen3-usb3.c) though.) > --> If I used the current renesas_usb3 (this means doesn't call phy_power_{on,off}(), > the issue didn't happen. > --> If I added phy_power_{on,off}() calling, the issue happened. > --> So, I'm thinking the APIs are related to the issue. Yes. > > - The generic phy APIs are in drivers/phy/phy-core.c. > --> The phy-rcar-gen3-usb[23] drivers call only pm_runtime_enable() before devm_phy_create(). > --> The phy-core will call pm_runtime_{get_sync,put}() in phy_{init,exit,power_{on,off}}. > --> So, IIUC, both devices of phy-. and will be handled by runtime PM APIs. > --> The runtime PM implementation of phy-core seems good to me. But...? I have digested the information that you and Geert provided, thanks! So, my conclusions so far is: The phy core is using runtime PM reference counting at phy_power_on|off(). Although it does that on the phy core device, which is a child device of the phy provider device. Because phy_power_off() is called during system suspend from phy consumer drivers like usb, the phy core device (child) and the phy provider device (parent) will never become runtime suspended (because the PM core has invoked pm_runtime_get_no_resume() for all device in the device prepare phase). Then, when genpd calls pm_runtime_force_suspend() at the suspend noirq phase for the phy provider device, the call to pm_runtime_set_suspended() in there, triggers the earlier error message, which is because the child (phy core device) is still runtime resumed. > >> Then this seems to point to that the driver may be misbehaving in some >> way. I can help to check what is going on. > > I guess so. But, I don't find yet... I think the below patch will help, although I am not sure if that is sufficient as a long term fix. Can you please try and see if it solves the problems? From: Ulf Hansson Date: Fri, 1 Dec 2017 09:07:54 +0100 Subject: [PATCH] phy: core: Move runtime PM reference counting to be done on the parent This is temporary hack, do not merge! The runtime PM deployment in the phy core is a bit unnecessary complicated, due to enabling runtime PM for the phy device. Moreover it causes problems for parent devices (phy providers) when dealing with system wide PM. Therefore, move the runtime PM reference counting to be done on the phy's parent device and drop to enable runtime PM for the phy device altogether. This simplifies for the phy provider driver, to deal with system wide PM, in case it also cares about keeping the runtime PM status of the device in sync with the state of the HW. Signed-off-by: Ulf Hansson --- drivers/phy/phy-core.c | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-) put_dev: @@ -831,7 +815,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); diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index b4964b0..837e50d 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -217,15 +217,12 @@ EXPORT_SYMBOL_GPL(phy_pm_runtime_forbid); int phy_init(struct phy *phy) { - int ret; + int ret = 0; if (!phy) return 0; - ret = phy_pm_runtime_get_sync(phy); - if (ret < 0 && ret != -ENOTSUPP) - return ret; - ret = 0; /* Override possible ret == -ENOTSUPP */ + pm_runtime_get_sync(phy->dev.parent); mutex_lock(&phy->mutex); if (phy->init_count == 0 && phy->ops->init) { @@ -239,22 +236,19 @@ 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); int phy_exit(struct phy *phy) { - int ret; + int ret = 0; if (!phy) return 0; - ret = phy_pm_runtime_get_sync(phy); - if (ret < 0 && ret != -ENOTSUPP) - return ret; - ret = 0; /* Override possible ret == -ENOTSUPP */ + pm_runtime_get_sync(phy->dev.parent); mutex_lock(&phy->mutex); if (phy->init_count == 1 && phy->ops->exit) { @@ -268,7 +262,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 +280,7 @@ int phy_power_on(struct phy *phy) goto out; } - ret = phy_pm_runtime_get_sync(phy); - if (ret < 0 && ret != -ENOTSUPP) - goto err_pm_sync; - - ret = 0; /* Override possible ret == -ENOTSUPP */ + pm_runtime_get_sync(phy->dev.parent); mutex_lock(&phy->mutex); if (phy->power_count == 0 && phy->ops->power_on) { @@ -306,8 +296,7 @@ int phy_power_on(struct phy *phy) err_pwr_on: mutex_unlock(&phy->mutex); - phy_pm_runtime_put_sync(phy); -err_pm_sync: + pm_runtime_put(phy->dev.parent); if (phy->pwr) regulator_disable(phy->pwr); out: @@ -333,7 +322,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 +763,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;