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

[Javier Carrasco <javier.carrasco.cruz@gmail.com>]

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>