lkml.org 
[lkml]   [2015]   [Sep]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v7 1/2] PM / sleep: Go direct_complete if driver has no callbacks
Date
On Wednesday, September 30, 2015 01:33:29 PM Ulf Hansson wrote:
> On 30 September 2015 at 11:58, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> > If a suitable prepare callback cannot be found for a given device and
> > its driver has no PM callbacks at all, assume that it can go direct to
> > complete when the system goes to sleep.
> >
> > The reason for this is that there's lots of devices in a system that do
> > no PM at all and there's no reason for them to prevent their ancestors
> > to do direct_complete if they can support it.
> >
> > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > ---
> >
> > Changes in v7:
> > - Reduce indentation by adding a label in device_prepare()
> >
> > Changes in v6:
> > - Add stub for !CONFIG_PM.
> > - Move implementation of device_check_pm_callbacks to power/main.c as it
> > doesn't belong to CONFIG_PM_SLEEP.
> > - Take dev->power.lock before modifying flag.
> >
> > Changes in v5:
> > - Check for all dev_pm_ops instances associated to a device, updating a
> > no_pm_callbacks flag at the times when that could change.
> >
> > drivers/base/dd.c | 3 +++
> > drivers/base/power/common.c | 27 +++++++++++++++++++++++++++
> > drivers/base/power/domain.c | 5 +++++
> > drivers/base/power/main.c | 8 ++++++++
> > drivers/base/power/power.h | 6 ++++++
> > include/linux/pm.h | 1 +
> > 6 files changed, 50 insertions(+)
> >
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index be0eb4639128..fe0e9cb684b8 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -205,6 +205,8 @@ static void driver_bound(struct device *dev)
> >
> > klist_add_tail(&dev->p->knode_driver, &dev->driver->p->klist_devices);
> >
> > + device_check_pm_callbacks(dev);
> > +
> > /*
> > * Make sure the device is no longer in one of the deferred lists and
> > * kick off retrying all pending devices
> > @@ -695,6 +697,7 @@ static void __device_release_driver(struct device *dev)
> > dev->pm_domain->dismiss(dev);
> >
> > klist_remove(&dev->p->knode_driver);
> > + device_check_pm_callbacks(dev);
> > if (dev->bus)
> > blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> > BUS_NOTIFY_UNBOUND_DRIVER,
> > diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> > index f32b802b98f4..1bba85f8bf8a 100644
> > --- a/drivers/base/power/common.c
> > +++ b/drivers/base/power/common.c
> > @@ -128,3 +128,30 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
> > dev->pm_domain->detach(dev, power_off);
> > }
> > EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
> > +
> > +static bool pm_ops_is_empty(const struct dev_pm_ops *ops)
> > +{
> > + if (!ops)
> > + return true;
> > +
> > + return !ops->prepare &&
> > + !ops->suspend &&
> > + !ops->suspend_late &&
> > + !ops->suspend_noirq &&
> > + !ops->resume_noirq &&
> > + !ops->resume_early &&
> > + !ops->resume &&
> > + !ops->complete;
> > +}
> > +
> > +void device_check_pm_callbacks(struct device *dev)
> > +{
> > + spin_lock_irq(&dev->power.lock);
> > + dev->power.no_pm_callbacks =
> > + (!dev->bus || pm_ops_is_empty(dev->bus->pm)) &&
> > + (!dev->class || pm_ops_is_empty(dev->class->pm)) &&
> > + (!dev->type || pm_ops_is_empty(dev->type->pm)) &&
> > + (!dev->pm_domain || pm_ops_is_empty(&dev->pm_domain->ops)) &&
> > + (!dev->driver || pm_ops_is_empty(dev->driver->pm));
> > + spin_unlock_irq(&dev->power.lock);
> > +}
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 16550c63d611..3cae1aa1885b 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -20,6 +20,8 @@
> > #include <linux/suspend.h>
> > #include <linux/export.h>
> >
> > +#include "power.h"
> > +
> > #define GENPD_RETRY_MAX_MS 250 /* Approximate */
> >
> > #define GENPD_DEV_CALLBACK(genpd, type, callback, dev) \
> > @@ -1305,6 +1307,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> >
> > list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
> >
> > + device_check_pm_callbacks(dev);
>
> Don't do this while the genpd mutex is held as there is no need to.
> Please move it outside the lock and only do it for non-error case.
>
> > out:
> > mutex_unlock(&genpd->lock);
> >
> > @@ -1369,6 +1372,8 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
> >
> > genpd_free_dev_data(dev, gpd_data);
> >
> > + device_check_pm_callbacks(dev);
> > +
> > return 0;
>
> I can't tell whether this is an interesting feature to use for devices
> that gets attached to the ACPI PM domain. Although, you currently
> doesn't deal with that case, and too me I think this looks a bit
> weird/unsymmetrical.

