On 25-08-20, 12:31, Ionela Voinescu wrote:

On Tuesday 25 Aug 2020 at 11:26:18 (+0530), Viresh Kumar wrote:

quoted

On 24-08-20, 22:02, Ionela Voinescu wrote:

quoted

The current frequency passed to arch_set_freq_scale() could end up
being 0, signaling an error in setting a new frequency. Also, if the
maximum frequency in 0, this will result in a division by 0 error.

Therefore, validate these input values before using them for the
setting of the frequency scale factor.

Signed-off-by: Ionela Voinescu <redacted>
Cc: Sudeep Holla <redacted>
Cc: Rafael J. Wysocki <redacted>
---
 drivers/base/arch_topology.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 75f72d684294..1aca82fcceb8 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c

@@ -33,6 +33,9 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
 	unsigned long scale;
 	int i;
 
+	if (!cur_freq || !max_freq)

We should probably use unlikely() here.

Rafael: Shouldn't this have a WARN_ON_ONCE() as well ?

I'll add the unlikely() as it's definitely useful.

I'm somewhat on the fence about WARN_ON_ONCE() here. Wouldn't it work
better in cpufreq_driver_fast_switch()? It would cover scenarios where
the default arch_set_freq_scale() is used and flag potential hardware
issues with setting frequency that are currently just ignored both here
and in sugov_fast_switch().

I think validation and the WARN (if required) must all happen at the
same place. Considering that there can be many callers of a routine,
like this one, it is better to put all that in the end function only.

Maybe we can add the same in the dummy arch_set_freq_scale() if
required.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel