From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B56AFC433FE for ; Fri, 29 Oct 2021 18:27:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 96A7F60FED for ; Fri, 29 Oct 2021 18:27:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230136AbhJ2S3i (ORCPT ); Fri, 29 Oct 2021 14:29:38 -0400 Received: from mail-ot1-f53.google.com ([209.85.210.53]:36401 "EHLO mail-ot1-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229498AbhJ2S3g (ORCPT ); Fri, 29 Oct 2021 14:29:36 -0400 Received: by mail-ot1-f53.google.com with SMTP id s23-20020a056830125700b00553e2ca2dccso9881693otp.3; Fri, 29 Oct 2021 11:27:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mu4HMb4tZXB/weqJB3pLyEJlJzC5RBpRbYVUncyfRaQ=; b=wYN073UdhEFGq8iOQDymhcyFQ1cvjFLGNSt63C0TKJq59COa6//6W8FBn96LlEssrH H5pn4uHe69A/ULs905yn2KGP0xHyVIIcR6sGoiBlw4ibCkyranM2wbAyShtknkCSayQf 8NLIXUECgPIIEzIYiOaoYkIi5ixJMCgPD7ACF5025nsLCGiezAW0x8SiEt+BMZxbvHKh ipuM5OqdGH+XKN+ONrKIfCq2IGNhuJBvLO0lhJil5Kfld6vcKyLz7aAfamSov60tKM0u W3UWprJ+bNexn7KChYbWxRf/DBDpcCVdT/dzZhQFpDnSg5btttEzKgj/jEZX5vnIrecN Txmw== X-Gm-Message-State: AOAM5314CfUvpPa537SLlFf/aZ2b56dpOl3jxauupkqTvv1HCLCgUAur 59drJbrpDb8EdHRqiEM03mLQWyl2h4MG0WkoqFQ= X-Google-Smtp-Source: ABdhPJw8b68iFylRPIBKkjttf46BZN35sZUnlr1Ny/PCElz9n6ckpJjtkmRdbxIIGjQu1WgbpWZZn346XQ7t9dCjLpI= X-Received: by 2002:a9d:a64:: with SMTP id 91mr2620592otg.198.1635532027425; Fri, 29 Oct 2021 11:27:07 -0700 (PDT) MIME-Version: 1.0 References: <20211026222626.39222-1-ulf.hansson@linaro.org> <20211027020235.GA1306582@rowland.harvard.edu> <20211027143343.GC1319606@rowland.harvard.edu> In-Reply-To: From: "Rafael J. Wysocki" Date: Fri, 29 Oct 2021 20:26:56 +0200 Message-ID: Subject: Re: [PATCH] PM: runtime: Allow rpm_resume() to succeed when runtime PM is disabled To: Ulf Hansson Cc: Alan Stern , "Rafael J . Wysocki" , "Rafael J . Wysocki" , Linux PM , Kevin Hilman , Maulik Shah , Linux ARM , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Oct 29, 2021 at 12:20 AM Ulf Hansson wrote: > > On Wed, 27 Oct 2021 at 16:33, Alan Stern wrote: > > > > On Wed, Oct 27, 2021 at 12:55:43PM +0200, Ulf Hansson wrote: > > > On Wed, 27 Oct 2021 at 04:02, Alan Stern wrote: > > > > > > > > On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote: > > > > > During system suspend, the PM core sets dev->power.is_suspended for the > > > > > device that is being suspended. This flag is also being used in > > > > > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime > > > > > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the > > > > > device. > > > > > > > > > > To make this behaviour a bit more useful, let's drop the check for the > > > > > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to > > > > > be limited to this anyway. > > > > > > > > > > Signed-off-by: Ulf Hansson > > > > > --- > > > > > drivers/base/power/runtime.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > > > > index ec94049442b9..fadc278e3a66 100644 > > > > > --- a/drivers/base/power/runtime.c > > > > > +++ b/drivers/base/power/runtime.c > > > > > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags) > > > > > repeat: > > > > > if (dev->power.runtime_error) > > > > > retval = -EINVAL; > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > > > > - && dev->power.runtime_status == RPM_ACTIVE) > > > > > + else if (dev->power.disable_depth > 0 && > > > > > + dev->power.runtime_status == RPM_ACTIVE) > > > > > > > > IIRC there was a good reason why the original code checked for > > > > disable_depth == 1 rather than > 0. But I don't remember exactly what > > > > the reason was. Maybe it had something to do with the fact that during > > > > a system sleep __device_suspend_late calls __pm_runtime_disable, and the > > > > code was checking that there were no other disables in effect. > > > > > > The check was introduced in the below commit: > > > > > > Commit 6f3c77b040fc > > > Author: Kevin Hilman > > > Date: Fri Sep 21 22:47:34 2012 +0000 > > > PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2 > > > > > > By reading the commit message it's pretty clear to me that the check > > > was added to cover only one specific use case, during system suspend. > > > > > > That is, that a driver may want to call pm_runtime_get_sync() from a > > > late/noirq callback (when the PM core has disabled runtime PM), to > > > understand whether the device is still powered on and accessible. > > > > > > > This is > > > > related to the documented behavior of rpm_resume (it's supposed to fail > > > > with -EACCES if the device is disabled for runtime PM, no matter what > > > > power state the device is in). > > > > > > > > That probably is also the explanation for why dev->power.is_suspended > > > > gets checked: It's how the code tells whether a system sleep is in > > > > progress. > > > > > > Yes, you are certainly correct about the current behaviour. It's there > > > for a reason. > > > > > > On the other hand I would be greatly surprised if this change would > > > cause any issues. Of course, I can't make guarantees, but I am, of > > > course, willing to help to fix problems if those happen. > > > > > > As a matter of fact, I think the current behaviour looks quite > > > inconsistent, as it depends on whether the device is being system > > > suspended. > > > > > > Moreover, for syscore devices (dev->power.syscore is set for them), > > > the PM core doesn't set the "is_suspended" flag. Those can benefit > > > from a common behaviour. > > > > > > Finally, I think the "is_suspended" flag actually needs to be > > > protected by a lock when set by the PM core, as it's being used in two > > > separate execution paths. Although, rather than adding a lock for > > > protection, we can just rely on the "disable_depth" in rpm_resume(). > > > It would be easier and makes the behaviour consistent too. > > > > As long as is_suspended isn't _written_ in two separate execution paths, > > we're probably okay without a lock -- provided the code doesn't mind > > getting an indefinite result when a read races with a write. > > Well, indefinite doesn't sound very good to me for these cases, even > if it most likely never will happen. > > > > > > > So overall, I suspect this change should not be made. But some other > > > > improvement (like a nice comment) might be in order. > > > > > > > > Alan Stern > > > > > > Thanks for reviewing! > > > > You're welcome. Whatever you eventually decide to do should be okay > > with me. I just wanted to make sure that you understood the deeper > > issue here and had given it some thought. For example, it may turn out > > that you can resolve matters simply by updating the documentation. > > I observed the issue on cpuidle-psci. The devices it operates upon are > assigned as syscore devices and these are hooked up to a genpd. > > A call to pm_runtime_get_sync() can happen even after the PM core has > disabled runtime PM in the "late" phase. So the error code is received > for these real use-cases. > > Now, as we currently don't check the return value of > pm_runtime_get_sync() in cpuidle-psci, it's not a big deal. But it > certainly seems worth fixing in my opinion. > > Let's see if Rafael has some thoughts around this. Am I thinking correctly that this is mostly about working around the limitations of pm_runtime_force_suspend()? From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9B583C433F5 for ; Fri, 29 Oct 2021 19:39:06 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 662FE60F58 for ; Fri, 29 Oct 2021 19:39:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 662FE60F58 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ocJfXANfqmIgx+9JChZIP4v3Yb+firDSoX3SoIOCWkA=; b=kfwvL+fWaXd6F3 JijDIoRgShEiBCEHdAgNYln3qnmMmSQJlxxU574W/VzhwEjiqk0zfanBKulp9lKTF0JXbqkMwPPdK C+u3t2vndqHVdH3k5Zv+Ax+v51OlN0/zPfoc/kfsVzNviDZ7NoYPJKBR5t+3hIjiDAdTu9pLQoQC2 dhR0VIXcXKGUzoDoaOkb879KjJsLnuCZEDjFmK6+lbc3PdVIBA3xMRxMqpsBTcEz9lrWEPezHpLLL Jp2uYdDfDzwYB8bNEcEwc8sRBE8elresOgpVop9tZKO7b3IMompo2pY2Pv5UH2GWq9DpEWvaK/rMH n8gymK90ZDODUIvKYGaw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mgXgM-00C1Qk-5n; Fri, 29 Oct 2021 19:36:55 +0000 Received: from mail-ot1-f54.google.com ([209.85.210.54]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mgWaq-00BmWz-Sm for linux-arm-kernel@lists.infradead.org; Fri, 29 Oct 2021 18:27:12 +0000 Received: by mail-ot1-f54.google.com with SMTP id t17-20020a056830083100b00553ced10177so14578413ots.1 for ; Fri, 29 Oct 2021 11:27:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mu4HMb4tZXB/weqJB3pLyEJlJzC5RBpRbYVUncyfRaQ=; b=yZLrHTP/PDKrOIQ99lG1qjvWqHyFv+/1oS7RE8sRalIqHry2Lh8EqFf+5RhuQSKpPU EKPSZhX/qgVHxSK1hOVthyoHuAL5CWEqEyhCmakTAmmEAZCwsL7YGwASktXXl0PEA6fb tYhfcX73FH0vXtSG7V3kCd/gdZxk0wGdLllL8NfqE3gCPKsuIpTXy5Niy859RvvWZ/Mk nPa8krDWXLma0OC5x9vLmyThwt6rBKVYGQUJWm2r5pG/hEm7lVtdqEcUBfjGHHEz18tC AKbqK9NkbJOy04ebYM/AEOeRxHimajlf18Pb+5uU6ETPBmTSgT00AqO6oUUkvjLRcrdX tUow== X-Gm-Message-State: AOAM532lDCTSoBHjdUNg+JS2EEa3BP3wJr5JKVGoW+mrKf+yxCuydnrQ LcE+pKVdNHTlqsBx2z8BZR0lWGQrp2eLQ3sX35wyBaTeUlo= X-Google-Smtp-Source: ABdhPJw8b68iFylRPIBKkjttf46BZN35sZUnlr1Ny/PCElz9n6ckpJjtkmRdbxIIGjQu1WgbpWZZn346XQ7t9dCjLpI= X-Received: by 2002:a9d:a64:: with SMTP id 91mr2620592otg.198.1635532027425; Fri, 29 Oct 2021 11:27:07 -0700 (PDT) MIME-Version: 1.0 References: <20211026222626.39222-1-ulf.hansson@linaro.org> <20211027020235.GA1306582@rowland.harvard.edu> <20211027143343.GC1319606@rowland.harvard.edu> In-Reply-To: From: "Rafael J. Wysocki" Date: Fri, 29 Oct 2021 20:26:56 +0200 Message-ID: Subject: Re: [PATCH] PM: runtime: Allow rpm_resume() to succeed when runtime PM is disabled To: Ulf Hansson Cc: Alan Stern , "Rafael J . Wysocki" , "Rafael J . Wysocki" , Linux PM , Kevin Hilman , Maulik Shah , Linux ARM , Linux Kernel Mailing List X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211029_112709_009391_CEC7ADF5 X-CRM114-Status: GOOD ( 61.38 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Oct 29, 2021 at 12:20 AM Ulf Hansson wrote: > > On Wed, 27 Oct 2021 at 16:33, Alan Stern wrote: > > > > On Wed, Oct 27, 2021 at 12:55:43PM +0200, Ulf Hansson wrote: > > > On Wed, 27 Oct 2021 at 04:02, Alan Stern wrote: > > > > > > > > On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote: > > > > > During system suspend, the PM core sets dev->power.is_suspended for the > > > > > device that is being suspended. This flag is also being used in > > > > > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime > > > > > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the > > > > > device. > > > > > > > > > > To make this behaviour a bit more useful, let's drop the check for the > > > > > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to > > > > > be limited to this anyway. > > > > > > > > > > Signed-off-by: Ulf Hansson > > > > > --- > > > > > drivers/base/power/runtime.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > > > > index ec94049442b9..fadc278e3a66 100644 > > > > > --- a/drivers/base/power/runtime.c > > > > > +++ b/drivers/base/power/runtime.c > > > > > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags) > > > > > repeat: > > > > > if (dev->power.runtime_error) > > > > > retval = -EINVAL; > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > > > > - && dev->power.runtime_status == RPM_ACTIVE) > > > > > + else if (dev->power.disable_depth > 0 && > > > > > + dev->power.runtime_status == RPM_ACTIVE) > > > > > > > > IIRC there was a good reason why the original code checked for > > > > disable_depth == 1 rather than > 0. But I don't remember exactly what > > > > the reason was. Maybe it had something to do with the fact that during > > > > a system sleep __device_suspend_late calls __pm_runtime_disable, and the > > > > code was checking that there were no other disables in effect. > > > > > > The check was introduced in the below commit: > > > > > > Commit 6f3c77b040fc > > > Author: Kevin Hilman > > > Date: Fri Sep 21 22:47:34 2012 +0000 > > > PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2 > > > > > > By reading the commit message it's pretty clear to me that the check > > > was added to cover only one specific use case, during system suspend. > > > > > > That is, that a driver may want to call pm_runtime_get_sync() from a > > > late/noirq callback (when the PM core has disabled runtime PM), to > > > understand whether the device is still powered on and accessible. > > > > > > > This is > > > > related to the documented behavior of rpm_resume (it's supposed to fail > > > > with -EACCES if the device is disabled for runtime PM, no matter what > > > > power state the device is in). > > > > > > > > That probably is also the explanation for why dev->power.is_suspended > > > > gets checked: It's how the code tells whether a system sleep is in > > > > progress. > > > > > > Yes, you are certainly correct about the current behaviour. It's there > > > for a reason. > > > > > > On the other hand I would be greatly surprised if this change would > > > cause any issues. Of course, I can't make guarantees, but I am, of > > > course, willing to help to fix problems if those happen. > > > > > > As a matter of fact, I think the current behaviour looks quite > > > inconsistent, as it depends on whether the device is being system > > > suspended. > > > > > > Moreover, for syscore devices (dev->power.syscore is set for them), > > > the PM core doesn't set the "is_suspended" flag. Those can benefit > > > from a common behaviour. > > > > > > Finally, I think the "is_suspended" flag actually needs to be > > > protected by a lock when set by the PM core, as it's being used in two > > > separate execution paths. Although, rather than adding a lock for > > > protection, we can just rely on the "disable_depth" in rpm_resume(). > > > It would be easier and makes the behaviour consistent too. > > > > As long as is_suspended isn't _written_ in two separate execution paths, > > we're probably okay without a lock -- provided the code doesn't mind > > getting an indefinite result when a read races with a write. > > Well, indefinite doesn't sound very good to me for these cases, even > if it most likely never will happen. > > > > > > > So overall, I suspect this change should not be made. But some other > > > > improvement (like a nice comment) might be in order. > > > > > > > > Alan Stern > > > > > > Thanks for reviewing! > > > > You're welcome. Whatever you eventually decide to do should be okay > > with me. I just wanted to make sure that you understood the deeper > > issue here and had given it some thought. For example, it may turn out > > that you can resolve matters simply by updating the documentation. > > I observed the issue on cpuidle-psci. The devices it operates upon are > assigned as syscore devices and these are hooked up to a genpd. > > A call to pm_runtime_get_sync() can happen even after the PM core has > disabled runtime PM in the "late" phase. So the error code is received > for these real use-cases. > > Now, as we currently don't check the return value of > pm_runtime_get_sync() in cpuidle-psci, it's not a big deal. But it > certainly seems worth fixing in my opinion. > > Let's see if Rafael has some thoughts around this. Am I thinking correctly that this is mostly about working around the limitations of pm_runtime_force_suspend()? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel