Re: [linux-pm] Run-time PM idea (was: Re: [RFC][PATCH 0/2] PM: Rearrange core suspend code)

[Rafael J. Wysocki]

On Monday 08 June 2009, Oliver Neukum wrote:
> Am Montag, 8. Juni 2009 13:29:26 schrieb Rafael J. Wysocki:
> 
> > But I need to be able to call __pm_schedule_resume() (at least) from
> > interrupt context and I can't use a mutex from there.  Otherwise I'd have
> > used a mutex. :-)
> 
> I see.
> 
> > Anyway, below is a version with synchronous resume.
> 
> You are assuming autosuspend should always be with a delay. Why?

I couldn't invent a valid case for doing it without a delay.  Perhaps my
imagination is constrained too much. ;-)

> Secondly, you are not using a counter. Therefore only one driver can
> control the PM state of a device at a given time. Is that wise?

I didn't think about it to be honest.  Obviously this patch doesn't cover all
of the possible cases and I'm not even sure it's worth trying to cover them
upfront.

> > + * __pm_schedule_suspend - Schedule run-time suspend of given device.
> > + * @dev: Device to suspend.
> > + * @delay: Time to wait before attempting to suspend the device.
> 
> In which unit of time? If this is to go into kerneldoc that must be specified.

That's in jiifies.  Yes, I should have documented it.

> > + * @autocancel: If set, the request will be cancelled during a resume from
> > a + *	system-wide sleep state if it happens before @delay elapses.
> 
> Why is this needed?

In some subsystems, like PCI, devices will be resumed by the BIOS
unconditionally in the majority of cases and then it's not worth trying to
complete run-time PM requests from before the suspend.

> > + */
> > +void __pm_schedule_suspend(struct device *dev, unsigned long delay,
> > +			   bool autocancel)
> 
> [..]
> 
> 
> > +
> > +/**
> > + * __pm_schedule_resume - Schedule run-time resume of given device.
> > + * @dev: Device to resume.
> > + * @autocancel: If set, the request will be cancelled during a resume from
> > a + *	system-wide sleep state if it happens before pm_autoresume() can be
> > run. + */
> 
> Eeek! This is a bad idea. You never want to a resume to be cancelled.

Sometimes I do (see above).

> > +void __pm_schedule_resume(struct device *dev, bool autocancel)
> 
> [..]
> > +int pm_resume_sync(struct device *dev)
> > +{
> > +	int error = 0;
> > +
> > +	pm_lock_device(dev);
> > +	if (dev->power.runtime_status == RPM_IDLE) {
> > +		/* ->autosuspend() hasn't started yet, no need to resume. */
> > +		pm_cancel_suspend(dev);
> > +		goto out;
> > +	}
> > +
> > +	if (dev->power.runtime_status == RPM_SUSPENDING) {
> > +		/*
> > +		 * The ->autosuspend() callback is being executed right now,
> > +		 * wait for it to complete.
> > +		 */
> > +		pm_unlock_device(dev);
> > +		cancel_delayed_work_sync(&dev->power.suspend_work);
> 
> That is the most glorious abuse of an API I've seen this year :-)

Heh.

OK, what would you do instead?

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