Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934093AbeAKMcU (ORCPT + 1 other); Thu, 11 Jan 2018 07:32:20 -0500 Received: from mail-it0-f46.google.com ([209.85.214.46]:46054 "EHLO mail-it0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932415AbeAKMcR (ORCPT ); Thu, 11 Jan 2018 07:32:17 -0500 X-Google-Smtp-Source: ACJfBouOOqkNjUnU7aSfoA0mV3PQb64lXfdKrOGN7Z7F01+HPGfhmNNsyeECDlJa1THbiPoix8f/JmK6GqXVT8YveJk= MIME-Version: 1.0 In-Reply-To: References: <4069324.Q7yJt6I4hJ@aspire.rjw.lan> <2074722.H5Hd6orL2D@aspire.rjw.lan> From: Ulf Hansson Date: Thu, 11 Jan 2018 13:32:15 +0100 Message-ID: Subject: Re: [PATCH v2] PM / runtime: Rework pm_runtime_force_suspend/resume() To: "Rafael J. Wysocki" Cc: "Rafael J. Wysocki" , Geert Uytterhoeven , Linux PM , Kevin Hilman , LKML , Lukas Wunner , Yoshihiro Shimoda , Linux-Renesas , USB list Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: [...] >>> Index: linux-pm/drivers/base/power/domain.c >>> =================================================================== >>> --- linux-pm.orig/drivers/base/power/domain.c >>> +++ linux-pm/drivers/base/power/domain.c >>> @@ -1048,8 +1048,9 @@ static int genpd_finish_suspend(struct d >>> if (ret) >>> return ret; >>> >>> - if (genpd->dev_ops.stop && genpd->dev_ops.start) { >>> - ret = pm_runtime_force_suspend(dev); >>> + if (genpd->dev_ops.stop && genpd->dev_ops.start && >>> + !pm_runtime_status_suspended(dev)) { >>> + ret = genpd_stop_dev(genpd, dev); >> >> Something like this existed there before, but because of other >> internal genpd reasons. It's an option but there are issues with it. > > OK > >> I think it's fragile because invoking the ->stop() callback may >> trigger calls to "disable" resources (like clocks, pinctrls, etc) for >> the device. Doing this at ->suspend_noirq() may be too late, as that >> subsystem/driver for the resource(s) may already be system suspended. >> This is the classic problem of suspending (and later resuming) devices >> in in-correct order. > > Well, this is what happens with the current code too. Correct. > > I mean if unmodified genpd_finish_suspend() runs, it will call > pm_runtime_force_suspend() and that will check the device's runtime PM > status in the first place. If that is "suspended", it will return > right away without doing anything (after disabling runtime PM, but > that is irrelevant here as runtime PM is already disabled). In turn, > if the runtime PM status is "active", it will run > genpd_runtime_suspend() (as the callback) and that will run the > driver's runtime PM callback and the genpd_stop_dev() right after it. > Thus, if calling genpd_stop_dev() at that point is problematic, it is > problematic already today. > > What the patch does is essentially to drop the driver's runtime PM > callback (should not be necessary) and the domain power off (certainly > not necessary) from that path, but to keep the genpd_stop_dev() > invocation that may be necessary in principle (the device is "active", > so genpd_runtime_suspend() has not run for it, so genpd_stop_dev() has > not been called for it yet, but it may be good to stop the clocks > before powering off the domain). I understand your suggested approach and it may work. > > The resume part is basically running genpd_start_dev() for the devices > for which genpd_stop_dev() was run by genpd_finish_suspend(), which is > because the subsequent driver callbacks may expect the clocks to be > enabled. > >> Today we deal with this, by drivers/subsystem assigning the right >> level of callback, as well as making sure the "dpm_list" is sorted >> correctly (yeah, we have device links as well). >> >> The point is, we can go for this solution, but is it good enough? > > I would like to treat it as a step away from what is there today, > retaining some of the existing functionality. > > From a quick look at the existing users of genpd that use the device > start/stop functionality, running genpd_stop_dev()/genpd_start_dev() > in the "noirq" phases should not be problematic for them at least. I guess so. Still, the error report by Geert worries me, but I don't think it worth to speculate, but rather test and see. > >> Another option, is to simply to remove (and not replace with something >> else) the calls to pm_runtime_force_ suspend|resume() from genpd. > > Right. > >> This moves the entire responsibility to the driver, to put its device into >> low power state during system suspend, but with the benefit of that it >> can decide to use the correct level of suspend/resume callbacks. > > Drivers of the devices in the "stop/start" domains will have to use > pm_runtime_force_ suspend|resume() internally then to benefit from the > domain's management of clocks, but that just may be the only way to > avoid more problems. Agree. > >> No matter how we do it, changing this may introduce some potential >> regressions from a power consumption point of view, however although >> only for those SoCs that uses the ->start|stop() callbacks. To >> mitigate these power regressions, some drivers needs to be fixed, but >> that seems inevitable anyway, doesn't it? > > Yes, it does. > > I would say let's try to go with this patch of mine as submitted and > see how far we can get with that. > > That essentially doesn't prevent us from dropping the > genpd_stop_dev()/genpd_start_dev() from the "noirq" stages if need be > after all, but dropping them right away may cause the fallout to be > more dramatic, at least in theory. > Well, my guesses is that would be a step to make the behavior a bit more deterministic and perhaps less fragile than today. On the other hand, I am also concerned about the future users of the ->stop|start() callbacks, including related drivers dealing with the affected devices. That in a sense, that converting the code in genpd to what you suggest, would still not encourage related drivers to behave correctly during system suspend/resume. Then down the road, when additional new users of the ->stop|start() callbacks may have been added, it may be even harder to drop calling them in from the noirq phases. So to conclude, I would prefer to drop calling pm_runtime_force_suspend|resume() from genpd, without a replacement, but I am willing to accept also your suggested alternative. Kind regards Uffe