lkml.org 
[lkml]   [2009]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch update] Re: [linux-pm] Run-time PM idea (was: Re: [RFC][PATCH 0/2] PM: Rearrange core suspend code)
    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.

    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.

    > +/**
    > + * 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?

    > +/**
    > + * 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.

    > +/**
    > + * 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.

    I haven't checked the details of the code yet. More later...

    Alan Stern



    \
     
     \ /
      Last update: 2009-06-10 23:17    [W:0.027 / U:31.760 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site