Good point.

It needs to be done in every situation where a PM domain is or can be used.

I guess we might require all PM domains to be attached to devices after
a successful probe at the latest (no PM domains should be attached/detached
after probe succeeds IOW), in which case it should be sufficient to do the
device_check_pm_callbacks() thing each time after probe successds.

Thoughts?

> > out:
> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > index 1710c26ba097..5af45d20b677 100644
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -131,6 +131,8 @@ void device_pm_add(struct device *dev)
> > dev_name(dev->parent));
> > list_add_tail(&dev->power.entry, &dpm_list);
> > mutex_unlock(&dpm_list_mtx);
> > +
> > + device_check_pm_callbacks(dev);
> > }
> >
> > /**
> > @@ -1569,6 +1571,11 @@ static int device_prepare(struct device *dev, pm_message_t state)
> >
> > dev->power.wakeup_path = device_may_wakeup(dev);
> >
> > + if (dev->power.no_pm_callbacks) {
> > + ret = 1; /* Let device go direct_complete */
> > + goto unlock;
> > + }
> > +
> > if (dev->pm_domain) {
> > info = "preparing power domain ";
> > callback = dev->pm_domain->ops.prepare;
> > @@ -1591,6 +1598,7 @@ static int device_prepare(struct device *dev, pm_message_t state)
> > if (callback)
> > ret = callback(dev);
> >
> > +unlock:
> > device_unlock(dev);
> >
> > if (ret < 0) {
> > diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> > index 998fa6b23084..f665a7850016 100644
> > --- a/drivers/base/power/power.h
> > +++ b/drivers/base/power/power.h
> > @@ -29,6 +29,8 @@ struct wake_irq {
> > extern void dev_pm_arm_wake_irq(struct wake_irq *wirq);
> > extern void dev_pm_disarm_wake_irq(struct wake_irq *wirq);
> >
> > +extern void device_check_pm_callbacks(struct device *dev);
> > +
> > #ifdef CONFIG_PM_SLEEP
> >
> > extern int device_wakeup_attach_irq(struct device *dev,
> > @@ -102,6 +104,10 @@ static inline void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
> > {
> > }
> >
> > +static inline void device_check_pm_callbacks(struct device *dev)
> > +{
> > +}
> > +
> > #endif
> >
> > #ifdef CONFIG_PM_SLEEP
> > diff --git a/include/linux/pm.h b/include/linux/pm.h
> > index 35d599e7250d..e334b9b8cd46 100644
> > --- a/include/linux/pm.h
> > +++ b/include/linux/pm.h
> > @@ -566,6 +566,7 @@ struct dev_pm_info {
> > bool ignore_children:1;
> > bool early_init:1; /* Owned by the PM core */
> > bool direct_complete:1; /* Owned by the PM core */
> > + bool no_pm_callbacks:1; /* Owned by the PM core */
>
> This flag is only being used while CONFIG_PM is set. I suggest we move
> within that ifdef.

I guess you mean CONFIG_PM_SLEEP?

> > spinlock_t lock;
> > #ifdef CONFIG_PM_SLEEP
> > struct list_head entry;

Thanks,
Rafael



\
 
 \ /
  Last update: 2015-09-30 15:01    [W:0.075 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site