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

[Anand Moon]

Hi Krzysztof,

Thanks for your review comments.

On Tue, 11 Mar 2025 at 23:00, Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On 10/03/2025 15:34, 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.
> >
>
> Subject: typo, exynos
Ok.
>
> > Signed-off-by: Anand Moon <linux.amoon@xxxxxxxxx>
> > ---
> > v4: used DEFINE_GUARD macro to guard exynos_tmu_data structure.
> >     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
> > v3: New patch
>
> If you ever use cleanup or guards, you must build your code with recent
> clang and W=1. Failure to do so means you ask reviewers manually to spot
> issues not visible in the context, instead of using tools. It's a NAK
> for me.
 Ok, I will check this next time before submitting the changes.
>
> > ---
> >  drivers/thermal/samsung/exynos_tmu.c | 25 +++++++++++--------------
> >  1 file changed, 11 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> > index a71cde0a4b17e..85f88c5e0f11c 100644
> > --- a/drivers/thermal/samsung/exynos_tmu.c
> > +++ b/drivers/thermal/samsung/exynos_tmu.c
> > @@ -12,6 +12,7 @@
> >   */
> >
> >  #include <linux/clk.h>
> > +#include <linux/cleanup.h>
> >  #include <linux/io.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/module.h>
> > @@ -199,6 +200,9 @@ struct exynos_tmu_data {
> >       void (*tmu_clear_irqs)(struct exynos_tmu_data *data);
> >  };
> >
> > +DEFINE_GUARD(exynos_tmu_data, struct exynos_tmu_data *,
>
> I do not understand why do you need custom guard.

I thought this should add a global guard to exynos_tmu_data using
mutex_lock and mutex_unlock.
I'll drop this if it turns out to be unnecessary.
>
> > +          mutex_lock(&_T->lock), mutex_unlock(&_T->lock))
> > +
> >  /*
> >   * TMU treats temperature as a mapped temperature code.
> >   * The temperature is converted differently depending on the calibration type.
> > @@ -256,7 +260,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> >       unsigned int status;
> >       int ret = 0;
> >
> > -     mutex_lock(&data->lock);
> > +     guard(mutex)(&data->lock);
>
> Which you do not use... Please don't use cleanup.h if you do not know
> it. It leads to bugs.
>
Ok, I will drop this include of cleanup.h.
>
> Best regards,
> Krzysztof

Thanks
-Anand