[PATCH v3 0/4] Exynos Thermal code improvement

[PATCH v3 0/4] Exynos Thermal code improvement

[Anand Moon]

Hi All,

This patch series is a rework of my previous patch series [1],
where the code changes were not adequately justified.

In this new series, I have improved the commit subject
and commit message to better explain the changes.

Tested on Odroid U3 amd XU4 SoC boards.

[1] https://lore.kernel.org/all/20220515064126.1424-1-linux.amoon@gmail.com/
[0] https://lore.kernel.org/lkml/CANAwSgS=08fVsqn95WHzSF71WTTyD2-=K2C6-BEz0tY0t6A1-g@mail.gmail.com/T/#m77e57120d230d57f34c29e1422d7fc5f5587ac30

Thanks
-Anand

Anand Moon (4):
  drivers/thermal/exynos: Refactor clk_sec initialization inside
    SOC-specific case
  drivers/thermal/exymos: Remove redundant IS_ERR() checks for clk_sec
    clock
  drivers/thermal/exymos: Fixed the efuse min max value for exynos5422
  drivers/thermal/exymos: Use guard notation when acquiring mutex

 drivers/thermal/samsung/exynos_tmu.c | 78 ++++++++++++++--------------
 1 file changed, 39 insertions(+), 39 deletions(-)


base-commit: ba643b6d84409e8a9057d5bdd6dd99255b1a88fe
-- 
2.48.1

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

[Anand Moon]

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.

Signed-off-by: Anand Moon <linux.amoon@gmail.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);
 	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");
+		} else {
+			ret = clk_prepare(data->clk_sec);
+			if (ret) {
+				dev_err(dev, "Failed to get clock\n");
+				return ret;
+			}
+		}
+	break;
 	case SOC_ARCH_EXYNOS5433:
 	case SOC_ARCH_EXYNOS7:
 		data->sclk = devm_clk_get(dev, "tmu_sclk");
-- 
2.48.1

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

[Lukasz Luba]

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@gmail.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

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

[Anand Moon]

Hi Lukasz,

Thanks for your review comments

On Fri, 28 Feb 2025 at 20:07, Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> 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)?
This clock (clk) is utilized for the Thermal Management Unit (TMU) and
the GPU in the Exynos 542x,
as specified in the user manual.
>
> If that was the case, then simplification here can go further, but after
> some fixes.
>
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.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.
Ok.
>
> >       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;
Ok
>
> --------------------------->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.
Thanks. I will recheck and update the code for the next version.
>
> Regards,
> Lukasz

Thanks
-Anand

[PATCH v3 2/4] drivers/thermal/exymos: Remove redundant IS_ERR() checks for clk_sec clock

[Anand Moon]

Remove unnecessary IS_ERR() checks for the clk_sec clock,
the clk_enable() and clk_disable() functions can handle NULL clock
pointers, so the additional checks are redundant and have been removed
to simplify the code.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v3: improve the commit message
---
 drivers/thermal/samsung/exynos_tmu.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 9c138772d380..8d26000c73aa 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -258,8 +258,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 
 	mutex_lock(&data->lock);
 	clk_enable(data->clk);
-	if (!IS_ERR(data->clk_sec))
-		clk_enable(data->clk_sec);
+	clk_enable(data->clk_sec);
 
 	status = readb(data->base + EXYNOS_TMU_REG_STATUS);
 	if (!status) {
@@ -269,8 +268,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 		data->tmu_clear_irqs(data);
 	}
 
-	if (!IS_ERR(data->clk_sec))
-		clk_disable(data->clk_sec);
+	clk_disable(data->clk_sec);
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
@@ -1115,8 +1113,7 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 err_clk:
 	clk_unprepare(data->clk);
 err_clk_sec:
-	if (!IS_ERR(data->clk_sec))
-		clk_unprepare(data->clk_sec);
+	clk_unprepare(data->clk_sec);
 	return ret;
 }
 
@@ -1128,8 +1125,7 @@ static void exynos_tmu_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(data->sclk);
 	clk_unprepare(data->clk);
-	if (!IS_ERR(data->clk_sec))
-		clk_unprepare(data->clk_sec);
+	clk_unprepare(data->clk_sec);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.48.1

Re: [PATCH v3 2/4] drivers/thermal/exymos: Remove redundant IS_ERR() checks for clk_sec clock

[Lukasz Luba]

On 2/16/25 19:58, Anand Moon wrote:
> Remove unnecessary IS_ERR() checks for the clk_sec clock,
> the clk_enable() and clk_disable() functions can handle NULL clock
> pointers, so the additional checks are redundant and have been removed
> to simplify the code.

This patch looks sane, just rework the 'goto' stuff in the
exynos_tmu_probe() maybe in the patch 1/4 so won't be needed here.

[PATCH v3 3/4] drivers/thermal/exymos: Fixed the efuse min max value for exynos5422

[Anand Moon]

As per Exynos5422 user manual e-Fuse range min~max range is 16~76.
if e-Fuse value is out of this range, then thermal sensor may not
sense thermal data properly. Refactors the efuse value
initialization logic within exynos_map_dt_data function by
replacing the nested if-else statements with a switch statement.
Ensures proper initialization of efuse values based on the SOC type.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v3: improve the commit message, fixing the warning wtth W=1
---
 drivers/thermal/samsung/exynos_tmu.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 8d26000c73aa..fe090c1a93ab 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -899,12 +899,23 @@ static int exynos_map_dt_data(struct platform_device *pdev)
 		data->gain = 8;
 		data->reference_voltage = 16;
 		data->efuse_value = 55;
-		if (data->soc != SOC_ARCH_EXYNOS5420 &&
-		    data->soc != SOC_ARCH_EXYNOS5420_TRIMINFO)
+		data->max_efuse_value = 100;
+		switch (data->soc) {
+		case SOC_ARCH_EXYNOS3250:
+		case SOC_ARCH_EXYNOS4412:
+		case SOC_ARCH_EXYNOS5250:
+		case SOC_ARCH_EXYNOS5260:
 			data->min_efuse_value = 40;
-		else
+			break;
+		case SOC_ARCH_EXYNOS5420:
+		case SOC_ARCH_EXYNOS5420_TRIMINFO:
+			data->min_efuse_value = 16;
+			data->max_efuse_value = 76;
+			break;
+		default:
 			data->min_efuse_value = 0;
-		data->max_efuse_value = 100;
+			break;
+		}
 		break;
 	case SOC_ARCH_EXYNOS5433:
 		data->tmu_set_low_temp = exynos5433_tmu_set_low_temp;
