* [PATCH] thermal: cpu_cooling: Fix NULL dereference in cpufreq_state2power
@ 2016-08-17 15:14 Brendan Jackman
2016-08-17 23:05 ` Viresh Kumar
0 siblings, 1 reply; 4+ messages in thread
From: Brendan Jackman @ 2016-08-17 15:14 UTC (permalink / raw)
To: linux-pm; +Cc: Amit Daniel Kachhap, Viresh Kumar, Javi Merino
Currently all CPU cooling devices share a
`struct thermal_cooling_device_ops` instance. The thermal core uses the
presence of functions in this struct to determine if a cooling device
has a power model (see cdev_is_power_actor). cpu_cooling.c adds the
power model functions to the shared struct when a device is registered
with a power model.
Therefore, if a CPU cooling device is registered using
[of_]cpufreq_power_cooling_register, _all_ devices will be determined to
have a power model, including any registered with
[of_]cpufreq_cooling_register. This can result in cpufreq_state2power
being called on a device where dyn_power_table is NULL.
With this commit, instead of having a shared thermal_cooling_device_ops
which is mutated, we have two versions: one with the power functions and
one without.
Signed-off-by: Brendan Jackman <brendan.jackman@arm.com>
Cc: Amit Daniel Kachhap <amit.kachhap@gmail.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Javi Merino <javi.merino@arm.com>
---
drivers/thermal/cpu_cooling.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 3788ed7..a32b417 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -740,12 +740,22 @@ static int cpufreq_power2state(struct thermal_cooling_device *cdev,
}
/* Bind cpufreq callbacks to thermal cooling device ops */
+
static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
.get_max_state = cpufreq_get_max_state,
.get_cur_state = cpufreq_get_cur_state,
.set_cur_state = cpufreq_set_cur_state,
};
+static struct thermal_cooling_device_ops cpufreq_power_cooling_ops = {
+ .get_max_state = cpufreq_get_max_state,
+ .get_cur_state = cpufreq_get_cur_state,
+ .set_cur_state = cpufreq_set_cur_state,
+ .get_requested_power = cpufreq_get_requested_power,
+ .state2power = cpufreq_state2power,
+ .power2state = cpufreq_power2state,
+};
+
/* Notifier for cpufreq policy change */
static struct notifier_block thermal_cpufreq_notifier_block = {
.notifier_call = cpufreq_thermal_notifier,
@@ -795,6 +805,7 @@ __cpufreq_cooling_register(struct device_node *np,
struct cpumask temp_mask;
unsigned int freq, i, num_cpus;
int ret;
+ struct thermal_cooling_device_ops *cooling_ops;
cpumask_and(&temp_mask, clip_cpus, cpu_online_mask);
policy = cpufreq_cpu_get(cpumask_first(&temp_mask));
@@ -850,10 +861,6 @@ __cpufreq_cooling_register(struct device_node *np,
cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);
if (capacitance) {
- cpufreq_cooling_ops.get_requested_power =
- cpufreq_get_requested_power;
- cpufreq_cooling_ops.state2power = cpufreq_state2power;
- cpufreq_cooling_ops.power2state = cpufreq_power2state;
cpufreq_dev->plat_get_static_power = plat_static_func;
ret = build_dyn_power_table(cpufreq_dev, capacitance);
@@ -861,6 +868,10 @@ __cpufreq_cooling_register(struct device_node *np,
cool_dev = ERR_PTR(ret);
goto free_table;
}
+
+ cooling_ops = &cpufreq_power_cooling_ops;
+ } else {
+ cooling_ops = &cpufreq_cooling_ops;
}
ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
@@ -885,7 +896,7 @@ __cpufreq_cooling_register(struct device_node *np,
cpufreq_dev->id);
cool_dev = thermal_of_cooling_device_register(np, dev_name, cpufreq_dev,
- &cpufreq_cooling_ops);
+ cooling_ops);
if (IS_ERR(cool_dev))
goto remove_idr;
--
2.9.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] thermal: cpu_cooling: Fix NULL dereference in cpufreq_state2power
2016-08-17 15:14 [PATCH] thermal: cpu_cooling: Fix NULL dereference in cpufreq_state2power Brendan Jackman
@ 2016-08-17 23:05 ` Viresh Kumar
2016-08-18 9:30 ` Javi Merino
0 siblings, 1 reply; 4+ messages in thread
From: Viresh Kumar @ 2016-08-17 23:05 UTC (permalink / raw)
To: Brendan Jackman; +Cc: linux-pm, Amit Daniel Kachhap, Javi Merino
On 17-08-16, 16:14, Brendan Jackman wrote:
> Currently all CPU cooling devices share a
> `struct thermal_cooling_device_ops` instance. The thermal core uses the
> presence of functions in this struct to determine if a cooling device
> has a power model (see cdev_is_power_actor). cpu_cooling.c adds the
> power model functions to the shared struct when a device is registered
> with a power model.
>
> Therefore, if a CPU cooling device is registered using
> [of_]cpufreq_power_cooling_register, _all_ devices will be determined to
> have a power model, including any registered with
> [of_]cpufreq_cooling_register. This can result in cpufreq_state2power
> being called on a device where dyn_power_table is NULL.
>
> With this commit, instead of having a shared thermal_cooling_device_ops
> which is mutated, we have two versions: one with the power functions and
> one without.
>
> Signed-off-by: Brendan Jackman <brendan.jackman@arm.com>
> Cc: Amit Daniel Kachhap <amit.kachhap@gmail.com>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Javi Merino <javi.merino@arm.com>
>
> ---
> drivers/thermal/cpu_cooling.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 3788ed7..a32b417 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -740,12 +740,22 @@ static int cpufreq_power2state(struct thermal_cooling_device *cdev,
> }
>
> /* Bind cpufreq callbacks to thermal cooling device ops */
> +
> static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
> .get_max_state = cpufreq_get_max_state,
> .get_cur_state = cpufreq_get_cur_state,
> .set_cur_state = cpufreq_set_cur_state,
> };
>
> +static struct thermal_cooling_device_ops cpufreq_power_cooling_ops = {
> + .get_max_state = cpufreq_get_max_state,
> + .get_cur_state = cpufreq_get_cur_state,
> + .set_cur_state = cpufreq_set_cur_state,
> + .get_requested_power = cpufreq_get_requested_power,
> + .state2power = cpufreq_state2power,
> + .power2state = cpufreq_power2state,
> +};
> +
> /* Notifier for cpufreq policy change */
> static struct notifier_block thermal_cpufreq_notifier_block = {
> .notifier_call = cpufreq_thermal_notifier,
> @@ -795,6 +805,7 @@ __cpufreq_cooling_register(struct device_node *np,
> struct cpumask temp_mask;
> unsigned int freq, i, num_cpus;
> int ret;
> + struct thermal_cooling_device_ops *cooling_ops;
>
> cpumask_and(&temp_mask, clip_cpus, cpu_online_mask);
> policy = cpufreq_cpu_get(cpumask_first(&temp_mask));
> @@ -850,10 +861,6 @@ __cpufreq_cooling_register(struct device_node *np,
> cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);
>
> if (capacitance) {
> - cpufreq_cooling_ops.get_requested_power =
> - cpufreq_get_requested_power;
> - cpufreq_cooling_ops.state2power = cpufreq_state2power;
> - cpufreq_cooling_ops.power2state = cpufreq_power2state;
> cpufreq_dev->plat_get_static_power = plat_static_func;
>
> ret = build_dyn_power_table(cpufreq_dev, capacitance);
> @@ -861,6 +868,10 @@ __cpufreq_cooling_register(struct device_node *np,
> cool_dev = ERR_PTR(ret);
> goto free_table;
> }
> +
> + cooling_ops = &cpufreq_power_cooling_ops;
> + } else {
> + cooling_ops = &cpufreq_cooling_ops;
> }
>
> ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
> @@ -885,7 +896,7 @@ __cpufreq_cooling_register(struct device_node *np,
> cpufreq_dev->id);
>
> cool_dev = thermal_of_cooling_device_register(np, dev_name, cpufreq_dev,
> - &cpufreq_cooling_ops);
> + cooling_ops);
> if (IS_ERR(cool_dev))
> goto remove_idr;
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] thermal: cpu_cooling: Fix NULL dereference in cpufreq_state2power
2016-08-17 23:05 ` Viresh Kumar
@ 2016-08-18 9:30 ` Javi Merino
2016-08-19 13:19 ` Zhang Rui
0 siblings, 1 reply; 4+ messages in thread
From: Javi Merino @ 2016-08-18 9:30 UTC (permalink / raw)
To: Viresh Kumar; +Cc: Brendan Jackman, linux-pm, Amit Daniel Kachhap
On Thu, Aug 18, 2016 at 04:35:07AM +0530, Viresh Kumar wrote:
> On 17-08-16, 16:14, Brendan Jackman wrote:
> > Currently all CPU cooling devices share a
> > `struct thermal_cooling_device_ops` instance. The thermal core uses the
> > presence of functions in this struct to determine if a cooling device
> > has a power model (see cdev_is_power_actor). cpu_cooling.c adds the
> > power model functions to the shared struct when a device is registered
> > with a power model.
> >
> > Therefore, if a CPU cooling device is registered using
> > [of_]cpufreq_power_cooling_register, _all_ devices will be determined to
> > have a power model, including any registered with
> > [of_]cpufreq_cooling_register. This can result in cpufreq_state2power
> > being called on a device where dyn_power_table is NULL.
> >
> > With this commit, instead of having a shared thermal_cooling_device_ops
> > which is mutated, we have two versions: one with the power functions and
> > one without.
> >
> > Signed-off-by: Brendan Jackman <brendan.jackman@arm.com>
> > Cc: Amit Daniel Kachhap <amit.kachhap@gmail.com>
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > Cc: Javi Merino <javi.merino@arm.com>
> >
> > ---
> > drivers/thermal/cpu_cooling.c | 21 ++++++++++++++++-----
> > 1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> > index 3788ed7..a32b417 100644
> > --- a/drivers/thermal/cpu_cooling.c
> > +++ b/drivers/thermal/cpu_cooling.c
> > @@ -740,12 +740,22 @@ static int cpufreq_power2state(struct thermal_cooling_device *cdev,
> > }
> >
> > /* Bind cpufreq callbacks to thermal cooling device ops */
> > +
> > static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
> > .get_max_state = cpufreq_get_max_state,
> > .get_cur_state = cpufreq_get_cur_state,
> > .set_cur_state = cpufreq_set_cur_state,
> > };
> >
> > +static struct thermal_cooling_device_ops cpufreq_power_cooling_ops = {
> > + .get_max_state = cpufreq_get_max_state,
> > + .get_cur_state = cpufreq_get_cur_state,
> > + .set_cur_state = cpufreq_set_cur_state,
> > + .get_requested_power = cpufreq_get_requested_power,
> > + .state2power = cpufreq_state2power,
> > + .power2state = cpufreq_power2state,
> > +};
> > +
> > /* Notifier for cpufreq policy change */
> > static struct notifier_block thermal_cpufreq_notifier_block = {
> > .notifier_call = cpufreq_thermal_notifier,
> > @@ -795,6 +805,7 @@ __cpufreq_cooling_register(struct device_node *np,
> > struct cpumask temp_mask;
> > unsigned int freq, i, num_cpus;
> > int ret;
> > + struct thermal_cooling_device_ops *cooling_ops;
> >
> > cpumask_and(&temp_mask, clip_cpus, cpu_online_mask);
> > policy = cpufreq_cpu_get(cpumask_first(&temp_mask));
> > @@ -850,10 +861,6 @@ __cpufreq_cooling_register(struct device_node *np,
> > cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);
> >
> > if (capacitance) {
> > - cpufreq_cooling_ops.get_requested_power =
> > - cpufreq_get_requested_power;
> > - cpufreq_cooling_ops.state2power = cpufreq_state2power;
> > - cpufreq_cooling_ops.power2state = cpufreq_power2state;
> > cpufreq_dev->plat_get_static_power = plat_static_func;
> >
> > ret = build_dyn_power_table(cpufreq_dev, capacitance);
> > @@ -861,6 +868,10 @@ __cpufreq_cooling_register(struct device_node *np,
> > cool_dev = ERR_PTR(ret);
> > goto free_table;
> > }
> > +
> > + cooling_ops = &cpufreq_power_cooling_ops;
> > + } else {
> > + cooling_ops = &cpufreq_cooling_ops;
> > }
> >
> > ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
> > @@ -885,7 +896,7 @@ __cpufreq_cooling_register(struct device_node *np,
> > cpufreq_dev->id);
> >
> > cool_dev = thermal_of_cooling_device_register(np, dev_name, cpufreq_dev,
> > - &cpufreq_cooling_ops);
> > + cooling_ops);
> > if (IS_ERR(cool_dev))
> > goto remove_idr;
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Yep, looks good to me.
Acked-by: Javi Merino <javi.merino@arm.com>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] thermal: cpu_cooling: Fix NULL dereference in cpufreq_state2power
2016-08-18 9:30 ` Javi Merino
@ 2016-08-19 13:19 ` Zhang Rui
0 siblings, 0 replies; 4+ messages in thread
From: Zhang Rui @ 2016-08-19 13:19 UTC (permalink / raw)
To: Javi Merino, Viresh Kumar; +Cc: Brendan Jackman, linux-pm, Amit Daniel Kachhap
On 四, 2016-08-18 at 10:30 +0100, Javi Merino wrote:
> On Thu, Aug 18, 2016 at 04:35:07AM +0530, Viresh Kumar wrote:
> >
> > On 17-08-16, 16:14, Brendan Jackman wrote:
> > >
> > > Currently all CPU cooling devices share a
> > > `struct thermal_cooling_device_ops` instance. The thermal core
> > > uses the
> > > presence of functions in this struct to determine if a cooling
> > > device
> > > has a power model (see cdev_is_power_actor). cpu_cooling.c adds
> > > the
> > > power model functions to the shared struct when a device is
> > > registered
> > > with a power model.
> > >
> > > Therefore, if a CPU cooling device is registered using
> > > [of_]cpufreq_power_cooling_register, _all_ devices will be
> > > determined to
> > > have a power model, including any registered with
> > > [of_]cpufreq_cooling_register. This can result in
> > > cpufreq_state2power
> > > being called on a device where dyn_power_table is NULL.
> > >
> > > With this commit, instead of having a shared
> > > thermal_cooling_device_ops
> > > which is mutated, we have two versions: one with the power
> > > functions and
> > > one without.
> > >
> > > Signed-off-by: Brendan Jackman <brendan.jackman@arm.com>
> > > Cc: Amit Daniel Kachhap <amit.kachhap@gmail.com>
> > > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > > Cc: Javi Merino <javi.merino@arm.com>
> > >
> > > ---
> > > drivers/thermal/cpu_cooling.c | 21 ++++++++++++++++-----
> > > 1 file changed, 16 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/thermal/cpu_cooling.c
> > > b/drivers/thermal/cpu_cooling.c
> > > index 3788ed7..a32b417 100644
> > > --- a/drivers/thermal/cpu_cooling.c
> > > +++ b/drivers/thermal/cpu_cooling.c
> > > @@ -740,12 +740,22 @@ static int cpufreq_power2state(struct
> > > thermal_cooling_device *cdev,
> > > }
> > >
> > > /* Bind cpufreq callbacks to thermal cooling device ops */
> > > +
> > > static struct thermal_cooling_device_ops cpufreq_cooling_ops = {
> > > .get_max_state = cpufreq_get_max_state,
> > > .get_cur_state = cpufreq_get_cur_state,
> > > .set_cur_state = cpufreq_set_cur_state,
> > > };
> > >
> > > +static struct thermal_cooling_device_ops
> > > cpufreq_power_cooling_ops = {
> > > + .get_max_state = cpufreq_get_max_state,
> > > + .get_cur_state = cpufreq_get_cur_state,
> > > + .set_cur_state = cpufreq_set_cur_state,
> > > + .get_requested_power =
> > > cpufreq_get_requested_power,
> > > + .state2power = cpufreq_state2power,
> > > + .power2state = cpufreq_power2state,
> > > +};
> > > +
> > > /* Notifier for cpufreq policy change */
> > > static struct notifier_block thermal_cpufreq_notifier_block = {
> > > .notifier_call = cpufreq_thermal_notifier,
> > > @@ -795,6 +805,7 @@ __cpufreq_cooling_register(struct device_node
> > > *np,
> > > struct cpumask temp_mask;
> > > unsigned int freq, i, num_cpus;
> > > int ret;
> > > + struct thermal_cooling_device_ops *cooling_ops;
> > >
> > > cpumask_and(&temp_mask, clip_cpus, cpu_online_mask);
> > > policy = cpufreq_cpu_get(cpumask_first(&temp_mask));
> > > @@ -850,10 +861,6 @@ __cpufreq_cooling_register(struct
> > > device_node *np,
> > > cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);
> > >
> > > if (capacitance) {
> > > - cpufreq_cooling_ops.get_requested_power =
> > > - cpufreq_get_requested_power;
> > > - cpufreq_cooling_ops.state2power =
> > > cpufreq_state2power;
> > > - cpufreq_cooling_ops.power2state =
> > > cpufreq_power2state;
> > > cpufreq_dev->plat_get_static_power =
> > > plat_static_func;
> > >
> > > ret = build_dyn_power_table(cpufreq_dev,
> > > capacitance);
> > > @@ -861,6 +868,10 @@ __cpufreq_cooling_register(struct
> > > device_node *np,
> > > cool_dev = ERR_PTR(ret);
> > > goto free_table;
> > > }
> > > +
> > > + cooling_ops = &cpufreq_power_cooling_ops;
> > > + } else {
> > > + cooling_ops = &cpufreq_cooling_ops;
> > > }
> > >
> > > ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
> > > @@ -885,7 +896,7 @@ __cpufreq_cooling_register(struct device_node
> > > *np,
> > > cpufreq_dev->id);
> > >
> > > cool_dev = thermal_of_cooling_device_register(np,
> > > dev_name, cpufreq_dev,
> > > - &cpufreq_c
> > > ooling_ops);
> > > + cooling_op
> > > s);
> > > if (IS_ERR(cool_dev))
> > > goto remove_idr;
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Yep, looks good to me.
>
> Acked-by: Javi Merino <javi.merino@arm.com>
Patch applied.
thanks,
rui
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-08-19 13:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-17 15:14 [PATCH] thermal: cpu_cooling: Fix NULL dereference in cpufreq_state2power Brendan Jackman
2016-08-17 23:05 ` Viresh Kumar
2016-08-18 9:30 ` Javi Merino
2016-08-19 13:19 ` Zhang Rui
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).