On Wed, Sep 04, 2024 at 08:41:30PM +0200, Javier Carrasco wrote:

On 04/09/2024 20:21, Dmitry Torokhov wrote:

quoted

Hi Javier,

On Wed, Sep 04, 2024 at 03:53:40PM +0200, Javier Carrasco wrote:

quoted

On 04/09/2024 06:47, Dmitry Torokhov wrote:

quoted

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

...

quoted

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

quoted

quoted

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

quoted

 
+	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