| 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 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
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)
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(-)