Convert misc input drivers to use new cleanup facilities

[PATCH 12/22] Input: iqs269a - use guard notation when acquiring mutex

Posted by Dmitry Torokhov 1 year, 3 months ago

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
@@ -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

Re: [PATCH 12/22] Input: iqs269a - use guard notation when acquiring mutex

Posted by Jeff LaBundy 1 year, 3 months ago

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

Re: [PATCH 12/22] Input: iqs269a - use guard notation when acquiring mutex

Posted by Javier Carrasco 1 year, 3 months ago

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

Re: [PATCH 12/22] Input: iqs269a - use guard notation when acquiring mutex

Posted by Dmitry Torokhov 1 year, 3 months ago

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.

-- 
Dmitry

Re: [PATCH 12/22] Input: iqs269a - use guard notation when acquiring mutex

Posted by Javier Carrasco 1 year, 3 months ago

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

Re: [PATCH 12/22] Input: iqs269a - use guard notation when acquiring mutex

Posted by Dmitry Torokhov 1 year, 3 months ago

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.

-- 
Dmitry