The function cppc_freq_invariance_init() may failed to create
kworker_fie, make it more robust by checking the return value to prevent
an invalid pointer dereference in kthread_destroy_worker(), which called
from cppc_freq_invariance_exit().

Signed-off-by: Liao Chang <liaochang1@huawei.com>
---
 drivers/cpufreq/cppc_cpufreq.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 022e3555407c..4432398c8592 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -220,7 +220,7 @@ static void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
 	}
 }
 
-static void __init cppc_freq_invariance_init(void)
+static int __init cppc_freq_invariance_init(void)
 {
 	struct sched_attr attr = {
 		.size		= sizeof(struct sched_attr),
@@ -246,19 +246,23 @@ static void __init cppc_freq_invariance_init(void)
 	}
 
 	if (fie_disabled)
-		return;
+		return 0;
 
 	kworker_fie = kthread_create_worker(0, "cppc_fie");
-	if (IS_ERR(kworker_fie))
-		return;
+	if (IS_ERR(kworker_fie)) {
+		ret = PTR_ERR(kworker_fie);
+		kworker_fie = NULL;
+		return ret;
+	}
 
 	ret = sched_setattr_nocheck(kworker_fie->task, &attr);
 	if (ret) {
 		pr_warn("%s: failed to set SCHED_DEADLINE: %d\n", __func__,
 			ret);
 		kthread_destroy_worker(kworker_fie);
-		return;
+		kworker_fie = NULL;
 	}
+	return ret;
 }
 
 static void cppc_freq_invariance_exit(void)
@@ -279,8 +283,9 @@ static inline void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
 {
 }
 
-static inline void cppc_freq_invariance_init(void)
+static inline int cppc_freq_invariance_init(void)
 {
+	return 0;
 }
 
 static inline void cppc_freq_invariance_exit(void)
@@ -969,7 +974,9 @@ static int __init cppc_cpufreq_init(void)
 		return -ENODEV;
 
 	cppc_check_hisi_workaround();
-	cppc_freq_invariance_init();
+	ret = cppc_freq_invariance_init();
+	if (ret < 0)
+		return ret;
 	populate_efficiency_class();
 
 	ret = cpufreq_register_driver(&cppc_cpufreq_driver);
-- 
2.34.1

On 16-08-23, 03:05, Liao Chang wrote:
> The function cppc_freq_invariance_init() may failed to create
> kworker_fie, make it more robust by checking the return value to prevent
> an invalid pointer dereference in kthread_destroy_worker(), which called
> from cppc_freq_invariance_exit().
> 
> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)

I think why it was designed this way was to make the driver work,
without invariance support, in the worst case instead of just failing
completely. The invariance thing is a good to have feature, but not
really necessary and so failing probing the driver for that isn't
worth it. We should print all error messages though.

-- 
viresh

Hi Viresh,

在 2023/8/16 11:46, Viresh Kumar 写道:
> On 16-08-23, 03:05, Liao Chang wrote:
>> The function cppc_freq_invariance_init() may failed to create
>> kworker_fie, make it more robust by checking the return value to prevent
>> an invalid pointer dereference in kthread_destroy_worker(), which called
>> from cppc_freq_invariance_exit().
>>
>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
>> ---
>>  drivers/cpufreq/cppc_cpufreq.c | 21 ++++++++++++++-------
>>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> I think why it was designed this way was to make the driver work,
> without invariance support, in the worst case instead of just failing
> completely. The invariance thing is a good to have feature, but not
> really necessary and so failing probing the driver for that isn't
> worth it. We should print all error messages though.
> 
Thanks for pointing that out. I think you are right that the kworker created
in the cppc driver is not the only arch_freq_scale updater, the ARCH provided
updater has more priority than the driver, so the driver should still work even
without kworker_fie supports.

If that is the case, i think the best thing to do is checking the error pointer
and printing an error message before calling kthread_destroy() in cppc_freq_invariance_exit(),
this is because at that point, it is really necessary to ensure the kworker_fie has
been initialized as expected, otherwise it will raise a NULL pointer exception.

I hope this makes sense, thanks.

-- 
BR
Liao, Chang

On 16-08-23, 15:27, Liao, Chang wrote:
> Hi Viresh,
> 
> 在 2023/8/16 11:46, Viresh Kumar 写道:
> > On 16-08-23, 03:05, Liao Chang wrote:
> >> The function cppc_freq_invariance_init() may failed to create
> >> kworker_fie, make it more robust by checking the return value to prevent
> >> an invalid pointer dereference in kthread_destroy_worker(), which called
> >> from cppc_freq_invariance_exit().
> >>
> >> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> >> ---
> >>  drivers/cpufreq/cppc_cpufreq.c | 21 ++++++++++++++-------
> >>  1 file changed, 14 insertions(+), 7 deletions(-)
> > 
> > I think why it was designed this way was to make the driver work,
> > without invariance support, in the worst case instead of just failing
> > completely. The invariance thing is a good to have feature, but not
> > really necessary and so failing probing the driver for that isn't
> > worth it. We should print all error messages though.
> > 
> Thanks for pointing that out. I think you are right that the kworker created
> in the cppc driver is not the only arch_freq_scale updater, the ARCH provided
> updater has more priority than the driver, so the driver should still work even
> without kworker_fie supports.
> 
> If that is the case, i think the best thing to do is checking the error pointer
> and printing an error message before calling kthread_destroy() in cppc_freq_invariance_exit(),
> this is because at that point, it is really necessary to ensure the kworker_fie has
> been initialized as expected, otherwise it will raise a NULL pointer exception.

Or just set fie_disabled to true ?

> I hope this makes sense, thanks.

It does.

-- 
viresh

在 2023/8/16 16:17, Viresh Kumar 写道:
> On 16-08-23, 15:27, Liao, Chang wrote:
>> Hi Viresh,
>>
>> 在 2023/8/16 11:46, Viresh Kumar 写道:
>>> On 16-08-23, 03:05, Liao Chang wrote:
>>>> The function cppc_freq_invariance_init() may failed to create
>>>> kworker_fie, make it more robust by checking the return value to prevent
>>>> an invalid pointer dereference in kthread_destroy_worker(), which called
>>>> from cppc_freq_invariance_exit().
>>>>
>>>> Signed-off-by: Liao Chang <liaochang1@huawei.com>
>>>> ---
>>>>  drivers/cpufreq/cppc_cpufreq.c | 21 ++++++++++++++-------
>>>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>>
>>> I think why it was designed this way was to make the driver work,
>>> without invariance support, in the worst case instead of just failing
>>> completely. The invariance thing is a good to have feature, but not
>>> really necessary and so failing probing the driver for that isn't
>>> worth it. We should print all error messages though.
>>>
>> Thanks for pointing that out. I think you are right that the kworker created
>> in the cppc driver is not the only arch_freq_scale updater, the ARCH provided
>> updater has more priority than the driver, so the driver should still work even
>> without kworker_fie supports.
>>
>> If that is the case, i think the best thing to do is checking the error pointer
>> and printing an error message before calling kthread_destroy() in cppc_freq_invariance_exit(),
>> this is because at that point, it is really necessary to ensure the kworker_fie has
>> been initialized as expected, otherwise it will raise a NULL pointer exception.
> 
> Or just set fie_disabled to true ?
Yes, I agree.

> 
>> I hope this makes sense, thanks.
> 
> It does.
> 

-- 
BR
Liao, Chang