Re: [PATCH v7 1/2] PM / sleep: Go direct_complete if driver has no callbacks

[Ulf Hansson]

[...]

>> > 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?

Ohh, I just realize that my comment was wrong. Sorry for the noise.

Why genpd needs special treatment is to take care off the
"non-probing" case. In other words when devices gets added to genpd
directly by using the pm_genpd_add_device() API. Perhaps that deserves
a comment though.

>
>> >   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?

Yes!

Kind regards
Uffe
--
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/