[PATCH 00/22] Convert misc input drivers to use new cleanup facilities

[PATCH 00/22] Convert misc input drivers to use new cleanup facilities

[Dmitry Torokhov]

Hi,

This series converts drivers found in drivers/input/misc to use new
__free() and guard() cleanup facilities that simplify the code and
ensure that all resources are released appropriately.

Thanks!

Dmitry Torokhov (22):
  Input: ad714x - use guard notation when acquiring mutex
  Input: ati_remote2 - use guard notation when acquiring mutex
  Input: cm109 - use guard notation when acquiring mutex and spinlock
  Input: cma3000_d0x - use guard notation when acquiring mutex
  Input: da7280 - use guard notation when acquiring mutex and spinlock
  Input: kxtj9 - use guard notation when acquiring mutex/disabling irq
  Input: drv260x - use guard notation when acquiring mutex
  Input: drv2665 - use guard notation when acquiring mutex
  Input: drv2667 - use guard notation when acquiring mutex
  Input: ideapad_slidebar - use guard notation when acquiring spinlock
  Input: ibm-panel - use guard notation when acquiring spinlock
  Input: iqs269a - use guard notation when acquiring mutex
  Input: iqs269a - use cleanup facility for fwnodes
  Input: iqs626a - use cleanup facility for fwnodes
  Input: iqs7222 - use cleanup facility for fwnodes
  Input: max8997_haptic - use guard notation when acquiring mutex
  Input: pegasus_notetaker - use guard notation when acquiring mutex
  Input: powermate - use guard notation when acquiring spinlock
  Input: pwm-beeper - use guard notation when acquiring spinlock
  Input: regulator-haptic - use guard notation when acquiring mutex
  Input: rotary_encoder - use guard notation when acquiring mutex
  Input: sparcspkr - use guard notation when acquiring spinlock

 drivers/input/misc/ad714x.c              |  12 +-
 drivers/input/misc/ati_remote2.c         |  57 +++-----
 drivers/input/misc/cm109.c               | 167 +++++++++++------------
 drivers/input/misc/cma3000_d0x.c         |  16 +--
 drivers/input/misc/da7280.c              |  26 ++--
 drivers/input/misc/drv260x.c             |  50 ++++---
 drivers/input/misc/drv2665.c             |  44 +++---
 drivers/input/misc/drv2667.c             |  44 +++---
 drivers/input/misc/ibm-panel.c           |   5 +-
 drivers/input/misc/ideapad_slidebar.c    |  22 +--
 drivers/input/misc/iqs269a.c             |  55 +++-----
 drivers/input/misc/iqs626a.c             |  22 +--
 drivers/input/misc/iqs7222.c             |  30 ++--
 drivers/input/misc/kxtj9.c               |  14 +-
 drivers/input/misc/max8997_haptic.c      |  15 +-
 drivers/input/misc/powermate.c           |  11 +-
 drivers/input/misc/pwm-beeper.c          |  12 +-
 drivers/input/misc/regulator-haptic.c    |  23 ++--
 drivers/input/misc/rotary_encoder.c      |  23 ++--
 drivers/input/misc/sparcspkr.c           |  10 +-
 drivers/input/tablet/pegasus_notetaker.c |  86 ++++++------
 21 files changed, 311 insertions(+), 433 deletions(-)

-- 
Dmitry

[PATCH 01/22] Input: ad714x - use guard notation when acquiring mutex

[Dmitry Torokhov]

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/ad714x.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/input/misc/ad714x.c b/drivers/input/misc/ad714x.c
index 1acd8429c56c..d106f37df6bc 100644
--- a/drivers/input/misc/ad714x.c
+++ b/drivers/input/misc/ad714x.c
@@ -941,7 +941,7 @@ static irqreturn_t ad714x_interrupt_thread(int irq, void *data)
 	struct ad714x_chip *ad714x = data;
 	int i;
 
-	mutex_lock(&ad714x->mutex);
+	guard(mutex)(&ad714x->mutex);
 
 	ad714x->read(ad714x, STG_LOW_INT_STA_REG, &ad714x->l_state, 3);
 
@@ -954,8 +954,6 @@ static irqreturn_t ad714x_interrupt_thread(int irq, void *data)
 	for (i = 0; i < ad714x->hw->touchpad_num; i++)
 		ad714x_touchpad_state_machine(ad714x, i);
 
-	mutex_unlock(&ad714x->mutex);
-
 	return IRQ_HANDLED;
 }
 
@@ -1169,13 +1167,11 @@ static int ad714x_suspend(struct device *dev)
 
 	dev_dbg(ad714x->dev, "%s enter\n", __func__);
 
-	mutex_lock(&ad714x->mutex);
+	guard(mutex)(&ad714x->mutex);
 
 	data = ad714x->hw->sys_cfg_reg[AD714X_PWR_CTRL] | 0x3;
 	ad714x->write(ad714x, AD714X_PWR_CTRL, data);
 
-	mutex_unlock(&ad714x->mutex);
-
 	return 0;
 }
 
@@ -1184,7 +1180,7 @@ static int ad714x_resume(struct device *dev)
 	struct ad714x_chip *ad714x = dev_get_drvdata(dev);
 	dev_dbg(ad714x->dev, "%s enter\n", __func__);
 
-	mutex_lock(&ad714x->mutex);
+	guard(mutex)(&ad714x->mutex);
 
 	/* resume to non-shutdown mode */
 
@@ -1197,8 +1193,6 @@ static int ad714x_resume(struct device *dev)
 
 	ad714x->read(ad714x, STG_LOW_INT_STA_REG, &ad714x->l_state, 3);
 
-	mutex_unlock(&ad714x->mutex);
-
 	return 0;
 }
 
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 01/22] Input: ad714x - use guard notation when acquiring mutex

[Javier Carrasco]

On 04/09/2024 06:42, 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/ad714x.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/misc/ad714x.c b/drivers/input/misc/ad714x.c
> index 1acd8429c56c..d106f37df6bc 100644
> --- a/drivers/input/misc/ad714x.c
> +++ b/drivers/input/misc/ad714x.c
> @@ -941,7 +941,7 @@ static irqreturn_t ad714x_interrupt_thread(int irq, void *data)
>  	struct ad714x_chip *ad714x = data;
>  	int i;
>  
> -	mutex_lock(&ad714x->mutex);
> +	guard(mutex)(&ad714x->mutex);
>  
>  	ad714x->read(ad714x, STG_LOW_INT_STA_REG, &ad714x->l_state, 3);
>  
> @@ -954,8 +954,6 @@ static irqreturn_t ad714x_interrupt_thread(int irq, void *data)
>  	for (i = 0; i < ad714x->hw->touchpad_num; i++)
>  		ad714x_touchpad_state_machine(ad714x, i);
>  
> -	mutex_unlock(&ad714x->mutex);
> -
>  	return IRQ_HANDLED;
>  }
>  
> @@ -1169,13 +1167,11 @@ static int ad714x_suspend(struct device *dev)
>  
>  	dev_dbg(ad714x->dev, "%s enter\n", __func__);
>  
> -	mutex_lock(&ad714x->mutex);
> +	guard(mutex)(&ad714x->mutex);
>  
>  	data = ad714x->hw->sys_cfg_reg[AD714X_PWR_CTRL] | 0x3;
>  	ad714x->write(ad714x, AD714X_PWR_CTRL, data);
>  
> -	mutex_unlock(&ad714x->mutex);
> -
>  	return 0;
>  }
>  
> @@ -1184,7 +1180,7 @@ static int ad714x_resume(struct device *dev)
>  	struct ad714x_chip *ad714x = dev_get_drvdata(dev);
>  	dev_dbg(ad714x->dev, "%s enter\n", __func__);
>  
> -	mutex_lock(&ad714x->mutex);
> +	guard(mutex)(&ad714x->mutex);
>  
>  	/* resume to non-shutdown mode */
>  
> @@ -1197,8 +1193,6 @@ static int ad714x_resume(struct device *dev)
>  
>  	ad714x->read(ad714x, STG_LOW_INT_STA_REG, &ad714x->l_state, 3);
>  
> -	mutex_unlock(&ad714x->mutex);
> -
>  	return 0;
>  }
>  

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 02/22] Input: ati_remote2 - use guard notation when acquiring mutex

[Dmitry Torokhov]

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/ati_remote2.c | 57 +++++++++++---------------------
 1 file changed, 19 insertions(+), 38 deletions(-)

diff --git a/drivers/input/misc/ati_remote2.c b/drivers/input/misc/ati_remote2.c
index 795f69edb4b2..e84649af801d 100644
--- a/drivers/input/misc/ati_remote2.c
+++ b/drivers/input/misc/ati_remote2.c
@@ -244,29 +244,21 @@ static int ati_remote2_open(struct input_dev *idev)
 	if (r) {
 		dev_err(&ar2->intf[0]->dev,
 			"%s(): usb_autopm_get_interface() = %d\n", __func__, r);
-		goto fail1;
+		return r;
 	}
 
-	mutex_lock(&ati_remote2_mutex);
+	scoped_guard(mutex, &ati_remote2_mutex) {
+		if (!(ar2->flags & ATI_REMOTE2_SUSPENDED)) {
+			r = ati_remote2_submit_urbs(ar2);
+			if (r)
+				break;
+		}
 
-	if (!(ar2->flags & ATI_REMOTE2_SUSPENDED)) {
-		r = ati_remote2_submit_urbs(ar2);
-		if (r)
-			goto fail2;
+		ar2->flags |= ATI_REMOTE2_OPENED;
 	}
 
-	ar2->flags |= ATI_REMOTE2_OPENED;
-
-	mutex_unlock(&ati_remote2_mutex);
-
 	usb_autopm_put_interface(ar2->intf[0]);
 
-	return 0;
-
- fail2:
-	mutex_unlock(&ati_remote2_mutex);
-	usb_autopm_put_interface(ar2->intf[0]);
- fail1:
 	return r;
 }
 
@@ -276,14 +268,12 @@ static void ati_remote2_close(struct input_dev *idev)
 
 	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __func__);
 
-	mutex_lock(&ati_remote2_mutex);
+	guard(mutex)(&ati_remote2_mutex);
 
 	if (!(ar2->flags & ATI_REMOTE2_SUSPENDED))
 		ati_remote2_kill_urbs(ar2);
 
 	ar2->flags &= ~ATI_REMOTE2_OPENED;
-
-	mutex_unlock(&ati_remote2_mutex);
 }
 
 static void ati_remote2_input_mouse(struct ati_remote2 *ar2)
@@ -713,16 +703,14 @@ static ssize_t ati_remote2_store_channel_mask(struct device *dev,
 		return r;
 	}
 
-	mutex_lock(&ati_remote2_mutex);
-
-	if (mask != ar2->channel_mask) {
-		r = ati_remote2_setup(ar2, mask);
-		if (!r)
-			ar2->channel_mask = mask;
+	scoped_guard(mutex, &ati_remote2_mutex) {
+		if (mask != ar2->channel_mask) {
+			r = ati_remote2_setup(ar2, mask);
+			if (!r)
+				ar2->channel_mask = mask;
+		}
 	}
 
-	mutex_unlock(&ati_remote2_mutex);
-
 	usb_autopm_put_interface(ar2->intf[0]);
 
 	return r ? r : count;
@@ -892,15 +880,13 @@ static int ati_remote2_suspend(struct usb_interface *interface,
 
 	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __func__);
 
-	mutex_lock(&ati_remote2_mutex);
+	guard(mutex)(&ati_remote2_mutex);
 
 	if (ar2->flags & ATI_REMOTE2_OPENED)
 		ati_remote2_kill_urbs(ar2);
 
 	ar2->flags |= ATI_REMOTE2_SUSPENDED;
 
-	mutex_unlock(&ati_remote2_mutex);
-
 	return 0;
 }
 
@@ -917,7 +903,7 @@ static int ati_remote2_resume(struct usb_interface *interface)
 
 	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __func__);
 
-	mutex_lock(&ati_remote2_mutex);
+	guard(mutex)(&ati_remote2_mutex);
 
 	if (ar2->flags & ATI_REMOTE2_OPENED)
 		r = ati_remote2_submit_urbs(ar2);
@@ -925,8 +911,6 @@ static int ati_remote2_resume(struct usb_interface *interface)
 	if (!r)
 		ar2->flags &= ~ATI_REMOTE2_SUSPENDED;
 
-	mutex_unlock(&ati_remote2_mutex);
-
 	return r;
 }
 
@@ -943,11 +927,11 @@ static int ati_remote2_reset_resume(struct usb_interface *interface)
 
 	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __func__);
 
-	mutex_lock(&ati_remote2_mutex);
+	guard(mutex)(&ati_remote2_mutex);
 
 	r = ati_remote2_setup(ar2, ar2->channel_mask);
 	if (r)
-		goto out;
+		return r;
 
 	if (ar2->flags & ATI_REMOTE2_OPENED)
 		r = ati_remote2_submit_urbs(ar2);
@@ -955,9 +939,6 @@ static int ati_remote2_reset_resume(struct usb_interface *interface)
 	if (!r)
 		ar2->flags &= ~ATI_REMOTE2_SUSPENDED;
 
- out:
-	mutex_unlock(&ati_remote2_mutex);
-
 	return r;
 }
 
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 02/22] Input: ati_remote2 - use guard notation when acquiring mutex

[Javier Carrasco]

On 04/09/2024 06:42, 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/ati_remote2.c | 57 +++++++++++---------------------
>  1 file changed, 19 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/input/misc/ati_remote2.c b/drivers/input/misc/ati_remote2.c
> index 795f69edb4b2..e84649af801d 100644
> --- a/drivers/input/misc/ati_remote2.c
> +++ b/drivers/input/misc/ati_remote2.c
> @@ -244,29 +244,21 @@ static int ati_remote2_open(struct input_dev *idev)
>  	if (r) {
>  		dev_err(&ar2->intf[0]->dev,
>  			"%s(): usb_autopm_get_interface() = %d\n", __func__, r);
> -		goto fail1;
> +		return r;
>  	}
>  
> -	mutex_lock(&ati_remote2_mutex);
> +	scoped_guard(mutex, &ati_remote2_mutex) {
> +		if (!(ar2->flags & ATI_REMOTE2_SUSPENDED)) {
> +			r = ati_remote2_submit_urbs(ar2);
> +			if (r)
> +				break;
> +		}
>  
> -	if (!(ar2->flags & ATI_REMOTE2_SUSPENDED)) {
> -		r = ati_remote2_submit_urbs(ar2);
> -		if (r)
> -			goto fail2;
> +		ar2->flags |= ATI_REMOTE2_OPENED;
>  	}
>  
> -	ar2->flags |= ATI_REMOTE2_OPENED;
> -
> -	mutex_unlock(&ati_remote2_mutex);
> -
>  	usb_autopm_put_interface(ar2->intf[0]);
>  
> -	return 0;
> -
> - fail2:
> -	mutex_unlock(&ati_remote2_mutex);
> -	usb_autopm_put_interface(ar2->intf[0]);
> - fail1:
>  	return r;
>  }
>  
> @@ -276,14 +268,12 @@ static void ati_remote2_close(struct input_dev *idev)
>  
>  	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __func__);
>  
> -	mutex_lock(&ati_remote2_mutex);
> +	guard(mutex)(&ati_remote2_mutex);
>  
>  	if (!(ar2->flags & ATI_REMOTE2_SUSPENDED))
>  		ati_remote2_kill_urbs(ar2);
>  
>  	ar2->flags &= ~ATI_REMOTE2_OPENED;
> -
> -	mutex_unlock(&ati_remote2_mutex);
>  }
>  
>  static void ati_remote2_input_mouse(struct ati_remote2 *ar2)
> @@ -713,16 +703,14 @@ static ssize_t ati_remote2_store_channel_mask(struct device *dev,
>  		return r;
>  	}
>  
> -	mutex_lock(&ati_remote2_mutex);
> -
> -	if (mask != ar2->channel_mask) {
> -		r = ati_remote2_setup(ar2, mask);
> -		if (!r)
> -			ar2->channel_mask = mask;
> +	scoped_guard(mutex, &ati_remote2_mutex) {
> +		if (mask != ar2->channel_mask) {
> +			r = ati_remote2_setup(ar2, mask);
> +			if (!r)
> +				ar2->channel_mask = mask;
> +		}
>  	}
>  
> -	mutex_unlock(&ati_remote2_mutex);
> -
>  	usb_autopm_put_interface(ar2->intf[0]);
>  
>  	return r ? r : count;
> @@ -892,15 +880,13 @@ static int ati_remote2_suspend(struct usb_interface *interface,
>  
>  	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __func__);
>  
> -	mutex_lock(&ati_remote2_mutex);
> +	guard(mutex)(&ati_remote2_mutex);
>  
>  	if (ar2->flags & ATI_REMOTE2_OPENED)
>  		ati_remote2_kill_urbs(ar2);
>  
>  	ar2->flags |= ATI_REMOTE2_SUSPENDED;
>  
> -	mutex_unlock(&ati_remote2_mutex);
> -
>  	return 0;
>  }
>  
> @@ -917,7 +903,7 @@ static int ati_remote2_resume(struct usb_interface *interface)
>  
>  	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __func__);
>  
> -	mutex_lock(&ati_remote2_mutex);
> +	guard(mutex)(&ati_remote2_mutex);
>  
>  	if (ar2->flags & ATI_REMOTE2_OPENED)
>  		r = ati_remote2_submit_urbs(ar2);
> @@ -925,8 +911,6 @@ static int ati_remote2_resume(struct usb_interface *interface)
>  	if (!r)
>  		ar2->flags &= ~ATI_REMOTE2_SUSPENDED;
>  
> -	mutex_unlock(&ati_remote2_mutex);
> -
>  	return r;
>  }
>  
> @@ -943,11 +927,11 @@ static int ati_remote2_reset_resume(struct usb_interface *interface)
>  
>  	dev_dbg(&ar2->intf[0]->dev, "%s()\n", __func__);
>  
> -	mutex_lock(&ati_remote2_mutex);
> +	guard(mutex)(&ati_remote2_mutex);
>  
>  	r = ati_remote2_setup(ar2, ar2->channel_mask);
>  	if (r)
> -		goto out;
> +		return r;
>  
>  	if (ar2->flags & ATI_REMOTE2_OPENED)
>  		r = ati_remote2_submit_urbs(ar2);
> @@ -955,9 +939,6 @@ static int ati_remote2_reset_resume(struct usb_interface *interface)
>  	if (!r)
>  		ar2->flags &= ~ATI_REMOTE2_SUSPENDED;
>  
> - out:
> -	mutex_unlock(&ati_remote2_mutex);
> -
>  	return r;
>  }
>  

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 03/22] Input: cm109 - use guard notation when acquiring mutex and spinlock

[Dmitry Torokhov]

Using guard notation makes the code more compact and error handling
more robust by ensuring that locks are released in all code paths
when control leaves critical section.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/cm109.c | 167 ++++++++++++++++++-------------------
 1 file changed, 79 insertions(+), 88 deletions(-)

diff --git a/drivers/input/misc/cm109.c b/drivers/input/misc/cm109.c
index 728325a2d574..0cfe5d4a573c 100644
--- a/drivers/input/misc/cm109.c
+++ b/drivers/input/misc/cm109.c
@@ -355,6 +355,35 @@ static void cm109_submit_buzz_toggle(struct cm109_dev *dev)
 			__func__, error);
 }
 
+static void cm109_submit_ctl(struct cm109_dev *dev)
+{
+	int error;
+
+	guard(spinlock_irqsave)(&dev->ctl_submit_lock);
+
+	dev->irq_urb_pending = 0;
+
+	if (unlikely(dev->shutdown))
+		return;
+
+	if (dev->buzzer_state)
+		dev->ctl_data->byte[HID_OR0] |= BUZZER_ON;
+	else
+		dev->ctl_data->byte[HID_OR0] &= ~BUZZER_ON;
+
+	dev->ctl_data->byte[HID_OR1] = dev->keybit;
+	dev->ctl_data->byte[HID_OR2] = dev->keybit;
+
+	dev->buzzer_pending = 0;
+	dev->ctl_urb_pending = 1;
+
+	error = usb_submit_urb(dev->urb_ctl, GFP_ATOMIC);
+	if (error)
+		dev_err(&dev->intf->dev,
+			"%s: usb_submit_urb (urb_ctl) failed %d\n",
+			__func__, error);
+}
+
 /*
  * IRQ handler
  */
@@ -362,8 +391,6 @@ static void cm109_urb_irq_callback(struct urb *urb)
 {
 	struct cm109_dev *dev = urb->context;
 	const int status = urb->status;
-	int error;
-	unsigned long flags;
 
 	dev_dbg(&dev->intf->dev, "### URB IRQ: [0x%02x 0x%02x 0x%02x 0x%02x] keybit=0x%02x\n",
 	     dev->irq_data->byte[0],
@@ -401,32 +428,7 @@ static void cm109_urb_irq_callback(struct urb *urb)
 	}
 
  out:
-
-	spin_lock_irqsave(&dev->ctl_submit_lock, flags);
-
-	dev->irq_urb_pending = 0;
-
-	if (likely(!dev->shutdown)) {
-
-		if (dev->buzzer_state)
-			dev->ctl_data->byte[HID_OR0] |= BUZZER_ON;
-		else
-			dev->ctl_data->byte[HID_OR0] &= ~BUZZER_ON;
-
-		dev->ctl_data->byte[HID_OR1] = dev->keybit;
-		dev->ctl_data->byte[HID_OR2] = dev->keybit;
-
-		dev->buzzer_pending = 0;
-		dev->ctl_urb_pending = 1;
-
-		error = usb_submit_urb(dev->urb_ctl, GFP_ATOMIC);
-		if (error)
-			dev_err(&dev->intf->dev,
-				"%s: usb_submit_urb (urb_ctl) failed %d\n",
-				__func__, error);
-	}
-
-	spin_unlock_irqrestore(&dev->ctl_submit_lock, flags);
+	cm109_submit_ctl(dev);
 }
 
 static void cm109_urb_ctl_callback(struct urb *urb)
@@ -434,7 +436,6 @@ static void cm109_urb_ctl_callback(struct urb *urb)
 	struct cm109_dev *dev = urb->context;
 	const int status = urb->status;
 	int error;
-	unsigned long flags;
 
 	dev_dbg(&dev->intf->dev, "### URB CTL: [0x%02x 0x%02x 0x%02x 0x%02x]\n",
 	     dev->ctl_data->byte[0],
@@ -449,35 +450,31 @@ static void cm109_urb_ctl_callback(struct urb *urb)
 				    __func__, status);
 	}
 