-- 
2.48.1

[PATCH v3 4/4] drivers/thermal/exymos: Use guard notation when acquiring mutex

[Anand Moon]

Using guard notation makes the code more compact and error handling
more robust by ensuring that mutexes are released in all code paths
when control leaves critical section.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v3: new patch
---
 drivers/thermal/samsung/exynos_tmu.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index fe090c1a93ab..a34ba3858d64 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -256,7 +256,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 	unsigned int status;
 	int ret = 0;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 	clk_enable(data->clk_sec);
 
@@ -270,7 +270,6 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 
 	clk_disable(data->clk_sec);
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 
 	return ret;
 }
@@ -292,13 +291,12 @@ static int exynos_thermal_zone_configure(struct platform_device *pdev)
 		return ret;
 	}
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 
 	data->tmu_set_crit_temp(data, temp / MCELSIUS);
 
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 
 	return 0;
 }
@@ -325,12 +323,11 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
 {
 	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 	data->tmu_control(pdev, on);
 	data->enabled = on;
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 }
 
 static void exynos_tmu_update_bit(struct exynos_tmu_data *data, int reg_off,
@@ -645,7 +642,7 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
 		 */
 		return -EAGAIN;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 
 	value = data->tmu_read(data);
@@ -655,7 +652,6 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
 		*temp = code_to_temp(data, value) * MCELSIUS;
 
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 
 	return ret;
 }
@@ -720,11 +716,10 @@ static int exynos_tmu_set_emulation(struct thermal_zone_device *tz, int temp)
 	if (temp && temp < MCELSIUS)
 		goto out;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 	data->tmu_set_emulation(data, temp);
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 	return 0;
 out:
 	return ret;
@@ -760,14 +755,13 @@ static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
 
 	thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 
 	/* TODO: take action based on particular interrupt */
 	data->tmu_clear_irqs(data);
 
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 
 	return IRQ_HANDLED;
 }
@@ -987,7 +981,7 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
 {
 	struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	clk_enable(data->clk);
 
 	if (low > INT_MIN)
@@ -1000,7 +994,6 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
 		data->tmu_disable_high(data);
 
 	clk_disable(data->clk);
-	mutex_unlock(&data->lock);
 
 	return 0;
 }
-- 
2.48.1

Re: [PATCH v3 4/4] drivers/thermal/exymos: Use guard notation when acquiring mutex

[Lukasz Luba]

On 2/16/25 19:58, Anand Moon wrote:
> Using guard notation makes the code more compact and error handling
> more robust by ensuring that mutexes are released in all code paths
> when control leaves critical section.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> v3: new patch
> ---
>   drivers/thermal/samsung/exynos_tmu.c | 21 +++++++--------------
>   1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index fe090c1a93ab..a34ba3858d64 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -256,7 +256,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>   	unsigned int status;
>   	int ret = 0;
>   
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>   	clk_enable(data->clk);
>   	clk_enable(data->clk_sec);
>   
> @@ -270,7 +270,6 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>   
>   	clk_disable(data->clk_sec);
>   	clk_disable(data->clk);
> -	mutex_unlock(&data->lock);
>   
>   	return ret;
>   }
> @@ -292,13 +291,12 @@ static int exynos_thermal_zone_configure(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>   	clk_enable(data->clk);
>   
>   	data->tmu_set_crit_temp(data, temp / MCELSIUS);
>   
>   	clk_disable(data->clk);
> -	mutex_unlock(&data->lock);
>   
>   	return 0;
>   }
> @@ -325,12 +323,11 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
>   {
>   	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>   
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>   	clk_enable(data->clk);
>   	data->tmu_control(pdev, on);
>   	data->enabled = on;
>   	clk_disable(data->clk);
> -	mutex_unlock(&data->lock);
>   }
>   
>   static void exynos_tmu_update_bit(struct exynos_tmu_data *data, int reg_off,
> @@ -645,7 +642,7 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
>   		 */
>   		return -EAGAIN;
>   
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>   	clk_enable(data->clk);
>   
>   	value = data->tmu_read(data);
> @@ -655,7 +652,6 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
>   		*temp = code_to_temp(data, value) * MCELSIUS;
>   
>   	clk_disable(data->clk);
> -	mutex_unlock(&data->lock);
>   
>   	return ret;
>   }
> @@ -720,11 +716,10 @@ static int exynos_tmu_set_emulation(struct thermal_zone_device *tz, int temp)
>   	if (temp && temp < MCELSIUS)
>   		goto out;
>   
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>   	clk_enable(data->clk);
>   	data->tmu_set_emulation(data, temp);
>   	clk_disable(data->clk);
> -	mutex_unlock(&data->lock);
>   	return 0;
>   out:
>   	return ret;
> @@ -760,14 +755,13 @@ static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
>   
>   	thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
>   
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>   	clk_enable(data->clk);
>   
>   	/* TODO: take action based on particular interrupt */
>   	data->tmu_clear_irqs(data);
>   
>   	clk_disable(data->clk);
> -	mutex_unlock(&data->lock);
>   
>   	return IRQ_HANDLED;
>   }
> @@ -987,7 +981,7 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
>   {
>   	struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
>   
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>   	clk_enable(data->clk);
>   
>   	if (low > INT_MIN)
> @@ -1000,7 +994,6 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
>   		data->tmu_disable_high(data);
>   
>   	clk_disable(data->clk);
> -	mutex_unlock(&data->lock);
>   
>   	return 0;
>   }

IMO you should be able to even use something like we have
core framework:

guard(thermal_zone)(tz);

Your mutex name is simply 'lock' in the struct exynos_tmu_data
so you should be able to leverage this by:

guard(exynos_tmu_data)(data);

Please try that.

Re: [PATCH v3 4/4] drivers/thermal/exymos: Use guard notation when acquiring mutex

[Anand Moon]

Hi Lukasz,

