Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5848a10e-e5bf-108f-3e3e-15e16332922d@gmail.com>
Date:   Wed, 15 Jun 2022 16:11:13 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v4 2/4] PM / devfreq: Fix kernel warning with cpufreq
 passive register fail
Content-Language: en-US
To:     Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>,
        MyungJoo Ham <myungjoo.ham@samsung.com>,
        Kyungmin Park <kyungmin.park@samsung.com>,
        Chanwoo Choi <cw00.choi@samsung.com>,
        Sibi Sankar <sibis@codeaurora.org>,
        Saravana Kannan <skannan@codeaurora.org>,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220614230950.426-1-ansuelsmth@gmail.com>
 <20220614230950.426-3-ansuelsmth@gmail.com>
From:   Chanwoo Choi <cwchoi00@gmail.com>
In-Reply-To: <20220614230950.426-3-ansuelsmth@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 22. 6. 15. 08:09, Christian 'Ansuel' Marangi wrote:
> When the cpufreq passive register path from the passive governor fails,
> the cpufreq_passive_unregister is called and a kernel WARNING is always
> reported.
> This is caused by the fact that the devfreq driver already call the
> governor unregister with the GOV_STOP, for this reason the second
> cpufreq_passive_unregister always return error and a WARN is printed
> from the WARN_ON function.
> Remove the unregister call from the error handling of the cpufreq register
> notifier as it's fundamentally wrong and already handled by the devfreq
> core code.
> 
> Fixes: a03dacb0316f ("PM / devfreq: Add cpu based scaling support to passive governor")
> Signed-off-by: Christian 'Ansuel' Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/devfreq/governor_passive.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 95de336f20d5..dcc9dd518197 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -331,7 +331,6 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
>  err_put_policy:
>  	cpufreq_cpu_put(policy);
>  err:
> -	WARN_ON(cpufreq_passive_unregister_notifier(devfreq));
>  
>  	return ret;
>  }

I think that it is necessary to free the resource when error happen.
Also, after merging the your patch1, I think that cpufreq_passive_unregister_notifier(devfreq)
will not return error. Instead, just 0 for success.

Instead, 'err_free_cpu_data' and 'err_put_policy' goto statement are wrong exception
handling. If fix the exception handling code in cpufreq_passive_register_notifier
as following and with your patch1, I'll handle the resource for free/un-registration
when error happen during cpufreq_passive_register_notifier.


diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index a35b39ac656c..0246e0731fc0 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -289,22 +289,23 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
                parent_cpu_data = kzalloc(sizeof(*parent_cpu_data),
                                                GFP_KERNEL);
                if (!parent_cpu_data) {
+                       cpufreq_cpu_put(policy);
                        ret = -ENOMEM;
-                       goto err_put_policy;
+                       goto err;
                }
 
                cpu_dev = get_cpu_device(cpu);
                if (!cpu_dev) {
                        dev_err(dev, "failed to get cpu device\n");
                        ret = -ENODEV;
-                       goto err_free_cpu_data;
+                       goto err;
                }
 
                opp_table = dev_pm_opp_get_opp_table(cpu_dev);
                if (IS_ERR(opp_table)) {
                        dev_err(dev, "failed to get opp_table of cpu%d\n", cpu);
                        ret = PTR_ERR(opp_table);
-                       goto err_free_cpu_data;
+                       goto err;
                }
 
                parent_cpu_data->dev = cpu_dev;
@@ -326,10 +327,6 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
 
        return ret;
 
-err_free_cpu_data:
-       kfree(parent_cpu_data);
-err_put_policy:
-       cpufreq_cpu_put(policy);
 err:
        WARN_ON(cpufreq_passive_unregister_notifier(devfreq));
 

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi