[PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Viresh Kumar]

of_cpufreq_cooling_register() can use frequency values from
policy->min/max/cpuinfo.min_freq/cpuinfo.max_freq, which are available only
after calling cpufreq_table_validate_and_show().

The right order of calling should be: cpufreq_table_validate_and_show() followed
by of_cpufreq_cooling_register(). Fix it.

Reported-by: Lukasz Majewski <l.majewski@samsung.com>
Reported-by: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
For 3.18.
---
 drivers/cpufreq/cpufreq-dt.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 8cba13d..22eb6e5 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -186,7 +186,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 {
 	struct cpufreq_dt_platform_data *pd;
 	struct cpufreq_frequency_table *freq_table;
-	struct thermal_cooling_device *cdev;
 	struct device_node *np;
 	struct private_data *priv;
 	struct device *cpu_dev;
@@ -269,20 +268,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		goto out_free_priv;
 	}
 
-	/*
-	 * For now, just loading the cooling device;
-	 * thermal DT code takes care of matching them.
-	 */
-	if (of_find_property(np, "#cooling-cells", NULL)) {
-		cdev = of_cpufreq_cooling_register(np, cpu_present_mask);
-		if (IS_ERR(cdev))
-			dev_err(cpu_dev,
-				"running cpufreq without cooling device: %ld\n",
-				PTR_ERR(cdev));
-		else
-			priv->cdev = cdev;
-	}
-
 	priv->cpu_dev = cpu_dev;
 	priv->cpu_reg = cpu_reg;
 	policy->driver_data = priv;
@@ -292,7 +277,22 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	if (ret) {
 		dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__,
 			ret);
-		goto out_cooling_unregister;
+		goto out_free_cpufreq_table;
+	}
+
+	/*
+	 * For now, just loading the cooling device;
+	 * thermal DT code takes care of matching them.
+	 */
+	if (of_find_property(np, "#cooling-cells", NULL)) {
+		priv->cdev = of_cpufreq_cooling_register(np, cpu_present_mask);
+		if (IS_ERR(priv->cdev)) {
+			dev_err(cpu_dev,
+				"running cpufreq without cooling device: %ld\n",
+				PTR_ERR(priv->cdev));
+
+			priv->cdev = NULL;
+		}
 	}
 
 	policy->cpuinfo.transition_latency = transition_latency;
@@ -305,8 +305,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 
 	return 0;
 
-out_cooling_unregister:
-	cpufreq_cooling_unregister(priv->cdev);
+out_free_cpufreq_table:
 	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
 out_free_priv:
 	kfree(priv);
-- 
2.0.3.693.g996b0fd

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 3899 bytes --]

Viresh