On Fri, 28 Feb 2025 at 22:58, Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 2/16/25 19:58, Anand Moon wrote:
> > Using guard notation makes the code more compact and error handling
> > more robust by ensuring that mutexes are released in all code paths
> > when control leaves critical section.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > v3: new patch
> > ---
> >   drivers/thermal/samsung/exynos_tmu.c | 21 +++++++--------------
> >   1 file changed, 7 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> > index fe090c1a93ab..a34ba3858d64 100644
> > --- a/drivers/thermal/samsung/exynos_tmu.c
> > +++ b/drivers/thermal/samsung/exynos_tmu.c
> > @@ -256,7 +256,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> >       unsigned int status;
> >       int ret = 0;
> >
> > -     mutex_lock(&data->lock);
> > +     guard(mutex)(&data->lock);
> >       clk_enable(data->clk);
> >       clk_enable(data->clk_sec);
> >
> > @@ -270,7 +270,6 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> >
> >       clk_disable(data->clk_sec);
> >       clk_disable(data->clk);
> > -     mutex_unlock(&data->lock);
> >
> >       return ret;
> >   }
> > @@ -292,13 +291,12 @@ static int exynos_thermal_zone_configure(struct platform_device *pdev)
> >               return ret;
> >       }
> >
> > -     mutex_lock(&data->lock);
> > +     guard(mutex)(&data->lock);
> >       clk_enable(data->clk);
> >
> >       data->tmu_set_crit_temp(data, temp / MCELSIUS);
> >
> >       clk_disable(data->clk);
> > -     mutex_unlock(&data->lock);
> >
> >       return 0;
> >   }
> > @@ -325,12 +323,11 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
> >   {
> >       struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> >
> > -     mutex_lock(&data->lock);
> > +     guard(mutex)(&data->lock);
> >       clk_enable(data->clk);
> >       data->tmu_control(pdev, on);
> >       data->enabled = on;
> >       clk_disable(data->clk);
> > -     mutex_unlock(&data->lock);
> >   }
> >
> >   static void exynos_tmu_update_bit(struct exynos_tmu_data *data, int reg_off,
> > @@ -645,7 +642,7 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
> >                */
> >               return -EAGAIN;
> >
> > -     mutex_lock(&data->lock);
> > +     guard(mutex)(&data->lock);
> >       clk_enable(data->clk);
> >
> >       value = data->tmu_read(data);
> > @@ -655,7 +652,6 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
> >               *temp = code_to_temp(data, value) * MCELSIUS;
> >
> >       clk_disable(data->clk);
> > -     mutex_unlock(&data->lock);
> >
> >       return ret;
> >   }
> > @@ -720,11 +716,10 @@ static int exynos_tmu_set_emulation(struct thermal_zone_device *tz, int temp)
> >       if (temp && temp < MCELSIUS)
> >               goto out;
> >
> > -     mutex_lock(&data->lock);
> > +     guard(mutex)(&data->lock);
> >       clk_enable(data->clk);
> >       data->tmu_set_emulation(data, temp);
> >       clk_disable(data->clk);
> > -     mutex_unlock(&data->lock);
> >       return 0;
> >   out:
> >       return ret;
> > @@ -760,14 +755,13 @@ static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
> >
> >       thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
> >
> > -     mutex_lock(&data->lock);
> > +     guard(mutex)(&data->lock);
> >       clk_enable(data->clk);
> >
> >       /* TODO: take action based on particular interrupt */
> >       data->tmu_clear_irqs(data);
> >
> >       clk_disable(data->clk);
> > -     mutex_unlock(&data->lock);
> >
> >       return IRQ_HANDLED;
> >   }
> > @@ -987,7 +981,7 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
> >   {
> >       struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
> >
> > -     mutex_lock(&data->lock);
> > +     guard(mutex)(&data->lock);
> >       clk_enable(data->clk);
> >
> >       if (low > INT_MIN)
> > @@ -1000,7 +994,6 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
> >               data->tmu_disable_high(data);
> >
> >       clk_disable(data->clk);
> > -     mutex_unlock(&data->lock);
> >
> >       return 0;
> >   }

Thanks for your review comments.
>
> IMO you should be able to even use something like we have
> core framework:
>
> guard(thermal_zone)(tz);
>
> Your mutex name is simply 'lock' in the struct exynos_tmu_data
> so you should be able to leverage this by:
>
> guard(exynos_tmu_data)(data);
>
> Please try that.

Ok, I will check this

Thanks
-Anand

Re: [PATCH v3 4/4] drivers/thermal/exymos: Use guard notation when acquiring mutex

[Anand Moon]

Hi Lukasz,

