| Message ID | 20240904044756.1047629-1-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Convert misc input drivers to use new cleanup facilities | expand |
On 04/09/2024 06:47, Dmitry Torokhov 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: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/misc/iqs269a.c | 46 +++++++++++++----------------------- > 1 file changed, 16 insertions(+), 30 deletions(-) > > diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c > index 843f8a3f3410..c34d847fa4af 100644 > --- a/drivers/input/misc/iqs269a.c > +++ b/drivers/input/misc/iqs269a.c ... > @@ -453,9 +449,9 @@ static int iqs269_ati_base_get(struct iqs269_private *iqs269, > if (ch_num >= IQS269_NUM_CH) > return -EINVAL; > > - mutex_lock(&iqs269->lock); > + guard(mutex)(&iqs269->lock); > + > engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); maybe scoped_guard() to keep the scope of the mutex as it used to be? > - mutex_unlock(&iqs269->lock); > > switch (engine_b & IQS269_CHx_ENG_B_ATI_BASE_MASK) { > case IQS269_CHx_ENG_B_ATI_BASE_75: > @@ -491,7 +487,7 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269, > if (target > IQS269_CHx_ENG_B_ATI_TARGET_MAX) > return -EINVAL; > > - mutex_lock(&iqs269->lock); > + guard(mutex)(&iqs269->lock); > > engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > > @@ -501,8 +497,6 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269, > ch_reg[ch_num].engine_b = cpu_to_be16(engine_b); > iqs269->ati_current = false; > > - mutex_unlock(&iqs269->lock); > - > return 0; > } > > @@ -515,10 +509,9 @@ static int iqs269_ati_target_get(struct iqs269_private *iqs269, > if (ch_num >= IQS269_NUM_CH) > return -EINVAL; > > - mutex_lock(&iqs269->lock); > - engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > - mutex_unlock(&iqs269->lock); > + guard(mutex)(&iqs269->lock); same here? > > + engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > *target = (engine_b & IQS269_CHx_ENG_B_ATI_TARGET_MASK) * 32; > > return 0; Best regards, Javier Carrasco
Hi Javier, On Wed, Sep 04, 2024 at 03:53:40PM +0200, Javier Carrasco wrote: > On 04/09/2024 06:47, Dmitry Torokhov 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: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/input/misc/iqs269a.c | 46 +++++++++++++----------------------- > > 1 file changed, 16 insertions(+), 30 deletions(-) > > > > diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c > > index 843f8a3f3410..c34d847fa4af 100644 > > --- a/drivers/input/misc/iqs269a.c > > +++ b/drivers/input/misc/iqs269a.c > > ... > > > @@ -453,9 +449,9 @@ static int iqs269_ati_base_get(struct iqs269_private *iqs269, > > if (ch_num >= IQS269_NUM_CH) > > return -EINVAL; > > > > - mutex_lock(&iqs269->lock); > > + guard(mutex)(&iqs269->lock); > > + > > engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > > maybe scoped_guard() to keep the scope of the mutex as it used to be? Thank you for looking over patches. It is just a few computations extra, so I decided not to use scoped_guard(). Note that original code was forced to release mutex early to avoid having to unlock it in all switch branches. > > > - mutex_unlock(&iqs269->lock); > > > > switch (engine_b & IQS269_CHx_ENG_B_ATI_BASE_MASK) { > > case IQS269_CHx_ENG_B_ATI_BASE_75: > > @@ -491,7 +487,7 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269, > > if (target > IQS269_CHx_ENG_B_ATI_TARGET_MAX) > > return -EINVAL; > > > > - mutex_lock(&iqs269->lock); > > + guard(mutex)(&iqs269->lock); > > > > engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > > > > @@ -501,8 +497,6 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269, > > ch_reg[ch_num].engine_b = cpu_to_be16(engine_b); > > iqs269->ati_current = false; > > > > - mutex_unlock(&iqs269->lock); > > - > > return 0; > > } > > > > @@ -515,10 +509,9 @@ static int iqs269_ati_target_get(struct iqs269_private *iqs269, > > if (ch_num >= IQS269_NUM_CH) > > return -EINVAL; > > > > - mutex_lock(&iqs269->lock); > > - engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > > - mutex_unlock(&iqs269->lock); > > + guard(mutex)(&iqs269->lock); > > same here? > > > > > + engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > > *target = (engine_b & IQS269_CHx_ENG_B_ATI_TARGET_MASK) * 32; Same here, calculating the line above will take no time at all... Thanks.
On 04/09/2024 20:21, Dmitry Torokhov wrote: > Hi Javier, > > On Wed, Sep 04, 2024 at 03:53:40PM +0200, Javier Carrasco wrote: >> On 04/09/2024 06:47, Dmitry Torokhov 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: Dmitry Torokhov <dmitry.torokhov@gmail.com> >>> --- >>> drivers/input/misc/iqs269a.c | 46 +++++++++++++----------------------- >>> 1 file changed, 16 insertions(+), 30 deletions(-) >>> >>> diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c >>> index 843f8a3f3410..c34d847fa4af 100644 >>> --- a/drivers/input/misc/iqs269a.c >>> +++ b/drivers/input/misc/iqs269a.c >> >> ... >> >>> @@ -453,9 +449,9 @@ static int iqs269_ati_base_get(struct iqs269_private *iqs269, >>> if (ch_num >= IQS269_NUM_CH) >>> return -EINVAL; >>> >>> - mutex_lock(&iqs269->lock); >>> + guard(mutex)(&iqs269->lock); >>> + >>> engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); >> >> maybe scoped_guard() to keep the scope of the mutex as it used to be? > > Thank you for looking over patches. > > It is just a few computations extra, so I decided not to use > scoped_guard(). Note that original code was forced to release mutex > early to avoid having to unlock it in all switch branches. > >> >>> - mutex_unlock(&iqs269->lock); >>> >>> switch (engine_b & IQS269_CHx_ENG_B_ATI_BASE_MASK) { >>> case IQS269_CHx_ENG_B_ATI_BASE_75: >>> @@ -491,7 +487,7 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269, >>> if (target > IQS269_CHx_ENG_B_ATI_TARGET_MAX) >>> return -EINVAL; >>> >>> - mutex_lock(&iqs269->lock); >>> + guard(mutex)(&iqs269->lock); >>> >>> engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); >>> >>> @@ -501,8 +497,6 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269, >>> ch_reg[ch_num].engine_b = cpu_to_be16(engine_b); >>> iqs269->ati_current = false; >>> >>> - mutex_unlock(&iqs269->lock); >>> - >>> return 0; >>> } >>> >>> @@ -515,10 +509,9 @@ static int iqs269_ati_target_get(struct iqs269_private *iqs269, >>> if (ch_num >= IQS269_NUM_CH) >>> return -EINVAL; >>> >>> - mutex_lock(&iqs269->lock); >>> - engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); >>> - mutex_unlock(&iqs269->lock); >>> + guard(mutex)(&iqs269->lock); >> >> same here? >> >>> >>> + engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); >>> *target = (engine_b & IQS269_CHx_ENG_B_ATI_TARGET_MASK) * 32; > > Same here, calculating the line above will take no time at all... > > Thanks. > As you pointed out, in reality the extra locked instructions will not make any difference. But as the conversion added instructions to be locked by the mutex without mentioning it, I thought it should be either left as it used to be with scoped_guard(), or explicitly mentioned in the description. No strong feelings against it, but out of curiosity, why would you rather use guard()? I think scoped_guard() is a better way to self-document what has to be accessed via mutex, and what not. Best regards, Javier Carrasco
On Wed, Sep 04, 2024 at 08:41:30PM +0200, Javier Carrasco wrote: > On 04/09/2024 20:21, Dmitry Torokhov wrote: > > Hi Javier, > > > > On Wed, Sep 04, 2024 at 03:53:40PM +0200, Javier Carrasco wrote: > >> On 04/09/2024 06:47, Dmitry Torokhov 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: Dmitry Torokhov <dmitry.torokhov@gmail.com> > >>> --- > >>> drivers/input/misc/iqs269a.c | 46 +++++++++++++----------------------- > >>> 1 file changed, 16 insertions(+), 30 deletions(-) > >>> > >>> diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c > >>> index 843f8a3f3410..c34d847fa4af 100644 > >>> --- a/drivers/input/misc/iqs269a.c > >>> +++ b/drivers/input/misc/iqs269a.c > >> > >> ... > >> > >>> @@ -453,9 +449,9 @@ static int iqs269_ati_base_get(struct iqs269_private *iqs269, > >>> if (ch_num >= IQS269_NUM_CH) > >>> return -EINVAL; > >>> > >>> - mutex_lock(&iqs269->lock); > >>> + guard(mutex)(&iqs269->lock); > >>> + > >>> engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > >> > >> maybe scoped_guard() to keep the scope of the mutex as it used to be? > > > > Thank you for looking over patches. > > > > It is just a few computations extra, so I decided not to use > > scoped_guard(). Note that original code was forced to release mutex > > early to avoid having to unlock it in all switch branches. > > > >> > >>> - mutex_unlock(&iqs269->lock); > >>> > >>> switch (engine_b & IQS269_CHx_ENG_B_ATI_BASE_MASK) { > >>> case IQS269_CHx_ENG_B_ATI_BASE_75: > >>> @@ -491,7 +487,7 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269, > >>> if (target > IQS269_CHx_ENG_B_ATI_TARGET_MAX) > >>> return -EINVAL; > >>> > >>> - mutex_lock(&iqs269->lock); > >>> + guard(mutex)(&iqs269->lock); > >>> > >>> engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > >>> > >>> @@ -501,8 +497,6 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269, > >>> ch_reg[ch_num].engine_b = cpu_to_be16(engine_b); > >>> iqs269->ati_current = false; > >>> > >>> - mutex_unlock(&iqs269->lock); > >>> - > >>> return 0; > >>> } > >>> > >>> @@ -515,10 +509,9 @@ static int iqs269_ati_target_get(struct iqs269_private *iqs269, > >>> if (ch_num >= IQS269_NUM_CH) > >>> return -EINVAL; > >>> > >>> - mutex_lock(&iqs269->lock); > >>> - engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > >>> - mutex_unlock(&iqs269->lock); > >>> + guard(mutex)(&iqs269->lock); > >> > >> same here? > >> > >>> > >>> + engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > >>> *target = (engine_b & IQS269_CHx_ENG_B_ATI_TARGET_MASK) * 32; > > > > Same here, calculating the line above will take no time at all... > > > > Thanks. > > > > As you pointed out, in reality the extra locked instructions will not > make any difference. But as the conversion added instructions to be > locked by the mutex without mentioning it, I thought it should be either > left as it used to be with scoped_guard(), or explicitly mentioned in > the description. > > No strong feelings against it, but out of curiosity, why would you > rather use guard()? I think scoped_guard() is a better way to > self-document what has to be accessed via mutex, and what not. Simply less indentation ;) and in this driver uniformity with for example iqs269_ati_target_set() where critical section does indeed extend to the whole function. Not super strong arguments either.
Hi Dmitry, On Tue, Sep 03, 2024 at 09:47:55PM -0700, Dmitry Torokhov 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: Dmitry Torokhov <dmitry.torokhov@gmail.com> Reviewed-by: Jeff LaBundy <jeff@labundy.com> > --- > drivers/input/misc/iqs269a.c | 46 +++++++++++++----------------------- > 1 file changed, 16 insertions(+), 30 deletions(-) > > diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c > index 843f8a3f3410..c34d847fa4af 100644 > --- a/drivers/input/misc/iqs269a.c > +++ b/drivers/input/misc/iqs269a.c > @@ -365,7 +365,7 @@ static int iqs269_ati_mode_set(struct iqs269_private *iqs269, > if (mode > IQS269_CHx_ENG_A_ATI_MODE_MAX) > return -EINVAL; > > - mutex_lock(&iqs269->lock); > + guard(mutex)(&iqs269->lock); > > engine_a = be16_to_cpu(ch_reg[ch_num].engine_a); > > @@ -375,8 +375,6 @@ static int iqs269_ati_mode_set(struct iqs269_private *iqs269, > ch_reg[ch_num].engine_a = cpu_to_be16(engine_a); > iqs269->ati_current = false; > > - mutex_unlock(&iqs269->lock); > - > return 0; > } > > @@ -389,9 +387,9 @@ static int iqs269_ati_mode_get(struct iqs269_private *iqs269, > if (ch_num >= IQS269_NUM_CH) > return -EINVAL; > > - mutex_lock(&iqs269->lock); > + guard(mutex)(&iqs269->lock); > + > engine_a = be16_to_cpu(ch_reg[ch_num].engine_a); > - mutex_unlock(&iqs269->lock); > > engine_a &= IQS269_CHx_ENG_A_ATI_MODE_MASK; > *mode = (engine_a >> IQS269_CHx_ENG_A_ATI_MODE_SHIFT); > @@ -429,7 +427,7 @@ static int iqs269_ati_base_set(struct iqs269_private *iqs269, > return -EINVAL; > } > > - mutex_lock(&iqs269->lock); > + guard(mutex)(&iqs269->lock); > > engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > > @@ -439,8 +437,6 @@ static int iqs269_ati_base_set(struct iqs269_private *iqs269, > ch_reg[ch_num].engine_b = cpu_to_be16(engine_b); > iqs269->ati_current = false; > > - mutex_unlock(&iqs269->lock); > - > return 0; > } > > @@ -453,9 +449,9 @@ static int iqs269_ati_base_get(struct iqs269_private *iqs269, > if (ch_num >= IQS269_NUM_CH) > return -EINVAL; > > - mutex_lock(&iqs269->lock); > + guard(mutex)(&iqs269->lock); > + > engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > - mutex_unlock(&iqs269->lock); > > switch (engine_b & IQS269_CHx_ENG_B_ATI_BASE_MASK) { > case IQS269_CHx_ENG_B_ATI_BASE_75: > @@ -491,7 +487,7 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269, > if (target > IQS269_CHx_ENG_B_ATI_TARGET_MAX) > return -EINVAL; > > - mutex_lock(&iqs269->lock); > + guard(mutex)(&iqs269->lock); > > engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > > @@ -501,8 +497,6 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269, > ch_reg[ch_num].engine_b = cpu_to_be16(engine_b); > iqs269->ati_current = false; > > - mutex_unlock(&iqs269->lock); > - > return 0; > } > > @@ -515,10 +509,9 @@ static int iqs269_ati_target_get(struct iqs269_private *iqs269, > if (ch_num >= IQS269_NUM_CH) > return -EINVAL; > > - mutex_lock(&iqs269->lock); > - engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > - mutex_unlock(&iqs269->lock); > + guard(mutex)(&iqs269->lock); > > + engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); > *target = (engine_b & IQS269_CHx_ENG_B_ATI_TARGET_MASK) * 32; > > return 0; > @@ -1199,7 +1192,7 @@ static int iqs269_dev_init(struct iqs269_private *iqs269) > { > int error; > > - mutex_lock(&iqs269->lock); > + guard(mutex)(&iqs269->lock); > > /* > * Early revisions of silicon require the following workaround in order > @@ -1210,19 +1203,19 @@ static int iqs269_dev_init(struct iqs269_private *iqs269) > error = regmap_multi_reg_write(iqs269->regmap, iqs269_tws_init, > ARRAY_SIZE(iqs269_tws_init)); > if (error) > - goto err_mutex; > + return error; > } > > error = regmap_update_bits(iqs269->regmap, IQS269_HALL_UI, > IQS269_HALL_UI_ENABLE, > iqs269->hall_enable ? ~0 : 0); > if (error) > - goto err_mutex; > + return error; > > error = regmap_raw_write(iqs269->regmap, IQS269_SYS_SETTINGS, > &iqs269->sys_reg, sizeof(iqs269->sys_reg)); > if (error) > - goto err_mutex; > + return error; > > /* > * The following delay gives the device time to deassert its RDY output > @@ -1232,10 +1225,7 @@ static int iqs269_dev_init(struct iqs269_private *iqs269) > > iqs269->ati_current = true; > > -err_mutex: > - mutex_unlock(&iqs269->lock); > - > - return error; > + return 0; > } > > static int iqs269_input_init(struct iqs269_private *iqs269) > @@ -1580,13 +1570,11 @@ static ssize_t hall_enable_store(struct device *dev, > if (error) > return error; > > - mutex_lock(&iqs269->lock); > + guard(mutex)(&iqs269->lock); > > iqs269->hall_enable = val; > iqs269->ati_current = false; > > - mutex_unlock(&iqs269->lock); > - > return count; > } > > @@ -1643,13 +1631,11 @@ static ssize_t rx_enable_store(struct device *dev, > if (val > 0xFF) > return -EINVAL; > > - mutex_lock(&iqs269->lock); > + guard(mutex)(&iqs269->lock); > > ch_reg[iqs269->ch_num].rx_enable = val; > iqs269->ati_current = false; > > - mutex_unlock(&iqs269->lock); > - > return count; > } > > -- > 2.46.0.469.g59c65b2a67-goog > Kind regards, Jeff LaBundy
diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c index 843f8a3f3410..c34d847fa4af 100644 --- a/drivers/input/misc/iqs269a.c +++ b/drivers/input/misc/iqs269a.c @@ -365,7 +365,7 @@ static int iqs269_ati_mode_set(struct iqs269_private *iqs269, if (mode > IQS269_CHx_ENG_A_ATI_MODE_MAX) return -EINVAL; - mutex_lock(&iqs269->lock); + guard(mutex)(&iqs269->lock); engine_a = be16_to_cpu(ch_reg[ch_num].engine_a); @@ -375,8 +375,6 @@ static int iqs269_ati_mode_set(struct iqs269_private *iqs269, ch_reg[ch_num].engine_a = cpu_to_be16(engine_a); iqs269->ati_current = false; - mutex_unlock(&iqs269->lock); - return 0; } @@ -389,9 +387,9 @@ static int iqs269_ati_mode_get(struct iqs269_private *iqs269, if (ch_num >= IQS269_NUM_CH) return -EINVAL; - mutex_lock(&iqs269->lock); + guard(mutex)(&iqs269->lock); + engine_a = be16_to_cpu(ch_reg[ch_num].engine_a); - mutex_unlock(&iqs269->lock); engine_a &= IQS269_CHx_ENG_A_ATI_MODE_MASK; *mode = (engine_a >> IQS269_CHx_ENG_A_ATI_MODE_SHIFT); @@ -429,7 +427,7 @@ static int iqs269_ati_base_set(struct iqs269_private *iqs269, return -EINVAL; } - mutex_lock(&iqs269->lock); + guard(mutex)(&iqs269->lock); engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); @@ -439,8 +437,6 @@ static int iqs269_ati_base_set(struct iqs269_private *iqs269, ch_reg[ch_num].engine_b = cpu_to_be16(engine_b); iqs269->ati_current = false; - mutex_unlock(&iqs269->lock); - return 0; } @@ -453,9 +449,9 @@ static int iqs269_ati_base_get(struct iqs269_private *iqs269, if (ch_num >= IQS269_NUM_CH) return -EINVAL; - mutex_lock(&iqs269->lock); + guard(mutex)(&iqs269->lock); + engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); - mutex_unlock(&iqs269->lock); switch (engine_b & IQS269_CHx_ENG_B_ATI_BASE_MASK) { case IQS269_CHx_ENG_B_ATI_BASE_75: @@ -491,7 +487,7 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269, if (target > IQS269_CHx_ENG_B_ATI_TARGET_MAX) return -EINVAL; - mutex_lock(&iqs269->lock); + guard(mutex)(&iqs269->lock); engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); @@ -501,8 +497,6 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269, ch_reg[ch_num].engine_b = cpu_to_be16(engine_b); iqs269->ati_current = false; - mutex_unlock(&iqs269->lock); - return 0; } @@ -515,10 +509,9 @@ static int iqs269_ati_target_get(struct iqs269_private *iqs269, if (ch_num >= IQS269_NUM_CH) return -EINVAL; - mutex_lock(&iqs269->lock); - engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); - mutex_unlock(&iqs269->lock); + guard(mutex)(&iqs269->lock); + engine_b = be16_to_cpu(ch_reg[ch_num].engine_b); *target = (engine_b & IQS269_CHx_ENG_B_ATI_TARGET_MASK) * 32; return 0; @@ -1199,7 +1192,7 @@ static int iqs269_dev_init(struct iqs269_private *iqs269) { int error; - mutex_lock(&iqs269->lock); + guard(mutex)(&iqs269->lock); /* * Early revisions of silicon require the following workaround in order @@ -1210,19 +1203,19 @@ static int iqs269_dev_init(struct iqs269_private *iqs269) error = regmap_multi_reg_write(iqs269->regmap, iqs269_tws_init, ARRAY_SIZE(iqs269_tws_init)); if (error) - goto err_mutex; + return error; } error = regmap_update_bits(iqs269->regmap, IQS269_HALL_UI, IQS269_HALL_UI_ENABLE, iqs269->hall_enable ? ~0 : 0); if (error) - goto err_mutex; + return error; error = regmap_raw_write(iqs269->regmap, IQS269_SYS_SETTINGS, &iqs269->sys_reg, sizeof(iqs269->sys_reg)); if (error) - goto err_mutex; + return error; /* * The following delay gives the device time to deassert its RDY output @@ -1232,10 +1225,7 @@ static int iqs269_dev_init(struct iqs269_private *iqs269) iqs269->ati_current = true; -err_mutex: - mutex_unlock(&iqs269->lock); - - return error; + return 0; } static int iqs269_input_init(struct iqs269_private *iqs269) @@ -1580,13 +1570,11 @@ static ssize_t hall_enable_store(struct device *dev, if (error) return error; - mutex_lock(&iqs269->lock); + guard(mutex)(&iqs269->lock); iqs269->hall_enable = val; iqs269->ati_current = false; - mutex_unlock(&iqs269->lock); - return count; } @@ -1643,13 +1631,11 @@ static ssize_t rx_enable_store(struct device *dev, if (val > 0xFF) return -EINVAL; - mutex_lock(&iqs269->lock); + guard(mutex)(&iqs269->lock); ch_reg[iqs269->ch_num].rx_enable = val; iqs269->ati_current = false; - mutex_unlock(&iqs269->lock); - return count; }
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: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/misc/iqs269a.c | 46 +++++++++++++----------------------- 1 file changed, 16 insertions(+), 30 deletions(-)