diff mbox series

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

Message ID 20240904044244.1042174-4-dmitry.torokhov@gmail.com (mailing list archive)
State Mainlined
Commit d8a43a83633a4770f00ee77079fbb7a2218cea2a
Headers show
Series Convert misc input drivers to use new cleanup facilities | expand

Commit Message

Dmitry Torokhov Sept. 4, 2024, 4:42 a.m. UTC 
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(-)

Comments

Javier Carrasco Sept. 4, 2024, 7:44 p.m. UTC | #1 
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>
diff mbox series

Patch

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