On Sat, 1 Mar 2025 at 00:06, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi Lukasz,
>
> On Fri, 28 Feb 2025 at 22:58, Lukasz Luba <lukasz.luba@arm.com> wrote:
> >
> >
> >
> > On 2/16/25 19:58, Anand Moon wrote:
> > > Using guard notation makes the code more compact and error handling
> > > more robust by ensuring that mutexes are released in all code paths
> > > when control leaves critical section.
> > >
> > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > ---
> > > v3: new patch
> > > ---
> > >   drivers/thermal/samsung/exynos_tmu.c | 21 +++++++--------------
> > >   1 file changed, 7 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> > > index fe090c1a93ab..a34ba3858d64 100644
> > > --- a/drivers/thermal/samsung/exynos_tmu.c
> > > +++ b/drivers/thermal/samsung/exynos_tmu.c
> > > @@ -256,7 +256,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> > >       unsigned int status;
> > >       int ret = 0;
> > >
> > > -     mutex_lock(&data->lock);
> > > +     guard(mutex)(&data->lock);
> > >       clk_enable(data->clk);
> > >       clk_enable(data->clk_sec);
> > >
> > > @@ -270,7 +270,6 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> > >
> > >       clk_disable(data->clk_sec);
> > >       clk_disable(data->clk);
> > > -     mutex_unlock(&data->lock);
> > >
> > >       return ret;
> > >   }
> > > @@ -292,13 +291,12 @@ static int exynos_thermal_zone_configure(struct platform_device *pdev)
> > >               return ret;
> > >       }
> > >
> > > -     mutex_lock(&data->lock);
> > > +     guard(mutex)(&data->lock);
> > >       clk_enable(data->clk);
> > >
> > >       data->tmu_set_crit_temp(data, temp / MCELSIUS);
> > >
> > >       clk_disable(data->clk);
> > > -     mutex_unlock(&data->lock);
> > >
> > >       return 0;
> > >   }
> > > @@ -325,12 +323,11 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
> > >   {
> > >       struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> > >
> > > -     mutex_lock(&data->lock);
> > > +     guard(mutex)(&data->lock);
> > >       clk_enable(data->clk);
> > >       data->tmu_control(pdev, on);
> > >       data->enabled = on;
> > >       clk_disable(data->clk);
> > > -     mutex_unlock(&data->lock);
> > >   }
> > >
> > >   static void exynos_tmu_update_bit(struct exynos_tmu_data *data, int reg_off,
> > > @@ -645,7 +642,7 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
> > >                */
> > >               return -EAGAIN;
> > >
> > > -     mutex_lock(&data->lock);
> > > +     guard(mutex)(&data->lock);
> > >       clk_enable(data->clk);
> > >
> > >       value = data->tmu_read(data);
> > > @@ -655,7 +652,6 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
> > >               *temp = code_to_temp(data, value) * MCELSIUS;
> > >
> > >       clk_disable(data->clk);
> > > -     mutex_unlock(&data->lock);
> > >
> > >       return ret;
> > >   }
> > > @@ -720,11 +716,10 @@ static int exynos_tmu_set_emulation(struct thermal_zone_device *tz, int temp)
> > >       if (temp && temp < MCELSIUS)
> > >               goto out;
> > >
> > > -     mutex_lock(&data->lock);
> > > +     guard(mutex)(&data->lock);
> > >       clk_enable(data->clk);
> > >       data->tmu_set_emulation(data, temp);
> > >       clk_disable(data->clk);
> > > -     mutex_unlock(&data->lock);
> > >       return 0;
> > >   out:
> > >       return ret;
> > > @@ -760,14 +755,13 @@ static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
> > >
> > >       thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
> > >
> > > -     mutex_lock(&data->lock);
> > > +     guard(mutex)(&data->lock);
> > >       clk_enable(data->clk);
> > >
> > >       /* TODO: take action based on particular interrupt */
> > >       data->tmu_clear_irqs(data);
> > >
> > >       clk_disable(data->clk);
> > > -     mutex_unlock(&data->lock);
> > >
> > >       return IRQ_HANDLED;
> > >   }
> > > @@ -987,7 +981,7 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
> > >   {
> > >       struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
> > >
> > > -     mutex_lock(&data->lock);
> > > +     guard(mutex)(&data->lock);
> > >       clk_enable(data->clk);
> > >
> > >       if (low > INT_MIN)
> > > @@ -1000,7 +994,6 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
> > >               data->tmu_disable_high(data);
> > >
> > >       clk_disable(data->clk);
> > > -     mutex_unlock(&data->lock);
> > >
> > >       return 0;
> > >   }
>
> Thanks for your review comments.
> >
> > IMO you should be able to even use something like we have
> > core framework:
> >
> > guard(thermal_zone)(tz);
> >
> > Your mutex name is simply 'lock' in the struct exynos_tmu_data
> > so you should be able to leverage this by:
> >
> > guard(exynos_tmu_data)(data);
> >

If I introduce the guard it creates a compilation error

amoon@anand-m920q:~/mainline/linux-exynos-6.y-devel$ vi
drivers/thermal/samsung/exynos_tmu.c +306
amoon@anand-m920q:~/mainline/linux-exynos-6.y-devel$ make -j$(nproc)
ARCH=arm CROSS_COMPILE=arm-none-eabi- LOCALVERSION=-u3ml dtbs zImage
modules
  CALL    scripts/checksyscalls.sh
  CHK     kernel/kheaders_data.tar.xz
  CC      drivers/thermal/samsung/exynos_tmu.o
  CC [M]  drivers/md/raid10.o
In file included from ./include/linux/irqflags.h:17,
                 from ./arch/arm/include/asm/bitops.h:28,
                 from ./include/linux/bitops.h:68,
                 from ./include/linux/kernel.h:23,
                 from ./include/linux/clk.h:13,
                 from drivers/thermal/samsung/exynos_tmu.c:14:
drivers/thermal/samsung/exynos_tmu.c: In function 'exynos_tmu_update_bit':
./include/linux/cleanup.h:258:9: error: unknown type name
'class_exynos_tmu_data_t'
  258 |         class_##_name##_t var
__cleanup(class_##_name##_destructor) =   \
      |         ^~~~~~
./include/linux/cleanup.h:309:9: note: in expansion of macro 'CLASS'
  309 |         CLASS(_name, __UNIQUE_ID(guard))
      |         ^~~~~
drivers/thermal/samsung/exynos_tmu.c:338:9: note: in expansion of macro 'guard'
  338 |         guard(exynos_tmu_data)(data);
      |         ^~~~~
drivers/thermal/samsung/exynos_tmu.c:338:9: error: cleanup argument
not a function
  CC [M]  drivers/md/raid5.o
./include/linux/cleanup.h:259:17: error: implicit declaration of
function 'class_exynos_tmu_data_constructor'
[-Wimplicit-function-declaration]
  259 |                 class_##_name##_constructor
      |                 ^~~~~~
./include/linux/cleanup.h:309:9: note: in expansion of macro 'CLASS'
  309 |         CLASS(_name, __UNIQUE_ID(guard))
      |         ^~~~~
drivers/thermal/samsung/exynos_tmu.c:338:9: note: in expansion of macro 'guard'
  338 |         guard(exynos_tmu_data)(data);
      |         ^~~~~
./include/linux/compiler.h:166:45: warning: unused variable
'__UNIQUE_ID_guard572' [-Wunused-variable]
  166 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_,
prefix), __COUNTER__)
      |                                             ^~~~~~~~~~~~
./include/linux/cleanup.h:258:27: note: in definition of macro 'CLASS'
  258 |         class_##_name##_t var
__cleanup(class_##_name##_destructor) =   \
      |                           ^~~
././include/linux/compiler_types.h:84:22: note: in expansion of macro '___PASTE'
   84 | #define __PASTE(a,b) ___PASTE(a,b)
      |                      ^~~~~~~~
./include/linux/compiler.h:166:29: note: in expansion of macro '__PASTE'
  166 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_,
prefix), __COUNTER__)
      |                             ^~~~~~~
././include/linux/compiler_types.h:84:22: note: in expansion of macro '___PASTE'
   84 | #define __PASTE(a,b) ___PASTE(a,b)
      |                      ^~~~~~~~
./include/linux/compiler.h:166:37: note: in expansion of macro '__PASTE'
  166 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_,
