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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 44627C433FE for ; Wed, 1 Dec 2021 17:44:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234529AbhLARrq (ORCPT ); Wed, 1 Dec 2021 12:47:46 -0500 Received: from mail-ot1-f51.google.com ([209.85.210.51]:34543 "EHLO mail-ot1-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229896AbhLARrk (ORCPT ); Wed, 1 Dec 2021 12:47:40 -0500 Received: by mail-ot1-f51.google.com with SMTP id x19-20020a9d7053000000b0055c8b39420bso36279095otj.1; Wed, 01 Dec 2021 09:44:19 -0800 (PST) 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=aTG2bIndClsttjhb32FaPxR4WjhBnxBORsGiPhw1vr0=; b=0+xsiZzA3AekWadlvA4ablD5Qz6tHwc8E7SIliHxSfQ3qMLyKayURBszx1LfvcLnoO amLaxb3ttLN/QDZLYYP31fwcGT4rHsBhLk3645T8KpgqXBW2GannJwCJqiTfiEH4e3Ke SuExaqcNtEXQ3SLSL42ptqkEsRR9nadkgzX0wS6hBei9NEiVam5z8WKwiGrregEW4cbK Rn/y7Is2Uo/m2UVUw+IyuCjgObBH0xDThmahj8QN9rW/Ic2C80YZemdOBJcjbemcXWMz 6oGndM83iMH+Xyd68IxXr//t/GNmKxpZ2w9XTaC2jRmbfuC0md89DQDfWCbJxe1uC4lD rb2g== X-Gm-Message-State: AOAM530lqDROgWzep1iwjpj4se/lcQ4apYBs3/QVADvrAWINSI/ppiQl JtWi6NR/NfRRHoTJdNvELyFEEacWRrfYM9EnpXs= X-Google-Smtp-Source: ABdhPJzqR4BVABAmrBVuKMozBgzlsRsXL7uQkh+NvCXkkIm8Q9IE3RirFYQdvWrzDiV9mYvn3riLirmBODmI+9r7/m4= X-Received: by 2002:a05:6830:348f:: with SMTP id c15mr7125021otu.254.1638380659175; Wed, 01 Dec 2021 09:44:19 -0800 (PST) MIME-Version: 1.0 References: <20211026222626.39222-1-ulf.hansson@linaro.org> <4380690.LvFx2qVVIh@kreacher> In-Reply-To: From: "Rafael J. Wysocki" Date: Wed, 1 Dec 2021 18:44:08 +0100 Message-ID: Subject: Re: [PATCH] PM: runtime: Allow rpm_resume() to succeed when runtime PM is disabled To: Ulf Hansson Cc: "Rafael J. Wysocki" , "Rafael J. Wysocki" , Alan Stern , 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 Wed, Dec 1, 2021 at 4:23 PM Ulf Hansson wrote: > > On Wed, 1 Dec 2021 at 14:49, Rafael J. Wysocki wrote: > > > > On Wed, Dec 1, 2021 at 10:02 AM Ulf Hansson wrote: > > > > > > On Tue, 30 Nov 2021 at 18:26, Rafael J. Wysocki wrote: > > > > > > > > On Tue, Nov 30, 2021 at 5:41 PM Ulf Hansson wrote: > > > > > > > > > > On Tue, 30 Nov 2021 at 14:02, Rafael J. Wysocki wrote: > > > > > > > > > > > > On Tue, Nov 30, 2021 at 12:58 PM Ulf Hansson wrote: > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Am I thinking correctly that this is mostly about working around the > > > > > > > > > > > > > limitations of pm_runtime_force_suspend()? > > > > > > > > > > > > > > > > > > > > > > > > No, this isn't related at all. > > > > > > > > > > > > > > > > > > > > > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using > > > > > > > > > > > > pm_runtime_force_suspend() would not work here. > > > > > > > > > > > > > > > > > > > > > > Just wanted to send a ping on this to see if we can come to a > > > > > > > > > > > conclusion. Or maybe we did? :-) > > > > > > > > > > > > > > > > > > > > > > I think in the end, what slightly bothers me, is that the behavior is > > > > > > > > > > > a bit inconsistent. Although, maybe it's the best we can do. > > > > > > > > > > > > > > > > > > > > I've been thinking about this and it looks like we can do better, but > > > > > > > > > > instead of talking about this I'd rather send a patch. > > > > > > > > > > > > > > > > > > Alright. > > > > > > > > > > > > > > > > > > I was thinking along the lines of make similar changes for > > > > > > > > > rpm_idle|suspend(). That would make the behaviour even more > > > > > > > > > consistent, I think. > > > > > > > > > > > > > > > > > > Perhaps that's what you have in mind? :-) > > > > > > > > > > > > > > > > Well, not exactly. > > > > > > > > > > > > > > > > The idea is to add another counter (called restrain_depth in the patch) > > > > > > > > to prevent rpm_resume() from running the callback when that is potentially > > > > > > > > problematic. With that, it is possible to actually distinguish devices > > > > > > > > with PM-runtime enabled and it allows the PM-runtime status to be checked > > > > > > > > when it is still known to be meaningful. > > > > > > > > > > > > > > Hmm, I don't quite understand the benefit of introducing a new flag > > > > > > > for this. rpm_resume() already checks the disable_depth to understand > > > > > > > when it's safe to invoke the callback. Maybe there is a reason why > > > > > > > that isn't sufficient? > > > > > > > > > > > > The problem is that disable_depth > 0 may very well mean that runtime > > > > > > PM has not been enabled at all for the given device which IMO is a > > > > > > problem. > > > > > > > > > > > > As it stands, it is necessary to make assumptions, like disable_depth > > > > > > == 1 meaning that runtime PM is really enabled, but the PM core has > > > > > > disabled it temporarily, which is somewhat questionable. > > > > > > > > > > > > Another problem with disabling is that it causes rpm_resume() to fail > > > > > > even if the status is RPM_ACTIVE and it has to do that exactly because > > > > > > it cannot know why runtime PM has been disabled. If it has never been > > > > > > enabled, rpm_resume() must fail, but if it has been disabled > > > > > > temporarily, rpm_resume() may return 1 when the status is RPM_ACTIVE. > > > > > > > > > > > > The new count allows the "enabled in general, but temporarily disabled > > > > > > at the moment" to be handled cleanly. > > > > > > > > > > My overall comment is that I fail to understand why we need to > > > > > distinguish between these two cases. To me, it shouldn't really > > > > > matter, *why* runtime PM is (or have been) disabled for the device. > > > > > > > > It matters if you want to trust the status, because "disabled" means > > > > "the status doesn't matter". > > > > > > Well, that doesn't really match how the runtime PM interface is being > > > used today. > > > > Well, I clearly disagree. > > Alright, then we can agree to disagree. :-) > > > > > > For example, we have a whole bunch of helper functions, allowing us to > > > update and check the runtime PM state of the device, even when the > > > disable_depth > 0. Some functions, like pm_runtime_set_active() for > > > example, even take parents and device-links into account. > > > > That's true, but that's for a purpose. > > > > If runtime PM becomes enabled after using pm_runtime_set_active(), the > > status should better be consistent with the settings of the parent > > etc. > > > > > > > > > > If you want the status to stay meaningful, but prevent callbacks from > > > > running, you need something else. > > > > > > > > > The important point is that the default state for a device is > > > > > RPM_SUSPENDED and someone has moved into RPM_ACTIVE, for whatever > > > > > reason. That should be sufficient to allow rpm_resume() to return '1' > > > > > when disable_depth > 0, shouldn't it? > > > > > > > > No, because there is no rule by which the status of devices with > > > > PM-runtime disabled must be RPM_SUSPENDED. > > > > > > That's not what I was trying to say. > > > > > > The initial/default runtime PM state for a device is RPM_SUSPENDED, > > > which is being set in pm_runtime_init(). Although, I agree that it > > > can't be trusted that this state actually reflects the state of the > > > HW, it's still a valid state for the device from a runtime PM point of > > > view. > > > > No, it is not. It's just the default. > > > > > However, and more importantly, if the state has moved to RPM_ACTIVE, > > > someone must have deliberately moved the device into that state. > > > > Sure, but it cannot be regarded as an indication on whether or not > > runtime PM is supported and has ever been enabled for the given > > device. > > > > Again, there is no rule regarding the status value for devices with > > runtime PM disabled, either way. > > If I understand correctly, that means you think the > pm_runtime_status_suspended() should really be converted to an > internal runtime PM interface, not being exported to users outside. > Right? Not really. I'm just saying that its usefulness is limited. My basic concern is that system-wide PM transitions must always invoke callbacks for devices with PM-runtime disabled, because they may (or may not) be functional regardless of the PM-runtime status and if they are functional, they must be suspended. And note that supporting system-wide PM is not optional and the only way to kind of disable it is to return an error from a device suspend callback (but that's nasty for some use cases). So the "Has PM-runtime been enabled?" question is really fundamental for system-wide PM and it is not sufficient to look at the PM-runtime status to find out, but if the PM-core itself disables PM-runtime (which is has to do at one point to prevent PM-runtime from racing with system-wide PM), it is hard to answer definitely in general. IMO the only way to make it possible to find that out in all cases is to make the PM core retain the power.disable_depth value and that can be done by making it use a different mechanism to prevent PM-runtime callbacks from being run. Alternatively, the current PM-runtime status could be "latched" during the PM-runtime disable operation if power.disable_depth is 0 (and that "latched" value would be initialized to "invalid" in case PM-runtime is never enabled). > When it comes to the pm_runtime_active() interface, I noticed that > it's actually completely broken, as it returns "true" when runtime PM > has been disabled for the device - no matter whether the state is > RPM_ACTIVE or not. It's quite heavily used, so I guess the behaviour > is what the callers expect? I'm not sure about that after looking at some of them briefly. I'm guessing that the original author of it wanted to use it in the system-wide suspend path to decide whether or not to suspend the device, but that obviously interferes with what the PM core does. Anway, it looks like its name should be changed to something like pm_runtime_active_or_disabled(), so it reflects the functionality. > > > > > For this reason, I believe it seems reasonable to trust it, both from HW > > > point of view, but definitely also from a runtime PM point of view. If > > > not, then what should we do? > > > > Trust it only when runtime PM is enabled, ie. dev->power.disable_depth == 0. > > > > That's exactly the reason why pm_runtime_suspended() returns false if > > runtime PM is disabled for the target device and if > > pm_runtime_suspended() is called during system-wide suspend and > > resume, it is not clear how to interpret its return value. > > To me, I don't think there is a problem interpreting > pm_runtime_suspended()'s return value. As you said, it tells whether > runtime PM has been enabled and whether the state is RPM_SUSPENDED. > > For those that want to know the state when runtime PM has been > disabled, they can use pm_runtime_status_suspended(). > > What's the issue? It may not match the real state of the device which may be relevant. > > > > If it returns true outside the system suspend-resume code path, that > > means "runtime PM has been enabled and the device has been > > runtime-suspended" and I want it to mean exactly the same thing during > > system-wide suspend and resume, so people don't need to wonder about > > the context in which the code is running. > > Well, even if this seems nice, I wonder how useful this would be in the end. > > Perhaps you have some concrete examples where this could improve things? For example, what if pm_runtime_force_suspend() is called for a device with PM-runtime disabled and the PM-runtime status equal to RPM_SUSPENDED? As I said above, the callback must be invoked for it in case it is functional and needs to be suspended, but that won't happen AFAICS. However, if pm_runtime_force_suspend() can tell whether or not PM-runtime has been enabled for the device, it can do the right thing (basically, avoid running the callback if it has been run once already by PM-runtime, which is the case when PM-runtime is enabled and the status is RPM_SUSPENDED). 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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 9986CC433EF for ; Wed, 1 Dec 2021 17:46:01 +0000 (UTC) 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=ZOi5Dey4SIeC/J577cKBurE8xa/F0a3PUiaxs7Qq+8E=; b=B05rapHHcSIrHc 8eyzcxefcYlXipy+M6sMPcdUjEv1IsQnvCY3gmgYtK13hinupPG0z5vd/xU2BN5lnLmTDuX/BKREp ZPnJZdatE1yteoN+EfyLD5amHO5lSG7dJwL2OQpyerubaWnQg1UZ8o0pPI7hqrdsldCs5W5jQxior 3gjnbxTUEp4LW2VAKht6SGLa5ALs/ElrRSX7UmfFfHk8atnQFIjUDiEwStcowzwguwgVYGhodc9FP qNUV1tSUaJyxf066YrDYZ6hXnrwnbTUm0RtcyLjepDhbRk1697GDMTmi9W3JWLiQ1k6H/VCH3I5M7 dxFIOh0IEcva6GnAy5rQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1msTea-009c2Q-OY; Wed, 01 Dec 2021 17:44:24 +0000 Received: from mail-ot1-f45.google.com ([209.85.210.45]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1msTeW-009c27-JN for linux-arm-kernel@lists.infradead.org; Wed, 01 Dec 2021 17:44:22 +0000 Received: by mail-ot1-f45.google.com with SMTP id r10-20020a056830080a00b0055c8fd2cebdso36227623ots.6 for ; Wed, 01 Dec 2021 09:44:19 -0800 (PST) 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=aTG2bIndClsttjhb32FaPxR4WjhBnxBORsGiPhw1vr0=; b=Gbqoo8yPU+qNM9zRGUyE/zW/d2btwakbxK9zFiOGui+4giehFcCUFMM7w6NIUODJXi XqDu5vVGLCjUILWFW3DhKKwr+oT/7Hh6/cvy/Mgky2uPgEiF+RGgLDFw6B4Lykge2b9/ RTBinLKw8+xEZJOPoB+Cs0PMbFXgctzE12Kwg1jGltL+8ty9QbKhjUm48E9hhSkRAcRe T3kw+b5Du6c4bm8wT41FfxRFTvGVC1ozLjMG52JzYljxU6FSJysx40OBUqOnzKv9MMHc 3nzF1xBJoceYCbGyBHlOpVGgN2dnt+QDHuTVQttn25BwBkjtLYJLtf0al5TxEgszsNVk vS+Q== X-Gm-Message-State: AOAM530FEgeZXNXqGacu1L2VVW6gD9coZ65CvaeU03ItK5hBrYaY1zNw eWa3jfODCYzrg/Orc/jesfmINBX+PpZnRMgV6LC5fCqWnnM= X-Google-Smtp-Source: ABdhPJzqR4BVABAmrBVuKMozBgzlsRsXL7uQkh+NvCXkkIm8Q9IE3RirFYQdvWrzDiV9mYvn3riLirmBODmI+9r7/m4= X-Received: by 2002:a05:6830:348f:: with SMTP id c15mr7125021otu.254.1638380659175; Wed, 01 Dec 2021 09:44:19 -0800 (PST) MIME-Version: 1.0 References: <20211026222626.39222-1-ulf.hansson@linaro.org> <4380690.LvFx2qVVIh@kreacher> In-Reply-To: From: "Rafael J. Wysocki" Date: Wed, 1 Dec 2021 18:44:08 +0100 Message-ID: Subject: Re: [PATCH] PM: runtime: Allow rpm_resume() to succeed when runtime PM is disabled To: Ulf Hansson Cc: "Rafael J. Wysocki" , "Rafael J. Wysocki" , Alan Stern , 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-20211201_094420_672984_3E442C2A X-CRM114-Status: GOOD ( 80.34 ) 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, Dec 1, 2021 at 4:23 PM Ulf Hansson wrote: > > On Wed, 1 Dec 2021 at 14:49, Rafael J. Wysocki wrote: > > > > On Wed, Dec 1, 2021 at 10:02 AM Ulf Hansson wrote: > > > > > > On Tue, 30 Nov 2021 at 18:26, Rafael J. Wysocki wrote: > > > > > > > > On Tue, Nov 30, 2021 at 5:41 PM Ulf Hansson wrote: > > > > > > > > > > On Tue, 30 Nov 2021 at 14:02, Rafael J. Wysocki wrote: > > > > > > > > > > > > On Tue, Nov 30, 2021 at 12:58 PM Ulf Hansson wrote: > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Am I thinking correctly that this is mostly about working around the > > > > > > > > > > > > > limitations of pm_runtime_force_suspend()? > > > > > > > > > > > > > > > > > > > > > > > > No, this isn't related at all. > > > > > > > > > > > > > > > > > > > > > > > > The cpuidle-psci driver doesn't have PM callbacks, thus using > > > > > > > > > > > > pm_runtime_force_suspend() would not work here. > > > > > > > > > > > > > > > > > > > > > > Just wanted to send a ping on this to see if we can come to a > > > > > > > > > > > conclusion. Or maybe we did? :-) > > > > > > > > > > > > > > > > > > > > > > I think in the end, what slightly bothers me, is that the behavior is > > > > > > > > > > > a bit inconsistent. Although, maybe it's the best we can do. > > > > > > > > > > > > > > > > > > > > I've been thinking about this and it looks like we can do better, but > > > > > > > > > > instead of talking about this I'd rather send a patch. > > > > > > > > > > > > > > > > > > Alright. > > > > > > > > > > > > > > > > > > I was thinking along the lines of make similar changes for > > > > > > > > > rpm_idle|suspend(). That would make the behaviour even more > > > > > > > > > consistent, I think. > > > > > > > > > > > > > > > > > > Perhaps that's what you have in mind? :-) > > > > > > > > > > > > > > > > Well, not exactly. > > > > > > > > > > > > > > > > The idea is to add another counter (called restrain_depth in the patch) > > > > > > > > to prevent rpm_resume() from running the callback when that is potentially > > > > > > > > problematic. With that, it is possible to actually distinguish devices > > > > > > > > with PM-runtime enabled and it allows the PM-runtime status to be checked > > > > > > > > when it is still known to be meaningful. > > > > > > > > > > > > > > Hmm, I don't quite understand the benefit of introducing a new flag > > > > > > > for this. rpm_resume() already checks the disable_depth to understand > > > > > > > when it's safe to invoke the callback. Maybe there is a reason why > > > > > > > that isn't sufficient? > > > > > > > > > > > > The problem is that disable_depth > 0 may very well mean that runtime > > > > > > PM has not been enabled at all for the given device which IMO is a > > > > > > problem. > > > > > > > > > > > > As it stands, it is necessary to make assumptions, like disable_depth > > > > > > == 1 meaning that runtime PM is really enabled, but the PM core has > > > > > > disabled it temporarily, which is somewhat questionable. > > > > > > > > > > > > Another problem with disabling is that it causes rpm_resume() to fail > > > > > > even if the status is RPM_ACTIVE and it has to do that exactly because > > > > > > it cannot know why runtime PM has been disabled. If it has never been > > > > > > enabled, rpm_resume() must fail, but if it has been disabled > > > > > > temporarily, rpm_resume() may return 1 when the status is RPM_ACTIVE. > > > > > > > > > > > > The new count allows the "enabled in general, but temporarily disabled > > > > > > at the moment" to be handled cleanly. > > > > > > > > > > My overall comment is that I fail to understand why we need to > > > > > distinguish between these two cases. To me, it shouldn't really > > > > > matter, *why* runtime PM is (or have been) disabled for the device. > > > > > > > > It matters if you want to trust the status, because "disabled" means > > > > "the status doesn't matter". > > > > > > Well, that doesn't really match how the runtime PM interface is being > > > used today. > > > > Well, I clearly disagree. > > Alright, then we can agree to disagree. :-) > > > > > > For example, we have a whole bunch of helper functions, allowing us to > > > update and check the runtime PM state of the device, even when the > > > disable_depth > 0. Some functions, like pm_runtime_set_active() for > > > example, even take parents and device-links into account. > > > > That's true, but that's for a purpose. > > > > If runtime PM becomes enabled after using pm_runtime_set_active(), the > > status should better be consistent with the settings of the parent > > etc. > > > > > > > > > > If you want the status to stay meaningful, but prevent callbacks from > > > > running, you need something else. > > > > > > > > > The important point is that the default state for a device is > > > > > RPM_SUSPENDED and someone has moved into RPM_ACTIVE, for whatever > > > > > reason. That should be sufficient to allow rpm_resume() to return '1' > > > > > when disable_depth > 0, shouldn't it? > > > > > > > > No, because there is no rule by which the status of devices with > > > > PM-runtime disabled must be RPM_SUSPENDED. > > > > > > That's not what I was trying to say. > > > > > > The initial/default runtime PM state for a device is RPM_SUSPENDED, > > > which is being set in pm_runtime_init(). Although, I agree that it > > > can't be trusted that this state actually reflects the state of the > > > HW, it's still a valid state for the device from a runtime PM point of > > > view. > > > > No, it is not. It's just the default. > > > > > However, and more importantly, if the state has moved to RPM_ACTIVE, > > > someone must have deliberately moved the device into that state. > > > > Sure, but it cannot be regarded as an indication on whether or not > > runtime PM is supported and has ever been enabled for the given > > device. > > > > Again, there is no rule regarding the status value for devices with > > runtime PM disabled, either way. > > If I understand correctly, that means you think the > pm_runtime_status_suspended() should really be converted to an > internal runtime PM interface, not being exported to users outside. > Right? Not really. I'm just saying that its usefulness is limited. My basic concern is that system-wide PM transitions must always invoke callbacks for devices with PM-runtime disabled, because they may (or may not) be functional regardless of the PM-runtime status and if they are functional, they must be suspended. And note that supporting system-wide PM is not optional and the only way to kind of disable it is to return an error from a device suspend callback (but that's nasty for some use cases). So the "Has PM-runtime been enabled?" question is really fundamental for system-wide PM and it is not sufficient to look at the PM-runtime status to find out, but if the PM-core itself disables PM-runtime (which is has to do at one point to prevent PM-runtime from racing with system-wide PM), it is hard to answer definitely in general. IMO the only way to make it possible to find that out in all cases is to make the PM core retain the power.disable_depth value and that can be done by making it use a different mechanism to prevent PM-runtime callbacks from being run. Alternatively, the current PM-runtime status could be "latched" during the PM-runtime disable operation if power.disable_depth is 0 (and that "latched" value would be initialized to "invalid" in case PM-runtime is never enabled). > When it comes to the pm_runtime_active() interface, I noticed that > it's actually completely broken, as it returns "true" when runtime PM > has been disabled for the device - no matter whether the state is > RPM_ACTIVE or not. It's quite heavily used, so I guess the behaviour > is what the callers expect? I'm not sure about that after looking at some of them briefly. I'm guessing that the original author of it wanted to use it in the system-wide suspend path to decide whether or not to suspend the device, but that obviously interferes with what the PM core does. Anway, it looks like its name should be changed to something like pm_runtime_active_or_disabled(), so it reflects the functionality. > > > > > For this reason, I believe it seems reasonable to trust it, both from HW > > > point of view, but definitely also from a runtime PM point of view. If > > > not, then what should we do? > > > > Trust it only when runtime PM is enabled, ie. dev->power.disable_depth == 0. > > > > That's exactly the reason why pm_runtime_suspended() returns false if > > runtime PM is disabled for the target device and if > > pm_runtime_suspended() is called during system-wide suspend and > > resume, it is not clear how to interpret its return value. > > To me, I don't think there is a problem interpreting > pm_runtime_suspended()'s return value. As you said, it tells whether > runtime PM has been enabled and whether the state is RPM_SUSPENDED. > > For those that want to know the state when runtime PM has been > disabled, they can use pm_runtime_status_suspended(). > > What's the issue? It may not match the real state of the device which may be relevant. > > > > If it returns true outside the system suspend-resume code path, that > > means "runtime PM has been enabled and the device has been > > runtime-suspended" and I want it to mean exactly the same thing during > > system-wide suspend and resume, so people don't need to wonder about > > the context in which the code is running. > > Well, even if this seems nice, I wonder how useful this would be in the end. > > Perhaps you have some concrete examples where this could improve things? For example, what if pm_runtime_force_suspend() is called for a device with PM-runtime disabled and the PM-runtime status equal to RPM_SUSPENDED? As I said above, the callback must be invoked for it in case it is functional and needs to be suspended, but that won't happen AFAICS. However, if pm_runtime_force_suspend() can tell whether or not PM-runtime has been enabled for the device, it can do the right thing (basically, avoid running the callback if it has been run once already by PM-runtime, which is the case when PM-runtime is enabled and the status is RPM_SUSPENDED). _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel