linux-kernel - Re: [PATCH v3 1/4] drivers/thermal/exynos: Refactor clk_sec initialization inside SOC-specific case

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

Message-ID: <e101aff2-a08e-4fed-8e38-df1aea44d23e@arm.com>
Date: Fri, 28 Feb 2025 14:37:24 +0000
From: Lukasz Luba <lukasz.luba@....com>
To: Anand Moon <linux.amoon@...il.com>
Cc: Krzysztof Kozlowski <krzk@...nel.org>,
 "open list:SAMSUNG THERMAL DRIVER" <linux-samsung-soc@...r.kernel.org>,
 Alim Akhtar <alim.akhtar@...sung.com>,
 Daniel Lezcano <daniel.lezcano@...aro.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 "open list:SAMSUNG THERMAL DRIVER" <linux-pm@...r.kernel.org>,
 Zhang Rui <rui.zhang@...el.com>,
 "moderated list:ARM/SAMSUNG S3C, S5P AND EXYNOS ARM ARCHITECTURES"
 <linux-arm-kernel@...ts.infradead.org>,
 Bartlomiej Zolnierkiewicz <bzolnier@...il.com>,
 open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/4] drivers/thermal/exynos: Refactor clk_sec
 initialization inside SOC-specific case

Hi Anand,

On 2/16/25 19:58, Anand Moon wrote:
> Refactor the initialization of the clk_sec clock to be inside the
> SOC_ARCH_EXYNOS5420_TRIMINFO case. It ensures that the clk_sec clock
> is only initialized for the specified SOC and not for other SOCs,
> thereby simplifying the code.

So IIUC there was no need to init that clock for other types of SoCs...
Do we know that for sure (e.g. from other TRMs)?

If that was the case, then simplification here can go further, but after
some fixes.

> 
> Signed-off-by: Anand Moon <linux.amoon@...il.com>
> ---
> v3: improve the commit message
> ---
>   drivers/thermal/samsung/exynos_tmu.c | 26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 47a99b3c5395..9c138772d380 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -1040,19 +1040,6 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>   	if (IS_ERR(data->clk))
>   		return dev_err_probe(dev, PTR_ERR(data->clk), "Failed to get clock\n");
>   
> -	data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif");
> -	if (IS_ERR(data->clk_sec)) {
> -		if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO)
> -			return dev_err_probe(dev, PTR_ERR(data->clk_sec),
> -					     "Failed to get triminfo clock\n");
> -	} else {
> -		ret = clk_prepare(data->clk_sec);
> -		if (ret) {
> -			dev_err(dev, "Failed to get clock\n");
> -			return ret;
> -		}
> -	}
> -
>   	ret = clk_prepare(data->clk);

Here the data->clk is now used in different order.

>   	if (ret) {
>   		dev_err(dev, "Failed to get clock\n");
> @@ -1060,6 +1047,19 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>   	}
>   
>   	switch (data->soc) {
> +	case SOC_ARCH_EXYNOS5420_TRIMINFO:
> +		data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif");
> +		if (IS_ERR(data->clk_sec)) {
> +			return dev_err_probe(dev, PTR_ERR(data->clk_sec),
> +					     "Failed to get triminfo clock\n");

Then here you shouldn't simply copy the old code. Now the data->clk
is first, so should be 'clk_unprepare()' before return of the function.

> +		} else {

You can get rid of this 'else' above and still be safe in your
refactoring.

> +			ret = clk_prepare(data->clk_sec);
> +			if (ret) {
> +				dev_err(dev, "Failed to get clock\n");
> +				return ret;
> +			}

Here you can further simplify this to something like:
-----------------------8<-------------------------------------

+	case SOC_ARCH_EXYNOS5420_TRIMINFO:
+		data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif");
+		if (IS_ERR(data->clk_sec)) {
+			clk_unprepare(data->clk); ///// <----
+			return dev_err_probe(dev, PTR_ERR(data->clk_sec),
+					     "Failed to get triminfo clock\n");
+		}
+		ret = clk_prepare(data->clk_sec);
+		if (ret) {
+			dev_err(dev, "Failed to get clock\n");
+			clk_unprepare(data->clk); ///// <----
+			return ret;
+		}
+
+	break;

--------------------------->8---------------------------------

Or with better 'goto' flow.

> +		}
> +	break;
>   	case SOC_ARCH_EXYNOS5433:
>   	case SOC_ARCH_EXYNOS7:
>   		data->sclk = devm_clk_get(dev, "tmu_sclk");

Also, you should revisit the 'goto' cleanup section at the bottom.

Regards,
Lukasz