prefix), __COUNTER__)
      |                                     ^~~~~~~
./include/linux/cleanup.h:309:22: note: in expansion of macro '__UNIQUE_ID'
  309 |         CLASS(_name, __UNIQUE_ID(guard))
      |                      ^~~~~~~~~~~
drivers/thermal/samsung/exynos_tmu.c:338:9: note: in expansion of macro 'guard'
  338 |         guard(exynos_tmu_data)(data);

Thanks
-Anand

Re: [PATCH v3 4/4] drivers/thermal/exymos: Use guard notation when acquiring mutex

[Lukasz Luba]

On 3/4/25 12:20, Anand Moon wrote:
> Hi Lukasz,
> 
> On Sat, 1 Mar 2025 at 00:06, Anand Moon <linux.amoon@gmail.com> wrote:
>>
>> Hi Lukasz,
>>
>> On Fri, 28 Feb 2025 at 22:58, Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>
>>>
>>>
>>> On 2/16/25 19:58, Anand Moon wrote:
>>>> Using guard notation makes the code more compact and error handling
>>>> more robust by ensuring that mutexes are released in all code paths
>>>> when control leaves critical section.
>>>>
>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>> ---
>>>> v3: new patch
>>>> ---
>>>>    drivers/thermal/samsung/exynos_tmu.c | 21 +++++++--------------
>>>>    1 file changed, 7 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>>> index fe090c1a93ab..a34ba3858d64 100644
>>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>>> @@ -256,7 +256,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>        unsigned int status;
>>>>        int ret = 0;
>>>>
>>>> -     mutex_lock(&data->lock);
>>>> +     guard(mutex)(&data->lock);
>>>>        clk_enable(data->clk);
>>>>        clk_enable(data->clk_sec);
>>>>
>>>> @@ -270,7 +270,6 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>
>>>>        clk_disable(data->clk_sec);
>>>>        clk_disable(data->clk);
>>>> -     mutex_unlock(&data->lock);
>>>>
>>>>        return ret;
>>>>    }
>>>> @@ -292,13 +291,12 @@ static int exynos_thermal_zone_configure(struct platform_device *pdev)
>>>>                return ret;
>>>>        }
>>>>
>>>> -     mutex_lock(&data->lock);
>>>> +     guard(mutex)(&data->lock);
>>>>        clk_enable(data->clk);
>>>>
>>>>        data->tmu_set_crit_temp(data, temp / MCELSIUS);
>>>>
>>>>        clk_disable(data->clk);
>>>> -     mutex_unlock(&data->lock);
>>>>
>>>>        return 0;
>>>>    }
>>>> @@ -325,12 +323,11 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
>>>>    {
>>>>        struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>>>>
>>>> -     mutex_lock(&data->lock);
>>>> +     guard(mutex)(&data->lock);
>>>>        clk_enable(data->clk);
>>>>        data->tmu_control(pdev, on);
>>>>        data->enabled = on;
>>>>        clk_disable(data->clk);
>>>> -     mutex_unlock(&data->lock);
>>>>    }
>>>>
>>>>    static void exynos_tmu_update_bit(struct exynos_tmu_data *data, int reg_off,
>>>> @@ -645,7 +642,7 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
>>>>                 */
>>>>                return -EAGAIN;
>>>>
>>>> -     mutex_lock(&data->lock);
>>>> +     guard(mutex)(&data->lock);
>>>>        clk_enable(data->clk);
>>>>
>>>>        value = data->tmu_read(data);
>>>> @@ -655,7 +652,6 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
>>>>                *temp = code_to_temp(data, value) * MCELSIUS;
>>>>
>>>>        clk_disable(data->clk);
>>>> -     mutex_unlock(&data->lock);
>>>>
>>>>        return ret;
>>>>    }
>>>> @@ -720,11 +716,10 @@ static int exynos_tmu_set_emulation(struct thermal_zone_device *tz, int temp)
>>>>        if (temp && temp < MCELSIUS)
>>>>                goto out;
>>>>
>>>> -     mutex_lock(&data->lock);
>>>> +     guard(mutex)(&data->lock);
>>>>        clk_enable(data->clk);
>>>>        data->tmu_set_emulation(data, temp);
>>>>        clk_disable(data->clk);
>>>> -     mutex_unlock(&data->lock);
>>>>        return 0;
>>>>    out:
>>>>        return ret;
>>>> @@ -760,14 +755,13 @@ static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
>>>>
>>>>        thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
>>>>
>>>> -     mutex_lock(&data->lock);
>>>> +     guard(mutex)(&data->lock);
>>>>        clk_enable(data->clk);
>>>>
>>>>        /* TODO: take action based on particular interrupt */
>>>>        data->tmu_clear_irqs(data);
>>>>
>>>>        clk_disable(data->clk);
>>>> -     mutex_unlock(&data->lock);
>>>>
>>>>        return IRQ_HANDLED;
>>>>    }
>>>> @@ -987,7 +981,7 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
>>>>    {
>>>>        struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
>>>>
>>>> -     mutex_lock(&data->lock);
>>>> +     guard(mutex)(&data->lock);
>>>>        clk_enable(data->clk);
>>>>
>>>>        if (low > INT_MIN)
>>>> @@ -1000,7 +994,6 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
>>>>                data->tmu_disable_high(data);
>>>>
>>>>        clk_disable(data->clk);
>>>> -     mutex_unlock(&data->lock);
>>>>
>>>>        return 0;
>>>>    }
>>
>> Thanks for your review comments.
>>>
>>> IMO you should be able to even use something like we have
>>> core framework:
>>>
>>> guard(thermal_zone)(tz);
>>>
>>> Your mutex name is simply 'lock' in the struct exynos_tmu_data
>>> so you should be able to leverage this by:
>>>
>>> guard(exynos_tmu_data)(data);
>>>
> 
> If I introduce the guard it creates a compilation error
> 
> amoon@anand-m920q:~/mainline/linux-exynos-6.y-devel$ vi
> drivers/thermal/samsung/exynos_tmu.c +306
> amoon@anand-m920q:~/mainline/linux-exynos-6.y-devel$ make -j$(nproc)
> ARCH=arm CROSS_COMPILE=arm-none-eabi- LOCALVERSION=-u3ml dtbs zImage
> modules
>    CALL    scripts/checksyscalls.sh
>    CHK     kernel/kheaders_data.tar.xz
>    CC      drivers/thermal/samsung/exynos_tmu.o
>    CC [M]  drivers/md/raid10.o
> In file included from ./include/linux/irqflags.h:17,
>                   from ./arch/arm/include/asm/bitops.h:28,
>                   from ./include/linux/bitops.h:68,
>                   from ./include/linux/kernel.h:23,
>                   from ./include/linux/clk.h:13,
>                   from drivers/thermal/samsung/exynos_tmu.c:14:
> drivers/thermal/samsung/exynos_tmu.c: In function 'exynos_tmu_update_bit':
> ./include/linux/cleanup.h:258:9: error: unknown type name
> 'class_exynos_tmu_data_t'
>    258 |         class_##_name##_t var
> __cleanup(class_##_name##_destructor) =   \
>        |         ^~~~~~
> ./include/linux/cleanup.h:309:9: note: in expansion of macro 'CLASS'
>    309 |         CLASS(_name, __UNIQUE_ID(guard))
>        |         ^~~~~
> drivers/thermal/samsung/exynos_tmu.c:338:9: note: in expansion of macro 'guard'
>    338 |         guard(exynos_tmu_data)(data);
>        |         ^~~~~
> drivers/thermal/samsung/exynos_tmu.c:338:9: error: cleanup argument
> not a function

[snip]

Right, you're missing the definition at the begging, like:

DEFINE_GUARD(exynos_tmu_data, struct exynos_tmu_data *, 
mutex_lock(&_T->lock),
              mutex_unlock(&_T->lock))

below the struct exynos_tmu_data definition.

Also, make sure you include the cleanup.h (it might not complain,
but it would be explicit and more clear)

Re: [PATCH v3 4/4] drivers/thermal/exymos: Use guard notation when acquiring mutex

[Anand Moon]

Hi Lukasz,

On Wed, 5 Mar 2025 at 14:12, Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 3/4/25 12:20, Anand Moon wrote:
> > Hi Lukasz,
> >
> > On Sat, 1 Mar 2025 at 00:06, Anand Moon <linux.amoon@gmail.com> wrote:
> >>
> >> Hi Lukasz,
> >>
> >> On Fri, 28 Feb 2025 at 22:58, Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>>
> >>>
> >>>
> >>> On 2/16/25 19:58, Anand Moon wrote:
> >>>> Using guard notation makes the code more compact and error handling
> >>>> more robust by ensuring that mutexes are released in all code paths
> >>>> when control leaves critical section.
> >>>>
> >>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >>>> ---
> >>>> v3: new patch
> >>>> ---
> >>>>    drivers/thermal/samsung/exynos_tmu.c | 21 +++++++--------------
> >>>>    1 file changed, 7 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> >>>> index fe090c1a93ab..a34ba3858d64 100644
> >>>> --- a/drivers/thermal/samsung/exynos_tmu.c
> >>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
> >>>> @@ -256,7 +256,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> >>>>        unsigned int status;
> >>>>        int ret = 0;
> >>>>
> >>>> -     mutex_lock(&data->lock);
> >>>> +     guard(mutex)(&data->lock);
> >>>>        clk_enable(data->clk);
> >>>>        clk_enable(data->clk_sec);
> >>>>
> >>>> @@ -270,7 +270,6 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> >>>>
> >>>>        clk_disable(data->clk_sec);
> >>>>        clk_disable(data->clk);
> >>>> -     mutex_unlock(&data->lock);
> >>>>
> >>>>        return ret;
> >>>>    }
> >>>> @@ -292,13 +291,12 @@ static int exynos_thermal_zone_configure(struct platform_device *pdev)
> >>>>                return ret;
> >>>>        }
> >>>>
> >>>> -     mutex_lock(&data->lock);
> >>>> +     guard(mutex)(&data->lock);
> >>>>        clk_enable(data->clk);
> >>>>
> >>>>        data->tmu_set_crit_temp(data, temp / MCELSIUS);
> >>>>
> >>>>        clk_disable(data->clk);
> >>>> -     mutex_unlock(&data->lock);
> >>>>
> >>>>        return 0;
> >>>>    }
> >>>> @@ -325,12 +323,11 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
> >>>>    {
> >>>>        struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> >>>>
> >>>> -     mutex_lock(&data->lock);
> >>>> +     guard(mutex)(&data->lock);
> >>>>        clk_enable(data->clk);
> >>>>        data->tmu_control(pdev, on);
> >>>>        data->enabled = on;
> >>>>        clk_disable(data->clk);
> >>>> -     mutex_unlock(&data->lock);
> >>>>    }
> >>>>
> >>>>    static void exynos_tmu_update_bit(struct exynos_tmu_data *data, int reg_off,
> >>>> @@ -645,7 +642,7 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
> >>>>                 */
> >>>>                return -EAGAIN;
> >>>>
> >>>> -     mutex_lock(&data->lock);
> >>>> +     guard(mutex)(&data->lock);
> >>>>        clk_enable(data->clk);
> >>>>
> >>>>        value = data->tmu_read(data);
> >>>> @@ -655,7 +652,6 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
> >>>>                *temp = code_to_temp(data, value) * MCELSIUS;
> >>>>
> >>>>        clk_disable(data->clk);
> >>>> -     mutex_unlock(&data->lock);
> >>>>
> >>>>        return ret;
> >>>>    }
> >>>> @@ -720,11 +716,10 @@ static int exynos_tmu_set_emulation(struct thermal_zone_device *tz, int temp)
> >>>>        if (temp && temp < MCELSIUS)
> >>>>                goto out;
> >>>>
> >>>> -     mutex_lock(&data->lock);
> >>>> +     guard(mutex)(&data->lock);
> >>>>        clk_enable(data->clk);
> >>>>        data->tmu_set_emulation(data, temp);
> >>>>        clk_disable(data->clk);
> >>>> -     mutex_unlock(&data->lock);
> >>>>        return 0;
> >>>>    out:
> >>>>        return ret;
> >>>> @@ -760,14 +755,13 @@ static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
> >>>>
> >>>>        thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
> >>>>
> >>>> -     mutex_lock(&data->lock);
> >>>> +     guard(mutex)(&data->lock);
> >>>>        clk_enable(data->clk);
> >>>>
> >>>>        /* TODO: take action based on particular interrupt */
> >>>>        data->tmu_clear_irqs(data);
> >>>>
> >>>>        clk_disable(data->clk);
> >>>> -     mutex_unlock(&data->lock);
> >>>>
> >>>>        return IRQ_HANDLED;
> >>>>    }
> >>>> @@ -987,7 +981,7 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
> >>>>    {
> >>>>        struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
> >>>>
> >>>> -     mutex_lock(&data->lock);
> >>>> +     guard(mutex)(&data->lock);
> >>>>        clk_enable(data->clk);
> >>>>
> >>>>        if (low > INT_MIN)
> >>>> @@ -1000,7 +994,6 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
> >>>>                data->tmu_disable_high(data);
> >>>>
> >>>>        clk_disable(data->clk);
> >>>> -     mutex_unlock(&data->lock);
> >>>>
> >>>>        return 0;
> >>>>    }
> >>
> >> Thanks for your review comments.
> >>>
> >>> IMO you should be able to even use something like we have
> >>> core framework:
> >>>
> >>> guard(thermal_zone)(tz);
> >>>
> >>> Your mutex name is simply 'lock' in the struct exynos_tmu_data
> >>> so you should be able to leverage this by:
> >>>
> >>> guard(exynos_tmu_data)(data);
> >>>
> >
> > If I introduce the guard it creates a compilation error
> >
> > amoon@anand-m920q:~/mainline/linux-exynos-6.y-devel$ vi
> > drivers/thermal/samsung/exynos_tmu.c +306
> > amoon@anand-m920q:~/mainline/linux-exynos-6.y-devel$ make -j$(nproc)
> > ARCH=arm CROSS_COMPILE=arm-none-eabi- LOCALVERSION=-u3ml dtbs zImage
> > modules
> >    CALL    scripts/checksyscalls.sh
> >    CHK     kernel/kheaders_data.tar.xz
> >    CC      drivers/thermal/samsung/exynos_tmu.o
> >    CC [M]  drivers/md/raid10.o
> > In file included from ./include/linux/irqflags.h:17,
> >                   from ./arch/arm/include/asm/bitops.h:28,
> >                   from ./include/linux/bitops.h:68,
> >                   from ./include/linux/kernel.h:23,
> >                   from ./include/linux/clk.h:13,
> >                   from drivers/thermal/samsung/exynos_tmu.c:14:
> > drivers/thermal/samsung/exynos_tmu.c: In function 'exynos_tmu_update_bit':
> > ./include/linux/cleanup.h:258:9: error: unknown type name
> > 'class_exynos_tmu_data_t'
> >    258 |         class_##_name##_t var
> > __cleanup(class_##_name##_destructor) =   \
> >        |         ^~~~~~
> > ./include/linux/cleanup.h:309:9: note: in expansion of macro 'CLASS'
> >    309 |         CLASS(_name, __UNIQUE_ID(guard))
> >        |         ^~~~~
> > drivers/thermal/samsung/exynos_tmu.c:338:9: note: in expansion of macro 'guard'
> >    338 |         guard(exynos_tmu_data)(data);
> >        |         ^~~~~
> > drivers/thermal/samsung/exynos_tmu.c:338:9: error: cleanup argument
> > not a function
>
> [snip]
>
> Right, you're missing the definition at the begging, like:
>
> DEFINE_GUARD(exynos_tmu_data, struct exynos_tmu_data *,
> mutex_lock(&_T->lock),
>               mutex_unlock(&_T->lock))
>
> below the struct exynos_tmu_data definition.
>
> Also, make sure you include the cleanup.h (it might not complain,
> but it would be explicit and more clear)

Thanks for this tip.
However, incorporating guard(exynos_tmu_data)(data); results
in a recursive deadlock with the mutex during initialization, as this
data structure is common to all the code configurations of Exynos TMU

Thanks
-Anand

Re: [PATCH v3 4/4] drivers/thermal/exymos: Use guard notation when acquiring mutex

[Lukasz Luba]

On 3/5/25 15:59, Anand Moon wrote:
> Hi Lukasz,
> 
> On Wed, 5 Mar 2025 at 14:12, Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>
>>
>> On 3/4/25 12:20, Anand Moon wrote:
>>> Hi Lukasz,
>>>
>>> On Sat, 1 Mar 2025 at 00:06, Anand Moon <linux.amoon@gmail.com> wrote:
>>>>
>>>> Hi Lukasz,
>>>>
>>>> On Fri, 28 Feb 2025 at 22:58, Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2/16/25 19:58, Anand Moon wrote:
>>>>>> Using guard notation makes the code more compact and error handling
>>>>>> more robust by ensuring that mutexes are released in all code paths
>>>>>> when control leaves critical section.
>>>>>>
>>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>>> ---
>>>>>> v3: new patch
>>>>>> ---
>>>>>>     drivers/thermal/samsung/exynos_tmu.c | 21 +++++++--------------
>>>>>>     1 file changed, 7 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>>>>> index fe090c1a93ab..a34ba3858d64 100644
>>>>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>>>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>>>>> @@ -256,7 +256,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>>>         unsigned int status;
>>>>>>         int ret = 0;
>>>>>>
>>>>>> -     mutex_lock(&data->lock);
>>>>>> +     guard(mutex)(&data->lock);
>>>>>>         clk_enable(data->clk);
>>>>>>         clk_enable(data->clk_sec);
>>>>>>
>>>>>> @@ -270,7 +270,6 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>>>
>>>>>>         clk_disable(data->clk_sec);
>>>>>>         clk_disable(data->clk);
>>>>>> -     mutex_unlock(&data->lock);
>>>>>>
>>>>>>         return ret;
>>>>>>     }
>>>>>> @@ -292,13 +291,12 @@ static int exynos_thermal_zone_configure(struct platform_device *pdev)
>>>>>>                 return ret;
>>>>>>         }
>>>>>>
>>>>>> -     mutex_lock(&data->lock);
>>>>>> +     guard(mutex)(&data->lock);
>>>>>>         clk_enable(data->clk);
>>>>>>
>>>>>>         data->tmu_set_crit_temp(data, temp / MCELSIUS);
>>>>>>
>>>>>>         clk_disable(data->clk);
>>>>>> -     mutex_unlock(&data->lock);
>>>>>>
>>>>>>         return 0;
>>>>>>     }
>>>>>> @@ -325,12 +323,11 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
>>>>>>     {
>>>>>>         struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>>>>>>
>>>>>> -     mutex_lock(&data->lock);
>>>>>> +     guard(mutex)(&data->lock);
>>>>>>         clk_enable(data->clk);
>>>>>>         data->tmu_control(pdev, on);
>>>>>>         data->enabled = on;
>>>>>>         clk_disable(data->clk);
>>>>>> -     mutex_unlock(&data->lock);
>>>>>>     }
>>>>>>
>>>>>>     static void exynos_tmu_update_bit(struct exynos_tmu_data *data, int reg_off,
>>>>>> @@ -645,7 +642,7 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
>>>>>>                  */
>>>>>>                 return -EAGAIN;
>>>>>>
>>>>>> -     mutex_lock(&data->lock);
>>>>>> +     guard(mutex)(&data->lock);
>>>>>>         clk_enable(data->clk);
>>>>>>
>>>>>>         value = data->tmu_read(data);
>>>>>> @@ -655,7 +652,6 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
>>>>>>                 *temp = code_to_temp(data, value) * MCELSIUS;
>>>>>>
>>>>>>         clk_disable(data->clk);
>>>>>> -     mutex_unlock(&data->lock);
>>>>>>
>>>>>>         return ret;
>>>>>>     }
>>>>>> @@ -720,11 +716,10 @@ static int exynos_tmu_set_emulation(struct thermal_zone_device *tz, int temp)
>>>>>>         if (temp && temp < MCELSIUS)
>>>>>>                 goto out;
>>>>>>
>>>>>> -     mutex_lock(&data->lock);
>>>>>> +     guard(mutex)(&data->lock);
>>>>>>         clk_enable(data->clk);
>>>>>>         data->tmu_set_emulation(data, temp);
>>>>>>         clk_disable(data->clk);
>>>>>> -     mutex_unlock(&data->lock);
>>>>>>         return 0;
>>>>>>     out:
>>>>>>         return ret;
>>>>>> @@ -760,14 +755,13 @@ static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
>>>>>>
>>>>>>         thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
>>>>>>
>>>>>> -     mutex_lock(&data->lock);
>>>>>> +     guard(mutex)(&data->lock);
>>>>>>         clk_enable(data->clk);
>>>>>>
>>>>>>         /* TODO: take action based on particular interrupt */
>>>>>>         data->tmu_clear_irqs(data);
>>>>>>
>>>>>>         clk_disable(data->clk);
>>>>>> -     mutex_unlock(&data->lock);
>>>>>>
>>>>>>         return IRQ_HANDLED;
>>>>>>     }
>>>>>> @@ -987,7 +981,7 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
>>>>>>     {
>>>>>>         struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
>>>>>>
>>>>>> -     mutex_lock(&data->lock);
>>>>>> +     guard(mutex)(&data->lock);
>>>>>>         clk_enable(data->clk);
>>>>>>
>>>>>>         if (low > INT_MIN)
>>>>>> @@ -1000,7 +994,6 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
>>>>>>                 data->tmu_disable_high(data);
>>>>>>
>>>>>>         clk_disable(data->clk);
>>>>>> -     mutex_unlock(&data->lock);
>>>>>>
>>>>>>         return 0;
>>>>>>     }
>>>>
>>>> Thanks for your review comments.
>>>>>
>>>>> IMO you should be able to even use something like we have
>>>>> core framework:
>>>>>
>>>>> guard(thermal_zone)(tz);
>>>>>
>>>>> Your mutex name is simply 'lock' in the struct exynos_tmu_data
>>>>> so you should be able to leverage this by:
>>>>>
>>>>> guard(exynos_tmu_data)(data);
>>>>>
>>>
>>> If I introduce the guard it creates a compilation error
>>>
>>> amoon@anand-m920q:~/mainline/linux-exynos-6.y-devel$ vi
>>> drivers/thermal/samsung/exynos_tmu.c +306
>>> amoon@anand-m920q:~/mainline/linux-exynos-6.y-devel$ make -j$(nproc)
>>> ARCH=arm CROSS_COMPILE=arm-none-eabi- LOCALVERSION=-u3ml dtbs zImage
>>> modules
>>>     CALL    scripts/checksyscalls.sh
>>>     CHK     kernel/kheaders_data.tar.xz
>>>     CC      drivers/thermal/samsung/exynos_tmu.o
>>>     CC [M]  drivers/md/raid10.o
>>> In file included from ./include/linux/irqflags.h:17,
>>>                    from ./arch/arm/include/asm/bitops.h:28,
>>>                    from ./include/linux/bitops.h:68,
>>>                    from ./include/linux/kernel.h:23,
>>>                    from ./include/linux/clk.h:13,
>>>                    from drivers/thermal/samsung/exynos_tmu.c:14:
>>> drivers/thermal/samsung/exynos_tmu.c: In function 'exynos_tmu_update_bit':
>>> ./include/linux/cleanup.h:258:9: error: unknown type name
>>> 'class_exynos_tmu_data_t'
>>>     258 |         class_##_name##_t var
>>> __cleanup(class_##_name##_destructor) =   \
>>>         |         ^~~~~~
>>> ./include/linux/cleanup.h:309:9: note: in expansion of macro 'CLASS'
>>>     309 |         CLASS(_name, __UNIQUE_ID(guard))
>>>         |         ^~~~~
>>> drivers/thermal/samsung/exynos_tmu.c:338:9: note: in expansion of macro 'guard'
>>>     338 |         guard(exynos_tmu_data)(data);
>>>         |         ^~~~~
>>> drivers/thermal/samsung/exynos_tmu.c:338:9: error: cleanup argument
>>> not a function
>>
>> [snip]
>>
>> Right, you're missing the definition at the begging, like:
>>
>> DEFINE_GUARD(exynos_tmu_data, struct exynos_tmu_data *,
>> mutex_lock(&_T->lock),
>>                mutex_unlock(&_T->lock))
>>
>> below the struct exynos_tmu_data definition.
>>
>> Also, make sure you include the cleanup.h (it might not complain,
>> but it would be explicit and more clear)
> 
> Thanks for this tip.
> However, incorporating guard(exynos_tmu_data)(data); results
> in a recursive deadlock with the mutex during initialization, as this
> data structure is common to all the code configurations of Exynos TMU
> 

Fair enough, it would be just a cosmetic change, so do fight with it
too much.

Please continue in v4 with your former approach with the 'guard'.