lkml.org 
[lkml]   [2009]   [Aug]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC] PCI: Runtime power management
    On Thu, 13 Aug 2009, Matthew Garrett wrote:

    > --- a/drivers/pci/pci-driver.c
    > +++ b/drivers/pci/pci-driver.c

    > +#ifdef CONFIG_PM_RUNTIME
    > +
    > +static int pci_pm_runtime_suspend(struct device *dev)
    > +{
    > + struct pci_dev *pci_dev = to_pci_dev(dev);
    > + struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
    > + int error;
    > +
    > + device_set_wakeup_enable(dev, 1);

    This is a userspace policy parameter. Kernel code should not alter it.
    Instead you should test device_may_wakeup.

    > + error = pci_enable_runtime_wake(pci_dev, true);
    > +
    > + if (error)
    > + return -EBUSY;
    > +
    > + if (pm && pm->runtime_suspend)
    > + error = pm->runtime_suspend(dev);
    > +
    > + if (error)
    > + goto out;
    > +
    > + error = pci_pm_suspend(dev);
    > +
    > + if (error)
    > + goto resume;
    > +
    > + disable_irq(pci_dev->irq);
    > + error = pci_pm_suspend_noirq(dev);
    > + enable_irq(pci_dev->irq);
    > +
    > + if (error)
    > + goto resume_noirq;
    > +
    > + return 0;
    > +
    > +resume_noirq:
    > + disable_irq(pci_dev->irq);
    > + pci_pm_resume_noirq(dev);
    > + enable_irq(pci_dev->irq);
    > +resume:
    > + pci_pm_resume(dev);
    > +out:
    > + pci_enable_runtime_wake(pci_dev, false);
    > + return error;
    > +}

    The goto statements and unwinding code don't match up.

    > +static int pci_pm_runtime_resume(struct device *dev)
    > +{
    > + struct pci_dev *pci_dev = to_pci_dev(dev);
    > + struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
    > + int error = 0;
    > +
    > + disable_irq(pci_dev->irq);
    > + error = pci_pm_resume_noirq(dev);
    > + enable_irq(pci_dev->irq);
    > +
    > + if (error)
    > + return error;
    > +
    > + error = pci_pm_resume(dev);
    > +
    > + if (error)
    > + return error;
    > +
    > + if (pm->runtime_resume)
    > + error = pm->runtime_resume(dev);
    > +
    > + if (error)
    > + return error;
    > +
    > + error = pci_enable_runtime_wake(pci_dev, false);
    > +
    > + if (error)
    > + return error;
    > +
    > + return 0;
    > +}

    Log an error message when something goes wrong?

    > +static void pci_pm_runtime_idle(struct device *dev)
    > +{
    > + struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
    > +
    > + if (pm && pm->runtime_idle)
    > + pm->runtime_idle(dev);
    > +
    > + pm_schedule_suspend(dev, 0);
    > +}

    This misses the point. The whole idea of runtime_idle is to tell you
    that the device is idle and might be ready to be suspended. If you're
    going to call pm_schedule_suspend anyway, there's no reason to invoke
    pm->runtime_idle.

    Alan Stern



    \
     
     \ /
      Last update: 2009-08-13 17:19    [W:3.218 / U:0.672 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site