| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <83199f0f-67a9-4fe7-9cbe-f3821ad4c041@gmail.com>
Date: Wed, 4 Sep 2024 21:44:54 +0200
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>, linux-input@...r.kernel.org
Cc: Michael Hennerich <michael.hennerich@...log.com>,
Ville Syrjala <syrjala@....fi>,
Support Opensource <support.opensource@...semi.com>,
Eddie James <eajames@...ux.ibm.com>, Andrey Moiseev <o2g.org.ru@...il.com>,
Hans de Goede <hdegoede@...hat.com>, Jeff LaBundy <jeff@...undy.com>,
linux-kernel@...r.kernel.org,
Javier Carrasco Cruz <javier.carrasco.cruz@...il.com>
Subject: Re: [PATCH 03/22] Input: cm109 - use guard notation when acquiring
mutex and spinlock
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@...il.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@...il.com>
Powered by blists - more mailing lists