On Mon, Nov 24, 2014 at 04:29:13PM +0530, Viresh Kumar wrote:
> of_cpufreq_cooling_register() can use frequency values from
> policy->min/max/cpuinfo.min_freq/cpuinfo.max_freq, which are available only
> after calling cpufreq_table_validate_and_show().
> 
> The right order of calling should be: cpufreq_table_validate_and_show() followed
> by of_cpufreq_cooling_register(). Fix it.
> 
> Reported-by: Lukasz Majewski <l.majewski@samsung.com>
> Reported-by: Eduardo Valentin <edubezval@gmail.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> ---
> For 3.18.
> ---
>  drivers/cpufreq/cpufreq-dt.c | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index 8cba13d..22eb6e5 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -186,7 +186,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>  {
>  	struct cpufreq_dt_platform_data *pd;
>  	struct cpufreq_frequency_table *freq_table;
> -	struct thermal_cooling_device *cdev;
>  	struct device_node *np;
>  	struct private_data *priv;
>  	struct device *cpu_dev;
> @@ -269,20 +268,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>  		goto out_free_priv;
>  	}
>  
> -	/*
> -	 * For now, just loading the cooling device;
> -	 * thermal DT code takes care of matching them.
> -	 */
> -	if (of_find_property(np, "#cooling-cells", NULL)) {
> -		cdev = of_cpufreq_cooling_register(np, cpu_present_mask);
> -		if (IS_ERR(cdev))
> -			dev_err(cpu_dev,
> -				"running cpufreq without cooling device: %ld\n",
> -				PTR_ERR(cdev));
> -		else
> -			priv->cdev = cdev;
> -	}
> -
>  	priv->cpu_dev = cpu_dev;
>  	priv->cpu_reg = cpu_reg;
>  	policy->driver_data = priv;
> @@ -292,7 +277,22 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>  	if (ret) {
>  		dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__,
>  			ret);
> -		goto out_cooling_unregister;
> +		goto out_free_cpufreq_table;
> +	}
> +
> +	/*
> +	 * For now, just loading the cooling device;
> +	 * thermal DT code takes care of matching them.
> +	 */
> +	if (of_find_property(np, "#cooling-cells", NULL)) {
> +		priv->cdev = of_cpufreq_cooling_register(np, cpu_present_mask);
> +		if (IS_ERR(priv->cdev)) {
> +			dev_err(cpu_dev,
> +				"running cpufreq without cooling device: %ld\n",
> +				PTR_ERR(priv->cdev));
> +
> +			priv->cdev = NULL;
> +		}

Is it possible to have this registration only when we have a
cpufreq driver up and running? The reasoning is that only after we have
a way to control cpu frequencies, it makes sense to have the cpu_cooling
device.

I am planing to have the following check in the cpu cooling code:
	if (!cpufreq_get_current_driver()) {
		dev_dbg(bgp->dev, "no cpufreq driver yet\n");
		return -EPROBE_DEFER;
	}

that is the way I think of checking if the cpufreq layer is ready to
have a cpu cooling on top of it. Currently, thermal drivers check this
before calling cpu cooling registration. But instead of having this
check in every driver, I would like to move it to cpu cooling.

However, for cpufreq-dt, the registration currently happens in the
init phase, not in probe, so cpufreq driver is not registered, and thus
the check won't work.

In this way, I believe the sequencing between cpu cooling and cpufreq-dt
would work fine.


>  	}
>  
>  	policy->cpuinfo.transition_latency = transition_latency;
> @@ -305,8 +305,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>  
>  	return 0;
>  
> -out_cooling_unregister:
> -	cpufreq_cooling_unregister(priv->cdev);
> +out_free_cpufreq_table:
>  	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
>  out_free_priv:
>  	kfree(priv);
> -- 
> 2.0.3.693.g996b0fd
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Viresh Kumar]

On 24 November 2014 at 23:40, Eduardo Valentin <edubezval@gmail.com> wrote:
> Is it possible to have this registration only when we have a
> cpufreq driver up and running? The reasoning is that only after we have
> a way to control cpu frequencies, it makes sense to have the cpu_cooling
> device.
>
> I am planing to have the following check in the cpu cooling code:
>         if (!cpufreq_get_current_driver()) {
>                 dev_dbg(bgp->dev, "no cpufreq driver yet\n");
>                 return -EPROBE_DEFER;
>         }
>
> that is the way I think of checking if the cpufreq layer is ready to
> have a cpu cooling on top of it. Currently, thermal drivers check this
> before calling cpu cooling registration. But instead of having this
> check in every driver, I would like to move it to cpu cooling.
>
> However, for cpufreq-dt, the registration currently happens in the
> init phase, not in probe, so cpufreq driver is not registered, and thus
> the check won't work.

This is how the phases are present in cpufreq drivers:
-> platform_init
    -> probe()
        ->cpufreq_driver->init()

And the cooling device is registered in cpufreq_driver->init() and by
the time ->init() is called, cpufreq_driver is valid.

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 3213 bytes --]


Viresh,

On Tue, Nov 25, 2014 at 04:27:44PM +0530, Viresh Kumar wrote:
> On 24 November 2014 at 23:40, Eduardo Valentin <edubezval@gmail.com> wrote:
> > Is it possible to have this registration only when we have a
> > cpufreq driver up and running? The reasoning is that only after we have
> > a way to control cpu frequencies, it makes sense to have the cpu_cooling
> > device.
> >
> > I am planing to have the following check in the cpu cooling code:
> >         if (!cpufreq_get_current_driver()) {
> >                 dev_dbg(bgp->dev, "no cpufreq driver yet\n");
> >                 return -EPROBE_DEFER;
> >         }
> >
> > that is the way I think of checking if the cpufreq layer is ready to
> > have a cpu cooling on top of it. Currently, thermal drivers check this
> > before calling cpu cooling registration. But instead of having this
> > check in every driver, I would like to move it to cpu cooling.
> >
> > However, for cpufreq-dt, the registration currently happens in the
> > init phase, not in probe, so cpufreq driver is not registered, and thus
> > the check won't work.
> 
> This is how the phases are present in cpufreq drivers:
> -> platform_init
>     -> probe()
>         ->cpufreq_driver->init()

You are right! I got confused because even with your patch, the
sequencing is not working. Looking to that behavior I, somehow, thought
the _init function in cpufreq-dt was about init() calls. But in fact, it
is driver initialization callback.


> 
> And the cooling device is registered in cpufreq_driver->init() and by
> the time ->init() is called, cpufreq_driver is valid.

However, by the time of ->init() the cpufreq_driver is not really ready.
Or at least, the cpufreq layer is not ready. A call to

cpufreq_frequency_get_table()

for instance, it is not working. The reasoning is because
cpufreq_add_dev() is still not finalized by the time driver->init() is
called. Meaning, the policy cannot be fetch because the cpu masks have
not been set by that time. More important, in order to fetch the cpufreq
table we would need to have the cpufreq_cpu_data properly initialized
per cpu. In other words, when we call driver->init() that happens
before the following code:
	cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);

	if (!recover_policy) {
		policy->user_policy.min = policy->min;
		policy->user_policy.max =
		policy->max;
	}

	down_write(&policy->rwsem);
	write_lock_irqsave(&cpufreq_driver_lock, flags);
	for_each_cpu(j, policy->cpus)
		per_cpu(cpufreq_cpu_data, j) = policy;
	write_unlock_irqrestore(&cpufreq_driver_lock, flags);

The thing is, with current code:
cpufreq_add_dev()
	-> cpufreq-dt->init() 
		-> of_cpufreq_cooling_register()
			-> cpufreq_frequency_get_table()
				-> returns NULL;

Returns invalid data because it is not initialized yet.

The cpufreq-dt would need to add the of based cpufreq cooling only when
cpufreq layer is ready. Any other better cpufreq driver callback to add
the cpu cooling?

We could sort this out by polling in thermal layer for the cpufreq table
until it gets ready, but I believe that would be a dirty hack.

Cheers,

Eduardo Valentin

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Viresh Kumar]

On 25 November 2014 at 07:14, Eduardo Valentin <edubezval@gmail.com> wrote:
> You are right! I got confused because even with your patch, the
> sequencing is not working. Looking to that behavior I, somehow, thought
> the _init function in cpufreq-dt was about init() calls. But in fact, it
> is driver initialization callback.

:)

> However, by the time of ->init() the cpufreq_driver is not really ready.

I agree.

> Or at least, the cpufreq layer is not ready. A call to
>
> cpufreq_frequency_get_table()
>
> for instance, it is not working.

I know the story you pasted here :)

> The cpufreq-dt would need to add the of based cpufreq cooling only when
> cpufreq layer is ready. Any other better cpufreq driver callback to add
> the cpu cooling?

There is nothing as of now atleast.

> We could sort this out by polling in thermal layer for the cpufreq table
> until it gets ready, but I believe that would be a dirty hack.

Yeah. Probably we can add a notifier for cpufreq-driver addition/removal.
That's the best we can do I believe.

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Rafael J. Wysocki]

