Re: [PATCH v2] PM / Domains: Call driver's noirq callbacks
Hi Mikko, [auto build test ERROR on pm/linux-next] [also build test ERROR on v4.12-rc6 next-20170621] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Mikko-Perttunen/PM-Domains-Call-driver-s-noirq-callbacks/20170621-45 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next config: sh-allmodconfig (attached as .config) compiler: sh4-linux-gnu-gcc (Debian 6.3.0-18) 6.3.0 20170516 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=sh All errors (new ones prefixed by >>): drivers/base/power/domain.c: In function 'pm_genpd_init': >> drivers/base/power/domain.c:1544:37: error: 'pm_genpd_poweroff_noirq' >> undeclared (first use in this function) genpd->domain.ops.poweroff_noirq = pm_genpd_poweroff_noirq; ^~~ drivers/base/power/domain.c:1544:37: note: each undeclared identifier is reported only once for each function it appears in vim +/pm_genpd_poweroff_noirq +1544 drivers/base/power/domain.c 1538 genpd->domain.ops.runtime_resume = genpd_runtime_resume; 1539 genpd->domain.ops.prepare = pm_genpd_prepare; 1540 genpd->domain.ops.suspend_noirq = pm_genpd_suspend_noirq; 1541 genpd->domain.ops.resume_noirq = pm_genpd_resume_noirq; 1542 genpd->domain.ops.freeze_noirq = pm_genpd_freeze_noirq; 1543 genpd->domain.ops.thaw_noirq = pm_genpd_thaw_noirq; > 1544 genpd->domain.ops.poweroff_noirq = pm_genpd_poweroff_noirq; 1545 genpd->domain.ops.restore_noirq = pm_genpd_restore_noirq; 1546 genpd->domain.ops.complete = pm_genpd_complete; 1547 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH v2] PM / Domains: Call driver's noirq callbacks
On 20.06.2017 17:18, Ulf Hansson wrote:
On 20 June 2017 at 15:38, Mikko Perttunen wrote:
Currently genpd installs its own suspend_noirq, resume_noirq,
and poweroff_noirq callbacks, but never calls down to the driver's
corresponding callbacks. Add these calls.
Signed-off-by: Mikko Perttunen
---
v2:
- Moved pm_generic_suspend_noirq to before pm_runtime_force_suspend,
and correspondingly pm_generic_resume_noirq after
pm_runtime_force_resume
- Added new pm_genpd_poweroff_noirq callback that is identical to
pm_genpd_suspend_noirq but calls the appropriate driver callback
drivers/base/power/domain.c | 50 -
1 file changed, 49 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index d3f1d96f75e9..b070ee58186d 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -919,6 +919,10 @@ static int pm_genpd_suspend_noirq(struct device *dev)
if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
return 0;
+ ret = pm_generic_suspend_noirq(dev);
+ if (ret)
+ return ret;
+
if (genpd->dev_ops.stop && genpd->dev_ops.start) {
ret = pm_runtime_force_suspend(dev);
if (ret)
@@ -961,6 +965,10 @@ static int pm_genpd_resume_noirq(struct device *dev)
if (genpd->dev_ops.stop && genpd->dev_ops.start)
ret = pm_runtime_force_resume(dev);
+ ret = pm_generic_resume_noirq(dev);
+ if (ret)
+ return ret;
+
return ret;
}
@@ -1015,6 +1023,46 @@ static int pm_genpd_thaw_noirq(struct device *dev)
}
/**
+ * pm_genpd_poweroff_noirq - Completion of hibernation of device in an
+ * I/O PM domain.
+ * @dev: Device to poweroff.
+ *
+ * Stop the device and remove power from the domain if all devices in it have
+ * been stopped.
+ */
+static int pm_genpd_poweroff_noirq(struct device *dev)
+{
+ struct generic_pm_domain *genpd;
+ int ret;
+
+ dev_dbg(dev, "%s()\n", __func__);
+
+ genpd = dev_to_genpd(dev);
+ if (IS_ERR(genpd))
+ return -EINVAL;
+
+ if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
+ return 0;
+
+ ret = pm_generic_poweroff_noirq(dev);
The only difference between pm_genpd_suspend_noirq() and
pm_genpd_poweroff_noirq() is the above line. Can we re-factor the code
so we avoid open code here, please.
I wasn't sure if the functions' complexity warranted adding a helper
function, but sure, I'll refactor this with a helper function.
+ if (ret)
+ return ret;
+
+ if (genpd->dev_ops.stop && genpd->dev_ops.start) {
+ ret = pm_runtime_force_suspend(dev);
+ if (ret)
+ return ret;
+ }
+
+ genpd_lock(genpd);
+ genpd->suspended_count++;
+ genpd_sync_power_off(genpd, true, 0);
+ genpd_unlock(genpd);
+
+ return 0;
+}
+
+/**
* pm_genpd_restore_noirq - Start of restore of device in an I/O PM domain.
* @dev: Device to resume.
*
@@ -1493,7 +1541,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
genpd->domain.ops.resume_noirq = pm_genpd_resume_noirq;
genpd->domain.ops.freeze_noirq = pm_genpd_freeze_noirq;
genpd->domain.ops.thaw_noirq = pm_genpd_thaw_noirq;
- genpd->domain.ops.poweroff_noirq = pm_genpd_suspend_noirq;
+ genpd->domain.ops.poweroff_noirq = pm_genpd_poweroff_noirq;
genpd->domain.ops.restore_noirq = pm_genpd_restore_noirq;
The pm_genpd_restore_noirq() doesn't invokes the lower level
->restore_noirq() callbacks. If you are going to change that for the
*poweroff* callback, certainly we should change that also for the
*restore* callbacks as well. Don't you think?
Moreover, what about the freeze and thaw callbacks, should these also
walk the lower level callbacks?
Yes, I'll add the calls to the rest of the ops as well.
Thanks,
Mikko.
genpd->domain.ops.complete = pm_genpd_complete;
--
2.1.4
Kind regards
Uffe
Re: [PATCH v2] PM / Domains: Call driver's noirq callbacks
On 20 June 2017 at 15:38, Mikko Perttunen wrote:
> Currently genpd installs its own suspend_noirq, resume_noirq,
> and poweroff_noirq callbacks, but never calls down to the driver's
> corresponding callbacks. Add these calls.
>
> Signed-off-by: Mikko Perttunen
> ---
> v2:
> - Moved pm_generic_suspend_noirq to before pm_runtime_force_suspend,
> and correspondingly pm_generic_resume_noirq after
> pm_runtime_force_resume
> - Added new pm_genpd_poweroff_noirq callback that is identical to
> pm_genpd_suspend_noirq but calls the appropriate driver callback
>
> drivers/base/power/domain.c | 50
> -
> 1 file changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index d3f1d96f75e9..b070ee58186d 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -919,6 +919,10 @@ static int pm_genpd_suspend_noirq(struct device *dev)
> if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
> return 0;
>
> + ret = pm_generic_suspend_noirq(dev);
> + if (ret)
> + return ret;
> +
> if (genpd->dev_ops.stop && genpd->dev_ops.start) {
> ret = pm_runtime_force_suspend(dev);
> if (ret)
> @@ -961,6 +965,10 @@ static int pm_genpd_resume_noirq(struct device *dev)
> if (genpd->dev_ops.stop && genpd->dev_ops.start)
> ret = pm_runtime_force_resume(dev);
>
> + ret = pm_generic_resume_noirq(dev);
> + if (ret)
> + return ret;
> +
> return ret;
> }
>
> @@ -1015,6 +1023,46 @@ static int pm_genpd_thaw_noirq(struct device *dev)
> }
>
> /**
> + * pm_genpd_poweroff_noirq - Completion of hibernation of device in an
> + * I/O PM domain.
> + * @dev: Device to poweroff.
> + *
> + * Stop the device and remove power from the domain if all devices in it have
> + * been stopped.
> + */
> +static int pm_genpd_poweroff_noirq(struct device *dev)
> +{
> + struct generic_pm_domain *genpd;
> + int ret;
> +
> + dev_dbg(dev, "%s()\n", __func__);
> +
> + genpd = dev_to_genpd(dev);
> + if (IS_ERR(genpd))
> + return -EINVAL;
> +
> + if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
> + return 0;
> +
> + ret = pm_generic_poweroff_noirq(dev);
The only difference between pm_genpd_suspend_noirq() and
pm_genpd_poweroff_noirq() is the above line. Can we re-factor the code
so we avoid open code here, please.
> + if (ret)
> + return ret;
> +
> + if (genpd->dev_ops.stop && genpd->dev_ops.start) {
> + ret = pm_runtime_force_suspend(dev);
> + if (ret)
> + return ret;
> + }
> +
> + genpd_lock(genpd);
> + genpd->suspended_count++;
> + genpd_sync_power_off(genpd, true, 0);
> + genpd_unlock(genpd);
> +
> + return 0;
> +}
> +
> +/**
> * pm_genpd_restore_noirq - Start of restore of device in an I/O PM domain.
> * @dev: Device to resume.
> *
> @@ -1493,7 +1541,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> genpd->domain.ops.resume_noirq = pm_genpd_resume_noirq;
> genpd->domain.ops.freeze_noirq = pm_genpd_freeze_noirq;
> genpd->domain.ops.thaw_noirq = pm_genpd_thaw_noirq;
> - genpd->domain.ops.poweroff_noirq = pm_genpd_suspend_noirq;
> + genpd->domain.ops.poweroff_noirq = pm_genpd_poweroff_noirq;
> genpd->domain.ops.restore_noirq = pm_genpd_restore_noirq;
The pm_genpd_restore_noirq() doesn't invokes the lower level
->restore_noirq() callbacks. If you are going to change that for the
*poweroff* callback, certainly we should change that also for the
*restore* callbacks as well. Don't you think?
Moreover, what about the freeze and thaw callbacks, should these also
walk the lower level callbacks?
> genpd->domain.ops.complete = pm_genpd_complete;
>
> --
> 2.1.4
>
Kind regards
Uffe
[PATCH v2] PM / Domains: Call driver's noirq callbacks
Currently genpd installs its own suspend_noirq, resume_noirq,
and poweroff_noirq callbacks, but never calls down to the driver's
corresponding callbacks. Add these calls.
Signed-off-by: Mikko Perttunen
---
v2:
- Moved pm_generic_suspend_noirq to before pm_runtime_force_suspend,
and correspondingly pm_generic_resume_noirq after
pm_runtime_force_resume
- Added new pm_genpd_poweroff_noirq callback that is identical to
pm_genpd_suspend_noirq but calls the appropriate driver callback
drivers/base/power/domain.c | 50 -
1 file changed, 49 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index d3f1d96f75e9..b070ee58186d 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -919,6 +919,10 @@ static int pm_genpd_suspend_noirq(struct device *dev)
if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
return 0;
+ ret = pm_generic_suspend_noirq(dev);
+ if (ret)
+ return ret;
+
if (genpd->dev_ops.stop && genpd->dev_ops.start) {
ret = pm_runtime_force_suspend(dev);
if (ret)
@@ -961,6 +965,10 @@ static int pm_genpd_resume_noirq(struct device *dev)
if (genpd->dev_ops.stop && genpd->dev_ops.start)
ret = pm_runtime_force_resume(dev);
+ ret = pm_generic_resume_noirq(dev);
+ if (ret)
+ return ret;
+
return ret;
}
@@ -1015,6 +1023,46 @@ static int pm_genpd_thaw_noirq(struct device *dev)
}
/**
+ * pm_genpd_poweroff_noirq - Completion of hibernation of device in an
+ * I/O PM domain.
+ * @dev: Device to poweroff.
+ *
+ * Stop the device and remove power from the domain if all devices in it have
+ * been stopped.
+ */
+static int pm_genpd_poweroff_noirq(struct device *dev)
+{
+ struct generic_pm_domain *genpd;
+ int ret;
+
+ dev_dbg(dev, "%s()\n", __func__);
+
+ genpd = dev_to_genpd(dev);
+ if (IS_ERR(genpd))
+ return -EINVAL;
+
+ if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
+ return 0;
+
+ ret = pm_generic_poweroff_noirq(dev);
+ if (ret)
+ return ret;
+
+ if (genpd->dev_ops.stop && genpd->dev_ops.start) {
+ ret = pm_runtime_force_suspend(dev);
+ if (ret)
+ return ret;
+ }
+
+ genpd_lock(genpd);
+ genpd->suspended_count++;
+ genpd_sync_power_off(genpd, true, 0);
+ genpd_unlock(genpd);
+
+ return 0;
+}
+
+/**
* pm_genpd_restore_noirq - Start of restore of device in an I/O PM domain.
* @dev: Device to resume.
*
@@ -1493,7 +1541,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
genpd->domain.ops.resume_noirq = pm_genpd_resume_noirq;
genpd->domain.ops.freeze_noirq = pm_genpd_freeze_noirq;
genpd->domain.ops.thaw_noirq = pm_genpd_thaw_noirq;
- genpd->domain.ops.poweroff_noirq = pm_genpd_suspend_noirq;
+ genpd->domain.ops.poweroff_noirq = pm_genpd_poweroff_noirq;
genpd->domain.ops.restore_noirq = pm_genpd_restore_noirq;
genpd->domain.ops.complete = pm_genpd_complete;
--
2.1.4