-	spin_lock_irqsave(&dev->ctl_submit_lock, flags);
+	guard(spinlock_irqsave)(&dev->ctl_submit_lock);
 
 	dev->ctl_urb_pending = 0;
 
-	if (likely(!dev->shutdown)) {
-
-		if (dev->buzzer_pending || status) {
-			dev->buzzer_pending = 0;
-			dev->ctl_urb_pending = 1;
-			cm109_submit_buzz_toggle(dev);
-		} else if (likely(!dev->irq_urb_pending)) {
-			/* ask for key data */
-			dev->irq_urb_pending = 1;
-			error = usb_submit_urb(dev->urb_irq, GFP_ATOMIC);
-			if (error)
-				dev_err(&dev->intf->dev,
-					"%s: usb_submit_urb (urb_irq) failed %d\n",
-					__func__, error);
-		}
-	}
+	if (unlikely(dev->shutdown))
+		return;
 
-	spin_unlock_irqrestore(&dev->ctl_submit_lock, flags);
+	if (dev->buzzer_pending || status) {
+		dev->buzzer_pending = 0;
+		dev->ctl_urb_pending = 1;
+		cm109_submit_buzz_toggle(dev);
+	} else if (likely(!dev->irq_urb_pending)) {
+		/* ask for key data */
+		dev->irq_urb_pending = 1;
+		error = usb_submit_urb(dev->urb_irq, GFP_ATOMIC);
+		if (error)
+			dev_err(&dev->intf->dev,
+				"%s: usb_submit_urb (urb_irq) failed %d\n",
+				__func__, error);
+	}
 }
 
 static void cm109_toggle_buzzer_async(struct cm109_dev *dev)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&dev->ctl_submit_lock, flags);
+	guard(spinlock_irqsave)(&dev->ctl_submit_lock);
 
 	if (dev->ctl_urb_pending) {
 		/* URB completion will resubmit */
@@ -486,8 +483,6 @@ static void cm109_toggle_buzzer_async(struct cm109_dev *dev)
 		dev->ctl_urb_pending = 1;
 		cm109_submit_buzz_toggle(dev);
 	}
-
-	spin_unlock_irqrestore(&dev->ctl_submit_lock, flags);
 }
 
 static void cm109_toggle_buzzer_sync(struct cm109_dev *dev, int on)
@@ -556,32 +551,30 @@ static int cm109_input_open(struct input_dev *idev)
 		return error;
 	}
 
-	mutex_lock(&dev->pm_mutex);
-
-	dev->buzzer_state = 0;
-	dev->key_code = -1;	/* no keys pressed */
-	dev->keybit = 0xf;
+	scoped_guard(mutex, &dev->pm_mutex) {
+		dev->buzzer_state = 0;
+		dev->key_code = -1;	/* no keys pressed */
+		dev->keybit = 0xf;
 
-	/* issue INIT */
-	dev->ctl_data->byte[HID_OR0] = HID_OR_GPO_BUZ_SPDIF;
-	dev->ctl_data->byte[HID_OR1] = dev->keybit;
-	dev->ctl_data->byte[HID_OR2] = dev->keybit;
-	dev->ctl_data->byte[HID_OR3] = 0x00;
+		/* issue INIT */
+		dev->ctl_data->byte[HID_OR0] = HID_OR_GPO_BUZ_SPDIF;
+		dev->ctl_data->byte[HID_OR1] = dev->keybit;
+		dev->ctl_data->byte[HID_OR2] = dev->keybit;
+		dev->ctl_data->byte[HID_OR3] = 0x00;
 
-	dev->ctl_urb_pending = 1;
-	error = usb_submit_urb(dev->urb_ctl, GFP_KERNEL);
-	if (error) {
-		dev->ctl_urb_pending = 0;
-		dev_err(&dev->intf->dev, "%s: usb_submit_urb (urb_ctl) failed %d\n",
-			__func__, error);
-	} else {
-		dev->open = 1;
+		dev->ctl_urb_pending = 1;
+		error = usb_submit_urb(dev->urb_ctl, GFP_KERNEL);
+		if (!error) {
+			dev->open = 1;
+			return 0;
+		}
 	}
 
-	mutex_unlock(&dev->pm_mutex);
+	dev->ctl_urb_pending = 0;
+	usb_autopm_put_interface(dev->intf);
 
-	if (error)
-		usb_autopm_put_interface(dev->intf);
+	dev_err(&dev->intf->dev, "%s: usb_submit_urb (urb_ctl) failed %d\n",
+		__func__, error);
 
 	return error;
 }
@@ -590,17 +583,15 @@ static void cm109_input_close(struct input_dev *idev)
 {
 	struct cm109_dev *dev = input_get_drvdata(idev);
 
-	mutex_lock(&dev->pm_mutex);
-
-	/*
-	 * Once we are here event delivery is stopped so we
-	 * don't need to worry about someone starting buzzer
-	 * again
-	 */
-	cm109_stop_traffic(dev);
-	dev->open = 0;
-
-	mutex_unlock(&dev->pm_mutex);
+	scoped_guard(mutex, &dev->pm_mutex) {
+		/*
+		 * Once we are here event delivery is stopped so we
+		 * don't need to worry about someone starting buzzer
+		 * again
+		 */
+		cm109_stop_traffic(dev);
+		dev->open = 0;
+	}
 
 	usb_autopm_put_interface(dev->intf);
 }
@@ -823,9 +814,9 @@ static int cm109_usb_suspend(struct usb_interface *intf, pm_message_t message)
 
 	dev_info(&intf->dev, "cm109: usb_suspend (event=%d)\n", message.event);
 
-	mutex_lock(&dev->pm_mutex);
+	guard(mutex)(&dev->pm_mutex);
+
 	cm109_stop_traffic(dev);
-	mutex_unlock(&dev->pm_mutex);
 
 	return 0;
 }
@@ -836,9 +827,9 @@ static int cm109_usb_resume(struct usb_interface *intf)
 
 	dev_info(&intf->dev, "cm109: usb_resume\n");
 
-	mutex_lock(&dev->pm_mutex);
+	guard(mutex)(&dev->pm_mutex);
+
 	cm109_restore_state(dev);
-	mutex_unlock(&dev->pm_mutex);
 
 	return 0;
 }
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 03/22] Input: cm109 - use guard notation when acquiring mutex and spinlock

[Javier Carrasco]

On 04/09/2024 06:42, Dmitry Torokhov wrote:
> Using guard notation makes the code more compact and error handling
> more robust by ensuring that locks are released in all code paths
> when control leaves critical section.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/cm109.c | 167 ++++++++++++++++++-------------------
>  1 file changed, 79 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/input/misc/cm109.c b/drivers/input/misc/cm109.c
> index 728325a2d574..0cfe5d4a573c 100644
> --- a/drivers/input/misc/cm109.c
> +++ b/drivers/input/misc/cm109.c
> @@ -355,6 +355,35 @@ static void cm109_submit_buzz_toggle(struct cm109_dev *dev)
>  			__func__, error);
>  }
>  
> +static void cm109_submit_ctl(struct cm109_dev *dev)
> +{
> +	int error;
> +
> +	guard(spinlock_irqsave)(&dev->ctl_submit_lock);
> +
> +	dev->irq_urb_pending = 0;
> +
> +	if (unlikely(dev->shutdown))
> +		return;
> +
> +	if (dev->buzzer_state)
> +		dev->ctl_data->byte[HID_OR0] |= BUZZER_ON;
> +	else
> +		dev->ctl_data->byte[HID_OR0] &= ~BUZZER_ON;
> +
> +	dev->ctl_data->byte[HID_OR1] = dev->keybit;
> +	dev->ctl_data->byte[HID_OR2] = dev->keybit;
> +
> +	dev->buzzer_pending = 0;
> +	dev->ctl_urb_pending = 1;
> +
> +	error = usb_submit_urb(dev->urb_ctl, GFP_ATOMIC);
> +	if (error)
> +		dev_err(&dev->intf->dev,
> +			"%s: usb_submit_urb (urb_ctl) failed %d\n",
> +			__func__, error);
> +}
> +
>  /*
>   * IRQ handler
>   */
> @@ -362,8 +391,6 @@ static void cm109_urb_irq_callback(struct urb *urb)
>  {
>  	struct cm109_dev *dev = urb->context;
>  	const int status = urb->status;
> -	int error;
> -	unsigned long flags;
>  
>  	dev_dbg(&dev->intf->dev, "### URB IRQ: [0x%02x 0x%02x 0x%02x 0x%02x] keybit=0x%02x\n",
>  	     dev->irq_data->byte[0],
> @@ -401,32 +428,7 @@ static void cm109_urb_irq_callback(struct urb *urb)
>  	}
>  
>   out:
> -
> -	spin_lock_irqsave(&dev->ctl_submit_lock, flags);
> -
> -	dev->irq_urb_pending = 0;
> -
> -	if (likely(!dev->shutdown)) {
> -
> -		if (dev->buzzer_state)
> -			dev->ctl_data->byte[HID_OR0] |= BUZZER_ON;
> -		else
> -			dev->ctl_data->byte[HID_OR0] &= ~BUZZER_ON;
> -
> -		dev->ctl_data->byte[HID_OR1] = dev->keybit;
> -		dev->ctl_data->byte[HID_OR2] = dev->keybit;
> -
> -		dev->buzzer_pending = 0;
> -		dev->ctl_urb_pending = 1;
> -
> -		error = usb_submit_urb(dev->urb_ctl, GFP_ATOMIC);
> -		if (error)
> -			dev_err(&dev->intf->dev,
> -				"%s: usb_submit_urb (urb_ctl) failed %d\n",
> -				__func__, error);
> -	}
> -
> -	spin_unlock_irqrestore(&dev->ctl_submit_lock, flags);
> +	cm109_submit_ctl(dev);
>  }
>  
>  static void cm109_urb_ctl_callback(struct urb *urb)
> @@ -434,7 +436,6 @@ static void cm109_urb_ctl_callback(struct urb *urb)
>  	struct cm109_dev *dev = urb->context;
>  	const int status = urb->status;
>  	int error;
> -	unsigned long flags;
>  
>  	dev_dbg(&dev->intf->dev, "### URB CTL: [0x%02x 0x%02x 0x%02x 0x%02x]\n",
>  	     dev->ctl_data->byte[0],
> @@ -449,35 +450,31 @@ static void cm109_urb_ctl_callback(struct urb *urb)
>  				    __func__, status);
>  	}
>  
> -	spin_lock_irqsave(&dev->ctl_submit_lock, flags);
> +	guard(spinlock_irqsave)(&dev->ctl_submit_lock);
>  
>  	dev->ctl_urb_pending = 0;
>  
> -	if (likely(!dev->shutdown)) {
> -
> -		if (dev->buzzer_pending || status) {
> -			dev->buzzer_pending = 0;
> -			dev->ctl_urb_pending = 1;
> -			cm109_submit_buzz_toggle(dev);
> -		} else if (likely(!dev->irq_urb_pending)) {
> -			/* ask for key data */
> -			dev->irq_urb_pending = 1;
> -			error = usb_submit_urb(dev->urb_irq, GFP_ATOMIC);
> -			if (error)
> -				dev_err(&dev->intf->dev,
> -					"%s: usb_submit_urb (urb_irq) failed %d\n",
> -					__func__, error);
> -		}
> -	}
> +	if (unlikely(dev->shutdown))
> +		return;
>  
> -	spin_unlock_irqrestore(&dev->ctl_submit_lock, flags);
> +	if (dev->buzzer_pending || status) {
> +		dev->buzzer_pending = 0;
> +		dev->ctl_urb_pending = 1;
> +		cm109_submit_buzz_toggle(dev);
> +	} else if (likely(!dev->irq_urb_pending)) {
> +		/* ask for key data */
> +		dev->irq_urb_pending = 1;
> +		error = usb_submit_urb(dev->urb_irq, GFP_ATOMIC);
> +		if (error)
> +			dev_err(&dev->intf->dev,
> +				"%s: usb_submit_urb (urb_irq) failed %d\n",
> +				__func__, error);
> +	}
>  }
>  
>  static void cm109_toggle_buzzer_async(struct cm109_dev *dev)
>  {
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&dev->ctl_submit_lock, flags);
> +	guard(spinlock_irqsave)(&dev->ctl_submit_lock);
>  
>  	if (dev->ctl_urb_pending) {
>  		/* URB completion will resubmit */
> @@ -486,8 +483,6 @@ static void cm109_toggle_buzzer_async(struct cm109_dev *dev)
>  		dev->ctl_urb_pending = 1;
>  		cm109_submit_buzz_toggle(dev);
>  	}
> -
> -	spin_unlock_irqrestore(&dev->ctl_submit_lock, flags);
>  }
>  
>  static void cm109_toggle_buzzer_sync(struct cm109_dev *dev, int on)
> @@ -556,32 +551,30 @@ static int cm109_input_open(struct input_dev *idev)
>  		return error;
>  	}
>  
> -	mutex_lock(&dev->pm_mutex);
> -
> -	dev->buzzer_state = 0;
> -	dev->key_code = -1;	/* no keys pressed */
> -	dev->keybit = 0xf;
> +	scoped_guard(mutex, &dev->pm_mutex) {
> +		dev->buzzer_state = 0;
> +		dev->key_code = -1;	/* no keys pressed */
> +		dev->keybit = 0xf;
>  
> -	/* issue INIT */
> -	dev->ctl_data->byte[HID_OR0] = HID_OR_GPO_BUZ_SPDIF;
> -	dev->ctl_data->byte[HID_OR1] = dev->keybit;
> -	dev->ctl_data->byte[HID_OR2] = dev->keybit;
> -	dev->ctl_data->byte[HID_OR3] = 0x00;
> +		/* issue INIT */
> +		dev->ctl_data->byte[HID_OR0] = HID_OR_GPO_BUZ_SPDIF;
> +		dev->ctl_data->byte[HID_OR1] = dev->keybit;
> +		dev->ctl_data->byte[HID_OR2] = dev->keybit;
> +		dev->ctl_data->byte[HID_OR3] = 0x00;
>  
> -	dev->ctl_urb_pending = 1;
> -	error = usb_submit_urb(dev->urb_ctl, GFP_KERNEL);
> -	if (error) {
> -		dev->ctl_urb_pending = 0;
> -		dev_err(&dev->intf->dev, "%s: usb_submit_urb (urb_ctl) failed %d\n",
> -			__func__, error);
> -	} else {
> -		dev->open = 1;
> +		dev->ctl_urb_pending = 1;
> +		error = usb_submit_urb(dev->urb_ctl, GFP_KERNEL);
> +		if (!error) {
> +			dev->open = 1;
> +			return 0;
> +		}
>  	}
>  
> -	mutex_unlock(&dev->pm_mutex);
> +	dev->ctl_urb_pending = 0;
> +	usb_autopm_put_interface(dev->intf);
>  
> -	if (error)
> -		usb_autopm_put_interface(dev->intf);
> +	dev_err(&dev->intf->dev, "%s: usb_submit_urb (urb_ctl) failed %d\n",
> +		__func__, error);
>  
>  	return error;
>  }
> @@ -590,17 +583,15 @@ static void cm109_input_close(struct input_dev *idev)
>  {
>  	struct cm109_dev *dev = input_get_drvdata(idev);
>  
> -	mutex_lock(&dev->pm_mutex);
> -
> -	/*
> -	 * Once we are here event delivery is stopped so we
> -	 * don't need to worry about someone starting buzzer
> -	 * again
> -	 */
> -	cm109_stop_traffic(dev);
> -	dev->open = 0;
> -
> -	mutex_unlock(&dev->pm_mutex);
> +	scoped_guard(mutex, &dev->pm_mutex) {
> +		/*
> +		 * Once we are here event delivery is stopped so we
> +		 * don't need to worry about someone starting buzzer
> +		 * again
> +		 */
> +		cm109_stop_traffic(dev);
> +		dev->open = 0;
> +	}
>  
>  	usb_autopm_put_interface(dev->intf);
>  }
> @@ -823,9 +814,9 @@ static int cm109_usb_suspend(struct usb_interface *intf, pm_message_t message)
>  
>  	dev_info(&intf->dev, "cm109: usb_suspend (event=%d)\n", message.event);
>  
> -	mutex_lock(&dev->pm_mutex);
> +	guard(mutex)(&dev->pm_mutex);
> +
>  	cm109_stop_traffic(dev);
> -	mutex_unlock(&dev->pm_mutex);
>  
>  	return 0;
>  }
> @@ -836,9 +827,9 @@ static int cm109_usb_resume(struct usb_interface *intf)
>  
>  	dev_info(&intf->dev, "cm109: usb_resume\n");
>  
> -	mutex_lock(&dev->pm_mutex);
> +	guard(mutex)(&dev->pm_mutex);
> +
>  	cm109_restore_state(dev);
> -	mutex_unlock(&dev->pm_mutex);
>  
>  	return 0;
>  }

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 04/22] Input: cma3000_d0x - use guard notation when acquiring mutex

[Dmitry Torokhov]

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/cma3000_d0x.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/input/misc/cma3000_d0x.c b/drivers/input/misc/cma3000_d0x.c
index 0c68e924a1cc..cfc12332bee1 100644
--- a/drivers/input/misc/cma3000_d0x.c
+++ b/drivers/input/misc/cma3000_d0x.c
@@ -217,15 +217,13 @@ static int cma3000_open(struct input_dev *input_dev)
 {
 	struct cma3000_accl_data *data = input_get_drvdata(input_dev);
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
 	if (!data->suspended)
 		cma3000_poweron(data);
 
 	data->opened = true;
 
-	mutex_unlock(&data->mutex);
-
 	return 0;
 }
 
@@ -233,40 +231,34 @@ static void cma3000_close(struct input_dev *input_dev)
 {
 	struct cma3000_accl_data *data = input_get_drvdata(input_dev);
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
 	if (!data->suspended)
 		cma3000_poweroff(data);
 
 	data->opened = false;
-
-	mutex_unlock(&data->mutex);
 }
 
 void cma3000_suspend(struct cma3000_accl_data *data)
 {
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
 	if (!data->suspended && data->opened)
 		cma3000_poweroff(data);
 
 	data->suspended = true;
-
-	mutex_unlock(&data->mutex);
 }
 EXPORT_SYMBOL(cma3000_suspend);
 
 
 void cma3000_resume(struct cma3000_accl_data *data)
 {
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
 	if (data->suspended && data->opened)
 		cma3000_poweron(data);
 
 	data->suspended = false;
-
-	mutex_unlock(&data->mutex);
 }
 EXPORT_SYMBOL(cma3000_resume);
 
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 04/22] Input: cma3000_d0x - use guard notation when acquiring mutex

[Javier Carrasco]

On 04/09/2024 06:42, 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/cma3000_d0x.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/input/misc/cma3000_d0x.c b/drivers/input/misc/cma3000_d0x.c
> index 0c68e924a1cc..cfc12332bee1 100644
> --- a/drivers/input/misc/cma3000_d0x.c
> +++ b/drivers/input/misc/cma3000_d0x.c
> @@ -217,15 +217,13 @@ static int cma3000_open(struct input_dev *input_dev)
>  {
>  	struct cma3000_accl_data *data = input_get_drvdata(input_dev);
>  
> -	mutex_lock(&data->mutex);
> +	guard(mutex)(&data->mutex);
>  
>  	if (!data->suspended)
>  		cma3000_poweron(data);
>  
>  	data->opened = true;
>  
> -	mutex_unlock(&data->mutex);
> -
>  	return 0;
>  }
>  
> @@ -233,40 +231,34 @@ static void cma3000_close(struct input_dev *input_dev)
>  {
>  	struct cma3000_accl_data *data = input_get_drvdata(input_dev);
>  
> -	mutex_lock(&data->mutex);
> +	guard(mutex)(&data->mutex);
>  
>  	if (!data->suspended)
>  		cma3000_poweroff(data);
>  
>  	data->opened = false;
> -
> -	mutex_unlock(&data->mutex);
>  }
>  
>  void cma3000_suspend(struct cma3000_accl_data *data)
>  {
> -	mutex_lock(&data->mutex);
> +	guard(mutex)(&data->mutex);
>  
>  	if (!data->suspended && data->opened)
>  		cma3000_poweroff(data);
>  
>  	data->suspended = true;
> -
> -	mutex_unlock(&data->mutex);
>  }
>  EXPORT_SYMBOL(cma3000_suspend);
>  
>  
>  void cma3000_resume(struct cma3000_accl_data *data)
>  {
> -	mutex_lock(&data->mutex);
> +	guard(mutex)(&data->mutex);
>  
>  	if (data->suspended && data->opened)
>  		cma3000_poweron(data);
>  
>  	data->suspended = false;
> -
> -	mutex_unlock(&data->mutex);
>  }
>  EXPORT_SYMBOL(cma3000_resume);
>  

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 05/22] Input: da7280 - use guard notation when acquiring mutex and spinlock

[Dmitry Torokhov]

Using guard notation makes the code more compact and error handling
more robust by ensuring that locks are released in all code paths
when control leaves critical section.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/da7280.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/input/misc/da7280.c b/drivers/input/misc/da7280.c
index 1629b7ea4cbd..e4a605c6af15 100644
--- a/drivers/input/misc/da7280.c
+++ b/drivers/input/misc/da7280.c
@@ -1263,39 +1263,37 @@ static int da7280_suspend(struct device *dev)
 {
 	struct da7280_haptic *haptics = dev_get_drvdata(dev);
 
-	mutex_lock(&haptics->input_dev->mutex);
+	guard(mutex)(&haptics->input_dev->mutex);
 
 	/*
 	 * Make sure no new requests will be submitted while device is
 	 * suspended.
 	 */
-	spin_lock_irq(&haptics->input_dev->event_lock);
-	haptics->suspended = true;
-	spin_unlock_irq(&haptics->input_dev->event_lock);
+	scoped_guard(spinlock_irq, &haptics->input_dev->event_lock) {
+		haptics->suspended = true;
+	}
 
 	da7280_haptic_stop(haptics);
 
-	mutex_unlock(&haptics->input_dev->mutex);
-
 	return 0;
 }
 
 static int da7280_resume(struct device *dev)
 {
 	struct da7280_haptic *haptics = dev_get_drvdata(dev);
-	int retval;
+	int error;
 
-	mutex_lock(&haptics->input_dev->mutex);
+	guard(mutex)(&haptics->input_dev->mutex);
 
-	retval = da7280_haptic_start(haptics);
-	if (!retval) {
-		spin_lock_irq(&haptics->input_dev->event_lock);
+	error = da7280_haptic_start(haptics);
+	if (error)
+		return error;
+
+	scoped_guard(spinlock_irq, &haptics->input_dev->event_lock) {
 		haptics->suspended = false;
-		spin_unlock_irq(&haptics->input_dev->event_lock);
 	}
 
-	mutex_unlock(&haptics->input_dev->mutex);
-	return retval;
+	return 0;
 }
 
 #ifdef CONFIG_OF
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 05/22] Input: da7280 - use guard notation when acquiring mutex and spinlock