On Tuesday, November 25, 2014 08:56:00 PM Viresh Kumar wrote:
> On 25 November 2014 at 07:14, Eduardo Valentin <edubezval@gmail.com> wrote:
> > You are right! I got confused because even with your patch, the
> > sequencing is not working. Looking to that behavior I, somehow, thought
> > the _init function in cpufreq-dt was about init() calls. But in fact, it
> > is driver initialization callback.
> 
> :)
> 
> > However, by the time of ->init() the cpufreq_driver is not really ready.
> 
> I agree.
> 
> > Or at least, the cpufreq layer is not ready. A call to
> >
> > cpufreq_frequency_get_table()
> >
> > for instance, it is not working.
> 
> I know the story you pasted here :)
> 
> > The cpufreq-dt would need to add the of based cpufreq cooling only when
> > cpufreq layer is ready. Any other better cpufreq driver callback to add
> > the cpu cooling?
> 
> There is nothing as of now atleast.
> 
> > We could sort this out by polling in thermal layer for the cpufreq table
> > until it gets ready, but I believe that would be a dirty hack.
> 
> Yeah. Probably we can add a notifier for cpufreq-driver addition/removal.
> That's the best we can do I believe.

Can we please avoid adding any new notifiers?

We can add a new cpufreq driver callback to be invoked by the core when
everything has been set up for this purpose.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Viresh Kumar]

On 26 November 2014 at 02:17, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Can we please avoid adding any new notifiers?

Sure :)

> We can add a new cpufreq driver callback to be invoked by the core when
> everything has been set up for this purpose.

Hmm, that will be better..

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Lukasz Majewski]

Hi Viresh,

> of_cpufreq_cooling_register() can use frequency values from
> policy->min/max/cpuinfo.min_freq/cpuinfo.max_freq, which are
> available only after calling cpufreq_table_validate_and_show().
> 
> The right order of calling should be:
> cpufreq_table_validate_and_show() followed by
> of_cpufreq_cooling_register(). Fix it.
> 
> Reported-by: Lukasz Majewski <l.majewski@samsung.com>
> Reported-by: Eduardo Valentin <edubezval@gmail.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> ---
> For 3.18.
> ---
>  drivers/cpufreq/cpufreq-dt.c | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq-dt.c
> b/drivers/cpufreq/cpufreq-dt.c index 8cba13d..22eb6e5 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -186,7 +186,6 @@ static int cpufreq_init(struct cpufreq_policy
> *policy) {
>  	struct cpufreq_dt_platform_data *pd;
>  	struct cpufreq_frequency_table *freq_table;
> -	struct thermal_cooling_device *cdev;
>  	struct device_node *np;
>  	struct private_data *priv;
>  	struct device *cpu_dev;
> @@ -269,20 +268,6 @@ static int cpufreq_init(struct cpufreq_policy
> *policy) goto out_free_priv;
>  	}
>  
> -	/*
> -	 * For now, just loading the cooling device;
> -	 * thermal DT code takes care of matching them.
> -	 */
> -	if (of_find_property(np, "#cooling-cells", NULL)) {
> -		cdev = of_cpufreq_cooling_register(np,
> cpu_present_mask);
> -		if (IS_ERR(cdev))
> -			dev_err(cpu_dev,
> -				"running cpufreq without cooling
> device: %ld\n",
> -				PTR_ERR(cdev));
> -		else
> -			priv->cdev = cdev;
> -	}
> -
>  	priv->cpu_dev = cpu_dev;
>  	priv->cpu_reg = cpu_reg;
>  	policy->driver_data = priv;
> @@ -292,7 +277,22 @@ static int cpufreq_init(struct cpufreq_policy
> *policy) if (ret) {
>  		dev_err(cpu_dev, "%s: invalid frequency table:
> %d\n", __func__, ret);
> -		goto out_cooling_unregister;
> +		goto out_free_cpufreq_table;
> +	}
> +
> +	/*
> +	 * For now, just loading the cooling device;
> +	 * thermal DT code takes care of matching them.
> +	 */
> +	if (of_find_property(np, "#cooling-cells", NULL)) {
> +		priv->cdev = of_cpufreq_cooling_register(np,
> cpu_present_mask);
> +		if (IS_ERR(priv->cdev)) {
> +			dev_err(cpu_dev,
> +				"running cpufreq without cooling
> device: %ld\n",
> +				PTR_ERR(priv->cdev));
> +
> +			priv->cdev = NULL;
> +		}
>  	}
>  
>  	policy->cpuinfo.transition_latency = transition_latency;
> @@ -305,8 +305,7 @@ static int cpufreq_init(struct cpufreq_policy
> *policy) 
>  	return 0;
>  
> -out_cooling_unregister:
> -	cpufreq_cooling_unregister(priv->cdev);
> +out_free_cpufreq_table:
>  	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
>  out_free_priv:
>  	kfree(priv);

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Rafael J. Wysocki]

