| 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
| ||
|
Date: Tue, 1 Nov 2022 11:23:45 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Shawn Guo <shawn.guo@...aro.org>
Cc: "Rafael J . Wysocki" <rafael@...nel.org>,
Kevin Hilman <khilman@...nel.org>,
Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] PM: domains: Consolidate genpd_restore_noirq() and genpd_resume_noirq()
On Tue, 1 Nov 2022 at 03:47, Shawn Guo <shawn.guo@...aro.org> wrote:
>
> Most of the logic between genpd_restore_noirq() and genpd_resume_noirq()
> are same except GENPD_STATE_OFF status reset for hibernation restore.
> The suspended_count decrement for restore should be the right thing to do
> anyway, considering there is an increment in genpd_finish_suspend() for
> hibernation.
>
> Consolidate genpd_restore_noirq() and genpd_resume_noirq() into
> genpd_finish_resume() and handle GENPD_STATE_OFF status reset for
> restore case specially.
>
> Signed-off-by: Shawn Guo <shawn.guo@...aro.org>
I have a comment, see more below.
Nevertheless, please add:
Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org>
> ---
> drivers/base/power/domain.c | 70 ++++++++++++++++++-------------------
> 1 file changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 54f6b0dd35fb..b81baeb38d81 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1247,12 +1247,14 @@ static int genpd_suspend_noirq(struct device *dev)
> }
>
> /**
> - * genpd_resume_noirq - Start of resume of device in an I/O PM domain.
> + * genpd_finish_resume - Completion of resume of device in an I/O PM domain.
> * @dev: Device to resume.
> + * @resume_noirq: Generic resume_noirq callback.
> *
> * Restore power to the device's PM domain, if necessary, and start the device.
> */
> -static int genpd_resume_noirq(struct device *dev)
> +static int genpd_finish_resume(struct device *dev,
> + int (*resume_noirq)(struct device *dev))
> {
> struct generic_pm_domain *genpd;
> int ret;
> @@ -1264,9 +1266,25 @@ static int genpd_resume_noirq(struct device *dev)
> return -EINVAL;
>
> if (device_wakeup_path(dev) && genpd_is_active_wakeup(genpd))
> - return pm_generic_resume_noirq(dev);
> + return resume_noirq(dev);
>
> genpd_lock(genpd);
> +
> + /*
> + * Special handling for hibernation restore:
> + * At this point suspended_count == 0 means we are being run for the
> + * first time for the given domain in the present cycle.
> + */
> + if (resume_noirq == pm_generic_restore_noirq &&
> + genpd->suspended_count++ == 0) {
> + /*
> + * The boot kernel might put the domain into arbitrary state,
> + * so make it appear as powered off to genpd_sync_power_on(),
> + * so that it tries to power it on in case it was really off.
> + */
> + genpd->status = GENPD_STATE_OFF;
This has really never worked as intended. Resetting the status like
this, needs more careful actions.
For example, if the genpd->status was GENPD_STATE_ON, the parent
domain's ->sd_count have been increased - so that needs to be adjusted
too.
By looking at patch3/3, I wonder if we shouldn't try to align the
hibernation behaviors so the above hack can be dropped. Do you think
that could work?
> + }
> +
> genpd_sync_power_on(genpd, true, 0);
> genpd->suspended_count--;
> genpd_unlock(genpd);
> @@ -1281,6 +1299,19 @@ static int genpd_resume_noirq(struct device *dev)
> return pm_generic_resume_noirq(dev);
> }
>
> +/**
> + * genpd_resume_noirq - Start of resume of device in an I/O PM domain.
> + * @dev: Device to resume.
> + *
> + * Restore power to the device's PM domain, if necessary, and start the device.
> + */
> +static int genpd_resume_noirq(struct device *dev)
> +{
> + dev_dbg(dev, "%s()\n", __func__);
> +
> + return genpd_finish_resume(dev, pm_generic_resume_noirq);
> +}
> +
> /**
> * genpd_freeze_noirq - Completion of freezing a device in an I/O PM domain.
> * @dev: Device to freeze.
> @@ -1366,40 +1397,9 @@ static int genpd_poweroff_noirq(struct device *dev)
> */
> static int genpd_restore_noirq(struct device *dev)
> {
> - struct generic_pm_domain *genpd;
> - int ret = 0;
> -
> dev_dbg(dev, "%s()\n", __func__);
>
> - genpd = dev_to_genpd(dev);
> - if (IS_ERR(genpd))
> - return -EINVAL;
> -
> - /*
> - * At this point suspended_count == 0 means we are being run for the
> - * first time for the given domain in the present cycle.
> - */
> - genpd_lock(genpd);
> - if (genpd->suspended_count++ == 0) {
> - /*
> - * The boot kernel might put the domain into arbitrary state,
> - * so make it appear as powered off to genpd_sync_power_on(),
> - * so that it tries to power it on in case it was really off.
> - */
> - genpd->status = GENPD_STATE_OFF;
> - }
> -
> - genpd_sync_power_on(genpd, true, 0);
> - genpd_unlock(genpd);
> -
> - if (genpd->dev_ops.stop && genpd->dev_ops.start &&
> - !pm_runtime_status_suspended(dev)) {
> - ret = genpd_start_dev(genpd, dev);
> - if (ret)
> - return ret;
> - }
> -
> - return pm_generic_restore_noirq(dev);
> + return genpd_finish_resume(dev, pm_generic_restore_noirq);
> }
>
> /**
Kind regards
Uffe
Powered by blists - more mailing lists