[Javier Carrasco]

On 04/09/2024 06:42, Dmitry Torokhov wrote:
> Using guard notation makes the code more compact and error handling
> more robust by ensuring that locks are released in all code paths
> when control leaves critical section.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/da7280.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/input/misc/da7280.c b/drivers/input/misc/da7280.c
> index 1629b7ea4cbd..e4a605c6af15 100644
> --- a/drivers/input/misc/da7280.c
> +++ b/drivers/input/misc/da7280.c
> @@ -1263,39 +1263,37 @@ static int da7280_suspend(struct device *dev)
>  {
>  	struct da7280_haptic *haptics = dev_get_drvdata(dev);
>  
> -	mutex_lock(&haptics->input_dev->mutex);
> +	guard(mutex)(&haptics->input_dev->mutex);
>  
>  	/*
>  	 * Make sure no new requests will be submitted while device is
>  	 * suspended.
>  	 */
> -	spin_lock_irq(&haptics->input_dev->event_lock);
> -	haptics->suspended = true;
> -	spin_unlock_irq(&haptics->input_dev->event_lock);
> +	scoped_guard(spinlock_irq, &haptics->input_dev->event_lock) {
> +		haptics->suspended = true;
> +	}
>  
>  	da7280_haptic_stop(haptics);
>  
> -	mutex_unlock(&haptics->input_dev->mutex);
> -
>  	return 0;
>  }
>  
>  static int da7280_resume(struct device *dev)
>  {
>  	struct da7280_haptic *haptics = dev_get_drvdata(dev);
> -	int retval;
> +	int error;
>  
> -	mutex_lock(&haptics->input_dev->mutex);
> +	guard(mutex)(&haptics->input_dev->mutex);
>  
> -	retval = da7280_haptic_start(haptics);
> -	if (!retval) {
> -		spin_lock_irq(&haptics->input_dev->event_lock);
> +	error = da7280_haptic_start(haptics);
> +	if (error)
> +		return error;
> +
> +	scoped_guard(spinlock_irq, &haptics->input_dev->event_lock) {
>  		haptics->suspended = false;
> -		spin_unlock_irq(&haptics->input_dev->event_lock);
>  	}
>  
> -	mutex_unlock(&haptics->input_dev->mutex);
> -	return retval;
> +	return 0;
>  }
>  
>  #ifdef CONFIG_OF

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 06/22] Input: kxtj9 - use guard notation when acquiring mutex/disabling irq

[Dmitry Torokhov]

Using guard notation makes the code more compact and error handling
more robust by ensuring that mutexes are released and interrupts are
re-enabled in all code paths when control leaves critical section.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/kxtj9.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/input/misc/kxtj9.c b/drivers/input/misc/kxtj9.c
index 837682cb2a7d..c6146bcee9f9 100644
--- a/drivers/input/misc/kxtj9.c
+++ b/drivers/input/misc/kxtj9.c
@@ -314,9 +314,8 @@ static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
 		return error;
 
 	/* Lock the device to prevent races with open/close (and itself) */