On Monday, November 24, 2014 04:29:13 PM Viresh Kumar wrote:
> of_cpufreq_cooling_register() can use frequency values from
> policy->min/max/cpuinfo.min_freq/cpuinfo.max_freq, which are available only
> after calling cpufreq_table_validate_and_show().
> 
> The right order of calling should be: cpufreq_table_validate_and_show() followed
> by of_cpufreq_cooling_register(). Fix it.
> 
> Reported-by: Lukasz Majewski <l.majewski@samsung.com>
> Reported-by: Eduardo Valentin <edubezval@gmail.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> ---
> For 3.18.

Plus "stable" I suppose?  Which ones?

> ---
>  drivers/cpufreq/cpufreq-dt.c | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index 8cba13d..22eb6e5 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -186,7 +186,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>  {
>  	struct cpufreq_dt_platform_data *pd;
>  	struct cpufreq_frequency_table *freq_table;
> -	struct thermal_cooling_device *cdev;
>  	struct device_node *np;
>  	struct private_data *priv;
>  	struct device *cpu_dev;
> @@ -269,20 +268,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>  		goto out_free_priv;
>  	}
>  
> -	/*
> -	 * For now, just loading the cooling device;
> -	 * thermal DT code takes care of matching them.
> -	 */
> -	if (of_find_property(np, "#cooling-cells", NULL)) {
> -		cdev = of_cpufreq_cooling_register(np, cpu_present_mask);
> -		if (IS_ERR(cdev))
> -			dev_err(cpu_dev,
> -				"running cpufreq without cooling device: %ld\n",
> -				PTR_ERR(cdev));
> -		else
> -			priv->cdev = cdev;
> -	}
> -
>  	priv->cpu_dev = cpu_dev;
>  	priv->cpu_reg = cpu_reg;
>  	policy->driver_data = priv;
> @@ -292,7 +277,22 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>  	if (ret) {
>  		dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__,
>  			ret);
> -		goto out_cooling_unregister;
> +		goto out_free_cpufreq_table;
> +	}
> +
> +	/*
> +	 * For now, just loading the cooling device;
> +	 * thermal DT code takes care of matching them.
> +	 */
> +	if (of_find_property(np, "#cooling-cells", NULL)) {
> +		priv->cdev = of_cpufreq_cooling_register(np, cpu_present_mask);
> +		if (IS_ERR(priv->cdev)) {
> +			dev_err(cpu_dev,
> +				"running cpufreq without cooling device: %ld\n",
> +				PTR_ERR(priv->cdev));
> +
> +			priv->cdev = NULL;
> +		}
>  	}
>  
>  	policy->cpuinfo.transition_latency = transition_latency;
> @@ -305,8 +305,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>  
>  	return 0;
>  
> -out_cooling_unregister:
> -	cpufreq_cooling_unregister(priv->cdev);
> +out_free_cpufreq_table:
>  	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
>  out_free_priv:
>  	kfree(priv);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Rafael J. Wysocki]

On Tuesday, November 25, 2014 10:49:32 PM Rafael J. Wysocki wrote:
> On Monday, November 24, 2014 04:29:13 PM Viresh Kumar wrote:
> > of_cpufreq_cooling_register() can use frequency values from
> > policy->min/max/cpuinfo.min_freq/cpuinfo.max_freq, which are available only
> > after calling cpufreq_table_validate_and_show().
> > 
> > The right order of calling should be: cpufreq_table_validate_and_show() followed
> > by of_cpufreq_cooling_register(). Fix it.
> > 
> > Reported-by: Lukasz Majewski <l.majewski@samsung.com>
> > Reported-by: Eduardo Valentin <edubezval@gmail.com>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > 
> > ---
> > For 3.18.
> 
> Plus "stable" I suppose?  Which ones?

And what bad things are going to happen if this is not pushed for 3.18?

