* [PATCH 01/10] tty: pty: use guard()s
2025-11-19 10:01 [PATCH 00/10] tty: another batch of conversions to guards Jiri Slaby (SUSE)
@ 2025-11-19 10:01 ` Jiri Slaby (SUSE)
2025-11-19 10:01 ` [PATCH 02/10] tty: n_tty: " Jiri Slaby (SUSE)
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jiri Slaby (SUSE) @ 2025-11-19 10:01 UTC (permalink / raw)
To: gregkh; +Cc: linux-serial, linux-kernel, Jiri Slaby (SUSE)
Use guards in the pty code. This improves readability, makes error
handling easier, and marks locked portions of code explicit. All that
while being sure the lock is unlocked.
pty_set_pktmode() is handled specially -- the conditions are inverted
and return called if conditions unmet. This avoid double nested 'if's.
The variable is renamed to want_pktmode so it is not confused with the
current state of pktmode.
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
drivers/tty/pty.c | 103 ++++++++++++++++++++--------------------------
1 file changed, 45 insertions(+), 58 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 8bb1a01fef2a..76188b8f3ba3 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -57,9 +57,8 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
set_bit(TTY_IO_ERROR, &tty->flags);
wake_up_interruptible(&tty->read_wait);
wake_up_interruptible(&tty->write_wait);
- spin_lock_irq(&tty->ctrl.lock);
- tty->ctrl.packet = false;
- spin_unlock_irq(&tty->ctrl.lock);
+ scoped_guard(spinlock_irq, &tty->ctrl.lock)
+ tty->ctrl.packet = false;
/* Review - krefs on tty_link ?? */
if (!tty->link)
return;
@@ -70,10 +69,9 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
set_bit(TTY_OTHER_CLOSED, &tty->flags);
#ifdef CONFIG_UNIX98_PTYS
if (tty->driver == ptm_driver) {
- mutex_lock(&devpts_mutex);
+ guard(mutex)(&devpts_mutex);
if (tty->link->driver_data)
devpts_pty_kill(tty->link->driver_data);
- mutex_unlock(&devpts_mutex);
}
#endif
tty_vhangup(tty->link);
@@ -157,21 +155,23 @@ static int pty_get_lock(struct tty_struct *tty, int __user *arg)
/* Set the packet mode on a pty */
static int pty_set_pktmode(struct tty_struct *tty, int __user *arg)
{
- int pktmode;
+ int want_pktmode;
- if (get_user(pktmode, arg))
+ if (get_user(want_pktmode, arg))
return -EFAULT;
- spin_lock_irq(&tty->ctrl.lock);
- if (pktmode) {
- if (!tty->ctrl.packet) {
- tty->link->ctrl.pktstatus = 0;
- smp_mb();
- tty->ctrl.packet = true;
- }
- } else
+ guard(spinlock_irq)(&tty->ctrl.lock);
+ if (!want_pktmode) {
tty->ctrl.packet = false;
- spin_unlock_irq(&tty->ctrl.lock);
+ return 0;
+ }
+
+ if (tty->ctrl.packet)
+ return 0;
+
+ tty->link->ctrl.pktstatus = 0;
+ smp_mb();
+ tty->ctrl.packet = true;
return 0;
}
@@ -210,10 +210,9 @@ static void pty_flush_buffer(struct tty_struct *tty)
tty_buffer_flush(to, NULL);
if (to->ctrl.packet) {
- spin_lock_irq(&tty->ctrl.lock);
+ guard(spinlock_irq)(&tty->ctrl.lock);
tty->ctrl.pktstatus |= TIOCPKT_FLUSHWRITE;
wake_up_interruptible(&to->read_wait);
- spin_unlock_irq(&tty->ctrl.lock);
}
}
@@ -252,17 +251,17 @@ static void pty_set_termios(struct tty_struct *tty,
STOP_CHAR(tty) == '\023' &&
START_CHAR(tty) == '\021');
if ((old_flow != new_flow) || extproc) {
- spin_lock_irq(&tty->ctrl.lock);
- if (old_flow != new_flow) {
- tty->ctrl.pktstatus &= ~(TIOCPKT_DOSTOP | TIOCPKT_NOSTOP);
- if (new_flow)
- tty->ctrl.pktstatus |= TIOCPKT_DOSTOP;
- else
- tty->ctrl.pktstatus |= TIOCPKT_NOSTOP;
+ scoped_guard(spinlock_irq, &tty->ctrl.lock) {
+ if (old_flow != new_flow) {
+ tty->ctrl.pktstatus &= ~(TIOCPKT_DOSTOP | TIOCPKT_NOSTOP);
+ if (new_flow)
+ tty->ctrl.pktstatus |= TIOCPKT_DOSTOP;
+ else
+ tty->ctrl.pktstatus |= TIOCPKT_NOSTOP;
+ }
+ if (extproc)
+ tty->ctrl.pktstatus |= TIOCPKT_IOCTL;
}
- if (extproc)
- tty->ctrl.pktstatus |= TIOCPKT_IOCTL;
- spin_unlock_irq(&tty->ctrl.lock);
wake_up_interruptible(&tty->link->read_wait);
}
}
@@ -286,9 +285,9 @@ static int pty_resize(struct tty_struct *tty, struct winsize *ws)
struct tty_struct *pty = tty->link;
/* For a PTY we need to lock the tty side */
- mutex_lock(&tty->winsize_mutex);
+ guard(mutex)(&tty->winsize_mutex);
if (!memcmp(ws, &tty->winsize, sizeof(*ws)))
- goto done;
+ return 0;
/* Signal the foreground process group of both ptys */
pgrp = tty_get_pgrp(tty);
@@ -304,8 +303,7 @@ static int pty_resize(struct tty_struct *tty, struct winsize *ws)
tty->winsize = *ws;
pty->winsize = *ws; /* Never used so will go away soon */
-done:
- mutex_unlock(&tty->winsize_mutex);
+
return 0;
}
@@ -321,28 +319,26 @@ static int pty_resize(struct tty_struct *tty, struct winsize *ws)
*/
static void pty_start(struct tty_struct *tty)
{
- unsigned long flags;
+ if (!tty->link || !tty->link->ctrl.packet)
+ return;
- if (tty->link && tty->link->ctrl.packet) {
- spin_lock_irqsave(&tty->ctrl.lock, flags);
+ scoped_guard(spinlock_irqsave, &tty->ctrl.lock) {
tty->ctrl.pktstatus &= ~TIOCPKT_STOP;
tty->ctrl.pktstatus |= TIOCPKT_START;
- spin_unlock_irqrestore(&tty->ctrl.lock, flags);
- wake_up_interruptible_poll(&tty->link->read_wait, EPOLLIN);
}
+ wake_up_interruptible_poll(&tty->link->read_wait, EPOLLIN);
}
static void pty_stop(struct tty_struct *tty)
{
- unsigned long flags;
+ if (!tty->link || !tty->link->ctrl.packet)
+ return;
- if (tty->link && tty->link->ctrl.packet) {
- spin_lock_irqsave(&tty->ctrl.lock, flags);
+ scoped_guard(spinlock_irqsave, &tty->ctrl.lock) {
tty->ctrl.pktstatus &= ~TIOCPKT_START;
tty->ctrl.pktstatus |= TIOCPKT_STOP;
- spin_unlock_irqrestore(&tty->ctrl.lock, flags);
- wake_up_interruptible_poll(&tty->link->read_wait, EPOLLIN);
}
+ wake_up_interruptible_poll(&tty->link->read_wait, EPOLLIN);
}
/**
@@ -705,15 +701,9 @@ static struct tty_struct *ptm_unix98_lookup(struct tty_driver *driver,
static struct tty_struct *pts_unix98_lookup(struct tty_driver *driver,
struct file *file, int idx)
{
- struct tty_struct *tty;
-
- mutex_lock(&devpts_mutex);
- tty = devpts_get_priv(file->f_path.dentry);
- mutex_unlock(&devpts_mutex);
+ guard(mutex)(&devpts_mutex);
/* Master must be open before slave */
- if (!tty)
- return ERR_PTR(-EIO);
- return tty;
+ return devpts_get_priv(file->f_path.dentry) ? : ERR_PTR(-EIO);
}
static int pty_unix98_install(struct tty_driver *driver, struct tty_struct *tty)
@@ -811,20 +801,17 @@ static int ptmx_open(struct inode *inode, struct file *filp)
}
/* find a device that is not in use. */
- mutex_lock(&devpts_mutex);
- index = devpts_new_index(fsi);
- mutex_unlock(&devpts_mutex);
+ scoped_guard(mutex, &devpts_mutex)
+ index = devpts_new_index(fsi);
retval = index;
if (index < 0)
goto out_put_fsi;
- mutex_lock(&tty_mutex);
- tty = tty_init_dev(ptm_driver, index);
- /* The tty returned here is locked so we can safely
- drop the mutex */
- mutex_unlock(&tty_mutex);
+ /* The tty returned here is locked so we can safely drop the mutex */
+ scoped_guard(mutex, &tty_mutex)
+ tty = tty_init_dev(ptm_driver, index);
retval = PTR_ERR(tty);
if (IS_ERR(tty))
--
2.51.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 02/10] tty: n_tty: use guard()s
2025-11-19 10:01 [PATCH 00/10] tty: another batch of conversions to guards Jiri Slaby (SUSE)
2025-11-19 10:01 ` [PATCH 01/10] tty: pty: use guard()s Jiri Slaby (SUSE)
@ 2025-11-19 10:01 ` Jiri Slaby (SUSE)
2025-11-19 10:01 ` [PATCH 03/10] tty: n_hdlc: simplify return from n_hdlc_tty_ioctl() Jiri Slaby (SUSE)
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jiri Slaby (SUSE) @ 2025-11-19 10:01 UTC (permalink / raw)
To: gregkh; +Cc: linux-serial, linux-kernel, Jiri Slaby (SUSE)
Use guards in the n_tty code. This improves readability, makes error
handling easier, and marks locked portions of code explicit. All that
while being sure the lock is unlocked.
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
drivers/tty/n_tty.c | 109 +++++++++++++++++++-------------------------
1 file changed, 48 insertions(+), 61 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 6af3f3a0b531..e6a0f5b40d0a 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -324,12 +324,9 @@ static void reset_buffer_flags(struct n_tty_data *ldata)
static void n_tty_packet_mode_flush(struct tty_struct *tty)
{
- unsigned long flags;
-
if (tty->link->ctrl.packet) {
- spin_lock_irqsave(&tty->ctrl.lock, flags);
- tty->ctrl.pktstatus |= TIOCPKT_FLUSHREAD;
- spin_unlock_irqrestore(&tty->ctrl.lock, flags);
+ scoped_guard(spinlock_irqsave, &tty->ctrl.lock)
+ tty->ctrl.pktstatus |= TIOCPKT_FLUSHREAD;
wake_up_interruptible(&tty->link->read_wait);
}
}
@@ -349,13 +346,12 @@ static void n_tty_packet_mode_flush(struct tty_struct *tty)
*/
static void n_tty_flush_buffer(struct tty_struct *tty)
{
- down_write(&tty->termios_rwsem);
+ guard(rwsem_write)(&tty->termios_rwsem);
reset_buffer_flags(tty->disc_data);
n_tty_kick_worker(tty);
if (tty->link)
n_tty_packet_mode_flush(tty);
- up_write(&tty->termios_rwsem);
}
/**
@@ -737,24 +733,22 @@ static void commit_echoes(struct tty_struct *tty)
size_t nr, old, echoed;
size_t head;
- mutex_lock(&ldata->output_lock);
- head = ldata->echo_head;
- ldata->echo_mark = head;
- old = ldata->echo_commit - ldata->echo_tail;
-
- /* Process committed echoes if the accumulated # of bytes
- * is over the threshold (and try again each time another
- * block is accumulated) */
- nr = head - ldata->echo_tail;
- if (nr < ECHO_COMMIT_WATERMARK ||
- (nr % ECHO_BLOCK > old % ECHO_BLOCK)) {
- mutex_unlock(&ldata->output_lock);
- return;
- }
+ scoped_guard(mutex, &ldata->output_lock) {
+ head = ldata->echo_head;
+ ldata->echo_mark = head;
+ old = ldata->echo_commit - ldata->echo_tail;
+
+ /*
+ * Process committed echoes if the accumulated # of bytes is over the threshold
+ * (and try again each time another block is accumulated)
+ */
+ nr = head - ldata->echo_tail;
+ if (nr < ECHO_COMMIT_WATERMARK || (nr % ECHO_BLOCK > old % ECHO_BLOCK))
+ return;
- ldata->echo_commit = head;
- echoed = __process_echoes(tty);
- mutex_unlock(&ldata->output_lock);
+ ldata->echo_commit = head;
+ echoed = __process_echoes(tty);
+ }
if (echoed && tty->ops->flush_chars)
tty->ops->flush_chars(tty);
@@ -768,10 +762,10 @@ static void process_echoes(struct tty_struct *tty)
if (ldata->echo_mark == ldata->echo_tail)
return;
- mutex_lock(&ldata->output_lock);
- ldata->echo_commit = ldata->echo_mark;
- echoed = __process_echoes(tty);
- mutex_unlock(&ldata->output_lock);
+ scoped_guard(mutex, &ldata->output_lock) {
+ ldata->echo_commit = ldata->echo_mark;
+ echoed = __process_echoes(tty);
+ }
if (echoed && tty->ops->flush_chars)
tty->ops->flush_chars(tty);
@@ -786,10 +780,9 @@ static void flush_echoes(struct tty_struct *tty)
ldata->echo_commit == ldata->echo_head)
return;
- mutex_lock(&ldata->output_lock);
+ guard(mutex)(&ldata->output_lock);
ldata->echo_commit = ldata->echo_head;
__process_echoes(tty);
- mutex_unlock(&ldata->output_lock);
}
/**
@@ -1078,18 +1071,19 @@ static void isig(int sig, struct tty_struct *tty)
if (L_NOFLSH(tty)) {
/* signal only */
__isig(sig, tty);
+ return;
+ }
- } else { /* signal and flush */
- up_read(&tty->termios_rwsem);
- down_write(&tty->termios_rwsem);
-
+ /* signal and flush */
+ up_read(&tty->termios_rwsem);
+ scoped_guard(rwsem_write, &tty->termios_rwsem) {
__isig(sig, tty);
/* clear echo buffer */
- mutex_lock(&ldata->output_lock);
- ldata->echo_head = ldata->echo_tail = 0;
- ldata->echo_mark = ldata->echo_commit = 0;
- mutex_unlock(&ldata->output_lock);
+ scoped_guard(mutex, &ldata->output_lock) {
+ ldata->echo_head = ldata->echo_tail = 0;
+ ldata->echo_mark = ldata->echo_commit = 0;
+ }
/* clear output buffer */
tty_driver_flush_buffer(tty);
@@ -1100,10 +1094,8 @@ static void isig(int sig, struct tty_struct *tty)
/* notify pty master of flush */
if (tty->link)
n_tty_packet_mode_flush(tty);
-
- up_write(&tty->termios_rwsem);
- down_read(&tty->termios_rwsem);
}
+ down_read(&tty->termios_rwsem);
}
/**
@@ -1683,7 +1675,7 @@ n_tty_receive_buf_common(struct tty_struct *tty, const u8 *cp, const u8 *fp,
size_t n, rcvd = 0;
int room, overflow;
- down_read(&tty->termios_rwsem);
+ guard(rwsem_read)(&tty->termios_rwsem);
do {
/*
@@ -1752,8 +1744,6 @@ n_tty_receive_buf_common(struct tty_struct *tty, const u8 *cp, const u8 *fp,
n_tty_kick_worker(tty);
}
- up_read(&tty->termios_rwsem);
-
return rcvd;
}
@@ -1879,10 +1869,9 @@ static void n_tty_close(struct tty_struct *tty)
if (tty->link)
n_tty_packet_mode_flush(tty);
- down_write(&tty->termios_rwsem);
+ guard(rwsem_write)(&tty->termios_rwsem);
vfree(ldata);
tty->disc_data = NULL;
- up_write(&tty->termios_rwsem);
}
/**
@@ -2247,10 +2236,10 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, u8 *kbuf,
u8 cs;
if (kb != kbuf)
break;
- spin_lock_irq(&tty->link->ctrl.lock);
- cs = tty->link->ctrl.pktstatus;
- tty->link->ctrl.pktstatus = 0;
- spin_unlock_irq(&tty->link->ctrl.lock);
+ scoped_guard(spinlock_irq, &tty->link->ctrl.lock) {
+ cs = tty->link->ctrl.pktstatus;
+ tty->link->ctrl.pktstatus = 0;
+ }
*kb++ = cs;
nr--;
break;
@@ -2357,7 +2346,7 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
return retval;
}
- down_read(&tty->termios_rwsem);
+ guard(rwsem_read)(&tty->termios_rwsem);
/* Write out any echoed characters that are still pending */
process_echoes(tty);
@@ -2395,9 +2384,8 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
struct n_tty_data *ldata = tty->disc_data;
while (nr > 0) {
- mutex_lock(&ldata->output_lock);
- num = tty->ops->write(tty, b, nr);
- mutex_unlock(&ldata->output_lock);
+ scoped_guard(mutex, &ldata->output_lock)
+ num = tty->ops->write(tty, b, nr);
if (num < 0) {
retval = num;
goto break_out;
@@ -2424,7 +2412,7 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
remove_wait_queue(&tty->write_wait, &wait);
if (nr && tty->fasync)
set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
- up_read(&tty->termios_rwsem);
+
return (b - buf) ? b - buf : retval;
}
@@ -2498,12 +2486,11 @@ static int n_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
case TIOCOUTQ:
return put_user(tty_chars_in_buffer(tty), (int __user *) arg);
case TIOCINQ:
- down_write(&tty->termios_rwsem);
- if (L_ICANON(tty) && !L_EXTPROC(tty))
- num = inq_canon(ldata);
- else
- num = read_cnt(ldata);
- up_write(&tty->termios_rwsem);
+ scoped_guard(rwsem_write, &tty->termios_rwsem)
+ if (L_ICANON(tty) && !L_EXTPROC(tty))
+ num = inq_canon(ldata);
+ else
+ num = read_cnt(ldata);
return put_user(num, (unsigned int __user *) arg);
default:
return n_tty_ioctl_helper(tty, cmd, arg);
--
2.51.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 03/10] tty: n_hdlc: simplify return from n_hdlc_tty_ioctl()
2025-11-19 10:01 [PATCH 00/10] tty: another batch of conversions to guards Jiri Slaby (SUSE)
2025-11-19 10:01 ` [PATCH 01/10] tty: pty: use guard()s Jiri Slaby (SUSE)
2025-11-19 10:01 ` [PATCH 02/10] tty: n_tty: " Jiri Slaby (SUSE)
@ 2025-11-19 10:01 ` Jiri Slaby (SUSE)
2025-11-19 10:01 ` [PATCH 04/10] tty: n_hdlc: use guard()s Jiri Slaby (SUSE)
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jiri Slaby (SUSE) @ 2025-11-19 10:01 UTC (permalink / raw)
To: gregkh; +Cc: linux-serial, linux-kernel, Jiri Slaby (SUSE)
The cases in the switch() of n_hdlc_tty_ioctl() can return immediately
-- no need to store into error and return later.
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
drivers/tty/n_hdlc.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
index 4a4dc58b866a..66bb8ce6ec40 100644
--- a/drivers/tty/n_hdlc.c
+++ b/drivers/tty/n_hdlc.c
@@ -584,7 +584,6 @@ static int n_hdlc_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
unsigned long arg)
{
struct n_hdlc *n_hdlc = tty->disc_data;
- int error = 0;
int count;
unsigned long flags;
struct n_hdlc_buf *buf = NULL;
@@ -603,8 +602,7 @@ static int n_hdlc_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
else
count = 0;
spin_unlock_irqrestore(&n_hdlc->rx_buf_list.spinlock, flags);
- error = put_user(count, (int __user *)arg);
- break;
+ return put_user(count, (int __user *)arg);
case TIOCOUTQ:
/* get the pending tx byte count in the driver */
@@ -616,8 +614,7 @@ static int n_hdlc_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
if (buf)
count += buf->count;
spin_unlock_irqrestore(&n_hdlc->tx_buf_list.spinlock, flags);
- error = put_user(count, (int __user *)arg);
- break;
+ return put_user(count, (int __user *)arg);
case TCFLSH:
switch (arg) {
@@ -628,11 +625,8 @@ static int n_hdlc_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
fallthrough; /* to default */
default:
- error = n_tty_ioctl_helper(tty, cmd, arg);
- break;
+ return n_tty_ioctl_helper(tty, cmd, arg);
}
- return error;
-
} /* end of n_hdlc_tty_ioctl() */
/**
--
2.51.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 04/10] tty: n_hdlc: use guard()s
2025-11-19 10:01 [PATCH 00/10] tty: another batch of conversions to guards Jiri Slaby (SUSE)
` (2 preceding siblings ...)
2025-11-19 10:01 ` [PATCH 03/10] tty: n_hdlc: simplify return from n_hdlc_tty_ioctl() Jiri Slaby (SUSE)
@ 2025-11-19 10:01 ` Jiri Slaby (SUSE)
2025-11-19 10:01 ` [PATCH 05/10] tty: moxa: " Jiri Slaby (SUSE)
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jiri Slaby (SUSE) @ 2025-11-19 10:01 UTC (permalink / raw)
To: gregkh; +Cc: linux-serial, linux-kernel, Jiri Slaby (SUSE)
Use guards in the n_hdlc code. This improves readability, makes error
handling easier, and marks locked portions of code explicit. All that
while being sure the lock is unlocked.
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
drivers/tty/n_hdlc.c | 67 +++++++++++++++++---------------------------
1 file changed, 26 insertions(+), 41 deletions(-)
diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
index 66bb8ce6ec40..3c9dcb0928c6 100644
--- a/drivers/tty/n_hdlc.c
+++ b/drivers/tty/n_hdlc.c
@@ -263,21 +263,18 @@ static int n_hdlc_tty_open(struct tty_struct *tty)
*/
static void n_hdlc_send_frames(struct n_hdlc *n_hdlc, struct tty_struct *tty)
{
- unsigned long flags;
struct n_hdlc_buf *tbuf;
ssize_t actual;
check_again:
-
- spin_lock_irqsave(&n_hdlc->tx_buf_list.spinlock, flags);
- if (n_hdlc->tbusy) {
- n_hdlc->woke_up = true;
- spin_unlock_irqrestore(&n_hdlc->tx_buf_list.spinlock, flags);
- return;
+ scoped_guard(spinlock_irqsave, &n_hdlc->tx_buf_list.spinlock) {
+ if (n_hdlc->tbusy) {
+ n_hdlc->woke_up = true;
+ return;
+ }
+ n_hdlc->tbusy = true;
+ n_hdlc->woke_up = false;
}
- n_hdlc->tbusy = true;
- n_hdlc->woke_up = false;
- spin_unlock_irqrestore(&n_hdlc->tx_buf_list.spinlock, flags);
tbuf = n_hdlc_buf_get(&n_hdlc->tx_buf_list);
while (tbuf) {
@@ -324,9 +321,8 @@ static void n_hdlc_send_frames(struct n_hdlc *n_hdlc, struct tty_struct *tty)
clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
/* Clear the re-entry flag */
- spin_lock_irqsave(&n_hdlc->tx_buf_list.spinlock, flags);
- n_hdlc->tbusy = false;
- spin_unlock_irqrestore(&n_hdlc->tx_buf_list.spinlock, flags);
+ scoped_guard(spinlock_irqsave, &n_hdlc->tx_buf_list.spinlock)
+ n_hdlc->tbusy = false;
if (n_hdlc->woke_up)
goto check_again;
@@ -585,7 +581,6 @@ static int n_hdlc_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
{
struct n_hdlc *n_hdlc = tty->disc_data;
int count;
- unsigned long flags;
struct n_hdlc_buf *buf = NULL;
pr_debug("%s() called %d\n", __func__, cmd);
@@ -594,26 +589,26 @@ static int n_hdlc_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
case FIONREAD:
/* report count of read data available */
/* in next available frame (if any) */
- spin_lock_irqsave(&n_hdlc->rx_buf_list.spinlock, flags);
- buf = list_first_entry_or_null(&n_hdlc->rx_buf_list.list,
- struct n_hdlc_buf, list_item);
- if (buf)
- count = buf->count;
- else
- count = 0;
- spin_unlock_irqrestore(&n_hdlc->rx_buf_list.spinlock, flags);
+ scoped_guard(spinlock_irqsave, &n_hdlc->rx_buf_list.spinlock) {
+ buf = list_first_entry_or_null(&n_hdlc->rx_buf_list.list,
+ struct n_hdlc_buf, list_item);
+ if (buf)
+ count = buf->count;
+ else
+ count = 0;
+ }
return put_user(count, (int __user *)arg);
case TIOCOUTQ:
/* get the pending tx byte count in the driver */
count = tty_chars_in_buffer(tty);
/* add size of next output frame in queue */
- spin_lock_irqsave(&n_hdlc->tx_buf_list.spinlock, flags);
- buf = list_first_entry_or_null(&n_hdlc->tx_buf_list.list,
- struct n_hdlc_buf, list_item);
- if (buf)
- count += buf->count;
- spin_unlock_irqrestore(&n_hdlc->tx_buf_list.spinlock, flags);
+ scoped_guard(spinlock_irqsave, &n_hdlc->tx_buf_list.spinlock) {
+ buf = list_first_entry_or_null(&n_hdlc->tx_buf_list.list,
+ struct n_hdlc_buf, list_item);
+ if (buf)
+ count += buf->count;
+ }
return put_user(count, (int __user *)arg);
case TCFLSH:
@@ -720,14 +715,10 @@ static struct n_hdlc *n_hdlc_alloc(void)
static void n_hdlc_buf_return(struct n_hdlc_buf_list *buf_list,
struct n_hdlc_buf *buf)
{
- unsigned long flags;
-
- spin_lock_irqsave(&buf_list->spinlock, flags);
+ guard(spinlock_irqsave)(&buf_list->spinlock);
list_add(&buf->list_item, &buf_list->list);
buf_list->count++;
-
- spin_unlock_irqrestore(&buf_list->spinlock, flags);
}
/**
@@ -738,14 +729,10 @@ static void n_hdlc_buf_return(struct n_hdlc_buf_list *buf_list,
static void n_hdlc_buf_put(struct n_hdlc_buf_list *buf_list,
struct n_hdlc_buf *buf)
{
- unsigned long flags;
-
- spin_lock_irqsave(&buf_list->spinlock, flags);
+ guard(spinlock_irqsave)(&buf_list->spinlock);
list_add_tail(&buf->list_item, &buf_list->list);
buf_list->count++;
-
- spin_unlock_irqrestore(&buf_list->spinlock, flags);
} /* end of n_hdlc_buf_put() */
/**
@@ -758,10 +745,9 @@ static void n_hdlc_buf_put(struct n_hdlc_buf_list *buf_list,
*/
static struct n_hdlc_buf *n_hdlc_buf_get(struct n_hdlc_buf_list *buf_list)
{
- unsigned long flags;
struct n_hdlc_buf *buf;
- spin_lock_irqsave(&buf_list->spinlock, flags);
+ guard(spinlock_irqsave)(&buf_list->spinlock);
buf = list_first_entry_or_null(&buf_list->list,
struct n_hdlc_buf, list_item);
@@ -770,7 +756,6 @@ static struct n_hdlc_buf *n_hdlc_buf_get(struct n_hdlc_buf_list *buf_list)
buf_list->count--;
}
- spin_unlock_irqrestore(&buf_list->spinlock, flags);
return buf;
} /* end of n_hdlc_buf_get() */
--
2.51.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 05/10] tty: moxa: use guard()s
2025-11-19 10:01 [PATCH 00/10] tty: another batch of conversions to guards Jiri Slaby (SUSE)
` (3 preceding siblings ...)
2025-11-19 10:01 ` [PATCH 04/10] tty: n_hdlc: use guard()s Jiri Slaby (SUSE)
@ 2025-11-19 10:01 ` Jiri Slaby (SUSE)
2025-11-19 10:01 ` [PATCH 06/10] tty: vt/keyboard: use __free() Jiri Slaby (SUSE)
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jiri Slaby (SUSE) @ 2025-11-19 10:01 UTC (permalink / raw)
To: gregkh; +Cc: linux-serial, linux-kernel, Jiri Slaby (SUSE)
Use guards in the moxa code. This improves readability, makes error
handling easier, and marks locked portions of code explicit. All that
while being sure the lock is unlocked.
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
drivers/tty/moxa.c | 169 ++++++++++++++++++++-------------------------
1 file changed, 73 insertions(+), 96 deletions(-)
diff --git a/drivers/tty/moxa.c b/drivers/tty/moxa.c
index 329b30fac8fc..1bb2376af85c 100644
--- a/drivers/tty/moxa.c
+++ b/drivers/tty/moxa.c
@@ -487,25 +487,20 @@ static void moxa_wait_finish(void __iomem *ofsAddr)
static void moxafunc(void __iomem *ofsAddr, u16 cmd, u16 arg)
{
- unsigned long flags;
- spin_lock_irqsave(&moxafunc_lock, flags);
+ guard(spinlock_irqsave)(&moxafunc_lock);
writew(arg, ofsAddr + FuncArg);
writew(cmd, ofsAddr + FuncCode);
moxa_wait_finish(ofsAddr);
- spin_unlock_irqrestore(&moxafunc_lock, flags);
}
static int moxafuncret(void __iomem *ofsAddr, u16 cmd, u16 arg)
{
- unsigned long flags;
- u16 ret;
- spin_lock_irqsave(&moxafunc_lock, flags);
+ guard(spinlock_irqsave)(&moxafunc_lock);
writew(arg, ofsAddr + FuncArg);
writew(cmd, ofsAddr + FuncCode);
moxa_wait_finish(ofsAddr);
- ret = readw(ofsAddr + FuncArg);
- spin_unlock_irqrestore(&moxafunc_lock, flags);
- return ret;
+
+ return readw(ofsAddr + FuncArg);
}
static void moxa_low_water_check(void __iomem *ofsAddr)
@@ -1002,11 +997,11 @@ static int moxa_init_board(struct moxa_board_conf *brd, struct device *dev)
if (ret)
goto err_free;
- spin_lock_bh(&moxa_lock);
- brd->ready = 1;
- if (!timer_pending(&moxaTimer))
- mod_timer(&moxaTimer, jiffies + HZ / 50);
- spin_unlock_bh(&moxa_lock);
+ scoped_guard(spinlock_bh, &moxa_lock) {
+ brd->ready = 1;
+ if (!timer_pending(&moxaTimer))
+ mod_timer(&moxaTimer, jiffies + HZ / 50);
+ }
first_idx = (brd - moxa_boards) * MAX_PORTS_PER_BOARD;
for (i = 0; i < brd->numPorts; i++)
@@ -1026,29 +1021,29 @@ static void moxa_board_deinit(struct moxa_board_conf *brd)
{
unsigned int a, opened, first_idx;
- mutex_lock(&moxa_openlock);
- spin_lock_bh(&moxa_lock);
- brd->ready = 0;
- spin_unlock_bh(&moxa_lock);
-
- /* pci hot-un-plug support */
- for (a = 0; a < brd->numPorts; a++)
- if (tty_port_initialized(&brd->ports[a].port))
- tty_port_tty_hangup(&brd->ports[a].port, false);
-
- for (a = 0; a < MAX_PORTS_PER_BOARD; a++)
- tty_port_destroy(&brd->ports[a].port);
+ scoped_guard(mutex, &moxa_openlock) {
+ scoped_guard(spinlock_bh, &moxa_lock)
+ brd->ready = 0;
- while (1) {
- opened = 0;
+ /* pci hot-un-plug support */
for (a = 0; a < brd->numPorts; a++)
if (tty_port_initialized(&brd->ports[a].port))
- opened++;
- mutex_unlock(&moxa_openlock);
- if (!opened)
- break;
- msleep(50);
- mutex_lock(&moxa_openlock);
+ tty_port_tty_hangup(&brd->ports[a].port, false);
+
+ for (a = 0; a < MAX_PORTS_PER_BOARD; a++)
+ tty_port_destroy(&brd->ports[a].port);
+
+ while (1) {
+ opened = 0;
+ for (a = 0; a < brd->numPorts; a++)
+ if (tty_port_initialized(&brd->ports[a].port))
+ opened++;
+ if (!opened)
+ break;
+ mutex_unlock(&moxa_openlock);
+ msleep(50);
+ mutex_lock(&moxa_openlock);
+ }
}
first_idx = (brd - moxa_boards) * MAX_PORTS_PER_BOARD;
@@ -1206,12 +1201,9 @@ static void moxa_shutdown(struct tty_port *port)
static bool moxa_carrier_raised(struct tty_port *port)
{
struct moxa_port *ch = container_of(port, struct moxa_port, port);
- int dcd;
- spin_lock_irq(&port->lock);
- dcd = ch->DCDState;
- spin_unlock_irq(&port->lock);
- return dcd;
+ guard(spinlock_irq)(&port->lock);
+ return ch->DCDState;
}
static void moxa_dtr_rts(struct tty_port *port, bool active)
@@ -1225,37 +1217,31 @@ static int moxa_open(struct tty_struct *tty, struct file *filp)
{
struct moxa_board_conf *brd;
struct moxa_port *ch;
- int port;
-
- port = tty->index;
- if (mutex_lock_interruptible(&moxa_openlock))
- return -ERESTARTSYS;
- brd = &moxa_boards[port / MAX_PORTS_PER_BOARD];
- if (!brd->ready) {
- mutex_unlock(&moxa_openlock);
- return -ENODEV;
- }
+ int port = tty->index;
- if (port % MAX_PORTS_PER_BOARD >= brd->numPorts) {
- mutex_unlock(&moxa_openlock);
- return -ENODEV;
- }
-
- ch = &brd->ports[port % MAX_PORTS_PER_BOARD];
- ch->port.count++;
- tty->driver_data = ch;
- tty_port_tty_set(&ch->port, tty);
- mutex_lock(&ch->port.mutex);
- if (!tty_port_initialized(&ch->port)) {
- ch->statusflags = 0;
- moxa_set_tty_param(tty, &tty->termios);
- MoxaPortLineCtrl(ch, true, true);
- MoxaPortEnable(ch);
- MoxaSetFifo(ch, ch->type == PORT_16550A);
- tty_port_set_initialized(&ch->port, true);
+ scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &moxa_openlock) {
+ brd = &moxa_boards[port / MAX_PORTS_PER_BOARD];
+ if (!brd->ready)
+ return -ENODEV;
+
+ if (port % MAX_PORTS_PER_BOARD >= brd->numPorts)
+ return -ENODEV;
+
+ ch = &brd->ports[port % MAX_PORTS_PER_BOARD];
+ ch->port.count++;
+ tty->driver_data = ch;
+ tty_port_tty_set(&ch->port, tty);
+
+ guard(mutex)(&ch->port.mutex);
+ if (!tty_port_initialized(&ch->port)) {
+ ch->statusflags = 0;
+ moxa_set_tty_param(tty, &tty->termios);
+ MoxaPortLineCtrl(ch, true, true);
+ MoxaPortEnable(ch);
+ MoxaSetFifo(ch, ch->type == PORT_16550A);
+ tty_port_set_initialized(&ch->port, true);
+ }
}
- mutex_unlock(&ch->port.mutex);
- mutex_unlock(&moxa_openlock);
return tty_port_block_til_ready(&ch->port, tty, filp);
}
@@ -1270,15 +1256,13 @@ static void moxa_close(struct tty_struct *tty, struct file *filp)
static ssize_t moxa_write(struct tty_struct *tty, const u8 *buf, size_t count)
{
struct moxa_port *ch = tty->driver_data;
- unsigned long flags;
int len;
if (ch == NULL)
return 0;
- spin_lock_irqsave(&moxa_lock, flags);
- len = MoxaPortWriteData(tty, buf, count);
- spin_unlock_irqrestore(&moxa_lock, flags);
+ scoped_guard(spinlock_irqsave, &moxa_lock)
+ len = MoxaPortWriteData(tty, buf, count);
set_bit(LOWWAIT, &ch->statusflags);
return len;
@@ -1349,12 +1333,10 @@ static int moxa_tiocmset(struct tty_struct *tty,
bool dtr_active, rts_active;
struct moxa_port *ch;
- mutex_lock(&moxa_openlock);
+ guard(mutex)(&moxa_openlock);
ch = tty->driver_data;
- if (!ch) {
- mutex_unlock(&moxa_openlock);
+ if (!ch)
return -EINVAL;
- }
MoxaPortGetLineOut(ch, &dtr_active, &rts_active);
if (set & TIOCM_RTS)
@@ -1366,7 +1348,7 @@ static int moxa_tiocmset(struct tty_struct *tty,
if (clear & TIOCM_DTR)
dtr_active = false;
MoxaPortLineCtrl(ch, dtr_active, rts_active);
- mutex_unlock(&moxa_openlock);
+
return 0;
}
@@ -1415,18 +1397,17 @@ static void moxa_hangup(struct tty_struct *tty)
static void moxa_new_dcdstate(struct moxa_port *p, u8 dcd)
{
- unsigned long flags;
dcd = !!dcd;
- spin_lock_irqsave(&p->port.lock, flags);
- if (dcd != p->DCDState) {
- p->DCDState = dcd;
- spin_unlock_irqrestore(&p->port.lock, flags);
- if (!dcd)
- tty_port_tty_hangup(&p->port, true);
+ scoped_guard(spinlock_irqsave, &p->port.lock) {
+ if (dcd == p->DCDState)
+ return;
+
+ p->DCDState = dcd;
}
- else
- spin_unlock_irqrestore(&p->port.lock, flags);
+
+ if (!dcd)
+ tty_port_tty_hangup(&p->port, true);
}
static int moxa_poll_port(struct moxa_port *p, unsigned int handle,
@@ -1494,7 +1475,7 @@ static void moxa_poll(struct timer_list *unused)
u16 __iomem *ip;
unsigned int card, port, served = 0;
- spin_lock(&moxa_lock);
+ guard(spinlock)(&moxa_lock);
for (card = 0; card < MAX_BOARDS; card++) {
brd = &moxa_boards[card];
if (!brd->ready)
@@ -1525,7 +1506,6 @@ static void moxa_poll(struct timer_list *unused)
if (served)
mod_timer(&moxaTimer, jiffies + HZ / 50);
- spin_unlock(&moxa_lock);
}
/******************************************************************************/
@@ -1861,13 +1841,11 @@ static int MoxaPortSetTermio(struct moxa_port *port, struct ktermios *termio,
baud = MoxaPortSetBaud(port, baud);
if (termio->c_iflag & (IXON | IXOFF | IXANY)) {
- spin_lock_irq(&moxafunc_lock);
+ guard(spinlock_irq)(&moxafunc_lock);
writeb(termio->c_cc[VSTART], ofsAddr + FuncArg);
writeb(termio->c_cc[VSTOP], ofsAddr + FuncArg1);
writeb(FC_SetXonXoff, ofsAddr + FuncCode);
moxa_wait_finish(ofsAddr);
- spin_unlock_irq(&moxafunc_lock);
-
}
return baud;
}
@@ -2098,13 +2076,13 @@ static int moxa_get_serial_info(struct tty_struct *tty,
if (!info)
return -ENODEV;
- mutex_lock(&info->port.mutex);
+ guard(mutex)(&info->port.mutex);
ss->type = info->type;
ss->line = info->port.tty->index;
ss->flags = info->port.flags;
ss->baud_base = 921600;
ss->close_delay = jiffies_to_msecs(info->port.close_delay) / 10;
- mutex_unlock(&info->port.mutex);
+
return 0;
}
@@ -2120,13 +2098,12 @@ static int moxa_set_serial_info(struct tty_struct *tty,
close_delay = msecs_to_jiffies(ss->close_delay * 10);
- mutex_lock(&info->port.mutex);
+ guard(mutex)(&info->port.mutex);
if (!capable(CAP_SYS_ADMIN)) {
if (close_delay != info->port.close_delay ||
ss->type != info->type ||
((ss->flags & ~ASYNC_USR_MASK) !=
(info->port.flags & ~ASYNC_USR_MASK))) {
- mutex_unlock(&info->port.mutex);
return -EPERM;
}
} else {
@@ -2136,7 +2113,7 @@ static int moxa_set_serial_info(struct tty_struct *tty,
info->type = ss->type;
}
- mutex_unlock(&info->port.mutex);
+
return 0;
}
--
2.51.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 06/10] tty: vt/keyboard: use __free()
2025-11-19 10:01 [PATCH 00/10] tty: another batch of conversions to guards Jiri Slaby (SUSE)
` (4 preceding siblings ...)
2025-11-19 10:01 ` [PATCH 05/10] tty: moxa: " Jiri Slaby (SUSE)
@ 2025-11-19 10:01 ` Jiri Slaby (SUSE)
2025-11-19 10:01 ` [PATCH 07/10] tty: vt/keyboard: simplify returns from vt_do_kbkeycode_ioctl() Jiri Slaby (SUSE)
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jiri Slaby (SUSE) @ 2025-11-19 10:01 UTC (permalink / raw)
To: gregkh; +Cc: linux-serial, linux-kernel, Jiri Slaby (SUSE)
The vt/keyboard code can use __free to ensure the temporary buffers are
freed. Perform the switch.
And even one non-temporary in kbd_connect(). There are fail paths, so
ensure the buffer is freed in them and not when returning 0 -- by
retain_and_null_ptr().
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
drivers/tty/vt/keyboard.c | 90 ++++++++++++++++-----------------------
1 file changed, 37 insertions(+), 53 deletions(-)
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index ee1d9c448c7e..65913a137862 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -1566,10 +1566,9 @@ static bool kbd_match(struct input_handler *handler, struct input_dev *dev)
static int kbd_connect(struct input_handler *handler, struct input_dev *dev,
const struct input_device_id *id)
{
- struct input_handle *handle;
int error;
- handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
+ struct input_handle __free(kfree) *handle = kzalloc(sizeof(*handle), GFP_KERNEL);
if (!handle)
return -ENOMEM;
@@ -1579,18 +1578,18 @@ static int kbd_connect(struct input_handler *handler, struct input_dev *dev,
error = input_register_handle(handle);
if (error)
- goto err_free_handle;
+ return error;
error = input_open_device(handle);
if (error)
goto err_unregister_handle;
+ retain_and_null_ptr(handle);
+
return 0;
err_unregister_handle:
input_unregister_handle(handle);
- err_free_handle:
- kfree(handle);
return error;
}
@@ -1683,17 +1682,15 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
{
unsigned long flags;
int asize;
- int ret = 0;
switch (cmd) {
case KDGKBDIACR:
{
struct kbdiacrs __user *a = udp;
- struct kbdiacr *dia;
int i;
- dia = kmalloc_array(MAX_DIACR, sizeof(struct kbdiacr),
- GFP_KERNEL);
+ struct kbdiacr __free(kfree) *dia = kmalloc_array(MAX_DIACR, sizeof(struct kbdiacr),
+ GFP_KERNEL);
if (!dia)
return -ENOMEM;
@@ -1713,20 +1710,17 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
spin_unlock_irqrestore(&kbd_event_lock, flags);
if (put_user(asize, &a->kb_cnt))
- ret = -EFAULT;
- else if (copy_to_user(a->kbdiacr, dia,
- asize * sizeof(struct kbdiacr)))
- ret = -EFAULT;
- kfree(dia);
- return ret;
+ return -EFAULT;
+ if (copy_to_user(a->kbdiacr, dia, asize * sizeof(struct kbdiacr)))
+ return -EFAULT;
+ return 0;
}
case KDGKBDIACRUC:
{
struct kbdiacrsuc __user *a = udp;
- void *buf;
- buf = kmalloc_array(MAX_DIACR, sizeof(struct kbdiacruc),
- GFP_KERNEL);
+ void __free(kfree) *buf = kmalloc_array(MAX_DIACR, sizeof(struct kbdiacruc),
+ GFP_KERNEL);
if (buf == NULL)
return -ENOMEM;
@@ -1740,18 +1734,17 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
spin_unlock_irqrestore(&kbd_event_lock, flags);
if (put_user(asize, &a->kb_cnt))
- ret = -EFAULT;
- else if (copy_to_user(a->kbdiacruc, buf,
- asize*sizeof(struct kbdiacruc)))
- ret = -EFAULT;
- kfree(buf);
- return ret;
+ return -EFAULT;
+ if (copy_to_user(a->kbdiacruc, buf, asize * sizeof(struct kbdiacruc)))
+ return -EFAULT;
+
+ return 0;
}
case KDSKBDIACR:
{
struct kbdiacrs __user *a = udp;
- struct kbdiacr *dia = NULL;
+ struct kbdiacr __free(kfree) *dia = NULL;
unsigned int ct;
int i;
@@ -1780,7 +1773,7 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
conv_8bit_to_uni(dia[i].result);
}
spin_unlock_irqrestore(&kbd_event_lock, flags);
- kfree(dia);
+
return 0;
}
@@ -1788,7 +1781,7 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
{
struct kbdiacrsuc __user *a = udp;
unsigned int ct;
- void *buf = NULL;
+ void __free(kfree) *buf = NULL;
if (!perm)
return -EPERM;
@@ -1811,11 +1804,10 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
ct * sizeof(struct kbdiacruc));
accent_table_size = ct;
spin_unlock_irqrestore(&kbd_event_lock, flags);
- kfree(buf);
return 0;
}
}
- return ret;
+ return 0;
}
/**
@@ -1934,7 +1926,7 @@ static int vt_kdskbent(unsigned char kbdmode, unsigned char idx,
unsigned char map, unsigned short val)
{
unsigned long flags;
- unsigned short *key_map, *new_map, oldval;
+ unsigned short *key_map, oldval;
if (!idx && val == K_NOSUCHMAP) {
spin_lock_irqsave(&kbd_event_lock, flags);
@@ -1965,7 +1957,7 @@ static int vt_kdskbent(unsigned char kbdmode, unsigned char idx,
return 0;
#endif
- new_map = kmalloc(sizeof(plain_map), GFP_KERNEL);
+ unsigned short __free(kfree) *new_map = kmalloc(sizeof(plain_map), GFP_KERNEL);
if (!new_map)
return -ENOMEM;
@@ -1977,17 +1969,14 @@ static int vt_kdskbent(unsigned char kbdmode, unsigned char idx,
if (keymap_count >= MAX_NR_OF_USER_KEYMAPS &&
!capable(CAP_SYS_RESOURCE)) {
spin_unlock_irqrestore(&kbd_event_lock, flags);
- kfree(new_map);
return -EPERM;
}
- key_maps[map] = new_map;
- key_map = new_map;
+ key_map = key_maps[map] = no_free_ptr(new_map);
key_map[0] = U(K_ALLOCATED);
for (j = 1; j < NR_KEYS; j++)
key_map[j] = U(K_HOLE);
keymap_count++;
- } else
- kfree(new_map);
+ }
oldval = U(key_map[idx]);
if (val == oldval)
@@ -2050,8 +2039,6 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
{
unsigned char kb_func;
unsigned long flags;
- char *kbs;
- int ret;
if (get_user(kb_func, &user_kdgkb->kb_func))
return -EFAULT;
@@ -2063,7 +2050,7 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
/* size should have been a struct member */
ssize_t len = sizeof(user_kdgkb->kb_string);
- kbs = kmalloc(len, GFP_KERNEL);
+ char __free(kfree) *kbs = kmalloc(len, GFP_KERNEL);
if (!kbs)
return -ENOMEM;
@@ -2071,20 +2058,20 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
len = strscpy(kbs, func_table[kb_func] ? : "", len);
spin_unlock_irqrestore(&func_buf_lock, flags);
- if (len < 0) {
- ret = -ENOSPC;
- break;
- }
- ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
- -EFAULT : 0;
- break;
+ if (len < 0)
+ return -ENOSPC;
+
+ if (copy_to_user(user_kdgkb->kb_string, kbs, len + 1))
+ return -EFAULT;
+
+ return 0;
}
case KDSKBSENT:
if (!perm || !capable(CAP_SYS_TTY_CONFIG))
return -EPERM;
- kbs = strndup_user(user_kdgkb->kb_string,
- sizeof(user_kdgkb->kb_string));
+ char __free(kfree) *kbs = strndup_user(user_kdgkb->kb_string,
+ sizeof(user_kdgkb->kb_string));
if (IS_ERR(kbs))
return PTR_ERR(kbs);
@@ -2092,13 +2079,10 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
kbs = vt_kdskbsent(kbs, kb_func);
spin_unlock_irqrestore(&func_buf_lock, flags);
- ret = 0;
- break;
+ return 0;
}
- kfree(kbs);
-
- return ret;
+ return 0;
}
int vt_do_kdskled(unsigned int console, int cmd, unsigned long arg, int perm)
--
2.51.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 07/10] tty: vt/keyboard: simplify returns from vt_do_kbkeycode_ioctl()
2025-11-19 10:01 [PATCH 00/10] tty: another batch of conversions to guards Jiri Slaby (SUSE)
` (5 preceding siblings ...)
2025-11-19 10:01 ` [PATCH 06/10] tty: vt/keyboard: use __free() Jiri Slaby (SUSE)
@ 2025-11-19 10:01 ` Jiri Slaby (SUSE)
2025-11-19 10:01 ` [PATCH 08/10] tty: vt/keyboard: use guard()s Jiri Slaby (SUSE)
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jiri Slaby (SUSE) @ 2025-11-19 10:01 UTC (permalink / raw)
To: gregkh; +Cc: linux-serial, linux-kernel, Jiri Slaby (SUSE)
Return immediately when something goes wrong in vt_do_kbkeycode_ioctl().
This makes the code flow more obvious.
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
drivers/tty/vt/keyboard.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 65913a137862..487518a696e6 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -1879,27 +1879,27 @@ int vt_do_kdskbmeta(unsigned int console, unsigned int arg)
return ret;
}
-int vt_do_kbkeycode_ioctl(int cmd, struct kbkeycode __user *user_kbkc,
- int perm)
+int vt_do_kbkeycode_ioctl(int cmd, struct kbkeycode __user *user_kbkc, int perm)
{
struct kbkeycode tmp;
- int kc = 0;
+ int kc;
if (copy_from_user(&tmp, user_kbkc, sizeof(struct kbkeycode)))
return -EFAULT;
+
switch (cmd) {
case KDGETKEYCODE:
kc = getkeycode(tmp.scancode);
- if (kc >= 0)
- kc = put_user(kc, &user_kbkc->keycode);
- break;
+ if (kc < 0)
+ return kc;
+ return put_user(kc, &user_kbkc->keycode);
case KDSETKEYCODE:
if (!perm)
return -EPERM;
- kc = setkeycode(tmp.scancode, tmp.keycode);
- break;
+ return setkeycode(tmp.scancode, tmp.keycode);
}
- return kc;
+
+ return 0;
}
static unsigned short vt_kdgkbent(unsigned char kbdmode, unsigned char idx,
--
2.51.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 08/10] tty: vt/keyboard: use guard()s
2025-11-19 10:01 [PATCH 00/10] tty: another batch of conversions to guards Jiri Slaby (SUSE)
` (6 preceding siblings ...)
2025-11-19 10:01 ` [PATCH 07/10] tty: vt/keyboard: simplify returns from vt_do_kbkeycode_ioctl() Jiri Slaby (SUSE)
@ 2025-11-19 10:01 ` Jiri Slaby (SUSE)
2025-11-19 10:01 ` [PATCH 09/10] serial: serial_core: simplify uart_ioctl() returns Jiri Slaby (SUSE)
2025-11-19 10:01 ` [PATCH 10/10] serial: serial_core: use guard()s Jiri Slaby (SUSE)
9 siblings, 0 replies; 11+ messages in thread
From: Jiri Slaby (SUSE) @ 2025-11-19 10:01 UTC (permalink / raw)
To: gregkh; +Cc: linux-serial, linux-kernel, Jiri Slaby (SUSE)
Use guards in the vt/keyboard code. This improves readability, makes
error handling easier, and marks locked portions of code explicit. All
that while being sure the lock is unlocked.
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
drivers/tty/vt/keyboard.c | 210 ++++++++++++++------------------------
1 file changed, 74 insertions(+), 136 deletions(-)
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 487518a696e6..d65fc60dd7be 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -424,8 +424,6 @@ static void do_compute_shiftstate(void)
/* We still have to export this method to vt.c */
void vt_set_leds_compute_shiftstate(void)
{
- unsigned long flags;
-
/*
* When VT is switched, the keyboard led needs to be set once.
* Ensure that after the switch is completed, the state of the
@@ -434,9 +432,8 @@ void vt_set_leds_compute_shiftstate(void)
vt_switch = true;
set_leds();
- spin_lock_irqsave(&kbd_event_lock, flags);
+ guard(spinlock_irqsave)(&kbd_event_lock);
do_compute_shiftstate();
- spin_unlock_irqrestore(&kbd_event_lock, flags);
}
/*
@@ -625,13 +622,12 @@ static void fn_compose(struct vc_data *vc)
static void fn_spawn_con(struct vc_data *vc)
{
- spin_lock(&vt_spawn_con.lock);
+ guard(spinlock)(&vt_spawn_con.lock);
if (vt_spawn_con.pid)
if (kill_pid(vt_spawn_con.pid, vt_spawn_con.sig, 1)) {
put_pid(vt_spawn_con.pid);
vt_spawn_con.pid = NULL;
}
- spin_unlock(&vt_spawn_con.lock);
}
static void fn_SAK(struct vc_data *vc)
@@ -762,13 +758,9 @@ static void k_fn(struct vc_data *vc, unsigned char value, char up_flag)
return;
if ((unsigned)value < ARRAY_SIZE(func_table)) {
- unsigned long flags;
-
- spin_lock_irqsave(&func_buf_lock, flags);
+ guard(spinlock_irqsave)(&func_buf_lock);
if (func_table[value])
puts_queue(vc, func_table[value]);
- spin_unlock_irqrestore(&func_buf_lock, flags);
-
} else
pr_err("k_fn called with value=%d\n", value);
}
@@ -1140,8 +1132,7 @@ static unsigned char getledstate(void)
void setledstate(struct kbd_struct *kb, unsigned int led)
{
- unsigned long flags;
- spin_lock_irqsave(&led_lock, flags);
+ guard(spinlock_irqsave)(&led_lock);
if (!(led & ~7)) {
ledioctl = led;
kb->ledmode = LED_SHOW_IOCTL;
@@ -1149,7 +1140,6 @@ void setledstate(struct kbd_struct *kb, unsigned int led)
kb->ledmode = LED_SHOW_FLAGS;
set_leds();
- spin_unlock_irqrestore(&led_lock, flags);
}
static inline unsigned char getleds(void)
@@ -1172,14 +1162,9 @@ static inline unsigned char getleds(void)
int vt_get_leds(unsigned int console, int flag)
{
struct kbd_struct *kb = &kbd_table[console];
- int ret;
- unsigned long flags;
- spin_lock_irqsave(&led_lock, flags);
- ret = vc_kbd_led(kb, flag);
- spin_unlock_irqrestore(&led_lock, flags);
-
- return ret;
+ guard(spinlock_irqsave)(&led_lock);
+ return vc_kbd_led(kb, flag);
}
EXPORT_SYMBOL_GPL(vt_get_leds);
@@ -1213,11 +1198,10 @@ void vt_set_led_state(unsigned int console, int leds)
void vt_kbd_con_start(unsigned int console)
{
struct kbd_struct *kb = &kbd_table[console];
- unsigned long flags;
- spin_lock_irqsave(&led_lock, flags);
+
+ guard(spinlock_irqsave)(&led_lock);
clr_vc_kbd_led(kb, VC_SCROLLOCK);
set_leds();
- spin_unlock_irqrestore(&led_lock, flags);
}
/**
@@ -1230,11 +1214,10 @@ void vt_kbd_con_start(unsigned int console)
void vt_kbd_con_stop(unsigned int console)
{
struct kbd_struct *kb = &kbd_table[console];
- unsigned long flags;
- spin_lock_irqsave(&led_lock, flags);
+
+ guard(spinlock_irqsave)(&led_lock);
set_vc_kbd_led(kb, VC_SCROLLOCK);
set_leds();
- spin_unlock_irqrestore(&led_lock, flags);
}
/*
@@ -1246,12 +1229,11 @@ void vt_kbd_con_stop(unsigned int console)
static void kbd_bh(struct tasklet_struct *unused)
{
unsigned int leds;
- unsigned long flags;
- spin_lock_irqsave(&led_lock, flags);
- leds = getleds();
- leds |= (unsigned int)kbd->lockstate << 8;
- spin_unlock_irqrestore(&led_lock, flags);
+ scoped_guard(spinlock_irqsave, &led_lock) {
+ leds = getleds();
+ leds |= (unsigned int)kbd->lockstate << 8;
+ }
if (vt_switch) {
ledstate = ~leds;
@@ -1525,15 +1507,13 @@ static void kbd_event(struct input_handle *handle, unsigned int event_type,
unsigned int event_code, int value)
{
/* We are called with interrupts disabled, just take the lock */
- spin_lock(&kbd_event_lock);
-
- if (event_type == EV_MSC && event_code == MSC_RAW &&
- kbd_is_hw_raw(handle->dev))
- kbd_rawcode(value);
- if (event_type == EV_KEY && event_code <= KEY_MAX)
- kbd_keycode(event_code, value, kbd_is_hw_raw(handle->dev));
-
- spin_unlock(&kbd_event_lock);
+ scoped_guard(spinlock, &kbd_event_lock) {
+ if (event_type == EV_MSC && event_code == MSC_RAW &&
+ kbd_is_hw_raw(handle->dev))
+ kbd_rawcode(value);
+ if (event_type == EV_KEY && event_code <= KEY_MAX)
+ kbd_keycode(event_code, value, kbd_is_hw_raw(handle->dev));
+ }
tasklet_schedule(&keyboard_tasklet);
do_poke_blanked_console = 1;
@@ -1680,7 +1660,6 @@ int __init kbd_init(void)
*/
int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
{
- unsigned long flags;
int asize;
switch (cmd) {
@@ -1696,18 +1675,14 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
/* Lock the diacriticals table, make a copy and then
copy it after we unlock */
- spin_lock_irqsave(&kbd_event_lock, flags);
-
- asize = accent_table_size;
- for (i = 0; i < asize; i++) {
- dia[i].diacr = conv_uni_to_8bit(
- accent_table[i].diacr);
- dia[i].base = conv_uni_to_8bit(
- accent_table[i].base);
- dia[i].result = conv_uni_to_8bit(
- accent_table[i].result);
+ scoped_guard(spinlock_irqsave, &kbd_event_lock) {
+ asize = accent_table_size;
+ for (i = 0; i < asize; i++) {
+ dia[i].diacr = conv_uni_to_8bit(accent_table[i].diacr);
+ dia[i].base = conv_uni_to_8bit(accent_table[i].base);
+ dia[i].result = conv_uni_to_8bit(accent_table[i].result);
+ }
}
- spin_unlock_irqrestore(&kbd_event_lock, flags);
if (put_user(asize, &a->kb_cnt))
return -EFAULT;
@@ -1726,12 +1701,10 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
/* Lock the diacriticals table, make a copy and then
copy it after we unlock */
- spin_lock_irqsave(&kbd_event_lock, flags);
-
- asize = accent_table_size;
- memcpy(buf, accent_table, asize * sizeof(struct kbdiacruc));
-
- spin_unlock_irqrestore(&kbd_event_lock, flags);
+ scoped_guard(spinlock_irqsave, &kbd_event_lock) {
+ asize = accent_table_size;
+ memcpy(buf, accent_table, asize * sizeof(struct kbdiacruc));
+ }
if (put_user(asize, &a->kb_cnt))
return -EFAULT;
@@ -1762,7 +1735,7 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
return PTR_ERR(dia);
}
- spin_lock_irqsave(&kbd_event_lock, flags);
+ guard(spinlock_irqsave)(&kbd_event_lock);
accent_table_size = ct;
for (i = 0; i < ct; i++) {
accent_table[i].diacr =
@@ -1772,7 +1745,6 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
accent_table[i].result =
conv_8bit_to_uni(dia[i].result);
}
- spin_unlock_irqrestore(&kbd_event_lock, flags);
return 0;
}
@@ -1797,13 +1769,12 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
ct, sizeof(struct kbdiacruc));
if (IS_ERR(buf))
return PTR_ERR(buf);
- }
- spin_lock_irqsave(&kbd_event_lock, flags);
+ }
+ guard(spinlock_irqsave)(&kbd_event_lock);
if (ct)
memcpy(accent_table, buf,
ct * sizeof(struct kbdiacruc));
accent_table_size = ct;
- spin_unlock_irqrestore(&kbd_event_lock, flags);
return 0;
}
}
@@ -1821,33 +1792,29 @@ int vt_do_diacrit(unsigned int cmd, void __user *udp, int perm)
int vt_do_kdskbmode(unsigned int console, unsigned int arg)
{
struct kbd_struct *kb = &kbd_table[console];
- int ret = 0;
- unsigned long flags;
- spin_lock_irqsave(&kbd_event_lock, flags);
+ guard(spinlock_irqsave)(&kbd_event_lock);
switch(arg) {
case K_RAW:
kb->kbdmode = VC_RAW;
- break;
+ return 0;
case K_MEDIUMRAW:
kb->kbdmode = VC_MEDIUMRAW;
- break;
+ return 0;
case K_XLATE:
kb->kbdmode = VC_XLATE;
do_compute_shiftstate();
- break;
+ return 0;
case K_UNICODE:
kb->kbdmode = VC_UNICODE;
do_compute_shiftstate();
- break;
+ return 0;
case K_OFF:
kb->kbdmode = VC_OFF;
- break;
+ return 0;
default:
- ret = -EINVAL;
+ return -EINVAL;
}
- spin_unlock_irqrestore(&kbd_event_lock, flags);
- return ret;
}
/**
@@ -1861,22 +1828,18 @@ int vt_do_kdskbmode(unsigned int console, unsigned int arg)
int vt_do_kdskbmeta(unsigned int console, unsigned int arg)
{
struct kbd_struct *kb = &kbd_table[console];
- int ret = 0;
- unsigned long flags;
- spin_lock_irqsave(&kbd_event_lock, flags);
+ guard(spinlock_irqsave)(&kbd_event_lock);
switch(arg) {
case K_METABIT:
clr_vc_kbd_mode(kb, VC_META);
- break;
+ return 0;
case K_ESCPREFIX:
set_vc_kbd_mode(kb, VC_META);
- break;
+ return 0;
default:
- ret = -EINVAL;
+ return -EINVAL;
}
- spin_unlock_irqrestore(&kbd_event_lock, flags);
- return ret;
}
int vt_do_kbkeycode_ioctl(int cmd, struct kbkeycode __user *user_kbkc, int perm)
@@ -1905,31 +1868,28 @@ int vt_do_kbkeycode_ioctl(int cmd, struct kbkeycode __user *user_kbkc, int perm)
static unsigned short vt_kdgkbent(unsigned char kbdmode, unsigned char idx,
unsigned char map)
{
- unsigned short *key_map, val;
- unsigned long flags;
+ unsigned short *key_map;
/* Ensure another thread doesn't free it under us */
- spin_lock_irqsave(&kbd_event_lock, flags);
+ guard(spinlock_irqsave)(&kbd_event_lock);
key_map = key_maps[map];
if (key_map) {
- val = U(key_map[idx]);
+ unsigned short val = U(key_map[idx]);
if (kbdmode != VC_UNICODE && KTYP(val) >= NR_TYPES)
- val = K_HOLE;
- } else
- val = idx ? K_HOLE : K_NOSUCHMAP;
- spin_unlock_irqrestore(&kbd_event_lock, flags);
+ return K_HOLE;
+ return val;
+ }
- return val;
+ return idx ? K_HOLE : K_NOSUCHMAP;
}
static int vt_kdskbent(unsigned char kbdmode, unsigned char idx,
unsigned char map, unsigned short val)
{
- unsigned long flags;
unsigned short *key_map, oldval;
if (!idx && val == K_NOSUCHMAP) {
- spin_lock_irqsave(&kbd_event_lock, flags);
+ guard(spinlock_irqsave)(&kbd_event_lock);
/* deallocate map */
key_map = key_maps[map];
if (map && key_map) {
@@ -1939,7 +1899,6 @@ static int vt_kdskbent(unsigned char kbdmode, unsigned char idx,
keymap_count--;
}
}
- spin_unlock_irqrestore(&kbd_event_lock, flags);
return 0;
}
@@ -1961,16 +1920,14 @@ static int vt_kdskbent(unsigned char kbdmode, unsigned char idx,
if (!new_map)
return -ENOMEM;
- spin_lock_irqsave(&kbd_event_lock, flags);
+ guard(spinlock_irqsave)(&kbd_event_lock);
key_map = key_maps[map];
if (key_map == NULL) {
int j;
- if (keymap_count >= MAX_NR_OF_USER_KEYMAPS &&
- !capable(CAP_SYS_RESOURCE)) {
- spin_unlock_irqrestore(&kbd_event_lock, flags);
+ if (keymap_count >= MAX_NR_OF_USER_KEYMAPS && !capable(CAP_SYS_RESOURCE))
return -EPERM;
- }
+
key_map = key_maps[map] = no_free_ptr(new_map);
key_map[0] = U(K_ALLOCATED);
for (j = 1; j < NR_KEYS; j++)
@@ -1980,19 +1937,15 @@ static int vt_kdskbent(unsigned char kbdmode, unsigned char idx,
oldval = U(key_map[idx]);
if (val == oldval)
- goto out;
+ return 0;
/* Attention Key */
- if ((oldval == K_SAK || val == K_SAK) && !capable(CAP_SYS_ADMIN)) {
- spin_unlock_irqrestore(&kbd_event_lock, flags);
+ if ((oldval == K_SAK || val == K_SAK) && !capable(CAP_SYS_ADMIN))
return -EPERM;
- }
key_map[idx] = U(val);
if (!map && (KTYP(oldval) == KT_SHIFT || KTYP(val) == KT_SHIFT))
do_compute_shiftstate();
-out:
- spin_unlock_irqrestore(&kbd_event_lock, flags);
return 0;
}
@@ -2038,7 +1991,6 @@ static char *vt_kdskbsent(char *kbs, unsigned char cur)
int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
{
unsigned char kb_func;
- unsigned long flags;
if (get_user(kb_func, &user_kdgkb->kb_func))
return -EFAULT;
@@ -2054,9 +2006,8 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
if (!kbs)
return -ENOMEM;
- spin_lock_irqsave(&func_buf_lock, flags);
- len = strscpy(kbs, func_table[kb_func] ? : "", len);
- spin_unlock_irqrestore(&func_buf_lock, flags);
+ scoped_guard(spinlock_irqsave, &func_buf_lock)
+ len = strscpy(kbs, func_table[kb_func] ? : "", len);
if (len < 0)
return -ENOSPC;
@@ -2075,9 +2026,8 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
if (IS_ERR(kbs))
return PTR_ERR(kbs);
- spin_lock_irqsave(&func_buf_lock, flags);
+ guard(spinlock_irqsave)(&func_buf_lock);
kbs = vt_kdskbsent(kbs, kb_func);
- spin_unlock_irqrestore(&func_buf_lock, flags);
return 0;
}
@@ -2088,16 +2038,14 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
int vt_do_kdskled(unsigned int console, int cmd, unsigned long arg, int perm)
{
struct kbd_struct *kb = &kbd_table[console];
- unsigned long flags;
unsigned char ucval;
switch(cmd) {
/* the ioctls below read/set the flags usually shown in the leds */
/* don't use them - they will go away without warning */
case KDGKBLED:
- spin_lock_irqsave(&kbd_event_lock, flags);
- ucval = kb->ledflagstate | (kb->default_ledflagstate << 4);
- spin_unlock_irqrestore(&kbd_event_lock, flags);
+ scoped_guard(spinlock_irqsave, &kbd_event_lock)
+ ucval = kb->ledflagstate | (kb->default_ledflagstate << 4);
return put_user(ucval, (char __user *)arg);
case KDSKBLED:
@@ -2105,11 +2053,11 @@ int vt_do_kdskled(unsigned int console, int cmd, unsigned long arg, int perm)
return -EPERM;
if (arg & ~0x77)
return -EINVAL;
- spin_lock_irqsave(&led_lock, flags);
- kb->ledflagstate = (arg & 7);
- kb->default_ledflagstate = ((arg >> 4) & 7);
- set_leds();
- spin_unlock_irqrestore(&led_lock, flags);
+ scoped_guard(spinlock_irqsave, &led_lock) {
+ kb->ledflagstate = (arg & 7);
+ kb->default_ledflagstate = ((arg >> 4) & 7);
+ set_leds();
+ }
return 0;
/* the ioctls below only set the lights, not the functions */
@@ -2166,11 +2114,8 @@ int vt_do_kdgkbmeta(unsigned int console)
*/
void vt_reset_unicode(unsigned int console)
{
- unsigned long flags;
-
- spin_lock_irqsave(&kbd_event_lock, flags);
+ guard(spinlock_irqsave)(&kbd_event_lock);
kbd_table[console].kbdmode = default_utf8 ? VC_UNICODE : VC_XLATE;
- spin_unlock_irqrestore(&kbd_event_lock, flags);
}
/**
@@ -2195,22 +2140,19 @@ int vt_get_shift_state(void)
void vt_reset_keyboard(unsigned int console)
{
struct kbd_struct *kb = &kbd_table[console];
- unsigned long flags;
- spin_lock_irqsave(&kbd_event_lock, flags);
+ guard(spinlock_irqsave)(&kbd_event_lock);
set_vc_kbd_mode(kb, VC_REPEAT);
clr_vc_kbd_mode(kb, VC_CKMODE);
clr_vc_kbd_mode(kb, VC_APPLIC);
clr_vc_kbd_mode(kb, VC_CRLF);
kb->lockstate = 0;
kb->slockstate = 0;
- spin_lock(&led_lock);
+ guard(spinlock)(&led_lock);
kb->ledmode = LED_SHOW_FLAGS;
kb->ledflagstate = kb->default_ledflagstate;
- spin_unlock(&led_lock);
/* do not do set_leds here because this causes an endless tasklet loop
when the keyboard hasn't been initialized yet */
- spin_unlock_irqrestore(&kbd_event_lock, flags);
}
/**
@@ -2240,11 +2182,9 @@ int vt_get_kbd_mode_bit(unsigned int console, int bit)
void vt_set_kbd_mode_bit(unsigned int console, int bit)
{
struct kbd_struct *kb = &kbd_table[console];
- unsigned long flags;
- spin_lock_irqsave(&kbd_event_lock, flags);
+ guard(spinlock_irqsave)(&kbd_event_lock);
set_vc_kbd_mode(kb, bit);
- spin_unlock_irqrestore(&kbd_event_lock, flags);
}
/**
@@ -2259,9 +2199,7 @@ void vt_set_kbd_mode_bit(unsigned int console, int bit)
void vt_clr_kbd_mode_bit(unsigned int console, int bit)
{
struct kbd_struct *kb = &kbd_table[console];
- unsigned long flags;
- spin_lock_irqsave(&kbd_event_lock, flags);
+ guard(spinlock_irqsave)(&kbd_event_lock);
clr_vc_kbd_mode(kb, bit);
- spin_unlock_irqrestore(&kbd_event_lock, flags);
}
--
2.51.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 09/10] serial: serial_core: simplify uart_ioctl() returns
2025-11-19 10:01 [PATCH 00/10] tty: another batch of conversions to guards Jiri Slaby (SUSE)
` (7 preceding siblings ...)
2025-11-19 10:01 ` [PATCH 08/10] tty: vt/keyboard: use guard()s Jiri Slaby (SUSE)
@ 2025-11-19 10:01 ` Jiri Slaby (SUSE)
2025-11-19 10:01 ` [PATCH 10/10] serial: serial_core: use guard()s Jiri Slaby (SUSE)
9 siblings, 0 replies; 11+ messages in thread
From: Jiri Slaby (SUSE) @ 2025-11-19 10:01 UTC (permalink / raw)
To: gregkh; +Cc: linux-serial, linux-kernel, Jiri Slaby (SUSE)
Neither uart_do_autoconfig(), nor uart_wait_modem_status() can return
-ENOIOCTLCMD. The ENOIOCTLCMD checks are there to check if 'cmd' matched
against TIOCSERCONFIG, and TIOCMIWAIT respectively. (With 0 or error in
'ret', it does not matter.)
Therefore, the code can simply return from the TIOCSERCONFIG and
TIOCMIWAIT spots immediately.
To be more explicit, use 'if' instead of switch-case for those single
values.
And return without jumping to the 'out' label -- it can be removed too.
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
drivers/tty/serial/serial_core.c | 35 ++++++++------------------------
1 file changed, 9 insertions(+), 26 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 4757293ece8c..74018fb8a4e7 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1560,37 +1560,20 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
void __user *uarg = (void __user *)arg;
int ret = -ENOIOCTLCMD;
-
- /*
- * These ioctls don't rely on the hardware to be present.
- */
- switch (cmd) {
- case TIOCSERCONFIG:
+ /* This ioctl doesn't rely on the hardware to be present. */
+ if (cmd == TIOCSERCONFIG) {
down_write(&tty->termios_rwsem);
ret = uart_do_autoconfig(tty, state);
up_write(&tty->termios_rwsem);
- break;
- }
-
- if (ret != -ENOIOCTLCMD)
- goto out;
-
- if (tty_io_error(tty)) {
- ret = -EIO;
- goto out;
+ return ret;
}
- /*
- * The following should only be used when hardware is present.
- */
- switch (cmd) {
- case TIOCMIWAIT:
- ret = uart_wait_modem_status(state, arg);
- break;
- }
+ if (tty_io_error(tty))
+ return -EIO;
- if (ret != -ENOIOCTLCMD)
- goto out;
+ /* This should only be used when the hardware is present. */
+ if (cmd == TIOCMIWAIT)
+ return uart_wait_modem_status(state, arg);
/* rs485_config requires more locking than others */
if (cmd == TIOCSRS485)
@@ -1638,7 +1621,7 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
mutex_unlock(&port->mutex);
if (cmd == TIOCSRS485)
up_write(&tty->termios_rwsem);
-out:
+
return ret;
}
--
2.51.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 10/10] serial: serial_core: use guard()s
2025-11-19 10:01 [PATCH 00/10] tty: another batch of conversions to guards Jiri Slaby (SUSE)
` (8 preceding siblings ...)
2025-11-19 10:01 ` [PATCH 09/10] serial: serial_core: simplify uart_ioctl() returns Jiri Slaby (SUSE)
@ 2025-11-19 10:01 ` Jiri Slaby (SUSE)
9 siblings, 0 replies; 11+ messages in thread
From: Jiri Slaby (SUSE) @ 2025-11-19 10:01 UTC (permalink / raw)
To: gregkh; +Cc: linux-serial, linux-kernel, Jiri Slaby (SUSE)
Use guards in the serial_core code. This improves readability, makes
error handling easier, and marks locked portions of code explicit. All
that while being sure the lock is unlocked.
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
drivers/tty/serial/serial_core.c | 127 +++++++++++++------------------
1 file changed, 55 insertions(+), 72 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 74018fb8a4e7..c532235f8d55 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1034,9 +1034,8 @@ static int uart_set_info_user(struct tty_struct *tty, struct serial_struct *ss)
{
struct uart_state *state = tty->driver_data;
struct tty_port *port = &state->port;
- int retval;
- down_write(&tty->termios_rwsem);
+ guard(rwsem_write)(&tty->termios_rwsem);
/*
* This semaphore protects port->count. It is also
* very useful to prevent opens. Also, take the
@@ -1044,11 +1043,8 @@ static int uart_set_info_user(struct tty_struct *tty, struct serial_struct *ss)
* module insertion/removal doesn't change anything
* under us.
*/
- mutex_lock(&port->mutex);
- retval = uart_set_info(tty, port, state, ss);
- mutex_unlock(&port->mutex);
- up_write(&tty->termios_rwsem);
- return retval;
+ guard(mutex)(&port->mutex);
+ return uart_set_info(tty, port, state, ss);
}
/**
@@ -1562,10 +1558,8 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
/* This ioctl doesn't rely on the hardware to be present. */
if (cmd == TIOCSERCONFIG) {
- down_write(&tty->termios_rwsem);
- ret = uart_do_autoconfig(tty, state);
- up_write(&tty->termios_rwsem);
- return ret;
+ guard(rwsem_write)(&tty->termios_rwsem);
+ return uart_do_autoconfig(tty, state);
}
if (tty_io_error(tty))
@@ -1579,46 +1573,46 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
if (cmd == TIOCSRS485)
down_write(&tty->termios_rwsem);
- mutex_lock(&port->mutex);
- uport = uart_port_check(state);
+ scoped_guard(mutex, &port->mutex) {
+ uport = uart_port_check(state);
- if (!uport || tty_io_error(tty)) {
- ret = -EIO;
- goto out_up;
- }
+ if (!uport || tty_io_error(tty)) {
+ ret = -EIO;
+ break;
+ }
- /*
- * All these rely on hardware being present and need to be
- * protected against the tty being hung up.
- */
+ /*
+ * All these rely on hardware being present and need to be
+ * protected against the tty being hung up.
+ */
- switch (cmd) {
- case TIOCSERGETLSR: /* Get line status register */
- ret = uart_get_lsr_info(tty, state, uarg);
- break;
+ switch (cmd) {
+ case TIOCSERGETLSR: /* Get line status register */
+ ret = uart_get_lsr_info(tty, state, uarg);
+ break;
- case TIOCGRS485:
- ret = uart_get_rs485_config(uport, uarg);
- break;
+ case TIOCGRS485:
+ ret = uart_get_rs485_config(uport, uarg);
+ break;
- case TIOCSRS485:
- ret = uart_set_rs485_config(tty, uport, uarg);
- break;
+ case TIOCSRS485:
+ ret = uart_set_rs485_config(tty, uport, uarg);
+ break;
- case TIOCSISO7816:
- ret = uart_set_iso7816_config(state->uart_port, uarg);
- break;
+ case TIOCSISO7816:
+ ret = uart_set_iso7816_config(state->uart_port, uarg);
+ break;
- case TIOCGISO7816:
- ret = uart_get_iso7816_config(state->uart_port, uarg);
- break;
- default:
- if (uport->ops->ioctl)
- ret = uport->ops->ioctl(uport, cmd, arg);
- break;
+ case TIOCGISO7816:
+ ret = uart_get_iso7816_config(state->uart_port, uarg);
+ break;
+ default:
+ if (uport->ops->ioctl)
+ ret = uport->ops->ioctl(uport, cmd, arg);
+ break;
+ }
}
-out_up:
- mutex_unlock(&port->mutex);
+
if (cmd == TIOCSRS485)
up_write(&tty->termios_rwsem);
@@ -1634,11 +1628,10 @@ static void uart_set_ldisc(struct tty_struct *tty)
if (!tty_port_initialized(port))
return;
- mutex_lock(&state->port.mutex);
+ guard(mutex)(&state->port.mutex);
uport = uart_port_check(state);
if (uport && uport->ops->set_ldisc)
uport->ops->set_ldisc(uport, &tty->termios);
- mutex_unlock(&state->port.mutex);
}
static void uart_set_termios(struct tty_struct *tty,
@@ -1712,9 +1705,8 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
state = drv->state + tty->index;
port = &state->port;
- spin_lock_irq(&port->lock);
+ guard(spinlock_irq)(&port->lock);
--port->count;
- spin_unlock_irq(&port->lock);
return;
}
@@ -1826,20 +1818,18 @@ static void uart_hangup(struct tty_struct *tty)
struct uart_state *state = tty->driver_data;
struct tty_port *port = &state->port;
struct uart_port *uport;
- unsigned long flags;
pr_debug("uart_hangup(%d)\n", tty->index);
- mutex_lock(&port->mutex);
+ guard(mutex)(&port->mutex);
uport = uart_port_check(state);
WARN(!uport, "hangup of detached port!\n");
if (tty_port_active(port)) {
uart_flush_buffer(tty);
uart_shutdown(tty, state);
- spin_lock_irqsave(&port->lock, flags);
- port->count = 0;
- spin_unlock_irqrestore(&port->lock, flags);
+ scoped_guard(spinlock_irqsave, &port->lock)
+ port->count = 0;
tty_port_set_active(port, false);
tty_port_tty_set(port, NULL);
if (uport && !uart_console(uport))
@@ -1847,7 +1837,6 @@ static void uart_hangup(struct tty_struct *tty)
wake_up_interruptible(&port->open_wait);
wake_up_interruptible(&port->delta_msr_wait);
}
- mutex_unlock(&port->mutex);
}
/* uport == NULL if uart_port has already been removed */
@@ -2952,11 +2941,11 @@ static ssize_t console_show(struct device *dev,
struct uart_port *uport;
bool console = false;
- mutex_lock(&port->mutex);
- uport = uart_port_check(state);
- if (uport)
- console = uart_console_registered(uport);
- mutex_unlock(&port->mutex);
+ scoped_guard(mutex, &port->mutex) {
+ uport = uart_port_check(state);
+ if (uport)
+ console = uart_console_registered(uport);
+ }
return sprintf(buf, "%c\n", console ? 'Y' : 'N');
}
@@ -3141,17 +3130,14 @@ static void serial_core_remove_one_port(struct uart_driver *drv,
struct tty_port *port = &state->port;
struct uart_port *uart_port;
- mutex_lock(&port->mutex);
- uart_port = uart_port_check(state);
- if (uart_port != uport)
- dev_alert(uport->dev, "Removing wrong port: %p != %p\n",
- uart_port, uport);
+ scoped_guard(mutex, &port->mutex) {
+ uart_port = uart_port_check(state);
+ if (uart_port != uport)
+ dev_alert(uport->dev, "Removing wrong port: %p != %p\n", uart_port, uport);
- if (!uart_port) {
- mutex_unlock(&port->mutex);
- return;
+ if (!uart_port)
+ return;
}
- mutex_unlock(&port->mutex);
/*
* Remove the devices from the tty layer
@@ -3180,11 +3166,10 @@ static void serial_core_remove_one_port(struct uart_driver *drv,
uport->type = PORT_UNKNOWN;
uport->port_dev = NULL;
- mutex_lock(&port->mutex);
+ guard(mutex)(&port->mutex);
WARN_ON(atomic_dec_return(&state->refcount) < 0);
wait_event(state->remove_wait, !atomic_read(&state->refcount));
state->uart_port = NULL;
- mutex_unlock(&port->mutex);
}
/**
@@ -3337,7 +3322,7 @@ void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port
struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
int ctrl_id = port->ctrl_id;
- mutex_lock(&port_mutex);
+ guard(mutex)(&port_mutex);
port->flags |= UPF_DEAD;
@@ -3349,8 +3334,6 @@ void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port
/* Drop the serial core controller device if no ports are using it */
if (!serial_core_ctrl_find(drv, phys_dev, ctrl_id))
serial_base_ctrl_device_remove(ctrl_dev);
-
- mutex_unlock(&port_mutex);
}
/**
--
2.51.1
^ permalink raw reply related [flat|nested] 11+ messages in thread