From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753901AbdKNJNr (ORCPT ); Tue, 14 Nov 2017 04:13:47 -0500 Received: from mail-it0-f49.google.com ([209.85.214.49]:52878 "EHLO mail-it0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752328AbdKNJNa (ORCPT ); Tue, 14 Nov 2017 04:13:30 -0500 X-Google-Smtp-Source: AGs4zMYtji8YdTtBlqw4KM3Tr0nVkotVfAxEcO0UlWkhlGpuDQeMw6h2YgWUPc9A834a0W8Nui0uSwAyvU7fthRpLV8= MIME-Version: 1.0 In-Reply-To: <7259587.Kcyqf0xrP6@aspire.rjw.lan> References: <1713438.irjm9MTSvo@aspire.rjw.lan> <7259587.Kcyqf0xrP6@aspire.rjw.lan> From: Ulf Hansson Date: Tue, 14 Nov 2017 10:13:29 +0100 Message-ID: Subject: Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status() To: "Rafael J. Wysocki" Cc: 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 13 November 2017 at 22:50, 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 > 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. Right, I do understand the reasons *why* it is like this. > > 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 ... Where and how is that guarantee made? [...] Kind regards Uffe