-	mutex_lock(&input_dev->mutex);
-
-	disable_irq(client->irq);
+	guard(mutex)(&input_dev->mutex);
+	guard(disable_irq)(&client->irq);
 
 	/*
 	 * Set current interval to the greater of the minimum interval or
@@ -326,9 +325,6 @@ static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
 
 	kxtj9_update_odr(tj9, tj9->last_poll_interval);
 
-	enable_irq(client->irq);
-	mutex_unlock(&input_dev->mutex);
-
 	return count;
 }
 
@@ -504,12 +500,11 @@ static int kxtj9_suspend(struct device *dev)
 	struct kxtj9_data *tj9 = i2c_get_clientdata(client);
 	struct input_dev *input_dev = tj9->input_dev;
 
-	mutex_lock(&input_dev->mutex);
+	guard(mutex)(&input_dev->mutex);
 
 	if (input_device_enabled(input_dev))
 		kxtj9_disable(tj9);
 
-	mutex_unlock(&input_dev->mutex);
 	return 0;
 }
 
@@ -519,12 +514,11 @@ static int kxtj9_resume(struct device *dev)
 	struct kxtj9_data *tj9 = i2c_get_clientdata(client);
 	struct input_dev *input_dev = tj9->input_dev;
 
-	mutex_lock(&input_dev->mutex);
+	guard(mutex)(&input_dev->mutex);
 
 	if (input_device_enabled(input_dev))
 		kxtj9_enable(tj9);
 
-	mutex_unlock(&input_dev->mutex);
 	return 0;
 }
 
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 06/22] Input: kxtj9 - use guard notation when acquiring mutex/disabling irq

[Javier Carrasco]

On 04/09/2024 06:42, Dmitry Torokhov wrote:
> Using guard notation makes the code more compact and error handling
> more robust by ensuring that mutexes are released and interrupts are
> re-enabled in all code paths when control leaves critical section.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/kxtj9.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/input/misc/kxtj9.c b/drivers/input/misc/kxtj9.c
> index 837682cb2a7d..c6146bcee9f9 100644
> --- a/drivers/input/misc/kxtj9.c
> +++ b/drivers/input/misc/kxtj9.c
> @@ -314,9 +314,8 @@ static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
>  		return error;
>  
>  	/* Lock the device to prevent races with open/close (and itself) */
> -	mutex_lock(&input_dev->mutex);
> -
> -	disable_irq(client->irq);
> +	guard(mutex)(&input_dev->mutex);
> +	guard(disable_irq)(&client->irq);
>  
>  	/*
>  	 * Set current interval to the greater of the minimum interval or
> @@ -326,9 +325,6 @@ static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
>  
>  	kxtj9_update_odr(tj9, tj9->last_poll_interval);
>  
> -	enable_irq(client->irq);
> -	mutex_unlock(&input_dev->mutex);
> -
>  	return count;
>  }
>  
> @@ -504,12 +500,11 @@ static int kxtj9_suspend(struct device *dev)
>  	struct kxtj9_data *tj9 = i2c_get_clientdata(client);
>  	struct input_dev *input_dev = tj9->input_dev;
>  
> -	mutex_lock(&input_dev->mutex);
> +	guard(mutex)(&input_dev->mutex);
>  
>  	if (input_device_enabled(input_dev))
>  		kxtj9_disable(tj9);
>  
> -	mutex_unlock(&input_dev->mutex);
>  	return 0;
>  }
>  
> @@ -519,12 +514,11 @@ static int kxtj9_resume(struct device *dev)
>  	struct kxtj9_data *tj9 = i2c_get_clientdata(client);
>  	struct input_dev *input_dev = tj9->input_dev;
>  
> -	mutex_lock(&input_dev->mutex);
> +	guard(mutex)(&input_dev->mutex);
>  
>  	if (input_device_enabled(input_dev))
>  		kxtj9_enable(tj9);
>  
> -	mutex_unlock(&input_dev->mutex);
>  	return 0;
>  }
>  

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 07/22] Input: drv260x - use guard notation when acquiring mutex

[Dmitry Torokhov]

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/drv260x.c | 50 +++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
index 61b503835aa6..96cd6a078c8a 100644
--- a/drivers/input/misc/drv260x.c
+++ b/drivers/input/misc/drv260x.c
@@ -537,64 +537,62 @@ static int drv260x_probe(struct i2c_client *client)
 static int drv260x_suspend(struct device *dev)
 {
 	struct drv260x_data *haptics = dev_get_drvdata(dev);
-	int ret = 0;
+	int error;
 
-	mutex_lock(&haptics->input_dev->mutex);
+	guard(mutex)(&haptics->input_dev->mutex);
 
 	if (input_device_enabled(haptics->input_dev)) {
-		ret = regmap_update_bits(haptics->regmap,
-					 DRV260X_MODE,
-					 DRV260X_STANDBY_MASK,
-					 DRV260X_STANDBY);
-		if (ret) {
+		error = regmap_update_bits(haptics->regmap,
+					   DRV260X_MODE,
+					   DRV260X_STANDBY_MASK,
+					   DRV260X_STANDBY);
+		if (error) {
 			dev_err(dev, "Failed to set standby mode\n");
-			goto out;
+			return error;
 		}
 
 		gpiod_set_value(haptics->enable_gpio, 0);
 
-		ret = regulator_disable(haptics->regulator);
-		if (ret) {
+		error = regulator_disable(haptics->regulator);
+		if (error) {
 			dev_err(dev, "Failed to disable regulator\n");
 			regmap_update_bits(haptics->regmap,
 					   DRV260X_MODE,
 					   DRV260X_STANDBY_MASK, 0);
+			return error;
 		}
 	}
-out:
-	mutex_unlock(&haptics->input_dev->mutex);
-	return ret;
+
+	return 0;
 }
 
 static int drv260x_resume(struct device *dev)
 {
 	struct drv260x_data *haptics = dev_get_drvdata(dev);
-	int ret = 0;
+	int error;
 
-	mutex_lock(&haptics->input_dev->mutex);
+	guard(mutex)(&haptics->input_dev->mutex);
 
 	if (input_device_enabled(haptics->input_dev)) {
-		ret = regulator_enable(haptics->regulator);
-		if (ret) {
+		error = regulator_enable(haptics->regulator);
+		if (error) {
 			dev_err(dev, "Failed to enable regulator\n");
-			goto out;
+			return error;
 		}
 
-		ret = regmap_update_bits(haptics->regmap,
-					 DRV260X_MODE,
-					 DRV260X_STANDBY_MASK, 0);
-		if (ret) {
+		error = regmap_update_bits(haptics->regmap,
+					   DRV260X_MODE,
+					   DRV260X_STANDBY_MASK, 0);
+		if (error) {
 			dev_err(dev, "Failed to unset standby mode\n");
 			regulator_disable(haptics->regulator);
-			goto out;
+			return error;
 		}
 
 		gpiod_set_value(haptics->enable_gpio, 1);
 	}
 
-out:
-	mutex_unlock(&haptics->input_dev->mutex);
-	return ret;
+	return 0;
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(drv260x_pm_ops, drv260x_suspend, drv260x_resume);
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 07/22] Input: drv260x - use guard notation when acquiring mutex

[Javier Carrasco]

On 04/09/2024 06:42, 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/drv260x.c | 50 +++++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
> index 61b503835aa6..96cd6a078c8a 100644
> --- a/drivers/input/misc/drv260x.c
> +++ b/drivers/input/misc/drv260x.c
> @@ -537,64 +537,62 @@ static int drv260x_probe(struct i2c_client *client)
>  static int drv260x_suspend(struct device *dev)
>  {
>  	struct drv260x_data *haptics = dev_get_drvdata(dev);
> -	int ret = 0;
> +	int error;
>  
> -	mutex_lock(&haptics->input_dev->mutex);
> +	guard(mutex)(&haptics->input_dev->mutex);
>  
>  	if (input_device_enabled(haptics->input_dev)) {
> -		ret = regmap_update_bits(haptics->regmap,
> -					 DRV260X_MODE,
> -					 DRV260X_STANDBY_MASK,
> -					 DRV260X_STANDBY);
> -		if (ret) {
> +		error = regmap_update_bits(haptics->regmap,
> +					   DRV260X_MODE,
> +					   DRV260X_STANDBY_MASK,
> +					   DRV260X_STANDBY);
> +		if (error) {
>  			dev_err(dev, "Failed to set standby mode\n");
> -			goto out;
> +			return error;
>  		}
>  
>  		gpiod_set_value(haptics->enable_gpio, 0);
>  
> -		ret = regulator_disable(haptics->regulator);
> -		if (ret) {
> +		error = regulator_disable(haptics->regulator);
> +		if (error) {
>  			dev_err(dev, "Failed to disable regulator\n");
>  			regmap_update_bits(haptics->regmap,
>  					   DRV260X_MODE,
>  					   DRV260X_STANDBY_MASK, 0);
> +			return error;
>  		}
>  	}
> -out:
> -	mutex_unlock(&haptics->input_dev->mutex);
> -	return ret;
> +
> +	return 0;
>  }
>  
>  static int drv260x_resume(struct device *dev)
>  {
>  	struct drv260x_data *haptics = dev_get_drvdata(dev);
> -	int ret = 0;
> +	int error;
>  
> -	mutex_lock(&haptics->input_dev->mutex);
> +	guard(mutex)(&haptics->input_dev->mutex);
>  
>  	if (input_device_enabled(haptics->input_dev)) {
> -		ret = regulator_enable(haptics->regulator);
> -		if (ret) {
> +		error = regulator_enable(haptics->regulator);
> +		if (error) {
>  			dev_err(dev, "Failed to enable regulator\n");
> -			goto out;
> +			return error;
>  		}
>  
> -		ret = regmap_update_bits(haptics->regmap,
> -					 DRV260X_MODE,
> -					 DRV260X_STANDBY_MASK, 0);
> -		if (ret) {
> +		error = regmap_update_bits(haptics->regmap,
> +					   DRV260X_MODE,
> +					   DRV260X_STANDBY_MASK, 0);
> +		if (error) {
>  			dev_err(dev, "Failed to unset standby mode\n");
>  			regulator_disable(haptics->regulator);
> -			goto out;
> +			return error;
>  		}
>  
>  		gpiod_set_value(haptics->enable_gpio, 1);
>  	}
>  
> -out:
> -	mutex_unlock(&haptics->input_dev->mutex);
> -	return ret;
> +	return 0;
>  }
>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(drv260x_pm_ops, drv260x_suspend, drv260x_resume);

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 08/22] Input: drv2665 - use guard notation when acquiring mutex

[Dmitry Torokhov]

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/drv2665.c | 44 +++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/input/misc/drv2665.c b/drivers/input/misc/drv2665.c
index f98e4d765307..77ec96c7db76 100644
--- a/drivers/input/misc/drv2665.c
+++ b/drivers/input/misc/drv2665.c
@@ -225,59 +225,57 @@ static int drv2665_probe(struct i2c_client *client)
 static int drv2665_suspend(struct device *dev)
 {
 	struct drv2665_data *haptics = dev_get_drvdata(dev);
-	int ret = 0;
+	int error;
 
-	mutex_lock(&haptics->input_dev->mutex);
+	guard(mutex)(&haptics->input_dev->mutex);
 
 	if (input_device_enabled(haptics->input_dev)) {
-		ret = regmap_update_bits(haptics->regmap, DRV2665_CTRL_2,
-					 DRV2665_STANDBY, DRV2665_STANDBY);
-		if (ret) {
+		error = regmap_update_bits(haptics->regmap, DRV2665_CTRL_2,
+					   DRV2665_STANDBY, DRV2665_STANDBY);
+		if (error) {
 			dev_err(dev, "Failed to set standby mode\n");
 			regulator_disable(haptics->regulator);
-			goto out;
+			return error;
 		}
 
-		ret = regulator_disable(haptics->regulator);
-		if (ret) {
+		error = regulator_disable(haptics->regulator);
+		if (error) {
 			dev_err(dev, "Failed to disable regulator\n");
 			regmap_update_bits(haptics->regmap,
 					   DRV2665_CTRL_2,
 					   DRV2665_STANDBY, 0);
+			return error;
 		}
 	}
-out:
-	mutex_unlock(&haptics->input_dev->mutex);
-	return ret;
+
+	return 0;
 }
 
 static int drv2665_resume(struct device *dev)
 {
 	struct drv2665_data *haptics = dev_get_drvdata(dev);
-	int ret = 0;
+	int error;
 
-	mutex_lock(&haptics->input_dev->mutex);
+	guard(mutex)(&haptics->input_dev->mutex);
 
 	if (input_device_enabled(haptics->input_dev)) {
-		ret = regulator_enable(haptics->regulator);
-		if (ret) {
+		error = regulator_enable(haptics->regulator);
+		if (error) {
 			dev_err(dev, "Failed to enable regulator\n");
-			goto out;
+			return error;
 		}
 
-		ret = regmap_update_bits(haptics->regmap, DRV2665_CTRL_2,
-					 DRV2665_STANDBY, 0);
-		if (ret) {
+		error = regmap_update_bits(haptics->regmap, DRV2665_CTRL_2,
+					   DRV2665_STANDBY, 0);
+		if (error) {
 			dev_err(dev, "Failed to unset standby mode\n");
 			regulator_disable(haptics->regulator);
-			goto out;
+			return error;
 		}
 
 	}
 
-out:
-	mutex_unlock(&haptics->input_dev->mutex);
-	return ret;
+	return 0;
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(drv2665_pm_ops, drv2665_suspend, drv2665_resume);
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 08/22] Input: drv2665 - use guard notation when acquiring mutex

[Javier Carrasco]

On 04/09/2024 06:42, 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/drv2665.c | 44 +++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/input/misc/drv2665.c b/drivers/input/misc/drv2665.c
> index f98e4d765307..77ec96c7db76 100644
> --- a/drivers/input/misc/drv2665.c
> +++ b/drivers/input/misc/drv2665.c
> @@ -225,59 +225,57 @@ static int drv2665_probe(struct i2c_client *client)
>  static int drv2665_suspend(struct device *dev)
>  {
>  	struct drv2665_data *haptics = dev_get_drvdata(dev);
> -	int ret = 0;
> +	int error;
>  
> -	mutex_lock(&haptics->input_dev->mutex);
> +	guard(mutex)(&haptics->input_dev->mutex);
>  
>  	if (input_device_enabled(haptics->input_dev)) {
> -		ret = regmap_update_bits(haptics->regmap, DRV2665_CTRL_2,
> -					 DRV2665_STANDBY, DRV2665_STANDBY);
> -		if (ret) {
> +		error = regmap_update_bits(haptics->regmap, DRV2665_CTRL_2,
> +					   DRV2665_STANDBY, DRV2665_STANDBY);
> +		if (error) {
>  			dev_err(dev, "Failed to set standby mode\n");
>  			regulator_disable(haptics->regulator);
> -			goto out;
> +			return error;
>  		}
>  
> -		ret = regulator_disable(haptics->regulator);
> -		if (ret) {
> +		error = regulator_disable(haptics->regulator);
> +		if (error) {
>  			dev_err(dev, "Failed to disable regulator\n");
>  			regmap_update_bits(haptics->regmap,
>  					   DRV2665_CTRL_2,
>  					   DRV2665_STANDBY, 0);
> +			return error;
>  		}
>  	}
> -out:
> -	mutex_unlock(&haptics->input_dev->mutex);
> -	return ret;
> +
> +	return 0;
>  }
>  
>  static int drv2665_resume(struct device *dev)
>  {
>  	struct drv2665_data *haptics = dev_get_drvdata(dev);
> -	int ret = 0;
> +	int error;
>  
> -	mutex_lock(&haptics->input_dev->mutex);
> +	guard(mutex)(&haptics->input_dev->mutex);
>  
>  	if (input_device_enabled(haptics->input_dev)) {
> -		ret = regulator_enable(haptics->regulator);
> -		if (ret) {
> +		error = regulator_enable(haptics->regulator);
> +		if (error) {
>  			dev_err(dev, "Failed to enable regulator\n");
> -			goto out;
> +			return error;
>  		}
>  
> -		ret = regmap_update_bits(haptics->regmap, DRV2665_CTRL_2,
> -					 DRV2665_STANDBY, 0);
> -		if (ret) {
> +		error = regmap_update_bits(haptics->regmap, DRV2665_CTRL_2,
> +					   DRV2665_STANDBY, 0);
> +		if (error) {
>  			dev_err(dev, "Failed to unset standby mode\n");
>  			regulator_disable(haptics->regulator);
> -			goto out;
> +			return error;
>  		}
>  
>  	}
>  
> -out:
> -	mutex_unlock(&haptics->input_dev->mutex);
> -	return ret;
> +	return 0;
>  }
>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(drv2665_pm_ops, drv2665_suspend, drv2665_resume);

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 09/22] Input: drv2667 - use guard notation when acquiring mutex

[Dmitry Torokhov]

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/drv2667.c | 44 +++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/input/misc/drv2667.c b/drivers/input/misc/drv2667.c
index ad49845374b9..906292625f84 100644
--- a/drivers/input/misc/drv2667.c
+++ b/drivers/input/misc/drv2667.c
@@ -402,59 +402,57 @@ static int drv2667_probe(struct i2c_client *client)
 static int drv2667_suspend(struct device *dev)
 {
 	struct drv2667_data *haptics = dev_get_drvdata(dev);
-	int ret = 0;
+	int error;
 
-	mutex_lock(&haptics->input_dev->mutex);
+	guard(mutex)(&haptics->input_dev->mutex);
 
 	if (input_device_enabled(haptics->input_dev)) {
-		ret = regmap_update_bits(haptics->regmap, DRV2667_CTRL_2,
-					 DRV2667_STANDBY, DRV2667_STANDBY);
-		if (ret) {
+		error = regmap_update_bits(haptics->regmap, DRV2667_CTRL_2,
+					   DRV2667_STANDBY, DRV2667_STANDBY);
+		if (error) {
 			dev_err(dev, "Failed to set standby mode\n");
 			regulator_disable(haptics->regulator);
-			goto out;
+			return error;
 		}
 
-		ret = regulator_disable(haptics->regulator);
-		if (ret) {
+		error = regulator_disable(haptics->regulator);
+		if (error) {
 			dev_err(dev, "Failed to disable regulator\n");
 			regmap_update_bits(haptics->regmap,
 					   DRV2667_CTRL_2,
 					   DRV2667_STANDBY, 0);
+			return error;
 		}
 	}
-out:
-	mutex_unlock(&haptics->input_dev->mutex);
-	return ret;
+
+	return 0;
 }
 
 static int drv2667_resume(struct device *dev)
 {
 	struct drv2667_data *haptics = dev_get_drvdata(dev);
-	int ret = 0;
+	int error;
 
-	mutex_lock(&haptics->input_dev->mutex);
+	guard(mutex)(&haptics->input_dev->mutex);
 
 	if (input_device_enabled(haptics->input_dev)) {
-		ret = regulator_enable(haptics->regulator);
-		if (ret) {
+		error = regulator_enable(haptics->regulator);
+		if (error) {
 			dev_err(dev, "Failed to enable regulator\n");
-			goto out;
+			return error;
 		}
 
-		ret = regmap_update_bits(haptics->regmap, DRV2667_CTRL_2,
-					 DRV2667_STANDBY, 0);
-		if (ret) {
+		error = regmap_update_bits(haptics->regmap, DRV2667_CTRL_2,
+					   DRV2667_STANDBY, 0);
+		if (error) {
 			dev_err(dev, "Failed to unset standby mode\n");
 			regulator_disable(haptics->regulator);
-			goto out;
+			return error;
 		}
 
 	}
 
-out:
-	mutex_unlock(&haptics->input_dev->mutex);
-	return ret;
+	return 0;
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(drv2667_pm_ops, drv2667_suspend, drv2667_resume);
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 09/22] Input: drv2667 - use guard notation when acquiring mutex

[Javier Carrasco]

On 04/09/2024 06:42, 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/drv2667.c | 44 +++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/input/misc/drv2667.c b/drivers/input/misc/drv2667.c
> index ad49845374b9..906292625f84 100644
> --- a/drivers/input/misc/drv2667.c
> +++ b/drivers/input/misc/drv2667.c
> @@ -402,59 +402,57 @@ static int drv2667_probe(struct i2c_client *client)
>  static int drv2667_suspend(struct device *dev)
>  {
>  	struct drv2667_data *haptics = dev_get_drvdata(dev);
> -	int ret = 0;
> +	int error;
>  
> -	mutex_lock(&haptics->input_dev->mutex);
> +	guard(mutex)(&haptics->input_dev->mutex);
>  
>  	if (input_device_enabled(haptics->input_dev)) {
> -		ret = regmap_update_bits(haptics->regmap, DRV2667_CTRL_2,
> -					 DRV2667_STANDBY, DRV2667_STANDBY);
> -		if (ret) {
> +		error = regmap_update_bits(haptics->regmap, DRV2667_CTRL_2,
> +					   DRV2667_STANDBY, DRV2667_STANDBY);
> +		if (error) {
>  			dev_err(dev, "Failed to set standby mode\n");
>  			regulator_disable(haptics->regulator);
> -			goto out;
> +			return error;
>  		}
>  
> -		ret = regulator_disable(haptics->regulator);
> -		if (ret) {
> +		error = regulator_disable(haptics->regulator);
> +		if (error) {
>  			dev_err(dev, "Failed to disable regulator\n");
>  			regmap_update_bits(haptics->regmap,
>  					   DRV2667_CTRL_2,
>  					   DRV2667_STANDBY, 0);
> +			return error;
>  		}
>  	}
> -out:
> -	mutex_unlock(&haptics->input_dev->mutex);
> -	return ret;
> +
> +	return 0;
>  }
>  
>  static int drv2667_resume(struct device *dev)
>  {
>  	struct drv2667_data *haptics = dev_get_drvdata(dev);
> -	int ret = 0;
> +	int error;
>  
> -	mutex_lock(&haptics->input_dev->mutex);
> +	guard(mutex)(&haptics->input_dev->mutex);
>  
>  	if (input_device_enabled(haptics->input_dev)) {
> -		ret = regulator_enable(haptics->regulator);
> -		if (ret) {
> +		error = regulator_enable(haptics->regulator);
> +		if (error) {
>  			dev_err(dev, "Failed to enable regulator\n");
> -			goto out;
> +			return error;
>  		}
>  
> -		ret = regmap_update_bits(haptics->regmap, DRV2667_CTRL_2,
> -					 DRV2667_STANDBY, 0);
> -		if (ret) {
> +		error = regmap_update_bits(haptics->regmap, DRV2667_CTRL_2,
> +					   DRV2667_STANDBY, 0);
> +		if (error) {
>  			dev_err(dev, "Failed to unset standby mode\n");
>  			regulator_disable(haptics->regulator);
> -			goto out;
> +			return error;
>  		}
>  
>  	}
>  
> -out:
> -	mutex_unlock(&haptics->input_dev->mutex);
> -	return ret;
> +	return 0;
>  }
>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(drv2667_pm_ops, drv2667_suspend, drv2667_resume);


Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 10/22] Input: ideapad_slidebar - use guard notation when acquiring spinlock

[Dmitry Torokhov]

Using guard notation makes the code more compact and error handling
more robust by ensuring that locks are released in all code paths
when control leaves critical section.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/ideapad_slidebar.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/input/misc/ideapad_slidebar.c b/drivers/input/misc/ideapad_slidebar.c
index fa4e7f67d713..592bd159a194 100644
--- a/drivers/input/misc/ideapad_slidebar.c
+++ b/drivers/input/misc/ideapad_slidebar.c
@@ -95,41 +95,29 @@ static struct platform_device *slidebar_platform_dev;
 
 static u8 slidebar_pos_get(void)
 {
-	u8 res;
-	unsigned long flags;
+	guard(spinlock_irqsave)(&io_lock);
 
-	spin_lock_irqsave(&io_lock, flags);
 	outb(0xf4, 0xff29);
 	outb(0xbf, 0xff2a);
-	res = inb(0xff2b);
-	spin_unlock_irqrestore(&io_lock, flags);
-
-	return res;
+	return inb(0xff2b);
 }
 
 static u8 slidebar_mode_get(void)
 {
-	u8 res;
-	unsigned long flags;
+	guard(spinlock_irqsave)(&io_lock);
 
-	spin_lock_irqsave(&io_lock, flags);
 	outb(0xf7, 0xff29);
 	outb(0x8b, 0xff2a);
-	res = inb(0xff2b);
-	spin_unlock_irqrestore(&io_lock, flags);
-
-	return res;
+	return inb(0xff2b);
 }
 
 static void slidebar_mode_set(u8 mode)
 {
-	unsigned long flags;
+	guard(spinlock_irqsave)(&io_lock);
 
-	spin_lock_irqsave(&io_lock, flags);
 	outb(0xf7, 0xff29);
 	outb(0x8b, 0xff2a);
 	outb(mode, 0xff2b);
-	spin_unlock_irqrestore(&io_lock, flags);
 }
 
 static bool slidebar_i8042_filter(unsigned char data, unsigned char str,
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 10/22] Input: ideapad_slidebar - use guard notation when acquiring spinlock

[Javier Carrasco]

On 04/09/2024 06:42, Dmitry Torokhov wrote:
> Using guard notation makes the code more compact and error handling
> more robust by ensuring that locks are released in all code paths
> when control leaves critical section.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/ideapad_slidebar.c | 22 +++++-----------------
>  1 file changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/input/misc/ideapad_slidebar.c b/drivers/input/misc/ideapad_slidebar.c
> index fa4e7f67d713..592bd159a194 100644
> --- a/drivers/input/misc/ideapad_slidebar.c
> +++ b/drivers/input/misc/ideapad_slidebar.c
> @@ -95,41 +95,29 @@ static struct platform_device *slidebar_platform_dev;
>  
>  static u8 slidebar_pos_get(void)
>  {
> -	u8 res;
> -	unsigned long flags;
> +	guard(spinlock_irqsave)(&io_lock);
>  
> -	spin_lock_irqsave(&io_lock, flags);
>  	outb(0xf4, 0xff29);
>  	outb(0xbf, 0xff2a);
> -	res = inb(0xff2b);
> -	spin_unlock_irqrestore(&io_lock, flags);
> -
> -	return res;
> +	return inb(0xff2b);
>  }
>  
>  static u8 slidebar_mode_get(void)
>  {
> -	u8 res;
> -	unsigned long flags;
> +	guard(spinlock_irqsave)(&io_lock);
>  
> -	spin_lock_irqsave(&io_lock, flags);
>  	outb(0xf7, 0xff29);
>  	outb(0x8b, 0xff2a);
> -	res = inb(0xff2b);
> -	spin_unlock_irqrestore(&io_lock, flags);
> -
> -	return res;
> +	return inb(0xff2b);
>  }
>  
>  static void slidebar_mode_set(u8 mode)
>  {
> -	unsigned long flags;
> +	guard(spinlock_irqsave)(&io_lock);
>  
> -	spin_lock_irqsave(&io_lock, flags);
>  	outb(0xf7, 0xff29);
>  	outb(0x8b, 0xff2a);
>  	outb(mode, 0xff2b);
> -	spin_unlock_irqrestore(&io_lock, flags);
>  }
>  
>  static bool slidebar_i8042_filter(unsigned char data, unsigned char str,

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 11/22] Input: ibm-panel - use guard notation when acquiring spinlock

[Dmitry Torokhov]

Using guard notation makes the code more compact and error handling
more robust by ensuring that locks are released in all code paths
when control leaves critical section.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/ibm-panel.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/input/misc/ibm-panel.c b/drivers/input/misc/ibm-panel.c
index 867ac7aa10d2..aa48f62d7ea0 100644
--- a/drivers/input/misc/ibm-panel.c
+++ b/drivers/input/misc/ibm-panel.c
@@ -77,12 +77,11 @@ static void ibm_panel_process_command(struct ibm_panel *panel)
 static int ibm_panel_i2c_slave_cb(struct i2c_client *client,
 				  enum i2c_slave_event event, u8 *val)
 {
-	unsigned long flags;
 	struct ibm_panel *panel = i2c_get_clientdata(client);
 
 	dev_dbg(&panel->input->dev, "event: %u data: %02x\n", event, *val);
 
-	spin_lock_irqsave(&panel->lock, flags);
+	guard(spinlock_irqsave)(&panel->lock);
 
 	switch (event) {
 	case I2C_SLAVE_STOP:
@@ -114,8 +113,6 @@ static int ibm_panel_i2c_slave_cb(struct i2c_client *client,
 		break;
 	}
 
-	spin_unlock_irqrestore(&panel->lock, flags);
-
 	return 0;
 }
 
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 11/22] Input: ibm-panel - use guard notation when acquiring spinlock

[Javier Carrasco]

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 locks are released in all code paths
> when control leaves critical section.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/ibm-panel.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/input/misc/ibm-panel.c b/drivers/input/misc/ibm-panel.c
> index 867ac7aa10d2..aa48f62d7ea0 100644
> --- a/drivers/input/misc/ibm-panel.c
> +++ b/drivers/input/misc/ibm-panel.c
> @@ -77,12 +77,11 @@ static void ibm_panel_process_command(struct ibm_panel *panel)
>  static int ibm_panel_i2c_slave_cb(struct i2c_client *client,
>  				  enum i2c_slave_event event, u8 *val)
>  {
> -	unsigned long flags;
>  	struct ibm_panel *panel = i2c_get_clientdata(client);
>  
>  	dev_dbg(&panel->input->dev, "event: %u data: %02x\n", event, *val);
>  
> -	spin_lock_irqsave(&panel->lock, flags);
> +	guard(spinlock_irqsave)(&panel->lock);
>  
>  	switch (event) {
>  	case I2C_SLAVE_STOP:
> @@ -114,8 +113,6 @@ static int ibm_panel_i2c_slave_cb(struct i2c_client *client,
>  		break;
>  	}
>  
> -	spin_unlock_irqrestore(&panel->lock, flags);
> -
>  	return 0;
>  }
>  

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Re: [PATCH 11/22] Input: ibm-panel - use guard notation when acquiring spinlock

[Eddie James]

On 9/3/24 23:47, Dmitry Torokhov wrote:
> Using guard notation makes the code more compact and error handling
> more robust by ensuring that locks are released in all code paths
> when control leaves critical section.


Reviewed-by: Eddie James <eajames@linux.ibm.com>


>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>   drivers/input/misc/ibm-panel.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/input/misc/ibm-panel.c b/drivers/input/misc/ibm-panel.c
> index 867ac7aa10d2..aa48f62d7ea0 100644
> --- a/drivers/input/misc/ibm-panel.c
> +++ b/drivers/input/misc/ibm-panel.c
> @@ -77,12 +77,11 @@ static void ibm_panel_process_command(struct ibm_panel *panel)
>   static int ibm_panel_i2c_slave_cb(struct i2c_client *client,
>   				  enum i2c_slave_event event, u8 *val)
>   {
> -	unsigned long flags;
>   	struct ibm_panel *panel = i2c_get_clientdata(client);
>   
>   	dev_dbg(&panel->input->dev, "event: %u data: %02x\n", event, *val);
>   
> -	spin_lock_irqsave(&panel->lock, flags);
> +	guard(spinlock_irqsave)(&panel->lock);
>   
>   	switch (event) {
>   	case I2C_SLAVE_STOP:
> @@ -114,8 +113,6 @@ static int ibm_panel_i2c_slave_cb(struct i2c_client *client,
>   		break;
>   	}
>   
> -	spin_unlock_irqrestore(&panel->lock, flags);
> -
>   	return 0;
>   }
>   

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

[Dmitry Torokhov]

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

[Javier Carrasco]

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

[Dmitry Torokhov]

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

[Javier Carrasco]

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

[Dmitry Torokhov]

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

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

[Jeff LaBundy]

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

[PATCH 13/22] Input: iqs269a - use cleanup facility for fwnodes

[Dmitry Torokhov]

Use __free(fwnode_handle) cleanup facility to ensure that references to
acquired fwnodes are dropped at appropriate times automatically.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/iqs269a.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
index c34d847fa4af..1851848e2cd3 100644
--- a/drivers/input/misc/iqs269a.c
+++ b/drivers/input/misc/iqs269a.c
@@ -550,7 +550,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
 			     const struct fwnode_handle *ch_node)
 {
 	struct i2c_client *client = iqs269->client;
-	struct fwnode_handle *ev_node;
 	struct iqs269_ch_reg *ch_reg;
 	u16 engine_a, engine_b;
 	unsigned int reg, val;
@@ -727,8 +726,9 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
 	}
 
 	for (i = 0; i < ARRAY_SIZE(iqs269_events); i++) {
-		ev_node = fwnode_get_named_child_node(ch_node,
-						      iqs269_events[i].name);
+		struct fwnode_handle *ev_node __free(fwnode_handle) =
+			fwnode_get_named_child_node(ch_node,
+						    iqs269_events[i].name);
 		if (!ev_node)
 			continue;
 
@@ -737,7 +737,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
 				dev_err(&client->dev,
 					"Invalid channel %u threshold: %u\n",
 					reg, val);
-				fwnode_handle_put(ev_node);
 				return -EINVAL;
 			}
 
@@ -751,7 +750,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
 				dev_err(&client->dev,
 					"Invalid channel %u hysteresis: %u\n",
 					reg, val);
-				fwnode_handle_put(ev_node);
 				return -EINVAL;
 			}
 
@@ -767,7 +765,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
 		}
 
 		error = fwnode_property_read_u32(ev_node, "linux,code", &val);
-		fwnode_handle_put(ev_node);
 		if (error == -EINVAL) {
 			continue;
 		} else if (error) {
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 13/22] Input: iqs269a - use cleanup facility for fwnodes

[Javier Carrasco]

On 04/09/2024 06:48, Dmitry Torokhov wrote:
> Use __free(fwnode_handle) cleanup facility to ensure that references to
> acquired fwnodes are dropped at appropriate times automatically.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/iqs269a.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
> index c34d847fa4af..1851848e2cd3 100644
> --- a/drivers/input/misc/iqs269a.c
> +++ b/drivers/input/misc/iqs269a.c
> @@ -550,7 +550,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
>  			     const struct fwnode_handle *ch_node)
>  {
>  	struct i2c_client *client = iqs269->client;
> -	struct fwnode_handle *ev_node;
>  	struct iqs269_ch_reg *ch_reg;
>  	u16 engine_a, engine_b;
>  	unsigned int reg, val;
> @@ -727,8 +726,9 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(iqs269_events); i++) {
> -		ev_node = fwnode_get_named_child_node(ch_node,
> -						      iqs269_events[i].name);
> +		struct fwnode_handle *ev_node __free(fwnode_handle) =
> +			fwnode_get_named_child_node(ch_node,
> +						    iqs269_events[i].name);
>  		if (!ev_node)
>  			continue;
>  
> @@ -737,7 +737,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
>  				dev_err(&client->dev,
>  					"Invalid channel %u threshold: %u\n",
>  					reg, val);
> -				fwnode_handle_put(ev_node);
>  				return -EINVAL;
>  			}
>  
> @@ -751,7 +750,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
>  				dev_err(&client->dev,
>  					"Invalid channel %u hysteresis: %u\n",
>  					reg, val);
> -				fwnode_handle_put(ev_node);
>  				return -EINVAL;
>  			}
>  
> @@ -767,7 +765,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
>  		}
>  
>  		error = fwnode_property_read_u32(ev_node, "linux,code", &val);
> -		fwnode_handle_put(ev_node);
>  		if (error == -EINVAL) {
>  			continue;
>  		} else if (error) {

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Re: [PATCH 13/22] Input: iqs269a - use cleanup facility for fwnodes

[Jeff LaBundy]

Hi Dmitry,

On Tue, Sep 03, 2024 at 09:48:05PM -0700, Dmitry Torokhov wrote:
> Use __free(fwnode_handle) cleanup facility to ensure that references to
> acquired fwnodes are dropped at appropriate times automatically.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

> ---
>  drivers/input/misc/iqs269a.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
> index c34d847fa4af..1851848e2cd3 100644
> --- a/drivers/input/misc/iqs269a.c
> +++ b/drivers/input/misc/iqs269a.c
> @@ -550,7 +550,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
>  			     const struct fwnode_handle *ch_node)
>  {
>  	struct i2c_client *client = iqs269->client;
> -	struct fwnode_handle *ev_node;
>  	struct iqs269_ch_reg *ch_reg;
>  	u16 engine_a, engine_b;
>  	unsigned int reg, val;
> @@ -727,8 +726,9 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(iqs269_events); i++) {
> -		ev_node = fwnode_get_named_child_node(ch_node,
> -						      iqs269_events[i].name);
> +		struct fwnode_handle *ev_node __free(fwnode_handle) =
> +			fwnode_get_named_child_node(ch_node,
> +						    iqs269_events[i].name);
>  		if (!ev_node)
>  			continue;
>  
> @@ -737,7 +737,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
>  				dev_err(&client->dev,
>  					"Invalid channel %u threshold: %u\n",
>  					reg, val);
> -				fwnode_handle_put(ev_node);
>  				return -EINVAL;
>  			}
>  
> @@ -751,7 +750,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
>  				dev_err(&client->dev,
>  					"Invalid channel %u hysteresis: %u\n",
>  					reg, val);
> -				fwnode_handle_put(ev_node);
>  				return -EINVAL;
>  			}
>  
> @@ -767,7 +765,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
>  		}
>  
>  		error = fwnode_property_read_u32(ev_node, "linux,code", &val);
> -		fwnode_handle_put(ev_node);
>  		if (error == -EINVAL) {
>  			continue;
>  		} else if (error) {
> -- 
> 2.46.0.469.g59c65b2a67-goog
> 

Kind regards,
Jeff LaBundy

[PATCH 14/22] Input: iqs626a - use cleanup facility for fwnodes

[Dmitry Torokhov]

Use __free(fwnode_handle) cleanup facility to ensure that references to
acquired fwnodes are dropped at appropriate times automatically.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/iqs626a.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
index 0dab54d3a060..7a6e6927f331 100644
--- a/drivers/input/misc/iqs626a.c
+++ b/drivers/input/misc/iqs626a.c
@@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
 {
 	struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
 	struct i2c_client *client = iqs626->client;
-	struct fwnode_handle *ev_node;
 	const char *ev_name;
 	u8 *thresh, *hyst;
 	unsigned int val;
@@ -501,6 +500,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
 		if (!iqs626_channels[ch_id].events[i])
 			continue;
 
+		struct fwnode_handle *ev_node __free(fwnode_handle) = NULL;
 		if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) {
 			/*
 			 * Trackpad touch events are simply described under the
@@ -530,7 +530,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
 					dev_err(&client->dev,
 						"Invalid input type: %u\n",
 						val);
-					fwnode_handle_put(ev_node);
 					return -EINVAL;
 				}
 
@@ -545,7 +544,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
 				dev_err(&client->dev,
 					"Invalid %s channel hysteresis: %u\n",
 					fwnode_get_name(ch_node), val);
-				fwnode_handle_put(ev_node);
 				return -EINVAL;
 			}
 
@@ -566,7 +564,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
 				dev_err(&client->dev,
 					"Invalid %s channel threshold: %u\n",
 					fwnode_get_name(ch_node), val);
-				fwnode_handle_put(ev_node);
 				return -EINVAL;
 			}
 
@@ -575,8 +572,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
 			else
 				*(thresh + iqs626_events[i].th_offs) = val;
 		}
-
-		fwnode_handle_put(ev_node);
 	}
 
 	return 0;
@@ -774,12 +769,12 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
 	for (i = 0; i < iqs626_channels[ch_id].num_ch; i++) {
 		u8 *ati_base = &sys_reg->tp_grp_reg.ch_reg_tp[i].ati_base;
 		u8 *thresh = &sys_reg->tp_grp_reg.ch_reg_tp[i].thresh;
-		struct fwnode_handle *tc_node;
 		char tc_name[10];
 
 		snprintf(tc_name, sizeof(tc_name), "channel-%d", i);
 
-		tc_node = fwnode_get_named_child_node(ch_node, tc_name);
+		struct fwnode_handle *tc_node __free(fwnode_handle) =
+				fwnode_get_named_child_node(ch_node, tc_name);
 		if (!tc_node)
 			continue;
 
@@ -790,7 +785,6 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
 				dev_err(&client->dev,
 					"Invalid %s %s ATI base: %u\n",
 					fwnode_get_name(ch_node), tc_name, val);
-				fwnode_handle_put(tc_node);
 				return -EINVAL;
 			}
 
@@ -803,14 +797,11 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
 				dev_err(&client->dev,
 					"Invalid %s %s threshold: %u\n",
 					fwnode_get_name(ch_node), tc_name, val);
-				fwnode_handle_put(tc_node);
 				return -EINVAL;
 			}
 
 			*thresh = val;
 		}
-
-		fwnode_handle_put(tc_node);
 	}
 
 	if (!fwnode_property_present(ch_node, "linux,keycodes"))
@@ -1233,7 +1224,6 @@ static int iqs626_parse_prop(struct iqs626_private *iqs626)
 {
 	struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
 	struct i2c_client *client = iqs626->client;
-	struct fwnode_handle *ch_node;
 	unsigned int val;
 	int error, i;
 	u16 general;
@@ -1375,13 +1365,13 @@ static int iqs626_parse_prop(struct iqs626_private *iqs626)
 	sys_reg->active = 0;
 
 	for (i = 0; i < ARRAY_SIZE(iqs626_channels); i++) {
-		ch_node = device_get_named_child_node(&client->dev,
-						      iqs626_channels[i].name);
+		struct fwnode_handle *ch_node __free(fwnode_handle) =
+			device_get_named_child_node(&client->dev,
+						    iqs626_channels[i].name);
 		if (!ch_node)
 			continue;
 
 		error = iqs626_parse_channel(iqs626, ch_node, i);
-		fwnode_handle_put(ch_node);
 		if (error)
 			return error;
 
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 14/22] Input: iqs626a - use cleanup facility for fwnodes

[Javier Carrasco]

On 04/09/2024 06:48, Dmitry Torokhov wrote:
> Use __free(fwnode_handle) cleanup facility to ensure that references to
> acquired fwnodes are dropped at appropriate times automatically.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/iqs626a.c | 22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> index 0dab54d3a060..7a6e6927f331 100644
> --- a/drivers/input/misc/iqs626a.c
> +++ b/drivers/input/misc/iqs626a.c
> @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  {
>  	struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
>  	struct i2c_client *client = iqs626->client;
> -	struct fwnode_handle *ev_node;
>  	const char *ev_name;
>  	u8 *thresh, *hyst;
>  	unsigned int val;
> @@ -501,6 +500,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  		if (!iqs626_channels[ch_id].events[i])
>  			continue;
>  
> +		struct fwnode_handle *ev_node __free(fwnode_handle) = NULL;
>  		if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) {
>  			/*
>  			 * Trackpad touch events are simply described under the
> @@ -530,7 +530,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  					dev_err(&client->dev,
>  						"Invalid input type: %u\n",
>  						val);
> -					fwnode_handle_put(ev_node);
>  					return -EINVAL;
>  				}
>  
> @@ -545,7 +544,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  				dev_err(&client->dev,
>  					"Invalid %s channel hysteresis: %u\n",
>  					fwnode_get_name(ch_node), val);
> -				fwnode_handle_put(ev_node);
>  				return -EINVAL;
>  			}
>  
> @@ -566,7 +564,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  				dev_err(&client->dev,
>  					"Invalid %s channel threshold: %u\n",
>  					fwnode_get_name(ch_node), val);
> -				fwnode_handle_put(ev_node);
>  				return -EINVAL;
>  			}
>  
> @@ -575,8 +572,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  			else
>  				*(thresh + iqs626_events[i].th_offs) = val;
>  		}
> -
> -		fwnode_handle_put(ev_node);
>  	}
>  
>  	return 0;
> @@ -774,12 +769,12 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
>  	for (i = 0; i < iqs626_channels[ch_id].num_ch; i++) {
>  		u8 *ati_base = &sys_reg->tp_grp_reg.ch_reg_tp[i].ati_base;
>  		u8 *thresh = &sys_reg->tp_grp_reg.ch_reg_tp[i].thresh;
> -		struct fwnode_handle *tc_node;
>  		char tc_name[10];
>  
>  		snprintf(tc_name, sizeof(tc_name), "channel-%d", i);
>  
> -		tc_node = fwnode_get_named_child_node(ch_node, tc_name);
> +		struct fwnode_handle *tc_node __free(fwnode_handle) =
> +				fwnode_get_named_child_node(ch_node, tc_name);
>  		if (!tc_node)
>  			continue;
>  
> @@ -790,7 +785,6 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
>  				dev_err(&client->dev,
>  					"Invalid %s %s ATI base: %u\n",
>  					fwnode_get_name(ch_node), tc_name, val);
> -				fwnode_handle_put(tc_node);
>  				return -EINVAL;
>  			}
>  
> @@ -803,14 +797,11 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
>  				dev_err(&client->dev,
>  					"Invalid %s %s threshold: %u\n",
>  					fwnode_get_name(ch_node), tc_name, val);
> -				fwnode_handle_put(tc_node);
>  				return -EINVAL;
>  			}
>  
>  			*thresh = val;
>  		}
> -
> -		fwnode_handle_put(tc_node);
>  	}
>  
>  	if (!fwnode_property_present(ch_node, "linux,keycodes"))
> @@ -1233,7 +1224,6 @@ static int iqs626_parse_prop(struct iqs626_private *iqs626)
>  {
>  	struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
>  	struct i2c_client *client = iqs626->client;
> -	struct fwnode_handle *ch_node;
>  	unsigned int val;
>  	int error, i;
>  	u16 general;
> @@ -1375,13 +1365,13 @@ static int iqs626_parse_prop(struct iqs626_private *iqs626)
>  	sys_reg->active = 0;
>  
>  	for (i = 0; i < ARRAY_SIZE(iqs626_channels); i++) {
> -		ch_node = device_get_named_child_node(&client->dev,
> -						      iqs626_channels[i].name);
> +		struct fwnode_handle *ch_node __free(fwnode_handle) =
> +			device_get_named_child_node(&client->dev,
> +						    iqs626_channels[i].name);
>  		if (!ch_node)
>  			continue;
>  
>  		error = iqs626_parse_channel(iqs626, ch_node, i);
> -		fwnode_handle_put(ch_node);
>  		if (error)
>  			return error;
>  

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Re: [PATCH 14/22] Input: iqs626a - use cleanup facility for fwnodes

[Jeff LaBundy]

Hi Dmitry,

On Tue, Sep 03, 2024 at 09:48:13PM -0700, Dmitry Torokhov wrote:
> Use __free(fwnode_handle) cleanup facility to ensure that references to
> acquired fwnodes are dropped at appropriate times automatically.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/iqs626a.c | 22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> index 0dab54d3a060..7a6e6927f331 100644
> --- a/drivers/input/misc/iqs626a.c
> +++ b/drivers/input/misc/iqs626a.c
> @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  {
>  	struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
>  	struct i2c_client *client = iqs626->client;
> -	struct fwnode_handle *ev_node;
>  	const char *ev_name;
>  	u8 *thresh, *hyst;
>  	unsigned int val;
> @@ -501,6 +500,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  		if (!iqs626_channels[ch_id].events[i])
>  			continue;
>  
> +		struct fwnode_handle *ev_node __free(fwnode_handle) = NULL;

This seems to deviate from what I understand to be a more conventional
style of declaring variables at the top of the scope, and separate from
actual code, like below:


	for (i = 0; i < ARRAY_SIZE(iqs626_events); i++) {
		struct fwnode_handle *ev_node __free(fwnode_handle);

		if (!iqs626_channels[ch_id].events[i])
			continue;

I also did not see any reason to explicitly declare the variable as NULL;
let me know in case I have misunderstood.

>  		if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) {
>  			/*
>  			 * Trackpad touch events are simply described under the
> @@ -530,7 +530,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  					dev_err(&client->dev,
>  						"Invalid input type: %u\n",
>  						val);
> -					fwnode_handle_put(ev_node);
>  					return -EINVAL;
>  				}
>  
> @@ -545,7 +544,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  				dev_err(&client->dev,
>  					"Invalid %s channel hysteresis: %u\n",
>  					fwnode_get_name(ch_node), val);
> -				fwnode_handle_put(ev_node);
>  				return -EINVAL;
>  			}
>  
> @@ -566,7 +564,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  				dev_err(&client->dev,
>  					"Invalid %s channel threshold: %u\n",
>  					fwnode_get_name(ch_node), val);
> -				fwnode_handle_put(ev_node);
>  				return -EINVAL;
>  			}
>  
> @@ -575,8 +572,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
>  			else
>  				*(thresh + iqs626_events[i].th_offs) = val;
>  		}
> -
> -		fwnode_handle_put(ev_node);
>  	}
>  
>  	return 0;
> @@ -774,12 +769,12 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
>  	for (i = 0; i < iqs626_channels[ch_id].num_ch; i++) {
>  		u8 *ati_base = &sys_reg->tp_grp_reg.ch_reg_tp[i].ati_base;
>  		u8 *thresh = &sys_reg->tp_grp_reg.ch_reg_tp[i].thresh;
> -		struct fwnode_handle *tc_node;
>  		char tc_name[10];
>  
>  		snprintf(tc_name, sizeof(tc_name), "channel-%d", i);
>  
> -		tc_node = fwnode_get_named_child_node(ch_node, tc_name);
> +		struct fwnode_handle *tc_node __free(fwnode_handle) =
> +				fwnode_get_named_child_node(ch_node, tc_name);

Same here.

>  		if (!tc_node)
>  			continue;
>  
> @@ -790,7 +785,6 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
>  				dev_err(&client->dev,
>  					"Invalid %s %s ATI base: %u\n",
>  					fwnode_get_name(ch_node), tc_name, val);
> -				fwnode_handle_put(tc_node);
>  				return -EINVAL;
>  			}
>  
> @@ -803,14 +797,11 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
>  				dev_err(&client->dev,
>  					"Invalid %s %s threshold: %u\n",
>  					fwnode_get_name(ch_node), tc_name, val);
> -				fwnode_handle_put(tc_node);
>  				return -EINVAL;
>  			}
>  
>  			*thresh = val;
>  		}
> -
> -		fwnode_handle_put(tc_node);
>  	}
>  
>  	if (!fwnode_property_present(ch_node, "linux,keycodes"))
> @@ -1233,7 +1224,6 @@ static int iqs626_parse_prop(struct iqs626_private *iqs626)
>  {
>  	struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
>  	struct i2c_client *client = iqs626->client;
> -	struct fwnode_handle *ch_node;
>  	unsigned int val;
>  	int error, i;
>  	u16 general;
> @@ -1375,13 +1365,13 @@ static int iqs626_parse_prop(struct iqs626_private *iqs626)
>  	sys_reg->active = 0;
>  
>  	for (i = 0; i < ARRAY_SIZE(iqs626_channels); i++) {
> -		ch_node = device_get_named_child_node(&client->dev,
> -						      iqs626_channels[i].name);
> +		struct fwnode_handle *ch_node __free(fwnode_handle) =
> +			device_get_named_child_node(&client->dev,
> +						    iqs626_channels[i].name);
>  		if (!ch_node)
>  			continue;
>  
>  		error = iqs626_parse_channel(iqs626, ch_node, i);
> -		fwnode_handle_put(ch_node);
>  		if (error)
>  			return error;
>  
> -- 
> 2.46.0.469.g59c65b2a67-goog
> 

Kind regards,
Jeff LaBundy

Re: [PATCH 14/22] Input: iqs626a - use cleanup facility for fwnodes

[Dmitry Torokhov]

Hi Jeff,

On Sun, Sep 08, 2024 at 07:02:41PM -0500, Jeff LaBundy wrote:
> Hi Dmitry,
> 
> On Tue, Sep 03, 2024 at 09:48:13PM -0700, Dmitry Torokhov wrote:
> > Use __free(fwnode_handle) cleanup facility to ensure that references to
> > acquired fwnodes are dropped at appropriate times automatically.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/misc/iqs626a.c | 22 ++++++----------------
> >  1 file changed, 6 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> > index 0dab54d3a060..7a6e6927f331 100644
> > --- a/drivers/input/misc/iqs626a.c
> > +++ b/drivers/input/misc/iqs626a.c
> > @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> >  {
> >  	struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
> >  	struct i2c_client *client = iqs626->client;
> > -	struct fwnode_handle *ev_node;
> >  	const char *ev_name;
> >  	u8 *thresh, *hyst;
> >  	unsigned int val;
> > @@ -501,6 +500,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> >  		if (!iqs626_channels[ch_id].events[i])
> >  			continue;
> >  
> > +		struct fwnode_handle *ev_node __free(fwnode_handle) = NULL;
> 
> This seems to deviate from what I understand to be a more conventional
> style of declaring variables at the top of the scope, and separate from
> actual code, like below:

This is follows Linus' guidance on combining declaration and
initialization for variables using __free() cleanup annotations (where
possible). These annotations are the reason for dropping
-Wdeclaration-after-statement from our makefiles. See b5ec6fd286df
("kbuild: Drop -Wdeclaration-after-statement") and discussion in
https://lore.kernel.org/all/CAHk-=wi-RyoUhbChiVaJZoZXheAwnJ7OO=Gxe85BkPAd93TwDA@mail.gmail.com

> 
> 
> 	for (i = 0; i < ARRAY_SIZE(iqs626_events); i++) {
> 		struct fwnode_handle *ev_node __free(fwnode_handle);
> 
> 		if (!iqs626_channels[ch_id].events[i])
> 			continue;

This will result in attempt to "put" a garbage pointer if we follow
"continue" code path. In general __free() pointers have to be
initialized, either to NULL or to a valid object (assuming that cleanup
function can deal with NULLs).

> 
> I also did not see any reason to explicitly declare the variable as NULL;
> let me know in case I have misunderstood.

See the above. Yes, in this particular case it will get a value in both
branches, but I feel it is too fragile and may get messed up if someone
refactors code.

> 
> >  		if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) {
> >  			/*
> >  			 * Trackpad touch events are simply described under the
> > @@ -530,7 +530,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> >  					dev_err(&client->dev,
> >  						"Invalid input type: %u\n",
> >  						val);
> > -					fwnode_handle_put(ev_node);
> >  					return -EINVAL;
> >  				}
> >  
> > @@ -545,7 +544,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> >  				dev_err(&client->dev,
> >  					"Invalid %s channel hysteresis: %u\n",
> >  					fwnode_get_name(ch_node), val);
> > -				fwnode_handle_put(ev_node);
> >  				return -EINVAL;
> >  			}
> >  
> > @@ -566,7 +564,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> >  				dev_err(&client->dev,
> >  					"Invalid %s channel threshold: %u\n",
> >  					fwnode_get_name(ch_node), val);
> > -				fwnode_handle_put(ev_node);
> >  				return -EINVAL;
> >  			}
> >  
> > @@ -575,8 +572,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> >  			else
> >  				*(thresh + iqs626_events[i].th_offs) = val;
> >  		}
> > -
> > -		fwnode_handle_put(ev_node);
> >  	}
> >  
> >  	return 0;
> > @@ -774,12 +769,12 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
> >  	for (i = 0; i < iqs626_channels[ch_id].num_ch; i++) {
> >  		u8 *ati_base = &sys_reg->tp_grp_reg.ch_reg_tp[i].ati_base;
> >  		u8 *thresh = &sys_reg->tp_grp_reg.ch_reg_tp[i].thresh;
> > -		struct fwnode_handle *tc_node;
> >  		char tc_name[10];
> >  
> >  		snprintf(tc_name, sizeof(tc_name), "channel-%d", i);
> >  
> > -		tc_node = fwnode_get_named_child_node(ch_node, tc_name);
> > +		struct fwnode_handle *tc_node __free(fwnode_handle) =
> > +				fwnode_get_named_child_node(ch_node, tc_name);
> 
> Same here.

Yes, combining declaration and initialization is preferred for such
pointers.

Thanks.

-- 
Dmitry

Re: [PATCH 14/22] Input: iqs626a - use cleanup facility for fwnodes

[Jeff LaBundy]

Hi Dmitry,

On Sun, Sep 08, 2024 at 06:31:26PM -0700, Dmitry Torokhov wrote:
> Hi Jeff,
> 
> On Sun, Sep 08, 2024 at 07:02:41PM -0500, Jeff LaBundy wrote:
> > Hi Dmitry,
> > 
> > On Tue, Sep 03, 2024 at 09:48:13PM -0700, Dmitry Torokhov wrote:
> > > Use __free(fwnode_handle) cleanup facility to ensure that references to
> > > acquired fwnodes are dropped at appropriate times automatically.
> > > 
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > >  drivers/input/misc/iqs626a.c | 22 ++++++----------------
> > >  1 file changed, 6 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
> > > index 0dab54d3a060..7a6e6927f331 100644
> > > --- a/drivers/input/misc/iqs626a.c
> > > +++ b/drivers/input/misc/iqs626a.c
> > > @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > >  {
> > >  	struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
> > >  	struct i2c_client *client = iqs626->client;
> > > -	struct fwnode_handle *ev_node;
> > >  	const char *ev_name;
> > >  	u8 *thresh, *hyst;
> > >  	unsigned int val;
> > > @@ -501,6 +500,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > >  		if (!iqs626_channels[ch_id].events[i])
> > >  			continue;
> > >  
> > > +		struct fwnode_handle *ev_node __free(fwnode_handle) = NULL;
> > 
> > This seems to deviate from what I understand to be a more conventional
> > style of declaring variables at the top of the scope, and separate from
> > actual code, like below:
> 
> This is follows Linus' guidance on combining declaration and
> initialization for variables using __free() cleanup annotations (where
> possible). These annotations are the reason for dropping
> -Wdeclaration-after-statement from our makefiles. See b5ec6fd286df
> ("kbuild: Drop -Wdeclaration-after-statement") and discussion in
> https://lore.kernel.org/all/CAHk-=wi-RyoUhbChiVaJZoZXheAwnJ7OO=Gxe85BkPAd93TwDA@mail.gmail.com

Understood; thank you for the reference.

> 
> > 
> > 
> > 	for (i = 0; i < ARRAY_SIZE(iqs626_events); i++) {
> > 		struct fwnode_handle *ev_node __free(fwnode_handle);
> > 
> > 		if (!iqs626_channels[ch_id].events[i])
> > 			continue;
> 
> This will result in attempt to "put" a garbage pointer if we follow
> "continue" code path. In general __free() pointers have to be
> initialized, either to NULL or to a valid object (assuming that cleanup
> function can deal with NULLs).

Great catch; I missed the fact that fwnode_handle_put() is implicitly
being called in all exit paths now.

> 
> > 
> > I also did not see any reason to explicitly declare the variable as NULL;
> > let me know in case I have misunderstood.
> 
> See the above. Yes, in this particular case it will get a value in both
> branches, but I feel it is too fragile and may get messed up if someone
> refactors code.

Based on the above point, I agree with your approach.

> 
> > 
> > >  		if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) {
> > >  			/*
> > >  			 * Trackpad touch events are simply described under the
> > > @@ -530,7 +530,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > >  					dev_err(&client->dev,
> > >  						"Invalid input type: %u\n",
> > >  						val);
> > > -					fwnode_handle_put(ev_node);
> > >  					return -EINVAL;
> > >  				}
> > >  
> > > @@ -545,7 +544,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > >  				dev_err(&client->dev,
> > >  					"Invalid %s channel hysteresis: %u\n",
> > >  					fwnode_get_name(ch_node), val);
> > > -				fwnode_handle_put(ev_node);
> > >  				return -EINVAL;
> > >  			}
> > >  
> > > @@ -566,7 +564,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > >  				dev_err(&client->dev,
> > >  					"Invalid %s channel threshold: %u\n",
> > >  					fwnode_get_name(ch_node), val);
> > > -				fwnode_handle_put(ev_node);
> > >  				return -EINVAL;
> > >  			}
> > >  
> > > @@ -575,8 +572,6 @@ iqs626_parse_events(struct iqs626_private *iqs626,
> > >  			else
> > >  				*(thresh + iqs626_events[i].th_offs) = val;
> > >  		}
> > > -
> > > -		fwnode_handle_put(ev_node);
> > >  	}
> > >  
> > >  	return 0;
> > > @@ -774,12 +769,12 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
> > >  	for (i = 0; i < iqs626_channels[ch_id].num_ch; i++) {
> > >  		u8 *ati_base = &sys_reg->tp_grp_reg.ch_reg_tp[i].ati_base;
> > >  		u8 *thresh = &sys_reg->tp_grp_reg.ch_reg_tp[i].thresh;
> > > -		struct fwnode_handle *tc_node;
> > >  		char tc_name[10];
> > >  
> > >  		snprintf(tc_name, sizeof(tc_name), "channel-%d", i);
> > >  
> > > -		tc_node = fwnode_get_named_child_node(ch_node, tc_name);
> > > +		struct fwnode_handle *tc_node __free(fwnode_handle) =
> > > +				fwnode_get_named_child_node(ch_node, tc_name);
> > 
> > Same here.
> 
> Yes, combining declaration and initialization is preferred for such
> pointers.

ACK. Please feel free to add:

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

> 
> Thanks.
> 
> -- 
> Dmitry

Thank you for the discussion!

Kind regards,
Jeff LaBundy

[PATCH 15/22] Input: iqs7222 - use cleanup facility for fwnodes

[Dmitry Torokhov]

Use __free(fwnode_handle) cleanup facility to ensure that references to
acquired fwnodes are dropped at appropriate times automatically.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/iqs7222.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
index 9ca5a743f19f..d9b87606ff7a 100644
--- a/drivers/input/misc/iqs7222.c
+++ b/drivers/input/misc/iqs7222.c
@@ -2385,9 +2385,9 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
 	for (i = 0; i < ARRAY_SIZE(iqs7222_kp_events); i++) {
 		const char *event_name = iqs7222_kp_events[i].name;
 		u16 event_enable = iqs7222_kp_events[i].enable;
-		struct fwnode_handle *event_node;
 
-		event_node = fwnode_get_named_child_node(chan_node, event_name);
+		struct fwnode_handle *event_node __free(fwnode_handle) =
+			fwnode_get_named_child_node(chan_node, event_name);
 		if (!event_node)
 			continue;
 
@@ -2408,7 +2408,6 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
 				dev_err(&client->dev,
 					"Invalid %s press timeout: %u\n",
 					fwnode_get_name(event_node), val);
-				fwnode_handle_put(event_node);
 				return -EINVAL;
 			}
 
@@ -2418,7 +2417,6 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
 			dev_err(&client->dev,
 				"Failed to read %s press timeout: %d\n",
 				fwnode_get_name(event_node), error);
-			fwnode_handle_put(event_node);
 			return error;
 		}
 
@@ -2429,7 +2427,6 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
 					    dev_desc->touch_link - (i ? 0 : 2),
 					    &iqs7222->kp_type[chan_index][i],
 					    &iqs7222->kp_code[chan_index][i]);
-		fwnode_handle_put(event_node);
 		if (error)
 			return error;
 
@@ -2604,10 +2601,10 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
 
 	for (i = 0; i < ARRAY_SIZE(iqs7222_sl_events); i++) {
 		const char *event_name = iqs7222_sl_events[i].name;
-		struct fwnode_handle *event_node;
 		enum iqs7222_reg_key_id reg_key;
 
-		event_node = fwnode_get_named_child_node(sldr_node, event_name);
+		struct fwnode_handle *event_node __free(fwnode_handle) =
+			fwnode_get_named_child_node(sldr_node, event_name);
 		if (!event_node)
 			continue;
 
@@ -2639,7 +2636,6 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
 					      : sldr_setup[4 + reg_offset],
 					    NULL,
 					    &iqs7222->sl_code[sldr_index][i]);
-		fwnode_handle_put(event_node);
 		if (error)
 			return error;
 
@@ -2742,9 +2738,9 @@ static int iqs7222_parse_tpad(struct iqs7222_private *iqs7222,
 
 	for (i = 0; i < ARRAY_SIZE(iqs7222_tp_events); i++) {
 		const char *event_name = iqs7222_tp_events[i].name;
-		struct fwnode_handle *event_node;
 
-		event_node = fwnode_get_named_child_node(tpad_node, event_name);
+		struct fwnode_handle *event_node __free(fwnode_handle) =
+			fwnode_get_named_child_node(tpad_node, event_name);
 		if (!event_node)
 			continue;
 
@@ -2760,7 +2756,6 @@ static int iqs7222_parse_tpad(struct iqs7222_private *iqs7222,
 					    iqs7222_tp_events[i].link, 1566,
 					    NULL,
 					    &iqs7222->tp_code[i]);
-		fwnode_handle_put(event_node);
 		if (error)
 			return error;
 
@@ -2818,9 +2813,9 @@ static int iqs7222_parse_reg_grp(struct iqs7222_private *iqs7222,
 				 int reg_grp_index)
 {
 	struct i2c_client *client = iqs7222->client;
-	struct fwnode_handle *reg_grp_node;
 	int error;
 
+	struct fwnode_handle *reg_grp_node __free(fwnode_handle) = NULL;
 	if (iqs7222_reg_grp_names[reg_grp]) {
 		char reg_grp_name[16];
 
@@ -2838,14 +2833,17 @@ static int iqs7222_parse_reg_grp(struct iqs7222_private *iqs7222,
 
 	error = iqs7222_parse_props(iqs7222, reg_grp_node, reg_grp_index,
 				    reg_grp, IQS7222_REG_KEY_NONE);
+	if (error)
+		return error;
 
-	if (!error && iqs7222_parse_extra[reg_grp])
+	if (iqs7222_parse_extra[reg_grp]) {
 		error = iqs7222_parse_extra[reg_grp](iqs7222, reg_grp_node,
 						     reg_grp_index);
+		if (error)
+			return error;
+	}
 
-	fwnode_handle_put(reg_grp_node);
-
-	return error;
+	return 0;
 }
 
 static int iqs7222_parse_all(struct iqs7222_private *iqs7222)
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 15/22] Input: iqs7222 - use cleanup facility for fwnodes

[Javier Carrasco]

Hi Dmitry,

On 04/09/2024 06:48, Dmitry Torokhov wrote:
> Use __free(fwnode_handle) cleanup facility to ensure that references to
> acquired fwnodes are dropped at appropriate times automatically.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/iqs7222.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
> index 9ca5a743f19f..d9b87606ff7a 100644
> --- a/drivers/input/misc/iqs7222.c
> +++ b/drivers/input/misc/iqs7222.c

...

> @@ -2818,9 +2813,9 @@ static int iqs7222_parse_reg_grp(struct iqs7222_private *iqs7222,
>  				 int reg_grp_index)
>  {
>  	struct i2c_client *client = iqs7222->client;
> -	struct fwnode_handle *reg_grp_node;
>  	int error;
>  

Nit: reg_grp_node could stay at the top (where it used to be), as you
are assigning it to NULL because there are no sensible assignments at
this point.

> +	struct fwnode_handle *reg_grp_node __free(fwnode_handle) = NULL;
>  	if (iqs7222_reg_grp_names[reg_grp]) {
>  		char reg_grp_name[16];
>  

Best regards,
Javier Carrasco

Re: [PATCH 15/22] Input: iqs7222 - use cleanup facility for fwnodes

[Dmitry Torokhov]

On Wed, Sep 04, 2024 at 12:50:44PM +0200, Javier Carrasco wrote:
> Hi Dmitry,
> 
> On 04/09/2024 06:48, Dmitry Torokhov wrote:
> > Use __free(fwnode_handle) cleanup facility to ensure that references to
> > acquired fwnodes are dropped at appropriate times automatically.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/misc/iqs7222.c | 30 ++++++++++++++----------------
> >  1 file changed, 14 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
> > index 9ca5a743f19f..d9b87606ff7a 100644
> > --- a/drivers/input/misc/iqs7222.c
> > +++ b/drivers/input/misc/iqs7222.c
> 
> ...
> 
> > @@ -2818,9 +2813,9 @@ static int iqs7222_parse_reg_grp(struct iqs7222_private *iqs7222,
> >  				 int reg_grp_index)
> >  {
> >  	struct i2c_client *client = iqs7222->client;
> > -	struct fwnode_handle *reg_grp_node;
> >  	int error;
> >  
> 
> Nit: reg_grp_node could stay at the top (where it used to be), as you
> are assigning it to NULL because there are no sensible assignments at
> this point.
> 
> > +	struct fwnode_handle *reg_grp_node __free(fwnode_handle) = NULL;
> >  	if (iqs7222_reg_grp_names[reg_grp]) {
> >  		char reg_grp_name[16];

I think this follows Linus' guidance (in spirit) to combine declaration
and initialization for objects using __cleanup(). If it was Rust I'd
written it as

	let reg_grp_node = if let Some(...) { ... } else { ... };

so declaration and initialization would be the same, but with C this is
the closest I could come up with.

Thanks.

-- 
Dmitry

Re: [PATCH 15/22] Input: iqs7222 - use cleanup facility for fwnodes

[Javier Carrasco]

On 04/09/2024 20:26, Dmitry Torokhov wrote:
> On Wed, Sep 04, 2024 at 12:50:44PM +0200, Javier Carrasco wrote:
>> Hi Dmitry,
>>
>> On 04/09/2024 06:48, Dmitry Torokhov wrote:
>>> Use __free(fwnode_handle) cleanup facility to ensure that references to
>>> acquired fwnodes are dropped at appropriate times automatically.
>>>
>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> ---
>>>  drivers/input/misc/iqs7222.c | 30 ++++++++++++++----------------
>>>  1 file changed, 14 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
>>> index 9ca5a743f19f..d9b87606ff7a 100644
>>> --- a/drivers/input/misc/iqs7222.c
>>> +++ b/drivers/input/misc/iqs7222.c
>>
>> ...
>>
>>> @@ -2818,9 +2813,9 @@ static int iqs7222_parse_reg_grp(struct iqs7222_private *iqs7222,
>>>  				 int reg_grp_index)
>>>  {
>>>  	struct i2c_client *client = iqs7222->client;
>>> -	struct fwnode_handle *reg_grp_node;
>>>  	int error;
>>>  
>>
>> Nit: reg_grp_node could stay at the top (where it used to be), as you
>> are assigning it to NULL because there are no sensible assignments at
>> this point.
>>
>>> +	struct fwnode_handle *reg_grp_node __free(fwnode_handle) = NULL;
>>>  	if (iqs7222_reg_grp_names[reg_grp]) {
>>>  		char reg_grp_name[16];
> 
> I think this follows Linus' guidance (in spirit) to combine declaration
> and initialization for objects using __cleanup(). If it was Rust I'd
> written it as
> 
> 	let reg_grp_node = if let Some(...) { ... } else { ... };
> 
> so declaration and initialization would be the same, but with C this is
> the closest I could come up with.
> 
> Thanks.
> 

Combining the declaration and initialization was right, no doubt about
that. I was just nitpicking that the variable declaration could have
been done at the top, as it used to be. The same as you did, but not
separating the declaration from the rest as there are no instructions in
between.

My second thought was that you might be attempting to declare the
variable as near as possible to the "some" initialization, so you moved
it a little bit to get it closer :)

Either way, if you did that on purpose, then

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Re: [PATCH 15/22] Input: iqs7222 - use cleanup facility for fwnodes

[Jeff LaBundy]

Hi Dmitry,

On Tue, Sep 03, 2024 at 09:48:25PM -0700, Dmitry Torokhov wrote:
> Use __free(fwnode_handle) cleanup facility to ensure that references to
> acquired fwnodes are dropped at appropriate times automatically.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/iqs7222.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
> index 9ca5a743f19f..d9b87606ff7a 100644
> --- a/drivers/input/misc/iqs7222.c
> +++ b/drivers/input/misc/iqs7222.c
> @@ -2385,9 +2385,9 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
>  	for (i = 0; i < ARRAY_SIZE(iqs7222_kp_events); i++) {
>  		const char *event_name = iqs7222_kp_events[i].name;
>  		u16 event_enable = iqs7222_kp_events[i].enable;
> -		struct fwnode_handle *event_node;
>  
> -		event_node = fwnode_get_named_child_node(chan_node, event_name);
> +		struct fwnode_handle *event_node __free(fwnode_handle) =
> +			fwnode_get_named_child_node(chan_node, event_name);

This leaves a newline amongst the declarations, but not in between the
declarations and code. I don't feel strongly against this, but it's
opposite of what I understand to be more common; please let me know in
case I have misunderstood. This comment applies to the other chunks as
well.

>  		if (!event_node)
>  			continue;
>  
> @@ -2408,7 +2408,6 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
>  				dev_err(&client->dev,
>  					"Invalid %s press timeout: %u\n",
>  					fwnode_get_name(event_node), val);
> -				fwnode_handle_put(event_node);
>  				return -EINVAL;
>  			}
>  
> @@ -2418,7 +2417,6 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
>  			dev_err(&client->dev,
>  				"Failed to read %s press timeout: %d\n",
>  				fwnode_get_name(event_node), error);
> -			fwnode_handle_put(event_node);
>  			return error;
>  		}
>  
> @@ -2429,7 +2427,6 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
>  					    dev_desc->touch_link - (i ? 0 : 2),
>  					    &iqs7222->kp_type[chan_index][i],
>  					    &iqs7222->kp_code[chan_index][i]);
> -		fwnode_handle_put(event_node);
>  		if (error)
>  			return error;
>  
> @@ -2604,10 +2601,10 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
>  
>  	for (i = 0; i < ARRAY_SIZE(iqs7222_sl_events); i++) {
>  		const char *event_name = iqs7222_sl_events[i].name;
> -		struct fwnode_handle *event_node;
>  		enum iqs7222_reg_key_id reg_key;
>  
> -		event_node = fwnode_get_named_child_node(sldr_node, event_name);
> +		struct fwnode_handle *event_node __free(fwnode_handle) =
> +			fwnode_get_named_child_node(sldr_node, event_name);
>  		if (!event_node)
>  			continue;
>  
> @@ -2639,7 +2636,6 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
>  					      : sldr_setup[4 + reg_offset],
>  					    NULL,
>  					    &iqs7222->sl_code[sldr_index][i]);
> -		fwnode_handle_put(event_node);
>  		if (error)
>  			return error;
>  
> @@ -2742,9 +2738,9 @@ static int iqs7222_parse_tpad(struct iqs7222_private *iqs7222,
>  
>  	for (i = 0; i < ARRAY_SIZE(iqs7222_tp_events); i++) {
>  		const char *event_name = iqs7222_tp_events[i].name;
> -		struct fwnode_handle *event_node;
>  
> -		event_node = fwnode_get_named_child_node(tpad_node, event_name);
> +		struct fwnode_handle *event_node __free(fwnode_handle) =
> +			fwnode_get_named_child_node(tpad_node, event_name);
>  		if (!event_node)
>  			continue;
>  
> @@ -2760,7 +2756,6 @@ static int iqs7222_parse_tpad(struct iqs7222_private *iqs7222,
>  					    iqs7222_tp_events[i].link, 1566,
>  					    NULL,
>  					    &iqs7222->tp_code[i]);
> -		fwnode_handle_put(event_node);
>  		if (error)
>  			return error;
>  
> @@ -2818,9 +2813,9 @@ static int iqs7222_parse_reg_grp(struct iqs7222_private *iqs7222,
>  				 int reg_grp_index)
>  {
>  	struct i2c_client *client = iqs7222->client;
> -	struct fwnode_handle *reg_grp_node;
>  	int error;
>  
> +	struct fwnode_handle *reg_grp_node __free(fwnode_handle) = NULL;
>  	if (iqs7222_reg_grp_names[reg_grp]) {
>  		char reg_grp_name[16];
>  
> @@ -2838,14 +2833,17 @@ static int iqs7222_parse_reg_grp(struct iqs7222_private *iqs7222,
>  
>  	error = iqs7222_parse_props(iqs7222, reg_grp_node, reg_grp_index,
>  				    reg_grp, IQS7222_REG_KEY_NONE);
> +	if (error)
> +		return error;
>  
> -	if (!error && iqs7222_parse_extra[reg_grp])
> +	if (iqs7222_parse_extra[reg_grp]) {
>  		error = iqs7222_parse_extra[reg_grp](iqs7222, reg_grp_node,
>  						     reg_grp_index);
> +		if (error)
> +			return error;
> +	}
>  
> -	fwnode_handle_put(reg_grp_node);
> -
> -	return error;
> +	return 0;
>  }
>  
>  static int iqs7222_parse_all(struct iqs7222_private *iqs7222)
> -- 
> 2.46.0.469.g59c65b2a67-goog
> 

Kind regards,
Jeff LaBundy

Re: [PATCH 15/22] Input: iqs7222 - use cleanup facility for fwnodes

[Dmitry Torokhov]

Hi Jeff,

On Sun, Sep 08, 2024 at 07:12:10PM -0500, Jeff LaBundy wrote:
> Hi Dmitry,
> 
> On Tue, Sep 03, 2024 at 09:48:25PM -0700, Dmitry Torokhov wrote:
> > Use __free(fwnode_handle) cleanup facility to ensure that references to
> > acquired fwnodes are dropped at appropriate times automatically.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/misc/iqs7222.c | 30 ++++++++++++++----------------
> >  1 file changed, 14 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
> > index 9ca5a743f19f..d9b87606ff7a 100644
> > --- a/drivers/input/misc/iqs7222.c
> > +++ b/drivers/input/misc/iqs7222.c
> > @@ -2385,9 +2385,9 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
> >  	for (i = 0; i < ARRAY_SIZE(iqs7222_kp_events); i++) {
> >  		const char *event_name = iqs7222_kp_events[i].name;
> >  		u16 event_enable = iqs7222_kp_events[i].enable;
> > -		struct fwnode_handle *event_node;
> >  
> > -		event_node = fwnode_get_named_child_node(chan_node, event_name);
> > +		struct fwnode_handle *event_node __free(fwnode_handle) =
> > +			fwnode_get_named_child_node(chan_node, event_name);
> 
> This leaves a newline amongst the declarations, but not in between the
> declarations and code. I don't feel strongly against this, but it's
> opposite of what I understand to be more common; please let me know in
> case I have misunderstood. This comment applies to the other chunks as
> well.

Right, so this is actually combining declaration and initialization case
that I mentioned in the other email, and it is closer in spirit to the
code and the allocation check below. That is why it is separated from
the declaration block.

Thanks.

-- 
Dmitry

Re: [PATCH 15/22] Input: iqs7222 - use cleanup facility for fwnodes

[Jeff LaBundy]

Hi Dmitry,

On Sun, Sep 08, 2024 at 06:34:21PM -0700, Dmitry Torokhov wrote:
> Hi Jeff,
> 
> On Sun, Sep 08, 2024 at 07:12:10PM -0500, Jeff LaBundy wrote:
> > Hi Dmitry,
> > 
> > On Tue, Sep 03, 2024 at 09:48:25PM -0700, Dmitry Torokhov wrote:
> > > Use __free(fwnode_handle) cleanup facility to ensure that references to
> > > acquired fwnodes are dropped at appropriate times automatically.
> > > 
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > >  drivers/input/misc/iqs7222.c | 30 ++++++++++++++----------------
> > >  1 file changed, 14 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
> > > index 9ca5a743f19f..d9b87606ff7a 100644
> > > --- a/drivers/input/misc/iqs7222.c
> > > +++ b/drivers/input/misc/iqs7222.c
> > > @@ -2385,9 +2385,9 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
> > >  	for (i = 0; i < ARRAY_SIZE(iqs7222_kp_events); i++) {
> > >  		const char *event_name = iqs7222_kp_events[i].name;
> > >  		u16 event_enable = iqs7222_kp_events[i].enable;
> > > -		struct fwnode_handle *event_node;
> > >  
> > > -		event_node = fwnode_get_named_child_node(chan_node, event_name);
> > > +		struct fwnode_handle *event_node __free(fwnode_handle) =
> > > +			fwnode_get_named_child_node(chan_node, event_name);
> > 
> > This leaves a newline amongst the declarations, but not in between the
> > declarations and code. I don't feel strongly against this, but it's
> > opposite of what I understand to be more common; please let me know in
> > case I have misunderstood. This comment applies to the other chunks as
> > well.
> 
> Right, so this is actually combining declaration and initialization case
> that I mentioned in the other email, and it is closer in spirit to the
> code and the allocation check below. That is why it is separated from
> the declaration block.

Thanks again for the background information; please feel free to add:

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

> 
> Thanks.
> 
> -- 
> Dmitry

Kind regards,
Jeff LaBundy

[PATCH 16/22] Input: max8997_haptic - use guard notation when acquiring mutex

[Dmitry Torokhov]

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/max8997_haptic.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/input/misc/max8997_haptic.c b/drivers/input/misc/max8997_haptic.c
index 11cac4b7dddc..2853455daef2 100644
--- a/drivers/input/misc/max8997_haptic.c
+++ b/drivers/input/misc/max8997_haptic.c
@@ -153,19 +153,19 @@ static void max8997_haptic_enable(struct max8997_haptic *chip)
 {
 	int error;
 
-	mutex_lock(&chip->mutex);
+	guard(mutex)(&chip->mutex);
 
 	error = max8997_haptic_set_duty_cycle(chip);
 	if (error) {
 		dev_err(chip->dev, "set_pwm_cycle failed, error: %d\n", error);
-		goto out;
+		return;
 	}
 
 	if (!chip->enabled) {
 		error = regulator_enable(chip->regulator);
 		if (error) {
 			dev_err(chip->dev, "Failed to enable regulator\n");
-			goto out;
+			return;
 		}
 		max8997_haptic_configure(chip);
 		if (chip->mode == MAX8997_EXTERNAL_MODE) {
@@ -173,19 +173,16 @@ static void max8997_haptic_enable(struct max8997_haptic *chip)
 			if (error) {
 				dev_err(chip->dev, "Failed to enable PWM\n");
 				regulator_disable(chip->regulator);
-				goto out;
+				return;
 			}
 		}
 		chip->enabled = true;
 	}
-
-out:
-	mutex_unlock(&chip->mutex);
 }
 
 static void max8997_haptic_disable(struct max8997_haptic *chip)
 {
-	mutex_lock(&chip->mutex);
+	guard(mutex)(&chip->mutex);
 
 	if (chip->enabled) {
 		chip->enabled = false;
@@ -194,8 +191,6 @@ static void max8997_haptic_disable(struct max8997_haptic *chip)
 			pwm_disable(chip->pwm);
 		regulator_disable(chip->regulator);
 	}
-
-	mutex_unlock(&chip->mutex);
 }
 
 static void max8997_haptic_play_effect_work(struct work_struct *work)
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 16/22] Input: max8997_haptic - use guard notation when acquiring mutex

[Javier Carrasco]

On 04/09/2024 06:48, 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/max8997_haptic.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/input/misc/max8997_haptic.c b/drivers/input/misc/max8997_haptic.c
> index 11cac4b7dddc..2853455daef2 100644
> --- a/drivers/input/misc/max8997_haptic.c
> +++ b/drivers/input/misc/max8997_haptic.c
> @@ -153,19 +153,19 @@ static void max8997_haptic_enable(struct max8997_haptic *chip)
>  {
>  	int error;
>  
> -	mutex_lock(&chip->mutex);
> +	guard(mutex)(&chip->mutex);
>  
>  	error = max8997_haptic_set_duty_cycle(chip);
>  	if (error) {
>  		dev_err(chip->dev, "set_pwm_cycle failed, error: %d\n", error);
> -		goto out;
> +		return;
>  	}
>  
>  	if (!chip->enabled) {
>  		error = regulator_enable(chip->regulator);
>  		if (error) {
>  			dev_err(chip->dev, "Failed to enable regulator\n");
> -			goto out;
> +			return;
>  		}
>  		max8997_haptic_configure(chip);
>  		if (chip->mode == MAX8997_EXTERNAL_MODE) {
> @@ -173,19 +173,16 @@ static void max8997_haptic_enable(struct max8997_haptic *chip)
>  			if (error) {
>  				dev_err(chip->dev, "Failed to enable PWM\n");
>  				regulator_disable(chip->regulator);
> -				goto out;
> +				return;
>  			}
>  		}
>  		chip->enabled = true;
>  	}
> -
> -out:
> -	mutex_unlock(&chip->mutex);
>  }
>  
>  static void max8997_haptic_disable(struct max8997_haptic *chip)
>  {
> -	mutex_lock(&chip->mutex);
> +	guard(mutex)(&chip->mutex);
>  
>  	if (chip->enabled) {
>  		chip->enabled = false;
> @@ -194,8 +191,6 @@ static void max8997_haptic_disable(struct max8997_haptic *chip)
>  			pwm_disable(chip->pwm);
>  		regulator_disable(chip->regulator);
>  	}
> -
> -	mutex_unlock(&chip->mutex);
>  }
>  
>  static void max8997_haptic_play_effect_work(struct work_struct *work)

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 17/22] Input: pegasus_notetaker - use guard notation when acquiring mutex

[Dmitry Torokhov]

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/tablet/pegasus_notetaker.c | 86 +++++++++++++-----------
 1 file changed, 48 insertions(+), 38 deletions(-)

diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
index a68da2988f9c..e1dc8365bfe9 100644
--- a/drivers/input/tablet/pegasus_notetaker.c
+++ b/drivers/input/tablet/pegasus_notetaker.c
@@ -214,6 +214,28 @@ static void pegasus_init(struct work_struct *work)
 			error);
 }
 
+static int __pegasus_open(struct pegasus *pegasus)
+{
+	int error;
+
+	guard(mutex)(&pegasus->pm_mutex);
+
+	pegasus->irq->dev = pegasus->usbdev;
+	if (usb_submit_urb(pegasus->irq, GFP_KERNEL))
+		return -EIO;
+
+	error = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
+	if (error) {
+		usb_kill_urb(pegasus->irq);
+		cancel_work_sync(&pegasus->init);
+		return error;
+	}
+
+	pegasus->is_open = true;
+	return 0;
+}
+
+
 static int pegasus_open(struct input_dev *dev)
 {
 	struct pegasus *pegasus = input_get_drvdata(dev);
@@ -223,39 +245,25 @@ static int pegasus_open(struct input_dev *dev)
 	if (error)
 		return error;
 
-	mutex_lock(&pegasus->pm_mutex);
-	pegasus->irq->dev = pegasus->usbdev;
-	if (usb_submit_urb(pegasus->irq, GFP_KERNEL)) {
-		error = -EIO;
-		goto err_autopm_put;
+	error = __pegasus_open(pegasus);
+	if (error) {
+		usb_autopm_put_interface(pegasus->intf);
+		return error;
 	}
 
-	error = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
-	if (error)
-		goto err_kill_urb;
-
-	pegasus->is_open = true;
-	mutex_unlock(&pegasus->pm_mutex);
 	return 0;
-
-err_kill_urb:
-	usb_kill_urb(pegasus->irq);
-	cancel_work_sync(&pegasus->init);
-err_autopm_put:
-	mutex_unlock(&pegasus->pm_mutex);
-	usb_autopm_put_interface(pegasus->intf);
-	return error;
 }
 
 static void pegasus_close(struct input_dev *dev)
 {
 	struct pegasus *pegasus = input_get_drvdata(dev);
 
-	mutex_lock(&pegasus->pm_mutex);
-	usb_kill_urb(pegasus->irq);
-	cancel_work_sync(&pegasus->init);
-	pegasus->is_open = false;
-	mutex_unlock(&pegasus->pm_mutex);
+	scoped_guard(mutex, &pegasus->pm_mutex) {
+		usb_kill_urb(pegasus->irq);
+		cancel_work_sync(&pegasus->init);
+
+		pegasus->is_open = false;
+	}
 
 	usb_autopm_put_interface(pegasus->intf);
 }
@@ -411,10 +419,10 @@ static int pegasus_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct pegasus *pegasus = usb_get_intfdata(intf);
 
-	mutex_lock(&pegasus->pm_mutex);
+	guard(mutex)(&pegasus->pm_mutex);
+
 	usb_kill_urb(pegasus->irq);
 	cancel_work_sync(&pegasus->init);
-	mutex_unlock(&pegasus->pm_mutex);
 
 	return 0;
 }
@@ -422,31 +430,33 @@ static int pegasus_suspend(struct usb_interface *intf, pm_message_t message)
 static int pegasus_resume(struct usb_interface *intf)
 {
 	struct pegasus *pegasus = usb_get_intfdata(intf);
-	int retval = 0;
 
-	mutex_lock(&pegasus->pm_mutex);
+	guard(mutex)(&pegasus->pm_mutex);
+
 	if (pegasus->is_open && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
-		retval = -EIO;
-	mutex_unlock(&pegasus->pm_mutex);
+		return -EIO;
 
-	return retval;
+	return 0;
 }
 
 static int pegasus_reset_resume(struct usb_interface *intf)
 {
 	struct pegasus *pegasus = usb_get_intfdata(intf);
-	int retval = 0;
+	int error;
+
+	guard(mutex)(&pegasus->pm_mutex);
 
-	mutex_lock(&pegasus->pm_mutex);
 	if (pegasus->is_open) {
-		retval = pegasus_set_mode(pegasus, PEN_MODE_XY,
+		error = pegasus_set_mode(pegasus, PEN_MODE_XY,
 					  NOTETAKER_LED_MOUSE);
-		if (!retval && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
-			retval = -EIO;
+		if (error)
+			return error;
+
+		if (usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
+			return -EIO;
 	}
-	mutex_unlock(&pegasus->pm_mutex);
 
-	return retval;
+	return 0;
 }
 
 static const struct usb_device_id pegasus_ids[] = {
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 17/22] Input: pegasus_notetaker - use guard notation when acquiring mutex

[Javier Carrasco]

On 04/09/2024 06:48, 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/tablet/pegasus_notetaker.c | 86 +++++++++++++-----------
>  1 file changed, 48 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
> index a68da2988f9c..e1dc8365bfe9 100644
> --- a/drivers/input/tablet/pegasus_notetaker.c
> +++ b/drivers/input/tablet/pegasus_notetaker.c
> @@ -214,6 +214,28 @@ static void pegasus_init(struct work_struct *work)
>  			error);
>  }
>  
> +static int __pegasus_open(struct pegasus *pegasus)
> +{
> +	int error;
> +
> +	guard(mutex)(&pegasus->pm_mutex);
> +
> +	pegasus->irq->dev = pegasus->usbdev;
> +	if (usb_submit_urb(pegasus->irq, GFP_KERNEL))
> +		return -EIO;
> +
> +	error = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
> +	if (error) {
> +		usb_kill_urb(pegasus->irq);
> +		cancel_work_sync(&pegasus->init);
> +		return error;
> +	}
> +
> +	pegasus->is_open = true;

Nit: blank line before return.

> +	return 0;
> +}

Nit: multiple blank lines.

> +
> +
>  static int pegasus_open(struct input_dev *dev)
>  {
>  	struct pegasus *pegasus = input_get_drvdata(dev);
> @@ -223,39 +245,25 @@ static int pegasus_open(struct input_dev *dev)
>  	if (error)
>  		return error;
>  
> -	mutex_lock(&pegasus->pm_mutex);
> -	pegasus->irq->dev = pegasus->usbdev;
> -	if (usb_submit_urb(pegasus->irq, GFP_KERNEL)) {
> -		error = -EIO;
> -		goto err_autopm_put;
> +	error = __pegasus_open(pegasus);
> +	if (error) {
> +		usb_autopm_put_interface(pegasus->intf);
> +		return error;
>  	}
>  
> -	error = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
> -	if (error)
> -		goto err_kill_urb;
> -
> -	pegasus->is_open = true;
> -	mutex_unlock(&pegasus->pm_mutex);
>  	return 0;
> -
> -err_kill_urb:
> -	usb_kill_urb(pegasus->irq);
> -	cancel_work_sync(&pegasus->init);
> -err_autopm_put:
> -	mutex_unlock(&pegasus->pm_mutex);
> -	usb_autopm_put_interface(pegasus->intf);
> -	return error;
>  }
>  
>  static void pegasus_close(struct input_dev *dev)
>  {
>  	struct pegasus *pegasus = input_get_drvdata(dev);
>  
> -	mutex_lock(&pegasus->pm_mutex);
> -	usb_kill_urb(pegasus->irq);
> -	cancel_work_sync(&pegasus->init);
> -	pegasus->is_open = false;
> -	mutex_unlock(&pegasus->pm_mutex);
> +	scoped_guard(mutex, &pegasus->pm_mutex) {
> +		usb_kill_urb(pegasus->irq);
> +		cancel_work_sync(&pegasus->init);
> +
> +		pegasus->is_open = false;
> +	}
>  
>  	usb_autopm_put_interface(pegasus->intf);
>  }
> @@ -411,10 +419,10 @@ static int pegasus_suspend(struct usb_interface *intf, pm_message_t message)
>  {
>  	struct pegasus *pegasus = usb_get_intfdata(intf);
>  
> -	mutex_lock(&pegasus->pm_mutex);
> +	guard(mutex)(&pegasus->pm_mutex);
> +
>  	usb_kill_urb(pegasus->irq);
>  	cancel_work_sync(&pegasus->init);
> -	mutex_unlock(&pegasus->pm_mutex);
>  
>  	return 0;
>  }
> @@ -422,31 +430,33 @@ static int pegasus_suspend(struct usb_interface *intf, pm_message_t message)
>  static int pegasus_resume(struct usb_interface *intf)
>  {
>  	struct pegasus *pegasus = usb_get_intfdata(intf);
> -	int retval = 0;
>  
> -	mutex_lock(&pegasus->pm_mutex);
> +	guard(mutex)(&pegasus->pm_mutex);
> +
>  	if (pegasus->is_open && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
> -		retval = -EIO;
> -	mutex_unlock(&pegasus->pm_mutex);
> +		return -EIO;
>  
> -	return retval;
> +	return 0;
>  }
>  
>  static int pegasus_reset_resume(struct usb_interface *intf)
>  {
>  	struct pegasus *pegasus = usb_get_intfdata(intf);
> -	int retval = 0;
> +	int error;
> +
> +	guard(mutex)(&pegasus->pm_mutex);
>  
> -	mutex_lock(&pegasus->pm_mutex);
>  	if (pegasus->is_open) {
> -		retval = pegasus_set_mode(pegasus, PEN_MODE_XY,
> +		error = pegasus_set_mode(pegasus, PEN_MODE_XY,
>  					  NOTETAKER_LED_MOUSE);
> -		if (!retval && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
> -			retval = -EIO;
> +		if (error)
> +			return error;
> +
> +		if (usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
> +			return -EIO;
>  	}
> -	mutex_unlock(&pegasus->pm_mutex);
>  
> -	return retval;
> +	return 0;
>  }
>  
>  static const struct usb_device_id pegasus_ids[] = {

Apart from the minor nitpicks,

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH v2 17/22] Input: pegasus_notetaker - use guard notation when acquiring mutex

[Dmitry Torokhov]

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.

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

V2: fixed whitespace issues, added Reviewed-by from Javier

 drivers/input/tablet/pegasus_notetaker.c |   86 +++++++++++++++++-------------
 1 file changed, 48 insertions(+), 38 deletions(-)

diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
index a68da2988f9c..8d6b71d59793 100644
--- a/drivers/input/tablet/pegasus_notetaker.c
+++ b/drivers/input/tablet/pegasus_notetaker.c
@@ -214,6 +214,28 @@ static void pegasus_init(struct work_struct *work)
 			error);
 }
 
+static int __pegasus_open(struct pegasus *pegasus)
+{
+	int error;
+
+	guard(mutex)(&pegasus->pm_mutex);
+
+	pegasus->irq->dev = pegasus->usbdev;
+	if (usb_submit_urb(pegasus->irq, GFP_KERNEL))
+		return -EIO;
+
+	error = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
+	if (error) {
+		usb_kill_urb(pegasus->irq);
+		cancel_work_sync(&pegasus->init);
+		return error;
+	}
+
+	pegasus->is_open = true;
+
+	return 0;
+}
+
 static int pegasus_open(struct input_dev *dev)
 {
 	struct pegasus *pegasus = input_get_drvdata(dev);
@@ -223,39 +245,25 @@ static int pegasus_open(struct input_dev *dev)
 	if (error)
 		return error;
 
-	mutex_lock(&pegasus->pm_mutex);
-	pegasus->irq->dev = pegasus->usbdev;
-	if (usb_submit_urb(pegasus->irq, GFP_KERNEL)) {
-		error = -EIO;
-		goto err_autopm_put;
+	error = __pegasus_open(pegasus);
+	if (error) {
+		usb_autopm_put_interface(pegasus->intf);
+		return error;
 	}
 
-	error = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
-	if (error)
-		goto err_kill_urb;
-
-	pegasus->is_open = true;
-	mutex_unlock(&pegasus->pm_mutex);
 	return 0;
-
-err_kill_urb:
-	usb_kill_urb(pegasus->irq);
-	cancel_work_sync(&pegasus->init);
-err_autopm_put:
-	mutex_unlock(&pegasus->pm_mutex);
-	usb_autopm_put_interface(pegasus->intf);
-	return error;
 }
 
 static void pegasus_close(struct input_dev *dev)
 {
 	struct pegasus *pegasus = input_get_drvdata(dev);
 
-	mutex_lock(&pegasus->pm_mutex);
-	usb_kill_urb(pegasus->irq);
-	cancel_work_sync(&pegasus->init);
-	pegasus->is_open = false;
-	mutex_unlock(&pegasus->pm_mutex);
+	scoped_guard(mutex, &pegasus->pm_mutex) {
+		usb_kill_urb(pegasus->irq);
+		cancel_work_sync(&pegasus->init);
+
+		pegasus->is_open = false;
+	}
 
 	usb_autopm_put_interface(pegasus->intf);
 }
@@ -411,10 +419,10 @@ static int pegasus_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct pegasus *pegasus = usb_get_intfdata(intf);
 
-	mutex_lock(&pegasus->pm_mutex);
+	guard(mutex)(&pegasus->pm_mutex);
+
 	usb_kill_urb(pegasus->irq);
 	cancel_work_sync(&pegasus->init);
-	mutex_unlock(&pegasus->pm_mutex);
 
 	return 0;
 }
@@ -422,31 +430,33 @@ static int pegasus_suspend(struct usb_interface *intf, pm_message_t message)
 static int pegasus_resume(struct usb_interface *intf)
 {
 	struct pegasus *pegasus = usb_get_intfdata(intf);
-	int retval = 0;
 
-	mutex_lock(&pegasus->pm_mutex);
+	guard(mutex)(&pegasus->pm_mutex);
+
 	if (pegasus->is_open && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
-		retval = -EIO;
-	mutex_unlock(&pegasus->pm_mutex);
+		return -EIO;
 
-	return retval;
+	return 0;
 }
 
 static int pegasus_reset_resume(struct usb_interface *intf)
 {
 	struct pegasus *pegasus = usb_get_intfdata(intf);
-	int retval = 0;
+	int error;
+
+	guard(mutex)(&pegasus->pm_mutex);
 
-	mutex_lock(&pegasus->pm_mutex);
 	if (pegasus->is_open) {
-		retval = pegasus_set_mode(pegasus, PEN_MODE_XY,
+		error = pegasus_set_mode(pegasus, PEN_MODE_XY,
 					  NOTETAKER_LED_MOUSE);
-		if (!retval && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
-			retval = -EIO;
+		if (error)
+			return error;
+
+		if (usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
+			return -EIO;
 	}
-	mutex_unlock(&pegasus->pm_mutex);
 
-	return retval;
+	return 0;
 }
 
 static const struct usb_device_id pegasus_ids[] = {

[PATCH 18/22] Input: powermate - use guard notation when acquiring spinlock

[Dmitry Torokhov]

Using guard notation makes the code more compact and error handling
more robust by ensuring that locks are released in all code paths
when control leaves critical section.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/powermate.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/input/misc/powermate.c b/drivers/input/misc/powermate.c
index 4b039abffc4b..ecb92ee5ebbc 100644
--- a/drivers/input/misc/powermate.c
+++ b/drivers/input/misc/powermate.c
@@ -194,22 +194,18 @@ static void powermate_sync_state(struct powermate_device *pm)
 static void powermate_config_complete(struct urb *urb)
 {
 	struct powermate_device *pm = urb->context;
-	unsigned long flags;
 
 	if (urb->status)
 		printk(KERN_ERR "powermate: config urb returned %d\n", urb->status);
 
-	spin_lock_irqsave(&pm->lock, flags);
+	guard(spinlock_irqsave)(&pm->lock);
 	powermate_sync_state(pm);
-	spin_unlock_irqrestore(&pm->lock, flags);
 }
 
 /* Set the LED up as described and begin the sync with the hardware if required */
 static void powermate_pulse_led(struct powermate_device *pm, int static_brightness, int pulse_speed,
 				int pulse_table, int pulse_asleep, int pulse_awake)
 {
-	unsigned long flags;
-
 	if (pulse_speed < 0)
 		pulse_speed = 0;
 	if (pulse_table < 0)
@@ -222,8 +218,7 @@ static void powermate_pulse_led(struct powermate_device *pm, int static_brightne
 	pulse_asleep = !!pulse_asleep;
 	pulse_awake = !!pulse_awake;
 
-
-	spin_lock_irqsave(&pm->lock, flags);
+	guard(spinlock_irqsave)(&pm->lock);
 
 	/* mark state updates which are required */
 	if (static_brightness != pm->static_brightness) {
@@ -245,8 +240,6 @@ static void powermate_pulse_led(struct powermate_device *pm, int static_brightne
 	}
 
 	powermate_sync_state(pm);
-
-	spin_unlock_irqrestore(&pm->lock, flags);
 }
 
 /* Callback from the Input layer when an event arrives from userspace to configure the LED */
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 18/22] Input: powermate - use guard notation when acquiring spinlock

[Javier Carrasco]

On 04/09/2024 06:49, Dmitry Torokhov wrote:
> Using guard notation makes the code more compact and error handling
> more robust by ensuring that locks are released in all code paths
> when control leaves critical section.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/powermate.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/misc/powermate.c b/drivers/input/misc/powermate.c
> index 4b039abffc4b..ecb92ee5ebbc 100644
> --- a/drivers/input/misc/powermate.c
> +++ b/drivers/input/misc/powermate.c
> @@ -194,22 +194,18 @@ static void powermate_sync_state(struct powermate_device *pm)
>  static void powermate_config_complete(struct urb *urb)
>  {
>  	struct powermate_device *pm = urb->context;
> -	unsigned long flags;
>  
>  	if (urb->status)
>  		printk(KERN_ERR "powermate: config urb returned %d\n", urb->status);
>  
> -	spin_lock_irqsave(&pm->lock, flags);
> +	guard(spinlock_irqsave)(&pm->lock);
>  	powermate_sync_state(pm);
> -	spin_unlock_irqrestore(&pm->lock, flags);
>  }
>  
>  /* Set the LED up as described and begin the sync with the hardware if required */
>  static void powermate_pulse_led(struct powermate_device *pm, int static_brightness, int pulse_speed,
>  				int pulse_table, int pulse_asleep, int pulse_awake)
>  {
> -	unsigned long flags;
> -
>  	if (pulse_speed < 0)
>  		pulse_speed = 0;
>  	if (pulse_table < 0)
> @@ -222,8 +218,7 @@ static void powermate_pulse_led(struct powermate_device *pm, int static_brightne
>  	pulse_asleep = !!pulse_asleep;
>  	pulse_awake = !!pulse_awake;
>  
> -
> -	spin_lock_irqsave(&pm->lock, flags);
> +	guard(spinlock_irqsave)(&pm->lock);
>  
>  	/* mark state updates which are required */
>  	if (static_brightness != pm->static_brightness) {
> @@ -245,8 +240,6 @@ static void powermate_pulse_led(struct powermate_device *pm, int static_brightne
>  	}
>  
>  	powermate_sync_state(pm);
> -
> -	spin_unlock_irqrestore(&pm->lock, flags);
>  }
>  
>  /* Callback from the Input layer when an event arrives from userspace to configure the LED */

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 19/22] Input: pwm-beeper - use guard notation when acquiring spinlock

[Dmitry Torokhov]

Using guard notation makes the code more compact and error handling
more robust by ensuring that locks are released in all code paths
when control leaves critical section.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/pwm-beeper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 5b9aedf4362f..0e19e97d98ec 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -203,9 +203,9 @@ static int pwm_beeper_suspend(struct device *dev)
 	 * beeper->suspended, but to ensure that pwm_beeper_event
 	 * does not re-submit work once flag is set.
 	 */
-	spin_lock_irq(&beeper->input->event_lock);
-	beeper->suspended = true;
-	spin_unlock_irq(&beeper->input->event_lock);
+	scoped_guard(spinlock_irq, &beeper->input->event_lock) {
+		beeper->suspended = true;
+	}
 
 	pwm_beeper_stop(beeper);
 
@@ -216,9 +216,9 @@ static int pwm_beeper_resume(struct device *dev)
 {
 	struct pwm_beeper *beeper = dev_get_drvdata(dev);
 
-	spin_lock_irq(&beeper->input->event_lock);
-	beeper->suspended = false;
-	spin_unlock_irq(&beeper->input->event_lock);
+	scoped_guard(spinlock_irq, &beeper->input->event_lock) {
+		beeper->suspended = false;
+	}
 
 	/* Let worker figure out if we should resume beeping */
 	schedule_work(&beeper->work);
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 19/22] Input: pwm-beeper - use guard notation when acquiring spinlock

[Javier Carrasco]

On 04/09/2024 06:49, Dmitry Torokhov wrote:
> Using guard notation makes the code more compact and error handling
> more robust by ensuring that locks are released in all code paths
> when control leaves critical section.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/pwm-beeper.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> index 5b9aedf4362f..0e19e97d98ec 100644
> --- a/drivers/input/misc/pwm-beeper.c
> +++ b/drivers/input/misc/pwm-beeper.c
> @@ -203,9 +203,9 @@ static int pwm_beeper_suspend(struct device *dev)
>  	 * beeper->suspended, but to ensure that pwm_beeper_event
>  	 * does not re-submit work once flag is set.
>  	 */
> -	spin_lock_irq(&beeper->input->event_lock);
> -	beeper->suspended = true;
> -	spin_unlock_irq(&beeper->input->event_lock);

I assume you know that you don't need the braces for the scoped_guard()
in these cases. If you prefer doing so to clarify (you are leaving an
empty line afterwards anyway, but still), I am ok with it.

Note that other users of scoped_guard() tend to use them without braces
for single instructions.

> +	scoped_guard(spinlock_irq, &beeper->input->event_lock) {
> +		beeper->suspended = true;
> +	}
>  
>  	pwm_beeper_stop(beeper);
>  
> @@ -216,9 +216,9 @@ static int pwm_beeper_resume(struct device *dev)
>  {
>  	struct pwm_beeper *beeper = dev_get_drvdata(dev);
>  
> -	spin_lock_irq(&beeper->input->event_lock);
> -	beeper->suspended = false;
> -	spin_unlock_irq(&beeper->input->event_lock);
> +	scoped_guard(spinlock_irq, &beeper->input->event_lock) {
> +		beeper->suspended = false;
> +	}
>  
>  	/* Let worker figure out if we should resume beeping */
>  	schedule_work(&beeper->work);

With or without braces,

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 20/22] Input: regulator-haptic - use guard notation when acquiring mutex

[Dmitry Torokhov]

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/regulator-haptic.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/input/misc/regulator-haptic.c b/drivers/input/misc/regulator-haptic.c
index 02f73b7c0462..41af6aefaa07 100644
--- a/drivers/input/misc/regulator-haptic.c
+++ b/drivers/input/misc/regulator-haptic.c
@@ -83,12 +83,10 @@ static void regulator_haptic_work(struct work_struct *work)
 	struct regulator_haptic *haptic = container_of(work,
 					struct regulator_haptic, work);
 
-	mutex_lock(&haptic->mutex);
+	guard(mutex)(&haptic->mutex);
 
 	if (!haptic->suspended)
 		regulator_haptic_set_voltage(haptic, haptic->magnitude);
-
-	mutex_unlock(&haptic->mutex);
 }
 
 static int regulator_haptic_play_effect(struct input_dev *input, void *data,
@@ -207,17 +205,14 @@ static int regulator_haptic_suspend(struct device *dev)
 	struct regulator_haptic *haptic = platform_get_drvdata(pdev);
 	int error;
 
-	error = mutex_lock_interruptible(&haptic->mutex);
-	if (error)
-		return error;
-
-	regulator_haptic_set_voltage(haptic, 0);
-
-	haptic->suspended = true;
+	scoped_guard(mutex_intr, &haptic->mutex) {
+		regulator_haptic_set_voltage(haptic, 0);
+		haptic->suspended = true;
 
-	mutex_unlock(&haptic->mutex);
+		return 0;
+	}
 
-	return 0;
+	return -EINTR;
 }
 
 static int regulator_haptic_resume(struct device *dev)
@@ -226,7 +221,7 @@ static int regulator_haptic_resume(struct device *dev)
 	struct regulator_haptic *haptic = platform_get_drvdata(pdev);
 	unsigned int magnitude;
 
-	mutex_lock(&haptic->mutex);
+	guard(mutex)(&haptic->mutex);
 
 	haptic->suspended = false;
 
@@ -234,8 +229,6 @@ static int regulator_haptic_resume(struct device *dev)
 	if (magnitude)
 		regulator_haptic_set_voltage(haptic, magnitude);
 
-	mutex_unlock(&haptic->mutex);
-
 	return 0;
 }
 
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 20/22] Input: regulator-haptic - use guard notation when acquiring mutex

