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

[Lukasz Luba <lukasz.luba@arm.com>]

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)