On Wednesday 10 June 2009, Alan Stern wrote:
> On Wed, 10 Jun 2009, Rafael J. Wysocki wrote:
>
> > > The idea is that if ->autosuspend() or ->autoresume() returns an error code,
> > > this is a situation the PM core cannot recover from by itself, so it shouldn't
> > > pretend it knows what's happened. Instead, it marks the device as "I don't
> > > know if it is safe to touch this" and won't handle it until the device driver
> > > or bus type clears the status.
>
> I'm still not sure this is a good idea. When would the device driver
> clear the status? The autosuspend and autoresume methods run
> asynchronously, so after they're done the driver doesn't get a chance
> to do anything.
>
> It might be best just to set the status to RPM_ACTIVE if a runtime
> suspend fails and RPM_SUSPENDED if a runtime resume fails.
>
> > Finally, I decided to follow the Oliver's suggestion that some error codes returned
> > by ->autosuspend() and ->autoresume() may be regarded as "go back to the
> > previous state" information. I chose to use -EAGAIN and -EBUSY for this
> > purpose.
>
> Maybe...
>
>
> > struct dev_pm_info {
> > pm_message_t power_state;
> > - unsigned can_wakeup:1;
> > - unsigned should_wakeup:1;
> > + unsigned int can_wakeup:1;
> > + unsigned int should_wakeup:1;
> > enum dpm_state status; /* Owned by the PM core */
> > #ifdef CONFIG_PM_SLEEP
> > struct list_head entry;
> > #endif
> > +#ifdef CONFIG_PM_RUNTIME
> > + struct delayed_work suspend_work;
> > + unsigned int suspend_aborted:1;
> > + struct work_struct resume_work;
> > + struct completion work_done;
> > + enum rpm_state runtime_status;
> > + spinlock_t lock;
> > +#endif
> > };
>
> You know, it doesn't make any sense to have a suspend and a resume
> both pending at the same time.
>
> So you could add only a delayed_work structure and use its embedded
> work_struct for resume requests.
I thought so too, but I was wrong. ;-)
If resume is requested while the suspend hasn't completed yet, we should
queue it (it's totally valid to request a suspending device to resume IMO), but
the delayed work is still being used by the workqueue code, so we can't modify
it.
> Also, you might borrow a trick from Dave Brownell. Define the RPM_*
> values so that the individual bits have meanings. Then instead of
> testing for multiple possible values of runtime_status, you could do a
> simple bit test.
Yes, I'm seriously considering using this approach.
> > +/**
> > + * pm_device_suspended - Check if given device has been suspended at run time.
> > + * @dev: Device to check.
> > + * @data: Ignored.
> > + *
> > + * Returns 0 if the device has been suspended or -EBUSY otherwise.
> > + */
> > +static int pm_device_suspended(struct device *dev, void *data)
> > +{
> > + int ret;
> > +
> > + spin_lock(&dev->power.lock);
> > +
> > + ret = dev->power.runtime_status == RPM_SUSPENDED ? 0 : -EBUSY;
> > +
> > + spin_unlock(&dev->power.lock);
>
> How does acquiring the lock help here?
OK, it doesn't.
> > +/**
> > + * pm_check_children - Check if all children of a device have been suspended.
> > + * @dev: Device to check.
> > + *
> > + * Returns 0 if all children of the device have been suspended or -EBUSY
> > + * otherwise.
> > + */
>
> We might want to do a runtime suspend even if the device's children
> aren't already suspended. For example, you could suspend a link while
> leaving the device on the other end of the link at full power --
> especially if powering down the device is slow but changing the link's
> power level is fast.
Well, this means that the dependencies between devices in the device tree are
pretty much useless for the run-time PM as far as the core is concerned. In
which case, why did you mention them at all?
> > +/**
> > + * pm_autosuspend - Run autosuspend callback of given device object's bus type.
> > + * @work: Work structure used for scheduling the execution of this function.
> > + *
> > + * Use @work to get the device object the suspend has been scheduled for,
> > + * check if the suspend request hasn't been cancelled and run the
> > + * ->autosuspend() callback from the device's bus type driver. Update the
> > + * run-time PM flags in the device object to reflect the current status of the
> > + * device.
> > + */
> > +static void pm_autosuspend(struct work_struct *work)
>
> Can we call this something else? "Autosuspend" implies that the
> suspend originated from within the kernel. How about "pm_suspend_work"
> or "pm_runtime_suspend"? Likewise for the resume routines.
OK
> I haven't checked the details of the code yet. More later...
OK, thanks.
Best,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
