From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758154Ab2IUVk5 (ORCPT ); Fri, 21 Sep 2012 17:40:57 -0400 Received: from mail-pb0-f46.google.com ([209.85.160.46]:48160 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752979Ab2IUVkz (ORCPT ); Fri, 21 Sep 2012 17:40:55 -0400 From: Kevin Hilman To: Alan Stern Cc: "Rafael J. Wysocki" , , , Santosh Shilimkar , Grygorii Strashko , Nishanth Menon , LKML Subject: Re: [PATCH RESEND] PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled Organization: Deep Root Systems, LLC References: Date: Fri, 21 Sep 2012 14:40:52 -0700 In-Reply-To: (Alan Stern's message of "Fri, 21 Sep 2012 16:22:13 -0400 (EDT)") Message-ID: <87vcf7dpgb.fsf@deeprootsystems.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Alan Stern writes: > On Fri, 21 Sep 2012, Rafael J. Wysocki wrote: > >> > Kevin makes a good case that pm_runtime_resume() and related functions >> > should succeed even when runtime PM is disabled, if the device is >> > already in the desired state. >> > >> > The same may be true for pm_runtime_suspend(). What do you think? >> >> I've discussed that with Kevin. The problem is that the runtime PM >> status may be changed at will when runtime PM is disabled by using >> __pm_runtime_set_status(), so the status generally cannod be trusted >> if power.disable_depth > 0. > > Hmmm. That same argument applies even when is_suspended is true. > Runtime PM might have been disabled beforehand by the driver, so you > still don't know whether to believe the status. > >> During system suspend, however, runtime PM is disabled by the core and >> if neither the driver nor the subsystem has disabled it in the meantime, >> the status should be actually valid. > > I suppose you could check that .disable_depth == 1. That would mean > only the core had disabled runtime PM. > >> > The way the patch is written contradicts the documentation: >> > >> > * A request to execute ->runtime_resume() will cancel any pending or >> > scheduled requests to execute the other callbacks for the same device, >> > except for scheduled autosuspends. >> >> I'm not sure where the contradiction is. The patch simply changes the >> behavior for disabled runtime PM, which is to return a nonzero value immediately >> anyway. > > It changes an error return to a non-error return. > > However, if we limit the effects to times when the system is > suspending then there shouldn't be any pending or scheduled requests > anyway. So okay, this isn't an issue. > >> > > > @@ -510,7 +510,8 @@ static int rpm_resume(struct device *dev, int rpmflags) >> > > > if (dev->power.runtime_error) >> > > > retval = -EINVAL; >> > > > else if (dev->power.disable_depth > 0) >> > > > - retval = -EACCES; >> > > > + retval = dev->power.is_suspended && >> > > > + dev->power.runtime_status == RPM_ACTIVE ? 1 : -EACCES; >> > > > if (retval) >> > > > goto out; >> > >> > Also, the is_suspended test seems irrelevant in general -- it makes >> > sense in terms of the scenario Kevin described but that's not the >> > stated purpose of the patch. >> >> Well, it is, although the changelog doesn't state it sufficiently clearly. :-) > > Good point. The changelog needs to be improved. > >> > Both of these problems can be addressed by writing the code as follows: >> > >> > if (dev->power.runtime_error) >> > retval = -EINVAL; >> > else if (dev->power.disable_depth > 0) { >> > >> > /* Special case: allow this if the device is already active */ >> > if (dev->power.runtime_status != RPM_ACTIVE) >> > retval = -EACCES; >> > } >> > if (retval) >> > goto out; >> >> We could do that too, but I'm a bit concerned about the situations where >> runtime PM is disabled by the driver itself or by the subsystem, because >> in those cases whoever disables runtime PM would have to make sure that the >> status still reflects the actual hardware state, but that's what the runtime >> PM framework is for (among other things). > > All right, let's use Kevin's original scheme but add a test for > disable_depth == 1. I suggest changing the ?: operator to a regular > "if" statement, because the new condition will be even longer than the > old one (which I found a little hard to read in the first place). > > And of course, a comment should be added to explain the reason for the > exception. > > Kevin, how does this sound? > Sounds good to me. I'll respin and try to make the changelog more clear. Thanks, Kevin