[Javier Carrasco]

On 04/09/2024 06:49, 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/regulator-haptic.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/input/misc/regulator-haptic.c b/drivers/input/misc/regulator-haptic.c
> index 02f73b7c0462..41af6aefaa07 100644
> --- a/drivers/input/misc/regulator-haptic.c
> +++ b/drivers/input/misc/regulator-haptic.c
> @@ -83,12 +83,10 @@ static void regulator_haptic_work(struct work_struct *work)
>  	struct regulator_haptic *haptic = container_of(work,
>  					struct regulator_haptic, work);
>  
> -	mutex_lock(&haptic->mutex);
> +	guard(mutex)(&haptic->mutex);
>  
>  	if (!haptic->suspended)
>  		regulator_haptic_set_voltage(haptic, haptic->magnitude);
> -
> -	mutex_unlock(&haptic->mutex);
>  }
>  
>  static int regulator_haptic_play_effect(struct input_dev *input, void *data,
> @@ -207,17 +205,14 @@ static int regulator_haptic_suspend(struct device *dev)
>  	struct regulator_haptic *haptic = platform_get_drvdata(pdev);

error became an unused variable and can be dropped.

>  	int error;
>  
> -	error = mutex_lock_interruptible(&haptic->mutex);
> -	if (error)
> -		return error;
> -
> -	regulator_haptic_set_voltage(haptic, 0);
> -
> -	haptic->suspended = true;
> +	scoped_guard(mutex_intr, &haptic->mutex) {
> +		regulator_haptic_set_voltage(haptic, 0);
> +		haptic->suspended = true;
>  
> -	mutex_unlock(&haptic->mutex);
> +		return 0;
> +	}
>  
> -	return 0;
> +	return -EINTR;
>  }
>  
>  static int regulator_haptic_resume(struct device *dev)
> @@ -226,7 +221,7 @@ static int regulator_haptic_resume(struct device *dev)
>  	struct regulator_haptic *haptic = platform_get_drvdata(pdev);
>  	unsigned int magnitude;
>  
> -	mutex_lock(&haptic->mutex);
> +	guard(mutex)(&haptic->mutex);
>  
>  	haptic->suspended = false;
>  
> @@ -234,8 +229,6 @@ static int regulator_haptic_resume(struct device *dev)
>  	if (magnitude)
>  		regulator_haptic_set_voltage(haptic, magnitude);
>  
> -	mutex_unlock(&haptic->mutex);
> -
>  	return 0;
>  }
>  

[PATCH v2 20/22] Input: regulator-haptic - use guard notation when acquiring mutex

[Dmitry Torokhov]

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

v2: drop no linger used "error" variable in regulator_haptic_suspend()

 drivers/input/misc/regulator-haptic.c |   24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/input/misc/regulator-haptic.c b/drivers/input/misc/regulator-haptic.c
index 02f73b7c0462..3666ba6d1f30 100644
--- a/drivers/input/misc/regulator-haptic.c
+++ b/drivers/input/misc/regulator-haptic.c
@@ -83,12 +83,10 @@ static void regulator_haptic_work(struct work_struct *work)
 	struct regulator_haptic *haptic = container_of(work,
 					struct regulator_haptic, work);
 
-	mutex_lock(&haptic->mutex);
+	guard(mutex)(&haptic->mutex);
 
 	if (!haptic->suspended)
 		regulator_haptic_set_voltage(haptic, haptic->magnitude);
-
-	mutex_unlock(&haptic->mutex);
 }
 
 static int regulator_haptic_play_effect(struct input_dev *input, void *data,
@@ -205,19 +203,15 @@ static int regulator_haptic_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct regulator_haptic *haptic = platform_get_drvdata(pdev);
-	int error;
 
-	error = mutex_lock_interruptible(&haptic->mutex);
-	if (error)
-		return error;
-
-	regulator_haptic_set_voltage(haptic, 0);
+	scoped_guard(mutex_intr, &haptic->mutex) {
+		regulator_haptic_set_voltage(haptic, 0);
+		haptic->suspended = true;
 
-	haptic->suspended = true;
-
-	mutex_unlock(&haptic->mutex);
+		return 0;
+	}
 
-	return 0;
+	return -EINTR;
 }
 
 static int regulator_haptic_resume(struct device *dev)
