| 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: <hhgcy6kkggxkvveftkk2dqzxskyyp542zwonocg7puouzuu4n2@bkccm5wbe2hj>
Date: Tue, 6 Aug 2024 10:07:23 +0200
From: Markus Schneider-Pargmann <msp@...libre.com>
To: Kevin Hilman <khilman@...libre.com>
Cc: Ulf Hansson <ulf.hansson@...aro.org>, Nishanth Menon <nm@...com>,
Vibhore Vardhan <vibhore@...com>, Dhruva Gole <d-gole@...com>, Akashdeep Kaur <a-kaur@...com>,
Sebin Francis <sebin.francis@...com>, linux-arm-kernel@...ts.infradead.org, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org, 20240801195422.2296347-1-msp@...libre.com
Subject: Re: [PATCH 1/3] pmdomain: ti_sci: add per-device latency constraint
management
Hi Kevin,
On Mon, Aug 05, 2024 at 04:38:39PM GMT, Kevin Hilman wrote:
> For each device in a TI SCI PM domain, check whether the device has
> any resume latency constraints set via per-device PM QoS. If
> constraints are set, send them to DM via the new SCI constraints API.
>
> Checking for constraints happen:
>
> 1) before SCI PM domain power off (->power_off() hook)
> 2) before system-wide suspend (via ->suspend() hook)
>
> For TI SCI devices that are runtime PM enabled, check (1) will be the
> primary method, and will happen when the TI SCI PM domain is powered
> off (e.g. when the runtime PM usecount of the last device in that
> domain goes to zero.)
>
> For devices that are either not runtime PM enabled, or are not yet
> runtime suspended (e.g. due to being used during the suspend path),
> the constraints check will happen by check(2).
>
> Since constraints can be sent by either (1) or (2), driver keeps track
> of whether a valid constraint has been sent already.
>
> An important detail here is that the PM domain driver inserts itself
> into the path of both the ->suspend() and ->resume() hook path
> of *all* devices in the PM domain. This allows generic PM domain code
> to handle the constraint management and communication with TI SCI.
>
> Further, this allows device drivers to use existing PM QoS APIs to
> add/update constraints.
>
> DM firmware clears constraints during its resume, so Linux has
> to check/update/send constraints each time system suspends.
>
> Co-developed-by: Vibhore Vardhan <vibhore@...com>
> Signed-off-by: Vibhore Vardhan <vibhore@...com>
> Signed-off-by: Kevin Hilman <khilman@...libre.com>
> Signed-off-by: Dhruva Gole <d-gole@...com>
Reviewed-by: Markus Schneider-Pargmann <msp@...libre.com>
In general this looks good, two small things below.
> ---
> drivers/pmdomain/ti/ti_sci_pm_domains.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 91 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c
> index 1510d5ddae3d..4dc48a97f9b8 100644
> --- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
> +++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
> @@ -13,6 +13,8 @@
> #include <linux/platform_device.h>
> #include <linux/pm_domain.h>
> #include <linux/slab.h>
> +#include <linux/pm_qos.h>
> +#include <linux/pm_runtime.h>
> #include <linux/soc/ti/ti_sci_protocol.h>
> #include <dt-bindings/soc/ti,sci_pm_domain.h>
>
> @@ -47,10 +49,46 @@ struct ti_sci_pm_domain {
> struct generic_pm_domain pd;
> struct list_head node;
> struct ti_sci_genpd_provider *parent;
> + s32 lat_constraint;
> + bool constraint_sent;
> };
>
> #define genpd_to_ti_sci_pd(gpd) container_of(gpd, struct ti_sci_pm_domain, pd)
>
> +static inline bool ti_sci_pd_is_valid_constraint(s32 val)
> +{
> + return val != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
> +}
> +
> +static int ti_sci_pd_send_constraint(struct device *dev, s32 val)
> +{
> + 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 = ti_sci->ops.pm_ops.set_latency_constraint(ti_sci, val, TISCI_MSG_CONSTRAINT_SET);
> + if (!ret) {
> + pd->constraint_sent = true;
> + dev_dbg(dev, "ti_sci_pd: ID:%d set latency constraint %d\n",
> + pd->idx, val);
> + } else {
> + dev_err(dev, "ti_sci_pd: set latency constraint failed: ret=%d\n",
> + ret);
> + }
> +
> + return ret;
> +}
> +
> +static inline void ti_sci_pd_clear_constraints(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);
> +
> + pd->lat_constraint = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
> + pd->constraint_sent = false;
> +}
> +
> /*
> * ti_sci_pd_power_off(): genpd power down hook
> * @domain: pointer to the powerdomain to power off
> @@ -59,6 +97,18 @@ static int ti_sci_pd_power_off(struct generic_pm_domain *domain)
> {
> struct ti_sci_pm_domain *pd = genpd_to_ti_sci_pd(domain);
> const struct ti_sci_handle *ti_sci = pd->parent->ti_sci;
> + struct pm_domain_data *pdd;
> +
> + list_for_each_entry(pdd, &domain->dev_list, list_node) {
> + struct device *dev = pdd->dev;
> + s32 val;
> +
> + /* If device has any resume latency constraints, send 'em */
> + val = dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY);
> + if (ti_sci_pd_is_valid_constraint(val) && !pd->constraint_sent)
> + ti_sci_pd_send_constraint(dev, val);
> + pd->lat_constraint = val;
> + }
>
> return ti_sci->ops.dev_ops.put_device(ti_sci, pd->idx);
> }
> @@ -79,6 +129,38 @@ static int ti_sci_pd_power_on(struct generic_pm_domain *domain)
> return ti_sci->ops.dev_ops.get_device(ti_sci, pd->idx);
> }
>
> +static int ti_sci_pd_resume(struct device *dev)
> +{
> + ti_sci_pd_clear_constraints(dev);
> + return pm_generic_resume(dev);
> +}
> +
> +static int ti_sci_pd_suspend(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);
> + s32 val;
> + int ret;
> +
> + ret = pm_generic_suspend(dev);
> + if (ret)
> + return ret;
> +
> + /* Check if device has any resume latency constraints */
> + val = dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY);
> + if (ti_sci_pd_is_valid_constraint(val) && !pd->constraint_sent) {
> + if (genpd && genpd->status == GENPD_STATE_OFF)
> + dev_warn(dev, "%s: %s: already off.\n", genpd->name, __func__);
> + else if (pm_runtime_suspended(dev))
> + dev_warn(dev, "%s: %s: already RPM suspended.\n", genpd->name, __func__);
> + else
> + ti_sci_pd_send_constraint(dev, val);
> + }
> + pd->lat_constraint = val;
Could you get rid of pd->constraint_sent? I don't really see a situation
where it would be necessary.
> +
> + return 0;
> +}
> +
> /*
> * ti_sci_pd_xlate(): translation service for TI SCI genpds
> * @genpdspec: DT identification data for the genpd
> @@ -188,7 +270,15 @@ static int ti_sci_pm_domain_probe(struct platform_device *pdev)
> pd->pd.power_on = ti_sci_pd_power_on;
> pd->idx = args.args[0];
> pd->parent = pd_provider;
> -
> + pd->lat_constraint = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
Optionally you could use ti_sci_pd_clear_constraints() here instead.
Best,
Markus
> + /*
> + * If SCI constraint functions are present, then firmware
> + * supports the constraints API.
> + */
> + if (pd_provider->ti_sci->ops.pm_ops.set_device_constraint) {
> + pd->pd.domain.ops.resume = ti_sci_pd_resume;
> + pd->pd.domain.ops.suspend = ti_sci_pd_suspend;
> + }
> pm_genpd_init(&pd->pd, NULL, true);
>
> list_add(&pd->node, &pd_provider->pd_list);
>
> --
> 2.46.0
>
Powered by blists - more mailing lists