> > ---
> >  drivers/cpufreq/cpufreq-dt.c | 35 +++++++++++++++++------------------
> >  1 file changed, 17 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> > index 8cba13d..22eb6e5 100644
> > --- a/drivers/cpufreq/cpufreq-dt.c
> > +++ b/drivers/cpufreq/cpufreq-dt.c
> > @@ -186,7 +186,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> >  {
> >  	struct cpufreq_dt_platform_data *pd;
> >  	struct cpufreq_frequency_table *freq_table;
> > -	struct thermal_cooling_device *cdev;
> >  	struct device_node *np;
> >  	struct private_data *priv;
> >  	struct device *cpu_dev;
> > @@ -269,20 +268,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> >  		goto out_free_priv;
> >  	}
> >  
> > -	/*
> > -	 * For now, just loading the cooling device;
> > -	 * thermal DT code takes care of matching them.
> > -	 */
> > -	if (of_find_property(np, "#cooling-cells", NULL)) {
> > -		cdev = of_cpufreq_cooling_register(np, cpu_present_mask);
> > -		if (IS_ERR(cdev))
> > -			dev_err(cpu_dev,
> > -				"running cpufreq without cooling device: %ld\n",
> > -				PTR_ERR(cdev));
> > -		else
> > -			priv->cdev = cdev;
> > -	}
> > -
> >  	priv->cpu_dev = cpu_dev;
> >  	priv->cpu_reg = cpu_reg;
> >  	policy->driver_data = priv;
> > @@ -292,7 +277,22 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> >  	if (ret) {
> >  		dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__,
> >  			ret);
> > -		goto out_cooling_unregister;
> > +		goto out_free_cpufreq_table;
> > +	}
> > +
> > +	/*
> > +	 * For now, just loading the cooling device;
> > +	 * thermal DT code takes care of matching them.
> > +	 */
> > +	if (of_find_property(np, "#cooling-cells", NULL)) {
> > +		priv->cdev = of_cpufreq_cooling_register(np, cpu_present_mask);
> > +		if (IS_ERR(priv->cdev)) {
> > +			dev_err(cpu_dev,
> > +				"running cpufreq without cooling device: %ld\n",
> > +				PTR_ERR(priv->cdev));
> > +
> > +			priv->cdev = NULL;
> > +		}
> >  	}
> >  
> >  	policy->cpuinfo.transition_latency = transition_latency;
> > @@ -305,8 +305,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> >  
> >  	return 0;
> >  
> > -out_cooling_unregister:
> > -	cpufreq_cooling_unregister(priv->cdev);
> > +out_free_cpufreq_table:
> >  	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
> >  out_free_priv:
> >  	kfree(priv);
> > 
> 
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Viresh Kumar]

On 26 November 2014 at 03:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> And what bad things are going to happen if this is not pushed for 3.18?

This is what Eduardo reported in one of the mails:

---

As an example, I am taking the ti-soc-thermal, but we already have other
of-thermal based drivers. Booting with this patch ti-soc-thermal
(of-based boot) loads fine, but the cpu_cooling never gets bound to the
thermal zone.

The thing is that the bind may happen before cpufreq-dt code loads the
cpufreq driver, and when cpu_cooling is checking what is the max freq,
by using cpufreq table, it won't be able to do it, as there is no table.

While, without the patch, it will use wrong in the binding, but after
it gets bound, and cpufreq loads, the max will be used correctly.

----

And so it looked like things aren't going to work smoothly in 3.18
and so I thought we should get it in.

But probably the problem will be worst only after applying Lukasz
patchset?

@Eduardo: Do you want Rafael to apply this for 3.18? or 3.19 will
work as well ?

--
viresh

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Viresh Kumar]

On 26 November 2014 at 11:27, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 26 November 2014 at 03:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> And what bad things are going to happen if this is not pushed for 3.18?
>
> This is what Eduardo reported in one of the mails:

https://lkml.org/lkml/2014/11/21/451

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 1945 bytes --]


Hello Viresh, Rafael,

