| Message ID | 20241104090351.1352997-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | PM: EM: Fix uninitialized power in em_create_perf_table | expand |
Hi Jinjie, On 11/4/24 09:03, Jinjie Ruan wrote: > In em_create_perf_table(), power is uninitialized and passed the pointer > to active_power() hook, but the hook function may not assign it and > return 0, such as mtk_cpufreq_get_cpu_power(), so the later zero check for Please fix the driver. I have checked that function. It must return -EINVAL when the 'policy' is not found. We cannot progress with power=0. > power is not invalid, initialize power to zero to fix it. > > Cc: stable@vger.kernel.org > Fixes: 7d9895c7fbfc ("PM / EM: introduce em_dev_register_perf_domain function") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > --- > kernel/power/energy_model.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c > index 927cc55ba0b3..866a3e9c05b2 100644 > --- a/kernel/power/energy_model.c > +++ b/kernel/power/energy_model.c > @@ -344,7 +344,7 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, > struct em_data_callback *cb, > unsigned long flags) > { > - unsigned long power, freq, prev_freq = 0; > + unsigned long power = 0, freq, prev_freq = 0; > int nr_states = pd->nr_perf_states; > int i, ret; > This patch proposal is just a workaround. When you send a patch to that MTK driver, I can review it for you so please add me on CC. Regards, Lukasz
Hi, On Monday 04 Nov 2024 at 17:03:51 (+0800), Jinjie Ruan wrote: > In em_create_perf_table(), power is uninitialized and passed the pointer > to active_power() hook, but the hook function may not assign it and > return 0, such as mtk_cpufreq_get_cpu_power(), so the later zero check for > power is not invalid, initialize power to zero to fix it. Sounds to me like a driver bug? We check the return value of active_power() first, so if the callback failed it should tell us. mtk_cpufreq_get_cpu_power() should only fail if we couldn't find a cpufreq policy IIUC, so why can't this return -EINVAL instead? Thanks, Quentin
On Monday 04 Nov 2024 at 09:14:36 (+0000), Lukasz Luba wrote: > Hi Jinjie, > > On 11/4/24 09:03, Jinjie Ruan wrote: > > In em_create_perf_table(), power is uninitialized and passed the pointer > > to active_power() hook, but the hook function may not assign it and > > return 0, such as mtk_cpufreq_get_cpu_power(), so the later zero check for > > Please fix the driver. I have checked that function. It must return > -EINVAL when the 'policy' is not found. We cannot progress with power=0. > > > > power is not invalid, initialize power to zero to fix it. > > > > Cc: stable@vger.kernel.org > > Fixes: 7d9895c7fbfc ("PM / EM: introduce em_dev_register_perf_domain function") > > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > > --- > > kernel/power/energy_model.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c > > index 927cc55ba0b3..866a3e9c05b2 100644 > > --- a/kernel/power/energy_model.c > > +++ b/kernel/power/energy_model.c > > @@ -344,7 +344,7 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, > > struct em_data_callback *cb, > > unsigned long flags) > > { > > - unsigned long power, freq, prev_freq = 0; > > + unsigned long power = 0, freq, prev_freq = 0; > > int nr_states = pd->nr_perf_states; > > int i, ret; > > > This patch proposal is just a workaround. > > When you send a patch to that MTK driver, I can review it for you so > please add me on CC. We raced, but +1 to the above :-)
On 2024/11/4 17:14, Lukasz Luba wrote: > Hi Jinjie, > > On 11/4/24 09:03, Jinjie Ruan wrote: >> In em_create_perf_table(), power is uninitialized and passed the pointer >> to active_power() hook, but the hook function may not assign it and >> return 0, such as mtk_cpufreq_get_cpu_power(), so the later zero check >> for > > Please fix the driver. I have checked that function. It must return > -EINVAL when the 'policy' is not found. We cannot progress with power=0. Thank you very much! It will be also good to fix the driver. > > >> power is not invalid, initialize power to zero to fix it. >> >> Cc: stable@vger.kernel.org >> Fixes: 7d9895c7fbfc ("PM / EM: introduce em_dev_register_perf_domain >> function") >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >> --- >> kernel/power/energy_model.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c >> index 927cc55ba0b3..866a3e9c05b2 100644 >> --- a/kernel/power/energy_model.c >> +++ b/kernel/power/energy_model.c >> @@ -344,7 +344,7 @@ static int em_create_perf_table(struct device >> *dev, struct em_perf_domain *pd, >> struct em_data_callback *cb, >> unsigned long flags) >> { >> - unsigned long power, freq, prev_freq = 0; >> + unsigned long power = 0, freq, prev_freq = 0; >> int nr_states = pd->nr_perf_states; >> int i, ret; >> > > > This patch proposal is just a workaround. > > When you send a patch to that MTK driver, I can review it for you so > please add me on CC. > > Regards, > Lukasz >
diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c index 927cc55ba0b3..866a3e9c05b2 100644 --- a/kernel/power/energy_model.c +++ b/kernel/power/energy_model.c @@ -344,7 +344,7 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, struct em_data_callback *cb, unsigned long flags) { - unsigned long power, freq, prev_freq = 0; + unsigned long power = 0, freq, prev_freq = 0; int nr_states = pd->nr_perf_states; int i, ret;
In em_create_perf_table(), power is uninitialized and passed the pointer to active_power() hook, but the hook function may not assign it and return 0, such as mtk_cpufreq_get_cpu_power(), so the later zero check for power is not invalid, initialize power to zero to fix it. Cc: stable@vger.kernel.org Fixes: 7d9895c7fbfc ("PM / EM: introduce em_dev_register_perf_domain function") Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- kernel/power/energy_model.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)