linux-kernel - Re: [PATCH 2/2] pmdomain: ti_sci: Add LPM abort sequence to suspend path

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPDyKForKeb=uTkOXWcJea-P9c+7wQ3ZRWFYpTXk=xLoQPsQaw@mail.gmail.com>
Date: Wed, 2 Jul 2025 14:06:22 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Kendall Willis <k-willis@...com>
Cc: nm@...com, kristo@...nel.org, ssantosh@...nel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	linux-pm@...r.kernel.org, d-gole@...com, vishalm@...com, sebin.francis@...com, 
	msp@...libre.com, khilman@...libre.com
Subject: Re: [PATCH 2/2] pmdomain: ti_sci: Add LPM abort sequence to suspend path

On Fri, 27 Jun 2025 at 22:49, Kendall Willis <k-willis@...com> wrote:
>
> Create ->suspend_late() and ->suspend_noirq() hooks to add abort sequence
> to any driver within the PM domain with either of those hooks. If a driver
> fails to suspend with those hooks, add a call to the DM to abort entering
> the LPM. The suspend hooks of the PM domains driver inserts itself into
> the suspend path of all devices connected to the TI SCI PM domain. So if
> any device in the PM domain with either a ->suspend_late() or
> ->suspend_noirq hook fails to suspend, the PM domain drivers suspend hook
> will send the abort call to the DM.
>
> Adding an abort call in the ->suspend() hook is not necessary. TI SCI
> suspend sends the message to the DM to prepare to enter a low power mode.
> TI SCI suspend ALWAYS occurs after the ->suspend() hook for all TI SCI
> devices has been called.
>
> Signed-off-by: Kendall Willis <k-willis@...com>
> ---
>  drivers/pmdomain/ti/ti_sci_pm_domains.c | 46 ++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c
> index 82df7e44250bb..975defc16271d 100644
> --- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
> +++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
> @@ -2,7 +2,7 @@
>  /*
>   * TI SCI Generic Power Domain Driver
>   *
> - * Copyright (C) 2015-2017 Texas Instruments Incorporated - http://www.ti.com/
> + * Copyright (C) 2015-2025 Texas Instruments Incorporated - http://www.ti.com/
>   *     J Keerthy <j-keerthy@...com>
>   *     Dave Gerlach <d-gerlach@...com>
>   */
> @@ -149,8 +149,47 @@ static int ti_sci_pd_suspend(struct device *dev)
>
>         return 0;
>  }
> +
> +static int ti_sci_pd_suspend_late(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +       struct ti_sci_pm_domain *pd = genpd_to_ti_sci_pd(genpd);
> +       const struct ti_sci_handle *ti_sci = pd->parent->ti_sci;
> +       int ret;
> +
> +       ret = pm_generic_suspend_late(dev);
> +       if (ret) {
> +               dev_err(dev, "%s: Failed to suspend. Abort entering low power mode.\n", __func__);
> +               if (ti_sci->ops.pm_ops.lpm_abort(ti_sci))
> +                       dev_err(dev, "%s: Failed to abort.\n", __func__);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int ti_sci_pd_suspend_noirq(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +       struct ti_sci_pm_domain *pd = genpd_to_ti_sci_pd(genpd);
> +       const struct ti_sci_handle *ti_sci = pd->parent->ti_sci;
> +       int ret;
> +
> +       ret = pm_generic_suspend_noirq(dev);
> +       if (ret) {
> +               dev_err(dev, "%s: Failed to suspend. Abort entering low power mode.\n", __func__);
> +               if (ti_sci->ops.pm_ops.lpm_abort(ti_sci))
> +                       dev_err(dev, "%s: Failed to abort.\n", __func__);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
>  #else
>  #define ti_sci_pd_suspend              NULL
> +#define ti_sci_pd_suspend_late         NULL
> +#define ti_sci_pd_suspend_noirq                NULL
>  #endif
>
>  /*
> @@ -264,6 +303,11 @@ static int ti_sci_pm_domain_probe(struct platform_device *pdev)
>                                     pd_provider->ti_sci->ops.pm_ops.set_latency_constraint)
>                                         pd->pd.domain.ops.suspend = ti_sci_pd_suspend;
>
> +                               if (pd_provider->ti_sci->ops.pm_ops.lpm_abort) {
> +                                       pd->pd.domain.ops.suspend_late = ti_sci_pd_suspend_late;
> +                                       pd->pd.domain.ops.suspend_noirq = ti_sci_pd_suspend_noirq;

This doesn't work as pm_genpd_init() is assigning the
->suspend_noirq() callback, hence overriding the callback.

Moreover I am not sure this is the correct thing to do, as having your
own ->suspend_noirq() callback would mean that you will be by-passing
genpd's internal reference counting to understand when it's fine to
power-off the PM domain, vi a the ->power_off() callback.

Can the LPM abort be handled from a ->complete() callback instead?

> +                               }
> +
>                                 pm_genpd_init(&pd->pd, NULL, true);
>
>                                 list_add(&pd->node, &pd_provider->pd_list);
> --
> 2.34.1
>

Kind regards
Uffe