On Wed, Nov 26, 2014 at 11:27:42AM +0530, Viresh Kumar wrote:
> On 26 November 2014 at 03:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > And what bad things are going to happen if this is not pushed for 3.18?
> 
> This is what Eduardo reported in one of the mails:
> 
> ---
> 
> As an example, I am taking the ti-soc-thermal, but we already have other
> of-thermal based drivers. Booting with this patch ti-soc-thermal
> (of-based boot) loads fine, but the cpu_cooling never gets bound to the
> thermal zone.
> 
> The thing is that the bind may happen before cpufreq-dt code loads the
> cpufreq driver, and when cpu_cooling is checking what is the max freq,
> by using cpufreq table, it won't be able to do it, as there is no table.
> 
> While, without the patch, it will use wrong in the binding, but after
> it gets bound, and cpufreq loads, the max will be used correctly.
> 
> ----
> 
> And so it looked like things aren't going to work smoothly in 3.18
> and so I thought we should get it in.
> 

The bug exists, it is there for 3.18. But also, as I explained in that
thread, the current code is able to initialize the of cooling device and
to register it in its thermal zone. But that works by lucky because we
do not have the proper return code checks in thermal core.

> But probably the problem will be worst only after applying Lukasz
> patchset?

The bug gets exposed by his patch.

> 
> @Eduardo: Do you want Rafael to apply this for 3.18? or 3.19 will
> work as well ?


I believe, for this case, 3.19 is best option. Looks like we have a
possible API massage coming, so, targetting 3.18 would be rushy. I'd
rather think of a proper way to fix this, scalable and that makes
sense to everybody, even if takes extra merge window, than rushing for a
fix that we may need to revisit it again in near future.


BR,

Eduardo Valentin
> 
> --
> viresh

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Rafael J. Wysocki]

On Wednesday, November 26, 2014 11:10:32 AM Eduardo Valentin wrote:
> 
> --T4sUOijqQbZv57TR
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
> 
> 
> Hello Viresh, Rafael,
> 
> On Wed, Nov 26, 2014 at 11:27:42AM +0530, Viresh Kumar wrote:
> > On 26 November 2014 at 03:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > And what bad things are going to happen if this is not pushed for 3.18?
> >=20
> > This is what Eduardo reported in one of the mails:
> >=20
> > ---
> >=20
> > As an example, I am taking the ti-soc-thermal, but we already have other
> > of-thermal based drivers. Booting with this patch ti-soc-thermal
> > (of-based boot) loads fine, but the cpu_cooling never gets bound to the
> > thermal zone.
> >=20
> > The thing is that the bind may happen before cpufreq-dt code loads the
> > cpufreq driver, and when cpu_cooling is checking what is the max freq,
> > by using cpufreq table, it won't be able to do it, as there is no table.
> >=20
> > While, without the patch, it will use wrong in the binding, but after
> > it gets bound, and cpufreq loads, the max will be used correctly.
> >=20
> > ----
> >=20
> > And so it looked like things aren't going to work smoothly in 3.18
> > and so I thought we should get it in.
> >=20
> 
> The bug exists, it is there for 3.18. But also, as I explained in that
> thread, the current code is able to initialize the of cooling device and
> to register it in its thermal zone. But that works by lucky because we
> do not have the proper return code checks in thermal core.
> 
> > But probably the problem will be worst only after applying Lukasz
> > patchset?
> 
> The bug gets exposed by his patch.
> 
> >=20
> > @Eduardo: Do you want Rafael to apply this for 3.18? or 3.19 will
> > work as well ?
> 
> 
> I believe, for this case, 3.19 is best option. Looks like we have a
> possible API massage coming, so, targetting 3.18 would be rushy. I'd
> rather think of a proper way to fix this, scalable and that makes
> sense to everybody, even if takes extra merge window, than rushing for a
> fix that we may need to revisit it again in near future.

That's my view as well.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] cpufreq-dt: register cooling device after validating cpufreq table

[Viresh Kumar]

On 26 November 2014 at 03:19, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Plus "stable" I suppose?  Which ones?

I haven't put stable tags intentionally as the driver was updated/renamed in
3.18 and whatever patch we produce wouldn't get applied to any stable.

So, if we want to fix this problem in 'stable' then we have to write another
similar patch..