Re: [PATCH v2 06/14] PM / cpu_domains: Setup PM domains for CPUs/clusters

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

On Thu, Aug 04 2016 at 09:59 -0600, Brendan Jackman wrote:
>Hi Lina,
>
>I spotted a couple more nits..
>
>> +static struct generic_pm_domain *of_init_cpu_pm_domain(struct device_node *dn,
>> +				const struct cpu_pd_ops *ops)
>> +{
>> +	struct cpu_pm_domain *pd = NULL;
>> +	struct generic_pm_domain *genpd = NULL;
>> +	int ret = -ENOMEM;
>> +
>> +	if (!of_device_is_available(dn))
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	genpd = kzalloc(sizeof(*genpd), GFP_KERNEL);
>> +	if (!genpd)
>> +		goto fail;
>> +
>> +	genpd->name = kstrndup(dn->full_name, CPU_PD_NAME_MAX, GFP_KERNEL);
>> +	if (!genpd->name)
>> +		goto fail;
>> +
>> +	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
>> +	if (!pd)
>> +		goto fail;
>> +
>> +	if (!zalloc_cpumask_var(&pd->cpus, GFP_KERNEL))
>> +		goto fail;
>> +
>> +	genpd->power_off = cpu_pd_power_off;
>> +	genpd->power_on = cpu_pd_power_on;
>> +	genpd->flags |= GENPD_FLAG_IRQ_SAFE;
>> +	genpd->of_node = dn;
>> +	pd->genpd = genpd;
>> +	pd->ops.power_on = ops->power_on;
>> +	pd->ops.power_off = ops->power_off;
>> +
>> +	INIT_LIST_HEAD_RCU(&pd->link);
>> +	mutex_lock(&cpu_pd_list_lock);
>> +	list_add_rcu(&pd->link, &of_cpu_pd_list);
>> +	mutex_unlock(&cpu_pd_list_lock);
>> +
>> +	/* Populate platform specific states from DT */
>> +	if (ops->populate_state_data) {
>> +		struct device_node *np;
>> +		int i;
>> +
>> +		/* Initialize the arm,idle-state properties */
>> +		ret = pm_genpd_of_parse_power_states(genpd);
>> +		if (ret) {
>> +			pr_warn("%s domain states not initialized (%d)\n",
>> +					dn->full_name, ret);
>> +			goto fail;
>> +		}
>> +		for (i = 0; i < genpd->state_count; i++) {
>> +			np = of_parse_phandle(dn, "domain-idle-states", i);
>> +			ret = ops->populate_state_data(np,
>> +					&genpd->states[i].param);
>> +			of_node_put(np);
>> +			if (ret)
>> +				goto fail;
>> +		}
>> +	}
>
>It seems a bit unfortunate to do of_parse_phandle for "domain-idle-states"
>again, when pm_genpd_of_parse_power_states has just done that. Maybe we could
>could add an `of_node` member to `struct genpd_power_state` and have
>pm_genpd_of_parse_power_states populate that?
>
Hmm.. that could be done.

>> +
>> +	/* Register the CPU genpd */
>> +	pr_debug("adding %s as CPU PM domain\n", pd->genpd->name);
>> +	ret = pm_genpd_init(pd->genpd, &simple_qos_governor, false);
>> +	if (ret) {
>> +		pr_err("Unable to initialize domain %s\n", dn->full_name);
>> +		goto fail;
>> +	}
>> +
>> +	ret = of_genpd_add_provider_simple(dn, pd->genpd);
>> +	if (ret)
>> +		pr_warn("Unable to add genpd %s as provider\n",
>> +				pd->genpd->name);
>> +
>> +	return pd->genpd;
>> +fail:
>> +
>> +	kfree(genpd->name);
>> +	kfree(genpd);
>> +	if (pd)
>> +		kfree(pd->cpus);
>> +	kfree(pd);
>> +	return ERR_PTR(ret);
>> +}
>> +
>
>
>> +int of_setup_cpu_pd_single(int cpu, const struct cpu_pd_ops *ops)
>> +{
>> +
>> +	struct device_node *dn;
>> +	struct generic_pm_domain *genpd;
>> +	struct cpu_pm_domain *cpu_pd;
>> +
>> +	dn = of_get_cpu_node(cpu, NULL);
>> +	if (!dn)
>> +		return -ENODEV;
>
>of_get_cpu_node increments the refcount so we need an of_node_put at some point.
>
>There are loads of instances around the kernel of of_get_cpu_node without
>corresponding of_node_put, though, I could be misunderstanding...
>
It should be done. An easily missed statementr, I suppose.

Thanks for all the nits. This is the best time for that.

Thanks,
Lina

>> +
>> +	dn = of_parse_phandle(dn, "power-domains", 0);
>> +	if (!dn)
>> +		return -ENODEV;
>> +
>> +	/* 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);
>> +
>> +	of_node_put(dn);
>> +	cpu_pd = to_cpu_pd(genpd);
>> +	if (!cpu_pd) {
>> +		pr_err("%s: Genpd was created outside CPU PM domains\n",
>> +				__func__);
>> +		return -ENOENT;
>> +	}
>> +
>> +	return cpu_pd_attach_cpu(cpu_pd, cpu);
>> +}
>
>Cheers,
>Brendan