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 EA93DC433F5 for ; Thu, 28 Oct 2021 22:20:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D395461056 for ; Thu, 28 Oct 2021 22:20:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231435AbhJ1WXJ (ORCPT ); Thu, 28 Oct 2021 18:23:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54880 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231235AbhJ1WXI (ORCPT ); Thu, 28 Oct 2021 18:23:08 -0400 Received: from mail-lf1-x134.google.com (mail-lf1-x134.google.com [IPv6:2a00:1450:4864:20::134]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6ED7FC061745 for ; Thu, 28 Oct 2021 15:20:40 -0700 (PDT) Received: by mail-lf1-x134.google.com with SMTP id bq11so16657525lfb.10 for ; Thu, 28 Oct 2021 15:20:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=MrMC6ps/Yy4KdTFANWsKX5xwpfm3xnZdTscIy4NxrwM=; b=r2rp15DmSFiQXaIqZojTMocwCNzQ7uibqUeLpOwuyknyyyrgRxBJtMfLSH7g8MnhGm AUE6u79hOGrrdXe9CoeYTZzrR2saPFLsgFWmkghX1eiKYEqd1ur4QtLJMKTxMYu4Ci+U pWPLRVN+3eyeIO2LIPyvN7lV7t4/xBLcXS3smaIqPoOwzs1EZN/iYzfgboGHJTGRFMOh GEe6Bo4h8XbGA9/tNdxZKWSWdoxjdgDZYJ/zmgvJT4+g7XZMn17KjL3q8Gy2lD8dQL9H iw736xfnE8WL++eYNmJqXwPgsFfrtoUVkEdMuCHJOVaD/pos+Q5M5P9FxP9PMHw8hUB5 13zA== 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=MrMC6ps/Yy4KdTFANWsKX5xwpfm3xnZdTscIy4NxrwM=; b=bTn1bYhkW0UYl7ur+S+f5RfFJZQtk+2EtIus2tl4FqhRvCceeAV8O8O46+eEh4AqeW L0Rm702rwFfP7XMHZvPUVzRsNIflxOTgIFzcXXL/pG83jNO/HRi620QhC4bpRoSRUADr RHUyk4Sr6LyeDsLi2G43ebEfA2siH264/DgNWJSDF/rHvKkSmiT8vI7K0+SEfkZwUVEB 4BxhZcDk7Yw1jySKh1YvquwgDqldlNwKGrmSRMu9Xl2/6Maha4fkbqq0kQYP5DAk5xHm aMshWzaDEjmVXABwa9xe9drIZ/uYNGp1Lk+YXXqP52e4/M+MaoJ4/m1CLv+X0syVnWBy WfVA== X-Gm-Message-State: AOAM532dvs1oyWtj3BMebvksrQEFWMsrOOQ4Qj4m2TjiXpfssT//qEoJ fWen/JGbJRIL397kyIRyjAT1mnd0vMKIchKQBtMNjA== X-Google-Smtp-Source: ABdhPJzVc+Tap1Ahd0sAw7lz95FWy8WEWdvKB4gdDXVSzeAd3fJljMWKzF7poGRnHwuGBovGkiFkH+H4vTavZjuMAr8= X-Received: by 2002:a19:ad4d:: with SMTP id s13mr6988529lfd.373.1635459638664; Thu, 28 Oct 2021 15:20:38 -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: <20211027143343.GC1319606@rowland.harvard.edu> From: Ulf Hansson Date: Fri, 29 Oct 2021 00:20:02 +0200 Message-ID: Subject: Re: [PATCH] PM: runtime: Allow rpm_resume() to succeed when runtime PM is disabled To: Alan Stern Cc: "Rafael J . Wysocki" , "Rafael J . Wysocki" , linux-pm@vger.kernel.org, Kevin Hilman , Maulik Shah , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. Again, thanks for your input! Kind regards Uffe 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 18B33C433F5 for ; Thu, 28 Oct 2021 22:22:15 +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 D91BE603E5 for ; Thu, 28 Oct 2021 22:22:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org D91BE603E5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.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=DDUxXvvVFLz6IzQYwv1joTVwlLItYBAVdH4YFKgdDLA=; b=q6lQzVhUNYIPNC O/LiwKSqoeyTg7DGBAyZ65xAF7bqfvXUaZtEVjpp+sWUYmuwa1YQLSbEI1poyXzbBmBlEtHOLBkW2 oO/lm1yVI6MqaoIUulpMwv3i9Da1X5oSUhWVv1KvTqfNpOFgAVCfDw8eyVmzGRabZyNawtEiN7/F1 aqZrZDleFr3ToU6cO/taL6RlKYFXg0+C+aUDmQNCm4VtWH0fQZKHEbMxwbH7ycNWOb06KrXO7a15t w5f0u0ZdOM0NhgsNj//JpVQBK465sYG61BlKlboZBNvbeYA436C3dSEL1S8tTugWKhMH/Xgc3qf8C cHiChgd+i+XSTieyZYAg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mgDlO-009EzM-Ti; Thu, 28 Oct 2021 22:20:47 +0000 Received: from mail-lf1-x12b.google.com ([2a00:1450:4864:20::12b]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mgDlK-009EyH-0D for linux-arm-kernel@lists.infradead.org; Thu, 28 Oct 2021 22:20:43 +0000 Received: by mail-lf1-x12b.google.com with SMTP id x27so16696147lfu.5 for ; Thu, 28 Oct 2021 15:20:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=MrMC6ps/Yy4KdTFANWsKX5xwpfm3xnZdTscIy4NxrwM=; b=r2rp15DmSFiQXaIqZojTMocwCNzQ7uibqUeLpOwuyknyyyrgRxBJtMfLSH7g8MnhGm AUE6u79hOGrrdXe9CoeYTZzrR2saPFLsgFWmkghX1eiKYEqd1ur4QtLJMKTxMYu4Ci+U pWPLRVN+3eyeIO2LIPyvN7lV7t4/xBLcXS3smaIqPoOwzs1EZN/iYzfgboGHJTGRFMOh GEe6Bo4h8XbGA9/tNdxZKWSWdoxjdgDZYJ/zmgvJT4+g7XZMn17KjL3q8Gy2lD8dQL9H iw736xfnE8WL++eYNmJqXwPgsFfrtoUVkEdMuCHJOVaD/pos+Q5M5P9FxP9PMHw8hUB5 13zA== 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=MrMC6ps/Yy4KdTFANWsKX5xwpfm3xnZdTscIy4NxrwM=; b=a4kZPOj3LrkRUuIC6tYKvKjAqfJDW7N5REZcZJ+pjQnRF9dHIDRkyAQwkpdVu1N5vN 6RxpBq/8psRV1FM5/IAnh4hFakWt4/6kWPUeyqnwVlQ5QT9TMocjktuzgKwL3MIyiRO6 k4/FspqUL6+20ckEodS5/5FkVrMK0cXek8QfSX9mb8yceVXRdqFhUy2AvjIYPVoMNxGe RdMBLRZgU4jbTaWI9DA+fX96DSxEH8gco3wwaryxQPCXQcjJ5KhI+znersrwOMux+AWl 4VynbW/uVEvdJlKzgQrcxyQtaspNZV74gWwA97Nr06+rNFc29mojt60dfPE0eRIIK0Ot YsYA== X-Gm-Message-State: AOAM531wa+uKcSG5rt/ocejx26Xy0w1Err5oln35z6JGUEGv3FYjb89P VhNmi935VxOKOT4Hx+UCeuCesucdl0+hE15LHQXksg== X-Google-Smtp-Source: ABdhPJzVc+Tap1Ahd0sAw7lz95FWy8WEWdvKB4gdDXVSzeAd3fJljMWKzF7poGRnHwuGBovGkiFkH+H4vTavZjuMAr8= X-Received: by 2002:a19:ad4d:: with SMTP id s13mr6988529lfd.373.1635459638664; Thu, 28 Oct 2021 15:20:38 -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: <20211027143343.GC1319606@rowland.harvard.edu> From: Ulf Hansson Date: Fri, 29 Oct 2021 00:20:02 +0200 Message-ID: Subject: Re: [PATCH] PM: runtime: Allow rpm_resume() to succeed when runtime PM is disabled To: Alan Stern Cc: "Rafael J . Wysocki" , "Rafael J . Wysocki" , linux-pm@vger.kernel.org, Kevin Hilman , Maulik Shah , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211028_152042_100312_7C9FF2AA X-CRM114-Status: GOOD ( 57.99 ) 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 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. Again, thanks for your input! Kind regards Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel