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 E1DB6C433FE for ; Wed, 27 Oct 2021 10:56:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C10556103B for ; Wed, 27 Oct 2021 10:56:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239786AbhJ0K6s (ORCPT ); Wed, 27 Oct 2021 06:58:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51260 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231473AbhJ0K6r (ORCPT ); Wed, 27 Oct 2021 06:58:47 -0400 Received: from mail-lj1-x234.google.com (mail-lj1-x234.google.com [IPv6:2a00:1450:4864:20::234]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3B043C061570 for ; Wed, 27 Oct 2021 03:56:22 -0700 (PDT) Received: by mail-lj1-x234.google.com with SMTP id 205so3948394ljf.9 for ; Wed, 27 Oct 2021 03:56:22 -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=VcwIbDXyqxOVXmwhFTh3M4U8pwTWBcfpjA1gKThuQwM=; b=DOleeR66hVCWRUSE1vVRgpe8SJwg3hXoPhnYGSWr2h/1QsY59933hT503CxAL7bTNk iAmCUx0TWh3pVoqWNx9NfD3cn8DWC6uQYvCRUH8Mze9MIUaGAlJOWh9Rs6Jb+LYtgoST cjvI7vr2NNcKcaTXT3/tV9MElrGguJ5w9z+JbcXFX/2ofZN8/UsF+gKVIIl+ByuNaQ8Y yPsk8wPrbdfXVROwTXzZuSRkjgb9arBztxSpf3SmGhRUuRLcpdrnjYCpgT+j4EHzVgHA DYQvE5KXJ/1CnQ6ufOLPUW7E3xStkYgXY4qDSJNfLOpWZV1eVezITj1sM6bh3c8kXPLb DFOg== 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=VcwIbDXyqxOVXmwhFTh3M4U8pwTWBcfpjA1gKThuQwM=; b=cUX06ntAL8p70FV44GpwnLVd0jOYQ8Otq2vDPoxOZ/u8O1I1OKyATXh2m5sJjW970g 11O38gBEavkDxKhh61NBng9MbUB1F71Exieb3+z9rh7Sop0OH+3wKiiD7WYugb3MA87t ZtSRLGPHnFUNkNd8RueM+FJ8oNGajTGpZhi6404giDJLRCJPIqpRZ7LbDN2xMCyC0AtC ksrTaydv7ymPwnCpz/mTcCbyLuNd3OTrG6AIWfhNI5VzP8c30gUjl8uuZ2shqSrd8kBX iZTHv2FHlhxnZwuFC2DDg9ImAc9a/0wg0TMrAebP5MoCayw83IFdoCCQCoBoEtESF36J LzHA== X-Gm-Message-State: AOAM531KkFosGYrUCnZS9Cg3U2LOoTFMVHqY0V5f5bl0s1XgvyrcK2F9 8K1yn9NV13lil6nzBPk5e+twZnZbav1EsYxxQFcI2g== X-Google-Smtp-Source: ABdhPJy1h3NelpYd9TSvKaUHWrMYA+ZVL3fwLTcVJEPZpatCHhPdaElqmnb17Fr40bMDmvrRBl+JpTVBgGxcDMAKBTQ= X-Received: by 2002:a2e:b550:: with SMTP id a16mr32975194ljn.229.1635332180469; Wed, 27 Oct 2021 03:56:20 -0700 (PDT) MIME-Version: 1.0 References: <20211026222626.39222-1-ulf.hansson@linaro.org> <20211027020235.GA1306582@rowland.harvard.edu> In-Reply-To: <20211027020235.GA1306582@rowland.harvard.edu> From: Ulf Hansson Date: Wed, 27 Oct 2021 12:55:43 +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 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. > > 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! 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 8EAB2C433EF for ; Wed, 27 Oct 2021 10:57:54 +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 56EE160C40 for ; Wed, 27 Oct 2021 10:57:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 56EE160C40 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=97Uk1T0zO+OHCT2siH3LbKkwCUecyIde2Eyl0+3wNws=; b=BashSRz2zKOntH FXTfZT3vTq+BQjxWa7rOagbQD1MxTD0NVivYVFqNjhr/ou+LUJfICL3kwBPpAV2HdGufmijzPhKMm wPcg7XdK41se9vvL+YJW0vTnrJh1rUscYxH8CCrwu3KmgHMOQfAbwMSgrc1IOma8qCQjkZ5So3Nxr a9Aa98RR5NqK47oD8Yn+r3knxfW/mh+UdyeyWP+hEpYFFjlY7043eH3OBEqmRR6KjB2qDp2XtqhSO zYPnlnBhSR+lPK/5cvWqMQx79mb1+5DxuKvyrRGrj7UCKGV7e6jWa3AGQk/MeFC/zkpW0jj66pilf Ql2vbrpxEwRbbTj1LUJA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mfgba-004daT-Py; Wed, 27 Oct 2021 10:56:26 +0000 Received: from mail-lj1-x233.google.com ([2a00:1450:4864:20::233]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mfgbW-004dZP-Jw for linux-arm-kernel@lists.infradead.org; Wed, 27 Oct 2021 10:56:24 +0000 Received: by mail-lj1-x233.google.com with SMTP id q16so4009374ljg.3 for ; Wed, 27 Oct 2021 03:56:21 -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=VcwIbDXyqxOVXmwhFTh3M4U8pwTWBcfpjA1gKThuQwM=; b=DOleeR66hVCWRUSE1vVRgpe8SJwg3hXoPhnYGSWr2h/1QsY59933hT503CxAL7bTNk iAmCUx0TWh3pVoqWNx9NfD3cn8DWC6uQYvCRUH8Mze9MIUaGAlJOWh9Rs6Jb+LYtgoST cjvI7vr2NNcKcaTXT3/tV9MElrGguJ5w9z+JbcXFX/2ofZN8/UsF+gKVIIl+ByuNaQ8Y yPsk8wPrbdfXVROwTXzZuSRkjgb9arBztxSpf3SmGhRUuRLcpdrnjYCpgT+j4EHzVgHA DYQvE5KXJ/1CnQ6ufOLPUW7E3xStkYgXY4qDSJNfLOpWZV1eVezITj1sM6bh3c8kXPLb DFOg== 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=VcwIbDXyqxOVXmwhFTh3M4U8pwTWBcfpjA1gKThuQwM=; b=BEPWJokfgNmUKyBqA4JbMzH03Lzyh6zum0XK1W6Nz2CBHJUHPQw4BTmPFkmjnFlUxz ZPM4lF6BAe9UpbmL5VUePjDsyjOqc+hev2U/1qtQ7kK2hNfw84T9eIQ/zbmmgmFXTWYH JEbA02/3OMeG0ZzhaJakXMWg87LUt5h4QbNshfYFtawbtVYI3aLtXKh0PVkBaHOrgn8o lPgS0+p2QJFS9ivnFRMpeP5459VKBOYITK2ubaF7GRLrImLJWDXHzsYk2d7PhSmCrg1U 1KZ0QgUGMQ6xnOeSLF1Bpqtiv/hYTV9BoVf1cAaZzcgHj63IssTkVWTXfsu3l/ut5286 BuOA== X-Gm-Message-State: AOAM531KYNX7OgY1k2fr8D7P3PZFUyk8Vs1z8TLwKnE38xZcbex5tUbb YpXOU1tywDog6bUcKa6gda11CND6JJ3W3iNCUBRQ66tVr2Y7bQ== X-Google-Smtp-Source: ABdhPJy1h3NelpYd9TSvKaUHWrMYA+ZVL3fwLTcVJEPZpatCHhPdaElqmnb17Fr40bMDmvrRBl+JpTVBgGxcDMAKBTQ= X-Received: by 2002:a2e:b550:: with SMTP id a16mr32975194ljn.229.1635332180469; Wed, 27 Oct 2021 03:56:20 -0700 (PDT) MIME-Version: 1.0 References: <20211026222626.39222-1-ulf.hansson@linaro.org> <20211027020235.GA1306582@rowland.harvard.edu> In-Reply-To: <20211027020235.GA1306582@rowland.harvard.edu> From: Ulf Hansson Date: Wed, 27 Oct 2021 12:55:43 +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-20211027_035622_691777_016FCB7B X-CRM114-Status: GOOD ( 42.50 ) 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 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. > > 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! Kind regards Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel