linux-kernel - Re: [PATCH RFC] pmdomain: core: add support for writeble power domain state

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: <20250228111134.p56fnrcztufuahic@lcpd911>
Date: Fri, 28 Feb 2025 16:41:34 +0530
From: Dhruva Gole <d-gole@...com>
To: Kamlesh Gurudasani <kamlesh@...com>
CC: Ulf Hansson <ulf.hansson@...aro.org>, <vigneshr@...com>,
        <linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Kevin Hilman
	<khilman@...libre.com>, Peng Fan <peng.fan@....nxp.com>,
        Cristian Marussi
	<cristian.marussi@....com>
Subject: Re: [PATCH RFC] pmdomain: core: add support for writeble power
 domain state

On Feb 21, 2025 at 19:18:10 +0530, Kamlesh Gurudasani wrote:
> Add support for writeable power domain states from debugfs.

ACK.

+CC'ed a few more folks who maybe interested...
 
> Defining GENPD_ALLOW_WRITE_DEBUGFS will enable writeable pd_state
> node in debugfs.
> 
> Signed-off-by: Kamlesh Gurudasani <kamlesh@...com>
> ---
> This has turn out to be really helpful when debugging SCMI protocol
> for power domain management.

Agreed, I have found this patch very good for turning off/on power
domains in a "raw" form if you will.
We have also been using something called k3conf on TI K3
class of devices like AM62x family where we basically use a user space
tool to send power on/off/ device status without it going via the kernel
genPD. This approach that Kamlesh is suggesting helps us leverage the
kernel stacks (like SCMI) but while also going via the pmdomain core
driver. This patch helps abstract underneath protocol layers used for power
domain operations in SOCs...

Here's how it looks for anyone wondering: [1]
We also talked about this at last year's Embedded Open Source Summit,
Seattle [2] where we mentioned we'd be upstreaming this patch and the
audience seemed interested to use it.

> 
> Reference has been taken from clock framework which provides similar
> CLOCK_ALLOW_WRITE_DEBUGFS, which helps to test clocks from debugfs.
> ---
>  drivers/pmdomain/core.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 9b2f28b34bb5..6aba0c672da0 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -1298,6 +1298,60 @@ late_initcall_sync(genpd_power_off_unused);
>  
>  #ifdef CONFIG_PM_SLEEP
>  
> +#ifdef GENPD_ALLOW_WRITE_DEBUGFS
> +/*
> + * This can be dangerous, therefore don't provide any real compile time
> + * configuration option for this feature.
> + * People who want to use this will need to modify the source code directly.
> + */
> +static int genpd_state_set(void *data, u64 val)
> +{
> +
> +	struct generic_pm_domain *genpd = data;
> +	int ret = 0;
> +
> +	ret = genpd_lock_interruptible(genpd);

Why has it been kept interruptible? Other places I see are just using
genpd_lock(pd);

> +	if (ret)
> +		return -ERESTARTSYS;
> +
> +	if (val == 1) {
> +		genpd->power_on(genpd);
> +		genpd->status = GENPD_STATE_ON;
> +	} else if (val == 0) {
> +		genpd->power_off(genpd);
> +		genpd->status = GENPD_STATE_OFF;
> +	}
> +
> +	genpd_unlock(genpd);
> +	return 0;
> +}
> +
> +#define pd_state_mode	0644

Where's this used?

> +
> +static int genpd_state_get(void *data, u64 *val)
> +{
> +
> +	struct generic_pm_domain *genpd = data;
> +	int ret = 0;
> +
> +	ret = genpd_lock_interruptible(genpd);
> +	if (ret)
> +		return -ERESTARTSYS;
> +
> +	if (genpd->status == GENPD_STATE_OFF)
> +		*val = 0;
> +	else
> +		*val = 1;
> +
> +	genpd_unlock(genpd);
> +	return ret;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(pd_state_fops, genpd_state_get,
> +			 genpd_state_set, "%llu\n");
> +
> +#endif /* GENPD_ALLOW_WRITE_DEBUGFS */
> +
>  /**
>   * genpd_sync_power_off - Synchronously power off a PM domain and its parents.
>   * @genpd: PM domain to power off, if possible.
> @@ -3639,6 +3693,11 @@ static void genpd_debug_add(struct generic_pm_domain *genpd)
>  	if (genpd->set_performance_state)
>  		debugfs_create_file("perf_state", 0444,
>  				    d, genpd, &perf_state_fops);
> +#ifdef GENPD_ALLOW_WRITE_DEBUGFS
> +	debugfs_create_file("pd_state", 0644, d, genpd,

Ah this is probably where you wanted to use the #define'd pd_state_mode I guess....

> +			    &pd_state_fops);
> +#endif /* GENPD_ALLOW_WRITE_DEBUGFS */
> +
>  }
>  
>  static int __init genpd_debug_init(void)
> @@ -3653,6 +3712,24 @@ static int __init genpd_debug_init(void)
>  	list_for_each_entry(genpd, &gpd_list, gpd_list_node)
>  		genpd_debug_add(genpd);
>  

Only minor cleanups pending, otherwise patch looks good to me. Hoping
more people can also try it out and jump on this thread with their
thoughts....

Refs:
[1] https://gist.github.com/DhruvaG2000/f3ec24252d4cf2800c97c2e336d0b5db
[2]
https://eoss24.sched.com/event/1aBEs/a-primer-to-clocks-and-power-management-through-arm-scmi-dhruva-gole-kamlesh-gurudasani-texas-instruments

-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated