linux-kernel - Re: [PATCH RESEND] PM / Runtime: let rpm_resume() succeed if RPM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.1209211611020.1323-100000@iolanthe.rowland.org>
Date:	Fri, 21 Sep 2012 16:22:13 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
cc:	Kevin Hilman <khilman@...prootsystems.com>,
	<linux-pm@...r.kernel.org>, <linux-omap@...r.kernel.org>,
	Santosh Shilimkar <santosh.shilimkar@...com>,
	Grygorii Strashko <grygorii.strashko@...com>,
	Nishanth Menon <nm@...com>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND] PM / Runtime: let rpm_resume() succeed if
 RPM_ACTIVE, even when disabled

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?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/