@@ -226,7 +220,7 @@ static int regulator_haptic_resume(struct device *dev)
 	struct regulator_haptic *haptic = platform_get_drvdata(pdev);
 	unsigned int magnitude;
 
-	mutex_lock(&haptic->mutex);
+	guard(mutex)(&haptic->mutex);
 
 	haptic->suspended = false;
 
@@ -234,8 +228,6 @@ static int regulator_haptic_resume(struct device *dev)
 	if (magnitude)
 		regulator_haptic_set_voltage(haptic, magnitude);
 
-	mutex_unlock(&haptic->mutex);
-
 	return 0;
 }
 

Re: [PATCH v2 20/22] Input: regulator-haptic - use guard notation when acquiring mutex

[Javier Carrasco]

On 04/09/2024 22:55, 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>
> ---
> 
> v2: drop no linger used "error" variable in regulator_haptic_suspend()
> 
>  drivers/input/misc/regulator-haptic.c |   24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/input/misc/regulator-haptic.c b/drivers/input/misc/regulator-haptic.c
> index 02f73b7c0462..3666ba6d1f30 100644
> --- a/drivers/input/misc/regulator-haptic.c
> +++ b/drivers/input/misc/regulator-haptic.c
> @@ -83,12 +83,10 @@ static void regulator_haptic_work(struct work_struct *work)
>  	struct regulator_haptic *haptic = container_of(work,
>  					struct regulator_haptic, work);
>  
> -	mutex_lock(&haptic->mutex);
> +	guard(mutex)(&haptic->mutex);
>  
>  	if (!haptic->suspended)
>  		regulator_haptic_set_voltage(haptic, haptic->magnitude);
> -
> -	mutex_unlock(&haptic->mutex);
>  }
>  
>  static int regulator_haptic_play_effect(struct input_dev *input, void *data,
> @@ -205,19 +203,15 @@ static int regulator_haptic_suspend(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct regulator_haptic *haptic = platform_get_drvdata(pdev);
> -	int error;
>  
> -	error = mutex_lock_interruptible(&haptic->mutex);
> -	if (error)
> -		return error;
> -
> -	regulator_haptic_set_voltage(haptic, 0);
> +	scoped_guard(mutex_intr, &haptic->mutex) {
> +		regulator_haptic_set_voltage(haptic, 0);
> +		haptic->suspended = true;
>  
> -	haptic->suspended = true;
> -
> -	mutex_unlock(&haptic->mutex);
> +		return 0;
> +	}
>  
> -	return 0;
> +	return -EINTR;
>  }
>  
>  static int regulator_haptic_resume(struct device *dev)
> @@ -226,7 +220,7 @@ static int regulator_haptic_resume(struct device *dev)
>  	struct regulator_haptic *haptic = platform_get_drvdata(pdev);
>  	unsigned int magnitude;
>  
> -	mutex_lock(&haptic->mutex);
> +	guard(mutex)(&haptic->mutex);
>  
>  	haptic->suspended = false;
>  
> @@ -234,8 +228,6 @@ static int regulator_haptic_resume(struct device *dev)
>  	if (magnitude)
>  		regulator_haptic_set_voltage(haptic, magnitude);
>  
> -	mutex_unlock(&haptic->mutex);
> -
>  	return 0;
>  }
>  

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Re: [PATCH 20/22] Input: regulator-haptic - use guard notation when acquiring mutex

