| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPDyKFrRZhU7qwWkTaPDioiBih-WBZjoM4irEuJcDyxNk4hVCA@mail.gmail.com>
Date: Wed, 30 Sep 2015 15:23:54 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc: Tomeu Vizoso <tomeu.vizoso@...labora.com>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
Alan Stern <stern@...land.harvard.edu>,
martyn.welch@...labora.co.uk,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Len Brown <len.brown@...el.com>,
Kevin Hilman <khilman@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Pavel Machek <pavel@....cz>
Subject: Re: [PATCH v7 1/2] PM / sleep: Go direct_complete if driver has no callbacks
[...]
>> > 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists