[PATCH v3] cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs

[PATCH v3] cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs

[Mike Tipton]

Currently, all SCMI devices with performance domains attempt to register
a cpufreq driver, even if their performance domains aren't used to
control the CPUs. The cpufreq framework only supports registering a
single driver, so only the first device will succeed. And if that device
isn't used for the CPUs, then cpufreq will scale the wrong domains.

To avoid this, return early from scmi_cpufreq_probe() if the probing
SCMI device isn't referenced by the CPU device phandles.

This keeps the existing assumption that all CPUs are controlled by a
single SCMI device.

Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
Changes in v3:
- Use dev_of_node(dev) instead of dev->of_node.
- Sanity check scmi_np.
- Pick up Reviewed-by from Peng.
- Link to v2: https://lore.kernel.org/all/20250421195206.3736128-1-quic_mdtipton@quicinc.com/

Changes in v2:
- Return -ENODEV instead of 0 for irrelevant devices.
- Link to v1: https://lore.kernel.org/all/20250411212941.1275572-1-quic_mdtipton@quicinc.com/

 drivers/cpufreq/scmi-cpufreq.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index 944e899eb1be..b63992de9fc7 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -393,6 +393,35 @@ static struct cpufreq_driver scmi_cpufreq_driver = {
 	.set_boost	= cpufreq_boost_set_sw,
 };
 
+static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
+{
+	struct device_node *scmi_np = dev_of_node(scmi_dev);
+	struct device_node *np;
+	struct device *cpu_dev;
+	int cpu, idx;
+
+	if (!scmi_np)
+		return false;
+
+	for_each_possible_cpu(cpu) {
+		cpu_dev = get_cpu_device(cpu);
+		if (!cpu_dev)
+			continue;
+
+		np = dev_of_node(cpu_dev);
+
+		if (of_parse_phandle(np, "clocks", 0) == scmi_np)
+			return true;
+
+		idx = of_property_match_string(np, "power-domain-names", "perf");
+
+		if (of_parse_phandle(np, "power-domains", idx) == scmi_np)
+			return true;
+	}
+
+	return false;
+}
+
 static int scmi_cpufreq_probe(struct scmi_device *sdev)
 {
 	int ret;
@@ -401,7 +430,7 @@ static int scmi_cpufreq_probe(struct scmi_device *sdev)
 
 	handle = sdev->handle;
 
-	if (!handle)
+	if (!handle || !scmi_dev_used_by_cpus(dev))
 		return -ENODEV;
 
 	scmi_cpufreq_driver.driver_data = sdev;
-- 
2.34.1

Re: [PATCH v3] cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs

[Mike Tipton]

On Mon, Apr 28, 2025 at 07:47:28AM -0700, Mike Tipton wrote:
> Currently, all SCMI devices with performance domains attempt to register
> a cpufreq driver, even if their performance domains aren't used to
> control the CPUs. The cpufreq framework only supports registering a
> single driver, so only the first device will succeed. And if that device
> isn't used for the CPUs, then cpufreq will scale the wrong domains.
> 
> To avoid this, return early from scmi_cpufreq_probe() if the probing
> SCMI device isn't referenced by the CPU device phandles.
> 
> This keeps the existing assumption that all CPUs are controlled by a
> single SCMI device.
> 
> Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---

Hi Sudeep / Viresh,

Any thoughts on this?

Thanks,
Mike

Re: [PATCH v3] cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs

[Sudeep Holla]

On Mon, May 05, 2025 at 06:25:50PM -0700, Mike Tipton wrote:
> On Mon, Apr 28, 2025 at 07:47:28AM -0700, Mike Tipton wrote:
> > Currently, all SCMI devices with performance domains attempt to register
> > a cpufreq driver, even if their performance domains aren't used to
> > control the CPUs. The cpufreq framework only supports registering a
> > single driver, so only the first device will succeed. And if that device
> > isn't used for the CPUs, then cpufreq will scale the wrong domains.
> > 
> > To avoid this, return early from scmi_cpufreq_probe() if the probing
> > SCMI device isn't referenced by the CPU device phandles.
> > 
> > This keeps the existing assumption that all CPUs are controlled by a
> > single SCMI device.
> > 
> > Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > ---
> 
> Hi Sudeep / Viresh,
> 
> Any thoughts on this?
> 

I have actually queued and forgot to respond. Though I realise the change
is not dependent on any other changes now.

Viresh, hope you are OK with me taking this change or do you prefer to take
it via your tree ? I am fine with that as well.

-- 
Regards,
Sudeep

Re: [PATCH v3] cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs

[Cristian Marussi]

On Mon, Apr 28, 2025 at 07:47:28AM -0700, Mike Tipton wrote:
> Currently, all SCMI devices with performance domains attempt to register
> a cpufreq driver, even if their performance domains aren't used to
> control the CPUs. The cpufreq framework only supports registering a
> single driver, so only the first device will succeed. And if that device
> isn't used for the CPUs, then cpufreq will scale the wrong domains.
> 

Hi,

bit of lagging behind, my bad.


> To avoid this, return early from scmi_cpufreq_probe() if the probing
> SCMI device isn't referenced by the CPU device phandles.
> 
> This keeps the existing assumption that all CPUs are controlled by a
> single SCMI device.
> 
> Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
> Changes in v3:
> - Use dev_of_node(dev) instead of dev->of_node.
> - Sanity check scmi_np.
> - Pick up Reviewed-by from Peng.
> - Link to v2: https://lore.kernel.org/all/20250421195206.3736128-1-quic_mdtipton@quicinc.com/
> 
> Changes in v2:
> - Return -ENODEV instead of 0 for irrelevant devices.
> - Link to v1: https://lore.kernel.org/all/20250411212941.1275572-1-quic_mdtipton@quicinc.com/
> 
>  drivers/cpufreq/scmi-cpufreq.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index 944e899eb1be..b63992de9fc7 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -393,6 +393,35 @@ static struct cpufreq_driver scmi_cpufreq_driver = {
>  	.set_boost	= cpufreq_boost_set_sw,
>  };
>  
> +static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
> +{
> +	struct device_node *scmi_np = dev_of_node(scmi_dev);
> +	struct device_node *np;
> +	struct device *cpu_dev;
> +	int cpu, idx;
> +
> +	if (!scmi_np)
> +		return false;
> +
> +	for_each_possible_cpu(cpu) {
> +		cpu_dev = get_cpu_device(cpu);
> +		if (!cpu_dev)
> +			continue;
> +
> +		np = dev_of_node(cpu_dev);
> +
> +		if (of_parse_phandle(np, "clocks", 0) == scmi_np)

Shouldn't this, on Success, be released by an of_node_put() (or, BETTER,
by some OF-related cleanup.h magic...)

> +			return true;
> +
> +		idx = of_property_match_string(np, "power-domain-names", "perf");
> +
> +		if (of_parse_phandle(np, "power-domains", idx) == scmi_np)

Same.

> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int scmi_cpufreq_probe(struct scmi_device *sdev)
>  {
>  	int ret;
> @@ -401,7 +430,7 @@ static int scmi_cpufreq_probe(struct scmi_device *sdev)
>  
>  	handle = sdev->handle;
>  
> -	if (!handle)
> +	if (!handle || !scmi_dev_used_by_cpus(dev))
>  		return -ENODEV;
>  
>  	scmi_cpufreq_driver.driver_data = sdev;

Other than the above glitches, LGTM.
(I gave it a go on JUNO and some emulated setup..)

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Tested-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

Re: [PATCH v3] cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs

[Sudeep Holla]

On Wed, May 07, 2025 at 12:59:45PM +0100, Cristian Marussi wrote:
> On Mon, Apr 28, 2025 at 07:47:28AM -0700, Mike Tipton wrote:
> > +static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
> > +{
> > +	struct device_node *scmi_np = dev_of_node(scmi_dev);
> > +	struct device_node *np;
> > +	struct device *cpu_dev;
> > +	int cpu, idx;
> > +
> > +	if (!scmi_np)
> > +		return false;
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		cpu_dev = get_cpu_device(cpu);
> > +		if (!cpu_dev)
> > +			continue;
> > +
> > +		np = dev_of_node(cpu_dev);
> > +
> > +		if (of_parse_phandle(np, "clocks", 0) == scmi_np)
> 
> Shouldn't this, on Success, be released by an of_node_put() (or, BETTER,
> by some OF-related cleanup.h magic...)
> 

Good catch, I missed this.

With the above issue fixed, you can add and post new version:
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

I will drop this patch now that you need to fix and repost. Also Viresh
may be away, so better to route via his tree when he is back as I can't
take it without his ack. I was holding off my PR for this, but I will
send SCMI PR without this now.

-- 
Regards,
Sudeep

Re: [PATCH v3] cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs

[Mike Tipton]

On Wed, May 07, 2025 at 02:12:36PM +0100, Sudeep Holla wrote:
> On Wed, May 07, 2025 at 12:59:45PM +0100, Cristian Marussi wrote:
> > On Mon, Apr 28, 2025 at 07:47:28AM -0700, Mike Tipton wrote:
> > > +static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
> > > +{
> > > +	struct device_node *scmi_np = dev_of_node(scmi_dev);
> > > +	struct device_node *np;
> > > +	struct device *cpu_dev;
> > > +	int cpu, idx;
> > > +
> > > +	if (!scmi_np)
> > > +		return false;
> > > +
> > > +	for_each_possible_cpu(cpu) {
> > > +		cpu_dev = get_cpu_device(cpu);
> > > +		if (!cpu_dev)
> > > +			continue;
> > > +
> > > +		np = dev_of_node(cpu_dev);
> > > +
> > > +		if (of_parse_phandle(np, "clocks", 0) == scmi_np)
> > 
> > Shouldn't this, on Success, be released by an of_node_put() (or, BETTER,
> > by some OF-related cleanup.h magic...)
> > 
> 
> Good catch, I missed this.
> 
> With the above issue fixed, you can add and post new version:
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks Cristian / Sudeep.

The cleanup.h __free() logic gets a bit cumbersome here, especially with
two struct *device_node to free in the same scope. And since there isn't
any complicated cleanup logic to unwind, then I'll just go with the
direct calls to of_node_put().

Also note we aren't calling of_node_put() in scmi_cpu_domain_id(),
either. I can fix that in a follow up patch.

I'll send v4 of this patch shortly.

Re: [PATCH v3] cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs

[Dan Carpenter]

On Wed, May 07, 2025 at 12:59:45PM +0100, Cristian Marussi wrote:
> > +static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
> > +{
> > +	struct device_node *scmi_np = dev_of_node(scmi_dev);
> > +	struct device_node *np;
> > +	struct device *cpu_dev;
> > +	int cpu, idx;
> > +
> > +	if (!scmi_np)
> > +		return false;
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		cpu_dev = get_cpu_device(cpu);
> > +		if (!cpu_dev)
> > +			continue;
> > +
> > +		np = dev_of_node(cpu_dev);
> > +
> > +		if (of_parse_phandle(np, "clocks", 0) == scmi_np)
> 
> Shouldn't this, on Success, be released by an of_node_put() (or, BETTER,
> by some OF-related cleanup.h magic...)
> 

The cleanup.h magic is __free(of_node_put) but dev_of_node() doesn't
take a reference so I don't think it's required.

regards,
dan carpenter

> > +			return true;
> > +
> > +		idx = of_property_match_string(np, "power-domain-names", "perf");
> > +
> > +		if (of_parse_phandle(np, "power-domains", idx) == scmi_np)
> 
> Same.
> 
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}

Re: [PATCH v3] cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs

[Cristian Marussi]

On Wed, May 07, 2025 at 04:18:34PM +0300, Dan Carpenter wrote:
> On Wed, May 07, 2025 at 12:59:45PM +0100, Cristian Marussi wrote:
> > > +static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
> > > +{
> > > +	struct device_node *scmi_np = dev_of_node(scmi_dev);
> > > +	struct device_node *np;
> > > +	struct device *cpu_dev;
> > > +	int cpu, idx;
> > > +
> > > +	if (!scmi_np)
> > > +		return false;
> > > +
> > > +	for_each_possible_cpu(cpu) {
> > > +		cpu_dev = get_cpu_device(cpu);
> > > +		if (!cpu_dev)
> > > +			continue;
> > > +
> > > +		np = dev_of_node(cpu_dev);
> > > +
> > > +		if (of_parse_phandle(np, "clocks", 0) == scmi_np)
> > 
> > Shouldn't this, on Success, be released by an of_node_put() (or, BETTER,
> > by some OF-related cleanup.h magic...)
> > 
> 
> The cleanup.h magic is __free(of_node_put) but dev_of_node() doesn't
> take a reference so I don't think it's required.
> 
 
I was referrring to the 2 (possibly) successfull of_parse_phandle() needs an
of_put() before returning. (at least looking at of_parse_phandle() comments)

Thanks,
Cristian