Re: [RFC v2 03/12] PM / cpu_domains: Setup PM domains for CPUs/clusters

[Lina Iyer <lina.iyer@linaro.org>]

On Fri, Feb 26 2016 at 12:10 -0700, Stephen Boyd wrote:
>On 02/12, Lina Iyer wrote:
>> diff --git a/drivers/base/power/cpu_domains.c b/drivers/base/power/cpu_domains.c
>> new file mode 100644
>> index 0000000..981592f
>> --- /dev/null
>> +++ b/drivers/base/power/cpu_domains.c
>> @@ -0,0 +1,267 @@
>> +
>> +/* List of CPU PM domains we care about */
>> +static LIST_HEAD(of_cpu_pd_list);
>> +static DEFINE_SPINLOCK(cpu_pd_list_lock);
>
>Can this be a mutex?
>
Yes, will change. The init function would not be called from atomic context.

>> +	genpd = kzalloc(sizeof(*(genpd)), GFP_KERNEL);
>
>Drop extra parenthesis
>

>> +	/* Register the CPU genpd */
>> +	pr_debug("adding %s as CPU PM domain.\n", pd->genpd->name);
>
>Drop the full stop?
>
OK

>> +	kfree(genpd);
>> +	kfree(genpd->name);
>
>Switch order so that name is freed first to avoid junk deref here.
>
Sounds good.

>> +	kfree(pd);
>> +	return ERR_PTR(ret);
>> +}
>> +
>> +static struct generic_pm_domain *of_get_cpu_domain(struct device_node *dn,
>> +		const struct cpu_pd_ops *ops, int cpu)
>> +{

>> +	genpd = of_genpd_get_from_provider(&args);
>> +	if (!IS_ERR(genpd))
>> +		goto skip_parent;
>
>Why not just return genpd and drop the goto?
>
Ok

>> +	genpd = of_init_cpu_pm_domain(dn, ops);
>> +	if (IS_ERR(genpd))
>> +		return genpd;
>> +
>> +	/* Is there a domain provider for this domain? */
>> +	ret = of_parse_phandle_with_args(dn, "power-domains",
>> +			"#power-domain-cells", 0, &args);
>> +	of_node_put(dn);
>
>Shouldn't this be of_node_put(args.np)? I suppose it's the same
>so this isn't too important.
>
>> +	if (ret < 0)
>> +		goto skip_parent;
>> +
>> +	/* Find its parent and attach this domain to it, recursively */
>> +	parent = of_get_cpu_domain(args.np, ops, cpu);
>
>Except that we use the np here. So perhaps move the of_node_put()
>down to the skip_parent goto?
>
Ok

>> +	if (IS_ERR(parent)) {
>> +		struct cpu_pm_domain *cpu_pd, *parent_cpu_pd;
>> +
>> +		ret = pm_genpd_add_subdomain(genpd, parent);
>
>parent is an error pointer here... isn't this always going to
>fail? Maybe that should be if (!IS_ERR(parent)) up there?
>
Good catch. Yes, it should be.

>> +		/*
>> +		 * Reference parent domain for easy access.
>> +		 * Note: We could be attached to a domain that is not a
>> +		 * CPU PM domain in that case dont reference the parent.
>
>s/dont/don't/
>
Done.

>> +	dn = of_get_cpu_node(cpu, NULL);
>> +	if (!dn)
>> +		return -ENODEV;
>> +
>> +	dn = of_parse_phandle(dn, "power-domains", 0);
>> +	if (!dn)
>> +		return -ENODEV;
>> +	of_node_put(dn);
>
>This should be put after of_get_cpu_domain().
>
Thanks for this review, Stephen.

Thanks,
Lina

>> +
>> +	/* Find the genpd for this CPU, create if not found */
>> +	genpd = of_get_cpu_domain(dn, ops, cpu);
>> +	if (IS_ERR(genpd))
>> +		return PTR_ERR(genpd);
>> +
>> +	return cpu_pd_attach_cpu(cpu);
>> +}
>> +EXPORT_SYMBOL(of_setup_cpu_pd_single);
>
>-- 
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>a Linux Foundation Collaborative Project