linux-kernel - RE: [PATCH] thermal: imx: fix for dependency on cpu-freq

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>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <DB3PR0402MB3916F122EA873F847A115C91F5DA0@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date:   Wed, 21 Nov 2018 01:58:58 +0000
From:   Anson Huang <anson.huang@....com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
CC:     Zhang Rui <rui.zhang@...el.com>,
        Eduardo Valentin <edubezval@...il.com>,
        Linux PM list <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH] thermal: imx: fix for dependency on cpu-freq

Hi, Viresh

Best Regards!
Anson Huang

> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@...aro.org]
> Sent: 2018年11月20日 18:49
> To: Anson Huang <anson.huang@....com>
> Cc: Zhang Rui <rui.zhang@...el.com>; Eduardo Valentin
> <edubezval@...il.com>; Linux PM list <linux-pm@...r.kernel.org>; Linux
> Kernel Mailing List <linux-kernel@...r.kernel.org>; dl-linux-imx
> <linux-imx@....com>
> Subject: Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
> 
> While I am aligned with the fact that we need to carry this code for backward
> compatibility, there are few things I would suggest to improve.
> 
> On Wed, Oct 24, 2018 at 12:10 PM Anson Huang <anson.huang@....com>
> wrote:
> >  static int imx_thermal_probe(struct platform_device *pdev)  { @@
> > -743,6 +745,7 @@ static int imx_thermal_probe(struct platform_device
> *pdev)
> >         regmap_write(map, data->socdata->sensor_ctrl + REG_SET,
> >                      data->socdata->power_down_mask);
> >
> > +#ifdef CONFIG_CPU_FREQ
> >         data->policy = cpufreq_cpu_get(0);
> >         if (!data->policy) {
> >                 pr_debug("%s: CPUFreq policy not found\n", __func__);
> > @@ -755,6 +758,7 @@ static int imx_thermal_probe(struct platform_device
> *pdev)
> >                         "failed to register cpufreq cooling device: %d\n",
> ret);
> >                 return ret;
> >         }
> > +#endif
> >
> >         data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
> >         if (IS_ERR(data->thermal_clk)) {
> 
> You missed the error handling code which unregisters cooling/cpufreq stuff.
> 
> And it would be better to write things in a somewhat better way, like this:
> 
> #ifdef CONFIG_CPU_FREQ
> 
> static int imx_thermal_register_legacy_cooling(...)
> {
>         ... current function body
> }
> 
> static void imx_thermal_unregister_legacy_cooling(...)
> {
>         new routine body to unregister things }
> 
> #else
> static inline  int imx_thermal_register_legacy_cooling(...)
> {
>         return 0;
> }
> 
> static void imx_thermal_unregister_legacy_cooling(...) { }
> 
> #endif
> 
> 
> And then you can get rid of ifdef hackery in the middle of probe().

Thanks for good suggestion, please help review the V2 patch I just sent out.

Anson.