On 11-12-18, 17:42, Gregory CLEMENT wrote:

What if this fails ?

+		struct clk *clk = clk_get(cpu_dev, 0);
+
+		if (IS_ERR(clk))
+			dev_warn(cpu_dev, "Cannot get clock for CPU %d\n", cpu);
+		else if (clk_is_match(clk, cur_clk))
+			cpumask_set_cpu(cpu, cpumask);
+
+		clk_put(clk);

So you will do clk_put() even if clk_get() failed, that will trigger a WARN() if
I am not wrong.

+	}
+}
+
+static int armada_8k_add_opp(struct clk *clk, struct device *cpu_dev,
+			     struct freq_table *freq_tables, int opps_index)
+{
+	unsigned int cur_frequency;
+	unsigned int freq;
+	int i, ret;
+
+	/* Get nominal (current) CPU frequency. */
+	cur_frequency = clk_get_rate(clk);
+
+	if (!cur_frequency) {
+		dev_err(cpu_dev,
+			"Failed to get clock rate for this CPU\n");
+		return -EINVAL;
+	}
+
+	freq_tables[opps_index].cpu_dev = cpu_dev;
+	for (i = 0; i < ARRAY_SIZE(opps_div); i++) {
+		freq = cur_frequency / opps_div[i];
+
+		ret = dev_pm_opp_add(cpu_dev, freq, 0);
+		if (ret)
+			return ret;
+
+		freq_tables[opps_index].freq[i] = freq;
+	}
+
+	return 0;
+}
+
+static void armada_8k_cpufreq_free_table(void)
+{
+	opps_index--;
+	for (; opps_index >= 0; opps_index--) {
+		int i;
+
+		for (i = 0; i < ARRAY_SIZE(opps_div); i++)
+			dev_pm_opp_remove(freq_tables[opps_index].cpu_dev,
+					  freq_tables[opps_index].freq[i]);
+	}
+
+	kfree(freq_tables);
+}
+
+static int __init armada_8k_cpufreq_init(void)
+{
+	int ret = 0, cpu, nb_cpus;
+	struct device_node *node;
+
+	node = of_find_compatible_node(NULL, NULL, "marvell,ap806-cpu-clock");
+	if (!node || !of_device_is_available(node))
+		return -ENODEV;
+
+	nb_cpus = num_possible_cpus();
+	freq_tables = kcalloc(nb_cpus, sizeof(*freq_tables), GFP_KERNEL);

Add a blank line here.

+	/*
+	 * For each CPU, this loop registers the operating points
+	 * supported (which are the nominal CPU frequency and full integer
+	 * divisions of it).
+	 */
+	for_each_possible_cpu(cpu) {
+		struct device *cpu_dev;
+		struct cpumask cpus;
+		struct clk *clk;
+		int i, skip;
+
+		skip = 0;
+		cpu_dev = get_cpu_device(cpu);
+
+		if (!cpu_dev) {
+			dev_err(cpu_dev, "Cannot get CPU %d\n", cpu);
+			continue;
+		}
+
+		clk = devm_clk_get(cpu_dev, 0);

I don't think you should call devm_*() helpers on the cpu_dev pointer, this is
not like the platform device structure. The problem is that no driver gets
attached/detached from cpu_dev and so the resources may never get free.

+
+		if (IS_ERR(clk)) {
+			dev_err(cpu_dev, "Cannot get clock for CPU %d\n", cpu);
+			ret = PTR_ERR(clk);
+			goto remove_opp;
+		}
+
+		for (i = 0; i < nb_cpus; i++) {
+			if (clk_is_match(clk, freq_tables[i].clk)) {
+				skip = 1;

Why not do devm_clk_put() from right here and "continue" after that. That way
you can avoid "skip" variable as well.

Also why are you doing this clk-match at all ? This looks similar to what
armada_8k_get_sharing_cpus() is doing. You can optimize this stuff better I
believe.

I will do this:

Create a cpumask variable and assign possible_cpus mask to it. And then after
each iteration of this loop, I clear "cpus" (returned from
armada_8k_get_sharing_cpus()) from the CPUs from that cpumask. That way the loop
will never try the second CPU from the same cpumask again.

+				break;
+			}
+		}
+		if (skip) {
+			devm_clk_put(cpu_dev, clk);
+			continue;
+		}
+
+		freq_tables[opps_index].clk = clk;
+
+		ret = armada_8k_add_opp(clk, cpu_dev, freq_tables, opps_index);

And this opps_index thing is not very clean. Having a global variable this way
makes things very messy, which is being updated from several routines.

So if armada_8k_add_opp() fails after adding few OPPs, we will now call
armada_8k_cpufreq_free_table() which will first do opps_index-- and so you may
miss freeing the OPPs added during the last call to armada_8k_add_opp().

This is very messy and requires serious cleanup to be honest. Also it will be
more preferred if armada_8k_add_opp() cleans up all the allocations it has done
on a call if it fails in the middle of it, rather than a bigger routine freeing
both successful and failed ones.

+		if (ret)
+			goto remove_opp;
+
+		opps_index++;
+		cpumask_clear(&cpus);
+		armada_8k_get_sharing_cpus(clk, &cpus);
+		dev_pm_opp_set_sharing_cpus(cpu_dev, &cpus);
+	}
+
+	armada_8k_pdev = platform_device_register_simple("cpufreq-dt", -1,
+							 NULL, 0);
+	ret = PTR_ERR_OR_ZERO(armada_8k_pdev);
+	if (ret)
+		goto remove_opp;
+	return 0;
+
+remove_opp:
+	armada_8k_cpufreq_free_table();
+	return ret;
+}
+module_init(armada_8k_cpufreq_init);
+
+static void __exit armada_8k_cpufreq_exit(void)
+{
+	armada_8k_cpufreq_free_table();
+	platform_device_unregister(armada_8k_pdev);
+}
+module_exit(armada_8k_cpufreq_exit);
+
+MODULE_AUTHOR("Gregory Clement [off-list ref]");
+MODULE_DESCRIPTION("Armada 8K cpufreq driver");
+MODULE_LICENSE("GPL");
-- 
2.19.2

-- 
viresh

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