[kernel test robot]

Hi Dmitry,

kernel test robot noticed the following build warnings:

[auto build test WARNING on dtor-input/next]
[also build test WARNING on dtor-input/for-linus hid/for-next linus/master v6.11-rc6 next-20240906]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dmitry-Torokhov/Input-ad714x-use-guard-notation-when-acquiring-mutex/20240905-085752
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
patch link:    https://lore.kernel.org/r/20240904044922.1049488-1-dmitry.torokhov%40gmail.com
patch subject: [PATCH 20/22] Input: regulator-haptic - use guard notation when acquiring mutex
config: x86_64-buildonly-randconfig-001-20240907 (https://download.01.org/0day-ci/archive/20240907/202409071004.IhekZKbM-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240907/202409071004.IhekZKbM-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409071004.IhekZKbM-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/input/misc/regulator-haptic.c: In function 'regulator_haptic_suspend':
>> drivers/input/misc/regulator-haptic.c:206:13: warning: unused variable 'error' [-Wunused-variable]
     206 |         int error;
         |             ^~~~~


vim +/error +206 drivers/input/misc/regulator-haptic.c

d64cb71bede87d Jaewon Kim       2014-12-17  201  
1a3e6c1ee47d00 Jonathan Cameron 2023-01-02  202  static int regulator_haptic_suspend(struct device *dev)
d64cb71bede87d Jaewon Kim       2014-12-17  203  {
d64cb71bede87d Jaewon Kim       2014-12-17  204  	struct platform_device *pdev = to_platform_device(dev);
d64cb71bede87d Jaewon Kim       2014-12-17  205  	struct regulator_haptic *haptic = platform_get_drvdata(pdev);
d64cb71bede87d Jaewon Kim       2014-12-17 @206  	int error;
d64cb71bede87d Jaewon Kim       2014-12-17  207  
aaa0758ff6b5c4 Dmitry Torokhov  2024-09-03  208  	scoped_guard(mutex_intr, &haptic->mutex) {
d64cb71bede87d Jaewon Kim       2014-12-17  209  		regulator_haptic_set_voltage(haptic, 0);
d64cb71bede87d Jaewon Kim       2014-12-17  210  		haptic->suspended = true;
d64cb71bede87d Jaewon Kim       2014-12-17  211  
d64cb71bede87d Jaewon Kim       2014-12-17  212  		return 0;
d64cb71bede87d Jaewon Kim       2014-12-17  213  	}
d64cb71bede87d Jaewon Kim       2014-12-17  214  
aaa0758ff6b5c4 Dmitry Torokhov  2024-09-03  215  	return -EINTR;
aaa0758ff6b5c4 Dmitry Torokhov  2024-09-03  216  }
aaa0758ff6b5c4 Dmitry Torokhov  2024-09-03  217  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 21/22] Input: rotary_encoder - use guard notation when acquiring mutex

[Dmitry Torokhov]

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/rotary_encoder.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index 6628fe540834..52761da9f999 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c
@@ -113,7 +113,7 @@ static irqreturn_t rotary_encoder_irq(int irq, void *dev_id)
 	struct rotary_encoder *encoder = dev_id;
 	unsigned int state;
 
-	mutex_lock(&encoder->access_mutex);
+	guard(mutex)(&encoder->access_mutex);
 
 	state = rotary_encoder_get_state(encoder);
 
@@ -136,8 +136,6 @@ static irqreturn_t rotary_encoder_irq(int irq, void *dev_id)
 		break;
 	}
 
-	mutex_unlock(&encoder->access_mutex);
-
 	return IRQ_HANDLED;
 }
 
@@ -146,7 +144,7 @@ static irqreturn_t rotary_encoder_half_period_irq(int irq, void *dev_id)
 	struct rotary_encoder *encoder = dev_id;
 	unsigned int state;
 
-	mutex_lock(&encoder->access_mutex);
+	guard(mutex)(&encoder->access_mutex);
 
 	state = rotary_encoder_get_state(encoder);
 
@@ -159,8 +157,6 @@ static irqreturn_t rotary_encoder_half_period_irq(int irq, void *dev_id)
 		}
 	}
 
-	mutex_unlock(&encoder->access_mutex);
-
 	return IRQ_HANDLED;
 }
 
@@ -169,22 +165,19 @@ static irqreturn_t rotary_encoder_quarter_period_irq(int irq, void *dev_id)
 	struct rotary_encoder *encoder = dev_id;
 	unsigned int state;
 
-	mutex_lock(&encoder->access_mutex);
+	guard(mutex)(&encoder->access_mutex);
 
 	state = rotary_encoder_get_state(encoder);
 
-	if ((encoder->last_stable + 1) % 4 == state)
+	if ((encoder->last_stable + 1) % 4 == state) {
 		encoder->dir = 1;
-	else if (encoder->last_stable == (state + 1) % 4)
+		rotary_encoder_report_event(encoder);
+	} else if (encoder->last_stable == (state + 1) % 4) {
 		encoder->dir = -1;
-	else
-		goto out;
-
-	rotary_encoder_report_event(encoder);
+		rotary_encoder_report_event(encoder);
+	}
 
-out:
 	encoder->last_stable = state;
-	mutex_unlock(&encoder->access_mutex);
 
 	return IRQ_HANDLED;
 }
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 21/22] Input: rotary_encoder - use guard notation when acquiring mutex

[Javier Carrasco]

On 04/09/2024 06:49, 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/rotary_encoder.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
> index 6628fe540834..52761da9f999 100644
> --- a/drivers/input/misc/rotary_encoder.c
> +++ b/drivers/input/misc/rotary_encoder.c
> @@ -113,7 +113,7 @@ static irqreturn_t rotary_encoder_irq(int irq, void *dev_id)
>  	struct rotary_encoder *encoder = dev_id;
>  	unsigned int state;
>  
> -	mutex_lock(&encoder->access_mutex);
> +	guard(mutex)(&encoder->access_mutex);
>  
>  	state = rotary_encoder_get_state(encoder);
>  
> @@ -136,8 +136,6 @@ static irqreturn_t rotary_encoder_irq(int irq, void *dev_id)
>  		break;
>  	}
>  
> -	mutex_unlock(&encoder->access_mutex);
> -
>  	return IRQ_HANDLED;
>  }
>  
> @@ -146,7 +144,7 @@ static irqreturn_t rotary_encoder_half_period_irq(int irq, void *dev_id)
>  	struct rotary_encoder *encoder = dev_id;
>  	unsigned int state;
>  
> -	mutex_lock(&encoder->access_mutex);
> +	guard(mutex)(&encoder->access_mutex);
>  
>  	state = rotary_encoder_get_state(encoder);
>  
> @@ -159,8 +157,6 @@ static irqreturn_t rotary_encoder_half_period_irq(int irq, void *dev_id)
>  		}
>  	}
>  
> -	mutex_unlock(&encoder->access_mutex);
> -
>  	return IRQ_HANDLED;
>  }
>  
> @@ -169,22 +165,19 @@ static irqreturn_t rotary_encoder_quarter_period_irq(int irq, void *dev_id)
>  	struct rotary_encoder *encoder = dev_id;
>  	unsigned int state;
>  
> -	mutex_lock(&encoder->access_mutex);
> +	guard(mutex)(&encoder->access_mutex);
>  
>  	state = rotary_encoder_get_state(encoder);
>  
> -	if ((encoder->last_stable + 1) % 4 == state)
> +	if ((encoder->last_stable + 1) % 4 == state) {
>  		encoder->dir = 1;
> -	else if (encoder->last_stable == (state + 1) % 4)
> +		rotary_encoder_report_event(encoder);
> +	} else if (encoder->last_stable == (state + 1) % 4) {
>  		encoder->dir = -1;
> -	else
> -		goto out;
> -
> -	rotary_encoder_report_event(encoder);
> +		rotary_encoder_report_event(encoder);
> +	}
>  
> -out:
>  	encoder->last_stable = state;
> -	mutex_unlock(&encoder->access_mutex);
>  
>  	return IRQ_HANDLED;
>  }

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 22/22] Input: sparcspkr - use guard notation when acquiring spinlock

[Dmitry Torokhov]

Using guard notation makes the code more compact and error handling
more robust by ensuring that locks are released in all code paths
when control leaves critical section.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/sparcspkr.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/input/misc/sparcspkr.c b/drivers/input/misc/sparcspkr.c
index 20020cbc0752..5de59ae90c67 100644
--- a/drivers/input/misc/sparcspkr.c
+++ b/drivers/input/misc/sparcspkr.c
@@ -69,7 +69,6 @@ static int bbc_spkr_event(struct input_dev *dev, unsigned int type, unsigned int
 	struct sparcspkr_state *state = dev_get_drvdata(dev->dev.parent);
 	struct bbc_beep_info *info = &state->u.bbc;
 	unsigned int count = 0;
-	unsigned long flags;
 
 	if (type != EV_SND)
 		return -1;
@@ -85,7 +84,7 @@ static int bbc_spkr_event(struct input_dev *dev, unsigned int type, unsigned int
 
 	count = bbc_count_to_reg(info, count);
 
-	spin_lock_irqsave(&state->lock, flags);
+	guard(spinlock_irqsave)(&state->lock);
 
 	if (count) {
 		sbus_writeb(0x01,                 info->regs + 0);
@@ -97,8 +96,6 @@ static int bbc_spkr_event(struct input_dev *dev, unsigned int type, unsigned int
 		sbus_writeb(0x00,                 info->regs + 0);
 	}
 
-	spin_unlock_irqrestore(&state->lock, flags);
-
 	return 0;
 }
 
@@ -107,7 +104,6 @@ static int grover_spkr_event(struct input_dev *dev, unsigned int type, unsigned
 	struct sparcspkr_state *state = dev_get_drvdata(dev->dev.parent);
 	struct grover_beep_info *info = &state->u.grover;
 	unsigned int count = 0;
-	unsigned long flags;
 
 	if (type != EV_SND)
 		return -1;
@@ -121,7 +117,7 @@ static int grover_spkr_event(struct input_dev *dev, unsigned int type, unsigned
 	if (value > 20 && value < 32767)
 		count = 1193182 / value;
 
-	spin_lock_irqsave(&state->lock, flags);
+	guard(spinlock_irqsave)(&state->lock);
 
 	if (count) {
 		/* enable counter 2 */
@@ -136,8 +132,6 @@ static int grover_spkr_event(struct input_dev *dev, unsigned int type, unsigned
 		sbus_writeb(sbus_readb(info->enable_reg) & 0xFC, info->enable_reg);
 	}
 
-	spin_unlock_irqrestore(&state->lock, flags);
-
 	return 0;
 }
 
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 22/22] Input: sparcspkr - use guard notation when acquiring spinlock

[Javier Carrasco]

On 04/09/2024 06:49, Dmitry Torokhov wrote:
> Using guard notation makes the code more compact and error handling
> more robust by ensuring that locks are released in all code paths
> when control leaves critical section.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/sparcspkr.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/input/misc/sparcspkr.c b/drivers/input/misc/sparcspkr.c
> index 20020cbc0752..5de59ae90c67 100644
> --- a/drivers/input/misc/sparcspkr.c
> +++ b/drivers/input/misc/sparcspkr.c
> @@ -69,7 +69,6 @@ static int bbc_spkr_event(struct input_dev *dev, unsigned int type, unsigned int
>  	struct sparcspkr_state *state = dev_get_drvdata(dev->dev.parent);
>  	struct bbc_beep_info *info = &state->u.bbc;
>  	unsigned int count = 0;
> -	unsigned long flags;
>  
>  	if (type != EV_SND)
>  		return -1;
> @@ -85,7 +84,7 @@ static int bbc_spkr_event(struct input_dev *dev, unsigned int type, unsigned int
>  
>  	count = bbc_count_to_reg(info, count);
>  
> -	spin_lock_irqsave(&state->lock, flags);
> +	guard(spinlock_irqsave)(&state->lock);
>  
>  	if (count) {
>  		sbus_writeb(0x01,                 info->regs + 0);
> @@ -97,8 +96,6 @@ static int bbc_spkr_event(struct input_dev *dev, unsigned int type, unsigned int
>  		sbus_writeb(0x00,                 info->regs + 0);
>  	}
>  
> -	spin_unlock_irqrestore(&state->lock, flags);
> -
>  	return 0;
>  }
>  
> @@ -107,7 +104,6 @@ static int grover_spkr_event(struct input_dev *dev, unsigned int type, unsigned
>  	struct sparcspkr_state *state = dev_get_drvdata(dev->dev.parent);
>  	struct grover_beep_info *info = &state->u.grover;
>  	unsigned int count = 0;
> -	unsigned long flags;
>  
>  	if (type != EV_SND)
>  		return -1;
> @@ -121,7 +117,7 @@ static int grover_spkr_event(struct input_dev *dev, unsigned int type, unsigned
>  	if (value > 20 && value < 32767)
>  		count = 1193182 / value;
>  
> -	spin_lock_irqsave(&state->lock, flags);
> +	guard(spinlock_irqsave)(&state->lock);
>  
>  	if (count) {
>  		/* enable counter 2 */
> @@ -136,8 +132,6 @@ static int grover_spkr_event(struct input_dev *dev, unsigned int type, unsigned
>  		sbus_writeb(sbus_readb(info->enable_reg) & 0xFC, info->enable_reg);
>  	}
>  
> -	spin_unlock_irqrestore(&state->lock, flags);
> -
>  	return 0;
>  }
>  

Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>