From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753160AbdKMV7F (ORCPT ); Mon, 13 Nov 2017 16:59:05 -0500 Received: from mail-oi0-f54.google.com ([209.85.218.54]:53582 "EHLO mail-oi0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753146AbdKMV7A (ORCPT ); Mon, 13 Nov 2017 16:59:00 -0500 X-Google-Smtp-Source: AGs4zMYqWRMYZ/VprNTW6aUT5vBwRELqikmwxpiZmfiwtd/rr10ZiK9gme1PPO8nFYXnppWY5rEHjXIConffpKgEj/o= MIME-Version: 1.0 In-Reply-To: <7259587.Kcyqf0xrP6@aspire.rjw.lan> References: <1713438.irjm9MTSvo@aspire.rjw.lan> <7259587.Kcyqf0xrP6@aspire.rjw.lan> From: "Rafael J. Wysocki" Date: Mon, 13 Nov 2017 22:58:59 +0100 X-Google-Sender-Auth: PZaLb7qXTyJL-PZ2SJ8AImx0hJY Message-ID: Subject: Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status() To: "Rafael J. Wysocki" Cc: Ulf Hansson , Linux PM , LKML , Greg Kroah-Hartman , Alan Stern , Linus Walleij Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 13, 2017 at 10:50 PM, Rafael J. Wysocki wrote: > On Monday, November 13, 2017 2:26:28 PM CET Ulf Hansson wrote: >> On 12 November 2017 at 01:27, Rafael J. Wysocki wrote: >> > From: Rafael J. Wysocki >> > >> > The check for "active" children in __pm_runtime_set_status(), when >> > trying to set the parent device status to "suspended", doesn't >> > really make sense, because in fact it is not invalid to set the >> > status of a device with runtime PM disabled to "suspended" in any >> > case. It is invalid to enable runtime PM for a device with its >> > status set to "suspended" while its child_count reference counter >> > is nonzero, but the check in __pm_runtime_set_status() doesn't >> > really cover that situation. >> >> The reason to why I changed this in commit a8636c89648a ("PM / >> Runtime: Don't allow to suspend a device with an active child") was >> because to get a consistent behavior. > > Well, it causes the function to return an error in a non-error situation, > which IMnsHO is a bug. > >> Because, setting the device's status to active (RPM_ACTIVE) via >> __pm_runtime_set_status(), requires its parent to also be active (in >> case the parent has runtime PM enabled). > > No!!! > > The check is in there, because the parent's usage_count is affected by that Actually, the parent's child_count is affected, but that doesn't matter here. > code and incrementing it in case the parent has runtime PM enabled and is > RPM_SUSPENDED leads to an inconsistent runtime PM state of the parent. IOW, > it would confuse the framework. > > There's no such issue if the runtime PM status of a child is set to RPM_SUSPENDED. > > It is all consistent without the check I'm removing and is made inconsistent > by that very check. > >> I would prefer to try maintain this consistency, but I also I realize >> that commit a8636c89648a, should also have been checking if the parent >> has runtime PM enabled (again for consistency), which it doesn't. >> >> What about fixing that instead? > > Runtime PM is *disabled* for the parent at this point, guaranteed, so there's > nothing to check, I'm afraid ... > > OTOH, the runtime PM status of the children doesn't matter here, because their > reference counters are not updated. Thanks, Rafael