linux-kernel - Re: [PATCH v5 2/9] cpufreq: mediatek: Add opp notification support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Thu, 5 May 2022 14:13:45 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Rex-BC Chen <rex-bc.chen@...iatek.com>
Cc:     rafael@...nel.org, robh+dt@...nel.org, krzk+dt@...nel.org,
        matthias.bgg@...il.com, jia-wei.chang@...iatek.com,
        roger.lu@...iatek.com, hsinyi@...gle.com, khilman@...libre.com,
        angelogioacchino.delregno@...labora.com, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org,
        Project_Global_Chrome_Upstream_Group@...iatek.com,
        "Andrew-sh.Cheng" <andrew-sh.cheng@...iatek.com>
Subject: Re: [PATCH v5 2/9] cpufreq: mediatek: Add opp notification support

On 04-05-22, 21:05, Rex-BC Chen wrote:
> From: "Andrew-sh.Cheng" <andrew-sh.cheng@...iatek.com>
> 
> >From this opp notifier, cpufreq should listen to opp notification and do

What happened with the extra ">" here ?

>  static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>  {
>  	struct device *cpu_dev;
> @@ -396,6 +458,17 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>  	info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
>  	dev_pm_opp_put(opp);
>  
> +	info->opp_cpu = cpu;
> +	info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier;
> +	ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb);
> +	if (ret) {
> +		dev_err(cpu_dev, "cpu%d: failed to register opp notifier\n", cpu);
> +		goto out_disable_inter_clock;
> +	}
> +
> +	mutex_init(&info->reg_lock);

You should always initialize a resource before its users. The notifier
callback, which can get called right after
dev_pm_opp_register_notifier() returns, will use this mutex.

> +	info->opp_freq = clk_get_rate(info->cpu_clk);
> +
>  	/*
>  	 * If SRAM regulator is present, software "voltage tracking" is needed
>  	 * for this CPU power domain.
> @@ -451,6 +524,9 @@ static void mtk_cpu_dvfs_info_release(struct mtk_cpu_dvfs_info *info)
>  	}
>  
>  	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
> +
> +	if (!IS_ERR_OR_NULL(info->cpu_dev))

cpu_dev can never be error here.

> +		dev_pm_opp_unregister_notifier(info->cpu_dev, &info->opp_nb);
>  }
>  
>  static int mtk_cpufreq_init(struct cpufreq_policy *policy)

I also asked you last time to stack things in a order so they are
easier for me to apply. Bugfixes, followed by simple cleanups, which
don't make behavioral changes, followed by real patches.

Now you have sent this patch at an early stage, which blocks me from
applying anything after this.

I can see the earlier comments weren't all considered, and it doesn't
look nice.

-- 
viresh