diff mbox series

[20/24] Input: serio_raw - use guard notation for locks and other resources

Message ID 20240905041732.2034348-21-dmitry.torokhov@gmail.com (mailing list archive)
State Mainlined
Commit 5b53a9d40c4f83b44de8da06af9a9c9b3fb14988
Headers show
Series Convert serio-related drivers to use new cleanup facilities | expand

Commit Message

Dmitry Torokhov Sept. 5, 2024, 4:17 a.m. UTC 
Use guard notation when acquiring mutexes and spinlocks, and when
pausing and resuming serio port. Such 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/serio/serio_raw.c | 121 +++++++++++++-------------------
 1 file changed, 49 insertions(+), 72 deletions(-)

Comments

Kees Bakker Oct. 21, 2024, 8:41 p.m. UTC | #1 
Op 05-09-2024 om 06:17 schreef Dmitry Torokhov:
> Use guard notation when acquiring mutexes and spinlocks, and when
> pausing and resuming serio port. Such 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/serio/serio_raw.c | 121 +++++++++++++-------------------
>   1 file changed, 49 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c
> index 0186d1b38f49..aef8301313b2 100644
> [...]
>   static ssize_t serio_raw_read(struct file *file, char __user *buffer,
> @@ -200,40 +185,32 @@ static ssize_t serio_raw_write(struct file *file, const char __user *buffer,
>   {
>   	struct serio_raw_client *client = file->private_data;
>   	struct serio_raw *serio_raw = client->serio_raw;
> -	int retval = 0;
> +	int written;
Didn't you forget to initialize `written` to zero?
> [...]
>
diff mbox series

Patch

diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c
index 0186d1b38f49..aef8301313b2 100644
--- a/drivers/input/serio/serio_raw.c
+++ b/drivers/input/serio/serio_raw.c
@@ -75,41 +75,31 @@  static int serio_raw_open(struct inode *inode, struct file *file)
 {
 	struct serio_raw *serio_raw;
 	struct serio_raw_client *client;
-	int retval;
 
-	retval = mutex_lock_interruptible(&serio_raw_mutex);
-	if (retval)
-		return retval;
+	scoped_guard(mutex_intr, &serio_raw_mutex) {
+		serio_raw = serio_raw_locate(iminor(inode));
+		if (!serio_raw)
+			return -ENODEV;
 
-	serio_raw = serio_raw_locate(iminor(inode));
-	if (!serio_raw) {
-		retval = -ENODEV;
-		goto out;
-	}
+		if (serio_raw->dead)
+			return -ENODEV;
 
-	if (serio_raw->dead) {
-		retval = -ENODEV;
-		goto out;
-	}
+		client = kzalloc(sizeof(*client), GFP_KERNEL);
+		if (!client)
+			return -ENOMEM;
 
-	client = kzalloc(sizeof(*client), GFP_KERNEL);
-	if (!client) {
-		retval = -ENOMEM;
-		goto out;
-	}
+		client->serio_raw = serio_raw;
+		file->private_data = client;
 
-	client->serio_raw = serio_raw;
-	file->private_data = client;
+		kref_get(&serio_raw->kref);
 
-	kref_get(&serio_raw->kref);
+		scoped_guard(serio_pause_rx, serio_raw->serio)
+			list_add_tail(&client->node, &serio_raw->client_list);
 
-	serio_pause_rx(serio_raw->serio);
-	list_add_tail(&client->node, &serio_raw->client_list);
-	serio_continue_rx(serio_raw->serio);
+		return 0;
+	}
 
-out:
-	mutex_unlock(&serio_raw_mutex);
-	return retval;
+	return -EINTR;
 }
 
 static void serio_raw_free(struct kref *kref)
@@ -126,9 +116,8 @@  static int serio_raw_release(struct inode *inode, struct file *file)
 	struct serio_raw_client *client = file->private_data;
 	struct serio_raw *serio_raw = client->serio_raw;
 
-	serio_pause_rx(serio_raw->serio);
-	list_del(&client->node);
-	serio_continue_rx(serio_raw->serio);
+	scoped_guard(serio_pause_rx, serio_raw->serio)
+		list_del(&client->node);
 
 	kfree(client);
 
@@ -139,19 +128,15 @@  static int serio_raw_release(struct inode *inode, struct file *file)
 
 static bool serio_raw_fetch_byte(struct serio_raw *serio_raw, char *c)
 {
-	bool empty;
+	guard(serio_pause_rx)(serio_raw->serio);
 
-	serio_pause_rx(serio_raw->serio);
-
-	empty = serio_raw->head == serio_raw->tail;
-	if (!empty) {
-		*c = serio_raw->queue[serio_raw->tail];
-		serio_raw->tail = (serio_raw->tail + 1) % SERIO_RAW_QUEUE_LEN;
-	}
+	if (serio_raw->head == serio_raw->tail)
+		return false; /* queue is empty */
 
-	serio_continue_rx(serio_raw->serio);
+	*c = serio_raw->queue[serio_raw->tail];
+	serio_raw->tail = (serio_raw->tail + 1) % SERIO_RAW_QUEUE_LEN;
 
-	return !empty;
+	return true;
 }
 
 static ssize_t serio_raw_read(struct file *file, char __user *buffer,
@@ -200,40 +185,32 @@  static ssize_t serio_raw_write(struct file *file, const char __user *buffer,
 {
 	struct serio_raw_client *client = file->private_data;
 	struct serio_raw *serio_raw = client->serio_raw;
-	int retval = 0;
+	int written;
 	unsigned char c;
 
-	retval = mutex_lock_interruptible(&serio_raw_mutex);
-	if (retval)
-		return retval;
+	scoped_guard(mutex_intr, &serio_raw_mutex) {
+		if (serio_raw->dead)
+			return -ENODEV;
 
-	if (serio_raw->dead) {
-		retval = -ENODEV;
-		goto out;
-	}
+		if (count > 32)
+			count = 32;
 
-	if (count > 32)
-		count = 32;
+		while (count--) {
+			if (get_user(c, buffer++))
+				return -EFAULT;
 
-	while (count--) {
-		if (get_user(c, buffer++)) {
-			retval = -EFAULT;
-			goto out;
-		}
+			if (serio_write(serio_raw->serio, c)) {
+				/* Either signal error or partial write */
+				return written ?: -EIO;
+			}
 
-		if (serio_write(serio_raw->serio, c)) {
-			/* Either signal error or partial write */
-			if (retval == 0)
-				retval = -EIO;
-			goto out;
+			written++;
 		}
 
-		retval++;
+		return written;
 	}
 
-out:
-	mutex_unlock(&serio_raw_mutex);
-	return retval;
+	return -EINTR;
 }
 
 static __poll_t serio_raw_poll(struct file *file, poll_table *wait)
@@ -379,10 +356,10 @@  static void serio_raw_hangup(struct serio_raw *serio_raw)
 {
 	struct serio_raw_client *client;
 
-	serio_pause_rx(serio_raw->serio);
-	list_for_each_entry(client, &serio_raw->client_list, node)
-		kill_fasync(&client->fasync, SIGIO, POLL_HUP);
-	serio_continue_rx(serio_raw->serio);
+	scoped_guard(serio_pause_rx, serio_raw->serio) {
+		list_for_each_entry(client, &serio_raw->client_list, node)
+			kill_fasync(&client->fasync, SIGIO, POLL_HUP);
+	}
 
 	wake_up_interruptible(&serio_raw->wait);
 }
@@ -394,10 +371,10 @@  static void serio_raw_disconnect(struct serio *serio)
 
 	misc_deregister(&serio_raw->dev);
 
-	mutex_lock(&serio_raw_mutex);
-	serio_raw->dead = true;
-	list_del_init(&serio_raw->node);
-	mutex_unlock(&serio_raw_mutex);
+	scoped_guard(mutex, &serio_raw_mutex) {
+		serio_raw->dead = true;
+		list_del_init(&serio_raw->node);
+	}
 
 	serio_raw_hangup(serio_raw);