| Message ID | 20250216195850.5352-5-linux.amoon@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Exynos Thermal code improvement | expand |
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.
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
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)
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'.
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; }
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(-)