[RFC/NOT FOR MERGING 1/5] arm: omap: fix up _od_suspend_noirq and _od_resume_noirq

[Felipe Balbi]

Hi,

On Thu, Oct 18, 2012 at 10:42:37AM -0700, Kevin Hilman wrote:
> Felipe Balbi <balbi at ti.com> writes:
> 
> > Hi,
> >
> > On Thu, Oct 18, 2012 at 09:34:15AM -0700, Kevin Hilman wrote:
> >> Felipe Balbi <balbi at ti.com> writes:
> >> 
> >> > current implementation doesn't take care about
> >> > drivers which don't provide *_noirq methods 
> >> 
> >> The generic ops handle this.  See below.
> >> 
> >> > and we could fall into a situation where we would suspend/resume
> >> > devices even though they haven't asked for it.
> >> 
> >> The following explanation doesn't explain this, so I dont' follow
> >> the "even though they haven't asked for it" part.
> >> 
> >> > One such case happened with the I2C driver which
> >> > was getting suspended during suspend_noirq() just
> >> > to be resume right after by any other device doing
> >> > an I2C transfer on its suspend method.
> >> 
> >> In order to be I2C to be runtime resumed after its noirq method has been
> >> called, that means the other device is doing an I2C xfer in its noirq
> >> method.  That is a bug.
> >> 
> >> > Signed-off-by: Felipe Balbi <balbi at ti.com>
> >> > ---
> >> >  arch/arm/plat-omap/omap_device.c | 8 ++++++++
> >> >  1 file changed, 8 insertions(+)
> >> >
> >> > diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
> >> > index 7a7d1f2..935f416 100644
> >> > --- a/arch/arm/plat-omap/omap_device.c
> >> > +++ b/arch/arm/plat-omap/omap_device.c
> >> > @@ -804,8 +804,12 @@ static int _od_suspend_noirq(struct device *dev)
> >> >  {
> >> >  	struct platform_device *pdev = to_platform_device(dev);
> >> >  	struct omap_device *od = to_omap_device(pdev);
> >> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> >> >  	int ret;
> >> >  
> >> > +	if (!pm || !pm->suspend_noirq)
> >> > +		return 0;
> >> > +
> >> 
> >> you just re implemented pm_generic_suspend_noirq() (c.f. drivers/base/power/generic_ops.c)
> >> 
> >> >  	/* Don't attempt late suspend on a driver that is not bound */
> >> >  	if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
> >> >  		return 0;
> >> > @@ -827,6 +831,10 @@ static int _od_resume_noirq(struct device *dev)
> >> >  {
> >> >  	struct platform_device *pdev = to_platform_device(dev);
> >> >  	struct omap_device *od = to_omap_device(pdev);
> >> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> >> > +
> >> > +	if (!pm || !pm->resume_noirq)
> >> > +		return 0;
> >> 
> >> and this is basically pm_generic_resume_noirq()
> >
> > not true, there is a slight different. Note that you only call
> > pm_generic_resume_noirq() after calling pm_runtime_resume()ing the
> > device:
> 
> >> static int _od_resume_noirq(struct device *dev)
> >> {
> >> 	struct platform_device *pdev = to_platform_device(dev);
> >> 	struct omap_device *od = to_omap_device(pdev);
> >> 
> >> 	if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
> >> 	    !pm_runtime_status_suspended(dev)) {
> >> 		od->flags &= ~OMAP_DEVICE_SUSPENDED;
> >> 		if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
> >> 			omap_device_enable(pdev);
> >> 		pm_generic_runtime_resume(dev);
> >
> > right here. IMHO this is a bug, if the define doesn't implement
> > resume_noirq, it's telling you that it has nothing to do at that time,
> 
> This is where the misunderstanding is.  I suggest you have a look
> at commit c03f007a8bf0e092caeb6856a5c8a850df10b974 where this feature
> was added, but I'll try to summarize:
> 
> The goal is simply this:
> 
> If, by the time .suspend_noirq has run, the driver is not already
> runtime suspended, then forcefully runtime suspend it.
> 
> That's it.

Yes, I got the intention, but to me it looks wrong.

> Again, this is required because system suspend disables runtime PM
> transitions at a certain point, if the device is used after that point,
> there is *no other way* for the driver to properly manage the idle
> transition (or, if there is, please explain it to me.)

Can you please let me know of any situation where e.g. I2C/SPI would be
needed after all its children are suspended ?

> > so you shouldn't forcefully resume the device.
> 
> Note it's only forcefully resumed *if* it was forcefully suspended by
> suspend_noirq.

likewise, you shouldn't forcefully runtime suspend a device. The device
driver needs to be required to provide suspend/resume functions if it
cares.

> > If the device dies, then it means that it needs to implement
> > resume_noirq. What you have here, is a "workaround" for badly written
> > device drivers IMHO. 
> 
> That's one way of looking at it.  The other way of looking at it is that
> by handling this at the PM domain level, the drivers can be much simpler,
> and thus less likely to get wrong.

You're still bypassing what the PM framework wants us to do, no ? What
if Rafael decides to drastically change the way system and runtime PM is
done and it ends up breaking what we have on OMAP ? If we stick to the
standard, the it's less likely to brea.

> But even then, with your proposed changes, I don't think the device will
> be properly idled (including the omap_device_idle) in these important cases:
> 
> 1) I2C is used by some other device *after* its ->suspend method has
>    been called.

how can that happen ? I2C's ->suspend method will only be called after
all its children are suspended.

> 2) I2C runtime PM is disabled from userspace
>    (echo off > /sys/devices/.../power/control)

that should not make a difference if you omap_device_idle() is written
as it should. Maybe what you need is a refactor to provide
omap_device_idle() and omap_device_runtime_idle(), the latter taking
care of the case where runtime pm can be disabled from userspace while
the former idling whenever it's called.

> but I'll take this up in the I2C driver patch itself.

sure :-)

> > Note also, that you could runtime resume devices which don't implement
> > resume_noirq().
> 
> Again, this is by design.

a very weird design IMHO. Either that or I'm really missing something
here.

> It doesn't matter if the driver has noirq methods.  If it is not yet
> runtime suspended, it is forceably runtime suspended.   The generic

if it's not yet runtime suspended, you need to call the driver's
->suspend_* method (depending on the suspend phase you are right now),
but "reusing" the runtime_suspend method sounds to me like another
"special OMAP requirement".

> noirq calls are just there in case the driver has noirq callbacks.

I get that, but why would you suspend a device which doesn't want to be
suspended in suspend_noirq(), but using its runtime_suspend method ?

If I2C dies on a suspend/resume transition, it's a bug in the I2C
driver, no ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121018/687f60a5/attachment.sig>