random tty fixes

PATCH In review Jiri Slaby (SUSE) 3 participants 28 messages Sep 19, 2023

Subsystems (2)

Documentation TTY layer and serial drivers

lore

Share & tools

lore

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:52 UTC #0 lore raw

cover random tty fixes

This is a collection of random fixes for tty I did while crawling through the code. Mostly done for readability and understandability. No behavior change intended (except for Documentation fixes).

Jiri Slaby (SUSE) (15):
tty: n_tty: use 'retval' instead of 'c'
tty: n_tty: rename and retype 'retval' in n_tty_ioctl()
tty: n_tty: use min3() in copy_from_read_buf()
tty: n_tty: invert the condition in copy_from_read_buf()
tty: n_tty: use do-while in n_tty_check_{,un}throttle()
tty: switch tty_{,un}throttle_safe() to return a bool
tty: invert return values of tty_{,un}throttle_safe()
tty: fix up and plug in tty_ioctl kernel-doc
tty: fix kernel-doc for functions in tty.h
tty: stop using ndash in kernel-doc
tty: tty_buffer: use bool for 'restart' in
tty_buffer_unlock_exclusive()
tty: convert THROTTLE constants into enum
tty: early return from send_break() on TTY_DRIVER_HARDWARE_BREAK
tty: don't check for signal_pending() in send_break()
tty: use 'if' in send_break() instead of 'goto'

 Documentation/driver-api/tty/index.rst     |   1 +
 Documentation/driver-api/tty/tty_ioctl.rst |  10 +
 drivers/tty/n_tty.c                        |  77 ++++---
 drivers/tty/tty.h                          |  13 +-
 drivers/tty/tty_buffer.c                   |   5 +-
 drivers/tty/tty_io.c                       |  36 ++--
 drivers/tty/tty_ioctl.c                    | 234 ++++++++++-----------
 drivers/tty/tty_port.c                     |   6 +-
 drivers/tty/vt/consolemap.c                |   2 +-
 drivers/tty/vt/vc_screen.c                 |   4 +-
 drivers/tty/vt/vt.c                        |   4 +-
 include/linux/tty.h                        |  25 +--
 12 files changed, 209 insertions(+), 208 deletions(-)
 create mode 100644 Documentation/driver-api/tty/tty_ioctl.rst

Signature · 1 line

2.42.0

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:52 UTC #1 lore raw

PATCH 1/15 tty: n_tty: use 'retval' instead of 'c'

In n_tty_read(), there is a separate int variable 'c' and is used only to hold an int value returned from job_control(). There is also a 'retval' variable typed ssize_t. So drop this single occurrence of 'c' and reuse 'retval' which is used on all other places to hold the value returned from n_tty_read().

Note that 'retval' needs not be initialized now. Drop that.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/n_tty.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

drivers/tty/n_tty.c +4 -5

index 6c9a408d67cd..71aa898b077a 100644
@@ -2154,9 +2154,8 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, u8 *kbuf,
 	struct n_tty_data *ldata = tty->disc_data;
 	u8 *kb = kbuf;
 	DEFINE_WAIT_FUNC(wait, woken_wake_function);
-	int c;
 	int minimum, time;
-	ssize_t retval = 0;
+	ssize_t retval;
 	long timeout;
 	bool packet;
 	size_t old_tail;

@@ -2192,9 +2191,9 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file, u8 *kbuf,
 		return kb - kbuf;
 	}
 
-	c = job_control(tty, file);
-	if (c < 0)
-		return c;
+	retval = job_control(tty, file);
+	if (retval < 0)
+		return retval;
 
 	/*
 	 *	Internal serialization of reads.

Signature · 1 line

2.42.0

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:52 UTC #2 lore raw

PATCH 2/15 tty: n_tty: rename and retype 'retval' in n_tty_ioctl()

In reply to Jiri Slaby (SUSE)

The value stored to the current 'retval' is number of characters. It is both obtained and put to user as unsigned. So make its type unsigned. And provided it's not a "return value" per se, rename it to 'num'.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/n_tty.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/tty/n_tty.c +4 -4

index 71aa898b077a..e917faa0b84c 100644
@@ -2498,7 +2498,7 @@ static int n_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
 		       unsigned long arg)
 {
 	struct n_tty_data *ldata = tty->disc_data;
-	int retval;
+	unsigned int num;
 
 	switch (cmd) {
 	case TIOCOUTQ:

@@ -2506,11 +2506,11 @@ static int n_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
 	case TIOCINQ:
 		down_write(&tty->termios_rwsem);
 		if (L_ICANON(tty) && !L_EXTPROC(tty))
-			retval = inq_canon(ldata);
+			num = inq_canon(ldata);
 		else
-			retval = read_cnt(ldata);
+			num = read_cnt(ldata);
 		up_write(&tty->termios_rwsem);
-		return put_user(retval, (unsigned int __user *) arg);
+		return put_user(num, (unsigned int __user *) arg);
 	default:
 		return n_tty_ioctl_helper(tty, cmd, arg);
 	}

Signature · 1 line

2.42.0

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:52 UTC #3 lore raw

PATCH 3/15 tty: n_tty: use min3() in copy_from_read_buf()

In reply to Jiri Slaby (SUSE)

n is a minimum of:
* available chars in the ring buffer
* available chars in the ring buffer till the end of the ring buffer
* requested number (*nr)

We can use min3() for that instead of two min()s.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/n_tty.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/tty/n_tty.c +1 -2

index e917faa0b84c..6a112910c058 100644
@@ -1965,8 +1965,7 @@ static bool copy_from_read_buf(const struct tty_struct *tty, u8 **kbp,
 	size_t head = smp_load_acquire(&ldata->commit_head);
 	size_t tail = MASK(ldata->read_tail);
 
-	n = min(head - ldata->read_tail, N_TTY_BUF_SIZE - tail);
-	n = min(*nr, n);
+	n = min3(head - ldata->read_tail, N_TTY_BUF_SIZE - tail, *nr);
 	if (n) {
 		u8 *from = read_buf_addr(ldata, tail);
 		memcpy(*kbp, from, n);

Signature · 1 line

2.42.0

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:52 UTC #4 lore raw

PATCH 4/15 tty: n_tty: invert the condition in copy_from_read_buf()

In reply to Jiri Slaby (SUSE)

Make "no numbers available" a fast quit from the function. And do the heavy work outside the 'if'. This makes the code more understandable and conforming to the common kernel coding style.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/n_tty.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

drivers/tty/n_tty.c +20 ~~-18~~

index 6a112910c058..922fb61b587a 100644
@@ -1966,24 +1966,26 @@ static bool copy_from_read_buf(const struct tty_struct *tty, u8 **kbp,
 	size_t tail = MASK(ldata->read_tail);
 
 	n = min3(head - ldata->read_tail, N_TTY_BUF_SIZE - tail, *nr);
-	if (n) {
-		u8 *from = read_buf_addr(ldata, tail);
-		memcpy(*kbp, from, n);
-		is_eof = n == 1 && *from == EOF_CHAR(tty);
-		tty_audit_add_data(tty, from, n);
-		zero_buffer(tty, from, n);
-		smp_store_release(&ldata->read_tail, ldata->read_tail + n);
-		/* Turn single EOF into zero-length read */
-		if (L_EXTPROC(tty) && ldata->icanon && is_eof &&
-		    (head == ldata->read_tail))
-			return false;
-		*kbp += n;
-		*nr -= n;
-
-		/* If we have more to copy, let the caller know */
-		return head != ldata->read_tail;
-	}
-	return false;
+	if (!n)
+		return false;
+
+	u8 *from = read_buf_addr(ldata, tail);
+	memcpy(*kbp, from, n);
+	is_eof = n == 1 && *from == EOF_CHAR(tty);
+	tty_audit_add_data(tty, from, n);
+	zero_buffer(tty, from, n);
+	smp_store_release(&ldata->read_tail, ldata->read_tail + n);
+
+	/* Turn single EOF into zero-length read */
+	if (L_EXTPROC(tty) && ldata->icanon && is_eof &&
+	    head == ldata->read_tail)
+		return false;
+
+	*kbp += n;
+	*nr -= n;
+
+	/* If we have more to copy, let the caller know */
+	return head != ldata->read_tail;
 }
 
 /**

Signature · 1 line

2.42.0

Ilpo Järvinen

Reviewed-by on v1 4/15

2023-09-19 09:56 UTC #5 lore raw

Re: [PATCH 04/15] tty: n_tty: invert the condition in copy_from_read_buf()

Quoted 1 line · Jiri Slaby (SUSE)

Make "no numbers available" a fast quit from the function. And do the

Did you really intend to write "numbers" and not e.g. characters?

The change itself looks good,

Reviewed-by Ilpo Järvinen <[email protected]>

Signature · 1 line

i.

Quoted 58 lines

heavy work outside the 'if'. This makes the code more understandable and conforming to the common kernel coding style.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/n_tty.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

drivers/tty/n_tty.c +20 ~~-18~~

index 6a112910c058..922fb61b587a 100644
@@ -1966,24 +1966,26 @@ static bool copy_from_read_buf(const struct tty_struct *tty, u8 **kbp,
 	size_t tail = MASK(ldata->read_tail);
 
 	n = min3(head - ldata->read_tail, N_TTY_BUF_SIZE - tail, *nr);
-	if (n) {
-		u8 *from = read_buf_addr(ldata, tail);
-		memcpy(*kbp, from, n);
-		is_eof = n == 1 && *from == EOF_CHAR(tty);
-		tty_audit_add_data(tty, from, n);
-		zero_buffer(tty, from, n);
-		smp_store_release(&ldata->read_tail, ldata->read_tail + n);
-		/* Turn single EOF into zero-length read */
-		if (L_EXTPROC(tty) && ldata->icanon && is_eof &&
-		    (head == ldata->read_tail))
-			return false;
-		*kbp += n;
-		*nr -= n;
-
-		/* If we have more to copy, let the caller know */
-		return head != ldata->read_tail;
-	}
-	return false;
+	if (!n)
+		return false;
+
+	u8 *from = read_buf_addr(ldata, tail);
+	memcpy(*kbp, from, n);
+	is_eof = n == 1 && *from == EOF_CHAR(tty);
+	tty_audit_add_data(tty, from, n);
+	zero_buffer(tty, from, n);
+	smp_store_release(&ldata->read_tail, ldata->read_tail + n);
+
+	/* Turn single EOF into zero-length read */
+	if (L_EXTPROC(tty) && ldata->icanon && is_eof &&
+	    head == ldata->read_tail)
+		return false;
+
+	*kbp += n;
+	*nr -= n;
+
+	/* If we have more to copy, let the caller know */
+	return head != ldata->read_tail;
 }
 
 /**

Jiri Slaby

author maintainer

2023-09-19 10:43 UTC #6 lore raw

Re: [PATCH 04/15] tty: n_tty: invert the condition in copy_from_read_buf()

Quoted 5 lines · Ilpo Järvinen

Quoted 1 line · Jiri Slaby (SUSE)
Make "no numbers available" a fast quit from the function. And do the
Did you really intend to write "numbers" and not e.g. characters?

What was I thinking? "n must be numbers", apparently. But I definitely meant "characters". Funny, that I missed it now, while re-reading and preparing the commit logs for submission.

thanks,

Signature · 2 lines

js
suse labs

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:52 UTC #7 lore raw

PATCH 5/15 tty: n_tty: use do-while in n_tty_check_{,un}throttle()

In reply to Jiri Slaby (SUSE)

This change gets rid of the complicated exit from the loops. It can be done much easier using do-while loops.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/n_tty.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

drivers/tty/n_tty.c +7 ~~-12~~

index 922fb61b587a..b34e6612aef6 100644
@@ -249,15 +249,12 @@ static void n_tty_check_throttle(struct tty_struct *tty)
 	if (ldata->icanon && ldata->canon_head == ldata->read_tail)
 		return;
 
-	while (1) {
-		int throttled;
+	do {
 		tty_set_flow_change(tty, TTY_THROTTLE_SAFE);
 		if (N_TTY_BUF_SIZE - read_cnt(ldata) >= TTY_THRESHOLD_THROTTLE)
 			break;
-		throttled = tty_throttle_safe(tty);
-		if (!throttled)
-			break;
-	}
+	} while (tty_throttle_safe(tty));
+
 	__tty_set_flow_change(tty, 0);
 }

@@ -279,16 +276,14 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
 	 * we won't get any more characters.
 	 */
 
-	while (1) {
-		int unthrottled;
+	do {
 		tty_set_flow_change(tty, TTY_UNTHROTTLE_SAFE);
 		if (chars_in_buffer(tty) > TTY_THRESHOLD_UNTHROTTLE)
 			break;
+
 		n_tty_kick_worker(tty);
-		unthrottled = tty_unthrottle_safe(tty);
-		if (!unthrottled)
-			break;
-	}
+	} while (tty_unthrottle_safe(tty));
+
 	__tty_set_flow_change(tty, 0);
 }

Signature · 1 line

2.42.0

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:52 UTC #8 lore raw

PATCH 6/15 tty: switch tty_{,un}throttle_safe() to return a bool

In reply to Jiri Slaby (SUSE)

They return 0 or 1 -- a boolean value, so make it clear than noone should expect negative or other values.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/tty_ioctl.c | 18 ++++++++----------
 include/linux/tty.h     |  4 ++--
 2 files changed, 10 insertions(+), 12 deletions(-)

drivers/tty/tty_ioctl.c +8 ~~-10~~

index 7958bf6d27c4..ba60fcf518e0 100644
@@ -124,17 +124,16 @@ EXPORT_SYMBOL(tty_unthrottle);
  *	conditions when throttling is conditional on factors evaluated prior to
  *	throttling.
  *
- *	Returns 0 if tty is throttled (or was already throttled)
+ *	Returns false if tty is throttled (or was already throttled)
  */
-
-int tty_throttle_safe(struct tty_struct *tty)
+bool tty_throttle_safe(struct tty_struct *tty)
 {
-	int ret = 0;
+	bool ret = false;
 
 	mutex_lock(&tty->throttle_mutex);
 	if (!tty_throttled(tty)) {
 		if (tty->flow_change != TTY_THROTTLE_SAFE)
-			ret = 1;
+			ret = true;
 		else {
 			set_bit(TTY_THROTTLED, &tty->flags);
 			if (tty->ops->throttle)

@@ -155,17 +154,16 @@ int tty_throttle_safe(struct tty_struct *tty)
  *	unthrottle due to race conditions when unthrottling is conditional
  *	on factors evaluated prior to unthrottling.
  *
- *	Returns 0 if tty is unthrottled (or was already unthrottled)
+ *	Returns false if tty is unthrottled (or was already unthrottled)
  */
-
-int tty_unthrottle_safe(struct tty_struct *tty)
+bool tty_unthrottle_safe(struct tty_struct *tty)
 {
-	int ret = 0;
+	bool ret = false;
 
 	mutex_lock(&tty->throttle_mutex);
 	if (tty_throttled(tty)) {
 		if (tty->flow_change != TTY_UNTHROTTLE_SAFE)
-			ret = 1;
+			ret = true;
 		else {
 			clear_bit(TTY_THROTTLED, &tty->flags);
 			if (tty->ops->unthrottle)

include/linux/tty.h +2 -2

index f002d0f25db7..59d675f345e9 100644
@@ -416,8 +416,8 @@ unsigned int tty_chars_in_buffer(struct tty_struct *tty);
 unsigned int tty_write_room(struct tty_struct *tty);
 void tty_driver_flush_buffer(struct tty_struct *tty);
 void tty_unthrottle(struct tty_struct *tty);
-int tty_throttle_safe(struct tty_struct *tty);
-int tty_unthrottle_safe(struct tty_struct *tty);
+bool tty_throttle_safe(struct tty_struct *tty);
+bool tty_unthrottle_safe(struct tty_struct *tty);
 int tty_do_resize(struct tty_struct *tty, struct winsize *ws);
 int tty_get_icount(struct tty_struct *tty,
 		struct serial_icounter_struct *icount);

Signature · 1 line

2.42.0

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:52 UTC #9 lore raw

PATCH 7/15 tty: invert return values of tty_{,un}throttle_safe()

In reply to Jiri Slaby (SUSE)

If tty_{,un}throttle_safe() returned true on success (similar to *_trylock()), it would make the conditions in callers more obvious. So perform the switch to these inverted values (and fix the callers).

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/n_tty.c     |  4 ++--
 drivers/tty/tty_ioctl.c | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

drivers/tty/n_tty.c +2 -2

index b34e6612aef6..f252d0b5a434 100644
@@ -253,7 +253,7 @@ static void n_tty_check_throttle(struct tty_struct *tty)
 		tty_set_flow_change(tty, TTY_THROTTLE_SAFE);
 		if (N_TTY_BUF_SIZE - read_cnt(ldata) >= TTY_THRESHOLD_THROTTLE)
 			break;
-	} while (tty_throttle_safe(tty));
+	} while (!tty_throttle_safe(tty));
 
 	__tty_set_flow_change(tty, 0);
 }

@@ -282,7 +282,7 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
 			break;
 
 		n_tty_kick_worker(tty);
-	} while (tty_unthrottle_safe(tty));
+	} while (!tty_unthrottle_safe(tty));
 
 	__tty_set_flow_change(tty, 0);
 }

drivers/tty/tty_ioctl.c +6 -6

index ba60fcf518e0..fb2f1ac5172f 100644
@@ -124,16 +124,16 @@ EXPORT_SYMBOL(tty_unthrottle);
  *	conditions when throttling is conditional on factors evaluated prior to
  *	throttling.
  *
- *	Returns false if tty is throttled (or was already throttled)
+ *	Returns true if tty is throttled (or was already throttled)
  */
 bool tty_throttle_safe(struct tty_struct *tty)
 {
-	bool ret = false;
+	bool ret = true;
 
 	mutex_lock(&tty->throttle_mutex);
 	if (!tty_throttled(tty)) {
 		if (tty->flow_change != TTY_THROTTLE_SAFE)
-			ret = true;
+			ret = false;
 		else {
 			set_bit(TTY_THROTTLED, &tty->flags);
 			if (tty->ops->throttle)

@@ -154,16 +154,16 @@ bool tty_throttle_safe(struct tty_struct *tty)
  *	unthrottle due to race conditions when unthrottling is conditional
  *	on factors evaluated prior to unthrottling.
  *
- *	Returns false if tty is unthrottled (or was already unthrottled)
+ *	Returns true if tty is unthrottled (or was already unthrottled)
  */
 bool tty_unthrottle_safe(struct tty_struct *tty)
 {
-	bool ret = false;
+	bool ret = true;
 
 	mutex_lock(&tty->throttle_mutex);
 	if (tty_throttled(tty)) {
 		if (tty->flow_change != TTY_UNTHROTTLE_SAFE)
-			ret = true;
+			ret = false;
 		else {
 			clear_bit(TTY_THROTTLED, &tty->flags);
 			if (tty->ops->unthrottle)

Signature · 1 line

2.42.0

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:52 UTC #10 lore raw

PATCH 8/15 tty: fix up and plug in tty_ioctl kernel-doc

In reply to Jiri Slaby (SUSE)

The ioctl helpers are well documented, except they are not plugged in the Documentation. So fix up the minor issues in the kernel-doc and plug it in.

The minor issues include:
* bad \t on every line (sphinx misinterprets the description otherwise)
* missing colon after Return
* superfluous \n after the comment
* make some struct members and constants a hyperlink
* and so on

Perhaps better to use --word-diff if one wants to see the "real" changes.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 Documentation/driver-api/tty/index.rst     |   1 +
 Documentation/driver-api/tty/tty_ioctl.rst |   7 +
 drivers/tty/tty_ioctl.c                    | 218 ++++++++++-----------
 3 files changed, 115 insertions(+), 111 deletions(-)
 create mode 100644 Documentation/driver-api/tty/tty_ioctl.rst

Documentation/driver-api/tty/index.rst +1 -0

index 2d32606a4278..b490da11f257 100644
@@ -36,6 +36,7 @@ In-detail description of the named TTY structures is in separate documents:
    tty_struct
    tty_ldisc
    tty_buffer
+   tty_ioctl
    tty_internals
 
 Writing TTY Driver

Documentation/driver-api/tty/tty_ioctl.rst new file +7 -0

new file mode 100644
index 000000000000..9b0be79fc15e
@@ -0,0 +1,7 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=================
+TTY IOCTL Helpers
+=================
+
+.. kernel-doc:: drivers/tty/tty_ioctl.c

drivers/tty/tty_ioctl.c +107 ~~-111~~

348 lines not on this page · raw message

Signature · 1 line

2.42.0

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:52 UTC #11 lore raw

PATCH 9/15 tty: fix kernel-doc for functions in tty.h

In reply to Jiri Slaby (SUSE)

tty_kref_get() is already included in Documentation, but is not properly formatted. Fix this.

tty_get_baud_rate() is neither properly formatted, nor is included. Fix both.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 Documentation/driver-api/tty/tty_ioctl.rst |  3 +++
 include/linux/tty.h                        | 21 +++++++++------------
 2 files changed, 12 insertions(+), 12 deletions(-)

Documentation/driver-api/tty/tty_ioctl.rst +3 -0

index 9b0be79fc15e..3ff1ac5e07f1 100644
@@ -5,3 +5,6 @@ TTY IOCTL Helpers
 =================
 
 .. kernel-doc:: drivers/tty/tty_ioctl.c
+
+.. kernel-doc:: include/linux/tty.h
+   :identifiers: tty_get_baud_rate

include/linux/tty.h +9 ~~-12~~

index 59d675f345e9..4b6340ac2af2 100644
@@ -390,14 +390,12 @@ int vcs_init(void);
 extern const struct class tty_class;
 
 /**
- *	tty_kref_get		-	get a tty reference
- *	@tty: tty device
+ * tty_kref_get - get a tty reference
+ * @tty: tty device
  *
- *	Return a new reference to a tty object. The caller must hold
- *	sufficient locks/counts to ensure that their existing reference cannot
- *	go away
+ * Returns: a new reference to a tty object. The caller must hold sufficient
+ * locks/counts to ensure that their existing reference cannot go away
  */
-
 static inline struct tty_struct *tty_kref_get(struct tty_struct *tty)
 {
 	if (tty)

@@ -435,14 +433,13 @@ void tty_encode_baud_rate(struct tty_struct *tty, speed_t ibaud,
 		speed_t obaud);
 
 /**
- *	tty_get_baud_rate	-	get tty bit rates
- *	@tty: tty to query
+ * tty_get_baud_rate - get tty bit rates
+ * @tty: tty to query
  *
- *	Returns the baud rate as an integer for this terminal. The
- *	termios lock must be held by the caller and the terminal bit
- *	flags may be updated.
+ * Returns: the baud rate as an integer for this terminal. The termios lock
+ * must be held by the caller and the terminal bit flags may be updated.
  *
- *	Locking: none
+ * Locking: none
  */
 static inline speed_t tty_get_baud_rate(struct tty_struct *tty)
 {

Signature · 1 line

2.42.0

Ilpo Järvinen

2023-09-19 10:07 UTC #12 lore raw

Re: [PATCH 09/15] tty: fix kernel-doc for functions in tty.h

Quoted 31 lines · Jiri Slaby (SUSE)

tty_kref_get() is already included in Documentation, but is not properly formatted. Fix this.
tty_get_baud_rate() is neither properly formatted, nor is included. Fix both.
Signed-off-by Jiri Slaby (SUSE) <[email protected]>
---
 Documentation/driver-api/tty/tty_ioctl.rst |  3 +++
 include/linux/tty.h                        | 21 +++++++++------------
 2 files changed, 12 insertions(+), 12 deletions(-)
Documentation/driver-api/tty/tty_ioctl.rst +3 -0
index 9b0be79fc15e..3ff1ac5e07f1 100644
@@ -5,3 +5,6 @@ TTY IOCTL Helpers
 =================
 
 .. kernel-doc:: drivers/tty/tty_ioctl.c
+
+.. kernel-doc:: include/linux/tty.h
+   :identifiers: tty_get_baud_rate
include/linux/tty.h +0 -0
index 59d675f345e9..4b6340ac2af2 100644
@@ -390,14 +390,12 @@ int vcs_init(void);
 extern const struct class tty_class;
 
 /**

include/linux/tty.h int vcs_init(void); · line 397

Quoted 10 lines

include/linux/tty.h +4 -5

- *	tty_kref_get		-	get a tty reference
- *	@tty: tty device
+ * tty_kref_get - get a tty reference
+ * @tty: tty device
  *
- *	Return a new reference to a tty object. The caller must hold
- *	sufficient locks/counts to ensure that their existing reference cannot
- *	go away
+ * Returns: a new reference to a tty object. The caller must hold sufficient
+ * locks/counts to ensure that their existing reference cannot go away

Shouldn't this have also Locking: entry instead of hiding the details into Return?

Quoted 12 lines

include/linux/tty.h +1 -3

  */
-
 static inline struct tty_struct *tty_kref_get(struct tty_struct *tty)
 {
 	if (tty)

@@ -435,14 +433,13 @@ void tty_encode_baud_rate(struct tty_struct *tty, speed_t ibaud,
 		speed_t obaud);
 
 /**
- *	tty_get_baud_rate	-	get tty bit rates
- *	@tty: tty to query
+ * tty_get_baud_rate - get tty bit rates

include/linux/tty.h void tty_encode_baud_rate(struct tty_struct *tty, speed_t ibaud, · line 442

Quoted 10 lines

include/linux/tty.h +4 -4

+ * @tty: tty to query
  *
- *	Returns the baud rate as an integer for this terminal. The
- *	termios lock must be held by the caller and the terminal bit
- *	flags may be updated.
+ * Returns: the baud rate as an integer for this terminal. The termios lock
+ * must be held by the caller and the terminal bit flags may be updated.
  *
- *	Locking: none
+ * Locking: none

Eh, the paragraph above says, "The termios lock must be held by the caller" so this "Locking: none" seems pretty bogus.

Quoted 4 lines

include/linux/tty.h +0 -0

  */
 static inline speed_t tty_get_baud_rate(struct tty_struct *tty)
 {

Signature · 1 line

i.

Jiri Slaby

author maintainer

2023-09-19 10:45 UTC #13 lore raw

Re: [PATCH 09/15] tty: fix kernel-doc for functions in tty.h

Quoted 32 lines · Ilpo Järvinen

Quoted 30 lines · Jiri Slaby (SUSE)
tty_kref_get() is already included in Documentation, but is not properly formatted. Fix this.
tty_get_baud_rate() is neither properly formatted, nor is included. Fix both.
Signed-off-by Jiri Slaby (SUSE) <[email protected]>
---
  Documentation/driver-api/tty/tty_ioctl.rst |  3 +++
  include/linux/tty.h                        | 21 +++++++++------------
  2 files changed, 12 insertions(+), 12 deletions(-)
Documentation/driver-api/tty/tty_ioctl.rst +3 -0
index 9b0be79fc15e..3ff1ac5e07f1 100644
@@ -5,3 +5,6 @@ TTY IOCTL Helpers
  =================
 
  .. kernel-doc:: drivers/tty/tty_ioctl.c
+
+.. kernel-doc:: include/linux/tty.h
+   :identifiers: tty_get_baud_rate
include/linux/tty.h +0 -0
index 59d675f345e9..4b6340ac2af2 100644
@@ -390,14 +390,12 @@ int vcs_init(void);
  extern const struct class tty_class;
 

Quoted 14 lines

include/linux/tty.h int vcs_init(void); · line 397

Quoted 11 lines

include/linux/tty.h +4 -5

  /**
- *	tty_kref_get		-	get a tty reference
- *	@tty: tty device
+ * tty_kref_get - get a tty reference
+ * @tty: tty device
   *
- *	Return a new reference to a tty object. The caller must hold
- *	sufficient locks/counts to ensure that their existing reference cannot
- *	go away
+ * Returns: a new reference to a tty object. The caller must hold sufficient
+ * locks/counts to ensure that their existing reference cannot go away

Shouldn't this have also Locking: entry instead of hiding the details into Return?

/me left to fix both in a separate patch.

thanks,

Signature · 2 lines

js
suse labs

Jiri Slaby

author maintainer

2023-09-19 10:47 UTC #14 lore raw

Re: [PATCH 09/15] tty: fix kernel-doc for functions in tty.h

Quoted 48 lines · Jiri Slaby

Quoted 38 lines · Ilpo Järvinen
Quoted 36 lines · Jiri Slaby (SUSE)
tty_kref_get() is already included in Documentation, but is not properly formatted. Fix this.
tty_get_baud_rate() is neither properly formatted, nor is included. Fix both.
Signed-off-by Jiri Slaby (SUSE) <[email protected]>
---
Documentation/driver-api/tty/tty_ioctl.rst | 3 +++ include/linux/tty.h | 21 +++++++++------------
  2 files changed, 12 insertions(+), 12 deletions(-)
Documentation/driver-api/tty/tty_ioctl.rst +0 -0
index 9b0be79fc15e..3ff1ac5e07f1 100644
@@ -5,3 +5,6 @@ TTY IOCTL Helpers
=================
.. kernel-doc:: drivers/tty/tty_ioctl.c
Documentation/driver-api/tty/tty_ioctl.rst +3 -0
+
+.. kernel-doc:: include/linux/tty.h
+   :identifiers: tty_get_baud_rate
include/linux/tty.h +2 -4
index 59d675f345e9..4b6340ac2af2 100644
@@ -390,14 +390,12 @@ int vcs_init(void);
   extern const struct class tty_class;
   /**
- *    tty_kref_get        -    get a tty reference
- *    @tty: tty device
+ * tty_kref_get - get a tty reference
+ * @tty: tty device
    *
- *    Return a new reference to a tty object. The caller must hold
- *    sufficient locks/counts to ensure that their existing 
Quoted 8 lines
Quoted 5 lines
reference cannot
- * go away
+ * Returns: a new reference to a tty object. The caller must hold sufficient
+ * locks/counts to ensure that their existing reference cannot go away
Shouldn't this have also Locking: entry instead of hiding the details into Return?

Quoted 1 line

/me left to fix both in a separate patch.

Ah, no. The Locking Alan introduced means what _this_ function locks. I am not sure we want to extend the meaning to _expected_ locks?

thanks,

Signature · 2 lines

js
suse labs

Ilpo Järvinen

2023-09-19 10:52 UTC #15 lore raw

Re: [PATCH 09/15] tty: fix kernel-doc for functions in tty.h

Quoted 50 lines · Jiri Slaby

Quoted 49 lines · Jiri Slaby
Quoted 47 lines · Ilpo Järvinen
Quoted 37 lines · Jiri Slaby (SUSE)
tty_kref_get() is already included in Documentation, but is not properly formatted. Fix this.
tty_get_baud_rate() is neither properly formatted, nor is included. Fix both.
Signed-off-by Jiri Slaby (SUSE) <[email protected]>
---
Documentation/driver-api/tty/tty_ioctl.rst | 3 +++ include/linux/tty.h | 21 +++++++++------------
  2 files changed, 12 insertions(+), 12 deletions(-)
Documentation/driver-api/tty/tty_ioctl.rst +0 -0
index 9b0be79fc15e..3ff1ac5e07f1 100644
@@ -5,3 +5,6 @@ TTY IOCTL Helpers
=================
.. kernel-doc:: drivers/tty/tty_ioctl.c
Documentation/driver-api/tty/tty_ioctl.rst +3 -0
+
+.. kernel-doc:: include/linux/tty.h
+   :identifiers: tty_get_baud_rate
include/linux/tty.h +2 -4
index 59d675f345e9..4b6340ac2af2 100644
@@ -390,14 +390,12 @@ int vcs_init(void);
   extern const struct class tty_class;
   /**
- *    tty_kref_get        -    get a tty reference
- *    @tty: tty device
+ * tty_kref_get - get a tty reference
+ * @tty: tty device
    *
- *    Return a new reference to a tty object. The caller must hold
- *    sufficient locks/counts to ensure that their existing reference
Quoted 5 lines
cannot
- * go away
+ * Returns: a new reference to a tty object. The caller must hold sufficient
+ * locks/counts to ensure that their existing reference cannot go away
Shouldn't this have also Locking: entry instead of hiding the details into Return?

Quoted 4 lines

Quoted 1 line
/me left to fix both in a separate patch.
Ah, no. The Locking Alan introduced means what _this_ function locks. I am not sure we want to extend the meaning to _expected_ locks?

There are plenty of examples with "Must be called with" or "Caller holds" in Locking. This is for humans anyway so one reading should understand it.

Signature · 1 line

i.

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:52 UTC #16 lore raw

PATCH 10/15 tty: stop using ndash in kernel-doc

In reply to Jiri Slaby (SUSE)

An ndash used instead of a single dash renders a bullet to the result. So use only single dashes in kernel-doc.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/tty_io.c        | 8 ++++----
 drivers/tty/tty_port.c      | 6 +++---
 drivers/tty/vt/consolemap.c | 2 +-
 drivers/tty/vt/vc_screen.c  | 4 ++--
 drivers/tty/vt/vt.c         | 4 ++--
 5 files changed, 12 insertions(+), 12 deletions(-)

drivers/tty/tty_io.c +4 -4

index 8a94e5a43c6d..2ed12ca7c832 100644
@@ -3300,7 +3300,7 @@ void tty_unregister_device(struct tty_driver *driver, unsigned index)
 EXPORT_SYMBOL(tty_unregister_device);
 
 /**
- * __tty_alloc_driver -- allocate tty driver
+ * __tty_alloc_driver - allocate tty driver
  * @lines: count of lines this driver can handle at most
  * @owner: module which is responsible for this driver
  * @flags: some of %TTY_DRIVER_ flags, will be set in driver->flags

@@ -3393,7 +3393,7 @@ static void destruct_tty_driver(struct kref *kref)
 }
 
 /**
- * tty_driver_kref_put -- drop a reference to a tty driver
+ * tty_driver_kref_put - drop a reference to a tty driver
  * @driver: driver of which to drop the reference
  *
  * The final put will destroy and free up the driver.

@@ -3405,7 +3405,7 @@ void tty_driver_kref_put(struct tty_driver *driver)
 EXPORT_SYMBOL(tty_driver_kref_put);
 
 /**
- * tty_register_driver -- register a tty driver
+ * tty_register_driver - register a tty driver
  * @driver: driver to register
  *
  * Called by a tty driver to register itself.

@@ -3470,7 +3470,7 @@ int tty_register_driver(struct tty_driver *driver)
 EXPORT_SYMBOL(tty_register_driver);
 
 /**
- * tty_unregister_driver -- unregister a tty driver
+ * tty_unregister_driver - unregister a tty driver
  * @driver: driver to unregister
  *
  * Called by a tty driver to unregister itself.

drivers/tty/tty_port.c +3 -3

index 624d104bd145..63c125250961 100644
@@ -79,7 +79,7 @@ const struct tty_port_client_operations tty_port_default_client_ops = {
 EXPORT_SYMBOL_GPL(tty_port_default_client_ops);
 
 /**
- * tty_port_init -- initialize tty_port
+ * tty_port_init - initialize tty_port
  * @port: tty_port to initialize
  *
  * Initializes the state of struct tty_port. When a port was initialized using

@@ -267,7 +267,7 @@ void tty_port_free_xmit_buf(struct tty_port *port)
 EXPORT_SYMBOL(tty_port_free_xmit_buf);
 
 /**
- * tty_port_destroy -- destroy inited port
+ * tty_port_destroy - destroy inited port
  * @port: tty port to be destroyed
  *
  * When a port was initialized using tty_port_init(), one has to destroy the

@@ -297,7 +297,7 @@ static void tty_port_destructor(struct kref *kref)
 }
 
 /**
- * tty_port_put -- drop a reference to tty_port
+ * tty_port_put - drop a reference to tty_port
  * @port: port to drop a reference of (can be NULL)
  *
  * The final put will destroy and free up the @port using

drivers/tty/vt/consolemap.c +1 -1

index f02d21e2a96e..5e39a4f430ee 100644
@@ -205,7 +205,7 @@ static enum translation_map inv_translate[MAX_NR_CONSOLES];
 				 FIELD_PREP(UNI_GLYPH_BITS, (glyph)))
 
 /**
- * struct uni_pagedict -- unicode directory
+ * struct uni_pagedict - unicode directory
  *
  * @uni_pgdir: 32*32*64 table with glyphs
  * @refcount: reference count of this structure

drivers/tty/vt/vc_screen.c +2 -2

index 829c4be66f3b..99c8e39d91b4 100644
@@ -174,7 +174,7 @@ vcs_poll_data_get(struct file *file)
 }
 
 /**
- * vcs_vc -- return VC for @inode
+ * vcs_vc - return VC for @inode
  * @inode: inode for which to return a VC
  * @viewed: returns whether this console is currently foreground (viewed)
  *

@@ -199,7 +199,7 @@ static struct vc_data *vcs_vc(struct inode *inode, bool *viewed)
 }
 
 /**
- * vcs_size -- return size for a VC in @vc
+ * vcs_size - return size for a VC in @vc
  * @vc: which VC
  * @attr: does it use attributes?
  * @unicode: is it unicode?

drivers/tty/vt/vt.c +2 -2

index 5c47f77804f0..f5004231cb6a 100644
@@ -2588,7 +2588,7 @@ static inline int vc_translate_ascii(const struct vc_data *vc, int c)
 
 
 /**
- * vc_sanitize_unicode -- Replace invalid Unicode code points with U+FFFD
+ * vc_sanitize_unicode - Replace invalid Unicode code points with U+FFFD
  * @c: the received character, or U+FFFD for invalid sequences.
  */
 static inline int vc_sanitize_unicode(const int c)

@@ -2600,7 +2600,7 @@ static inline int vc_sanitize_unicode(const int c)
 }
 
 /**
- * vc_translate_unicode -- Combine UTF-8 into Unicode in @vc_utf_char
+ * vc_translate_unicode - Combine UTF-8 into Unicode in @vc_utf_char
  * @vc: virtual console
  * @c: character to translate
  * @rescan: we return true if we need more (continuation) data

Signature · 1 line

2.42.0

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:53 UTC #17 lore raw

PATCH 11/15 tty: tty_buffer: use bool for 'restart' in tty_buffer_unlock_exclusive()

In reply to Jiri Slaby (SUSE)

It's a boolean value, so no need for 'int' there.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/tty_buffer.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/tty/tty_buffer.c +2 -3

index 5f6d0cf67571..f8883afbeeba 100644
@@ -69,12 +69,11 @@ EXPORT_SYMBOL_GPL(tty_buffer_lock_exclusive);
 void tty_buffer_unlock_exclusive(struct tty_port *port)
 {
 	struct tty_bufhead *buf = &port->buf;
-	int restart;
-
-	restart = buf->head->commit != buf->head->read;
+	bool restart = buf->head->commit != buf->head->read;
 
 	atomic_dec(&buf->priority);
 	mutex_unlock(&buf->lock);
+
 	if (restart)
 		queue_work(system_unbound_wq, &buf->work);
 }

Signature · 1 line

2.42.0

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:53 UTC #18 lore raw

PATCH 12/15 tty: convert THROTTLE constants into enum

In reply to Jiri Slaby (SUSE)

And make an explicit constant for zero too. This allows for easier type checking of the parameter.

Note: tty_struct::flow_change is kept as int because include/tty.h (tty_struct) doesn't see tty/tty.h (this enum).

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/tty.h       | 13 +++++++++----
 drivers/tty/tty_ioctl.c |  2 +-
 2 files changed, 10 insertions(+), 5 deletions(-)

drivers/tty/tty.h +9 -4

index 50862f98273e..93cf5ef1e857 100644
@@ -41,15 +41,20 @@ enum {
 };
 
 /* Values for tty->flow_change */
-#define TTY_THROTTLE_SAFE	1
-#define TTY_UNTHROTTLE_SAFE	2
+enum tty_flow_change {
+	TTY_FLOW_NO_CHANGE,
+	TTY_THROTTLE_SAFE,
+	TTY_UNTHROTTLE_SAFE,
+};
 
-static inline void __tty_set_flow_change(struct tty_struct *tty, int val)
+static inline void __tty_set_flow_change(struct tty_struct *tty,
+					 enum tty_flow_change val)
 {
 	tty->flow_change = val;
 }
 
-static inline void tty_set_flow_change(struct tty_struct *tty, int val)
+static inline void tty_set_flow_change(struct tty_struct *tty,
+				       enum tty_flow_change val)
 {
 	tty->flow_change = val;
 	smp_mb();

drivers/tty/tty_ioctl.c +1 -1

index 42c793e9d131..4b499301a3db 100644
@@ -104,7 +104,7 @@ void tty_unthrottle(struct tty_struct *tty)
 	if (test_and_clear_bit(TTY_THROTTLED, &tty->flags) &&
 	    tty->ops->unthrottle)
 		tty->ops->unthrottle(tty);
-	tty->flow_change = 0;
+	tty->flow_change = TTY_FLOW_NO_CHANGE;
 	up_write(&tty->termios_rwsem);
 }
 EXPORT_SYMBOL(tty_unthrottle);

Signature · 1 line

2.42.0

Ilpo Järvinen

2023-09-19 10:10 UTC #19 lore raw

Re: [PATCH 12/15] tty: convert THROTTLE constants into enum

Quoted 5 lines · Jiri Slaby (SUSE)

And make an explicit constant for zero too. This allows for easier type checking of the parameter.
Note: tty_struct::flow_change is kept as int because include/tty.h (tty_struct) doesn't see tty/tty.h (this enum).

And it cannot moved there because of what?

Signature · 1 line

i.

Quoted 49 lines

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/tty.h       | 13 +++++++++----
 drivers/tty/tty_ioctl.c |  2 +-
 2 files changed, 10 insertions(+), 5 deletions(-)

drivers/tty/tty.h +9 -4

index 50862f98273e..93cf5ef1e857 100644
@@ -41,15 +41,20 @@ enum {
 };
 
 /* Values for tty->flow_change */
-#define TTY_THROTTLE_SAFE	1
-#define TTY_UNTHROTTLE_SAFE	2
+enum tty_flow_change {
+	TTY_FLOW_NO_CHANGE,
+	TTY_THROTTLE_SAFE,
+	TTY_UNTHROTTLE_SAFE,
+};
 
-static inline void __tty_set_flow_change(struct tty_struct *tty, int val)
+static inline void __tty_set_flow_change(struct tty_struct *tty,
+					 enum tty_flow_change val)
 {
 	tty->flow_change = val;
 }
 
-static inline void tty_set_flow_change(struct tty_struct *tty, int val)
+static inline void tty_set_flow_change(struct tty_struct *tty,
+				       enum tty_flow_change val)
 {
 	tty->flow_change = val;
 	smp_mb();

drivers/tty/tty_ioctl.c +1 -1

index 42c793e9d131..4b499301a3db 100644
@@ -104,7 +104,7 @@ void tty_unthrottle(struct tty_struct *tty)
 	if (test_and_clear_bit(TTY_THROTTLED, &tty->flags) &&
 	    tty->ops->unthrottle)
 		tty->ops->unthrottle(tty);
-	tty->flow_change = 0;
+	tty->flow_change = TTY_FLOW_NO_CHANGE;
 	up_write(&tty->termios_rwsem);
 }
 EXPORT_SYMBOL(tty_unthrottle);

Jiri Slaby

author maintainer

2023-09-19 10:51 UTC #20 lore raw

Re: [PATCH 12/15] tty: convert THROTTLE constants into enum

Quoted 9 lines · Ilpo Järvinen

Quoted 5 lines · Jiri Slaby (SUSE)
And make an explicit constant for zero too. This allows for easier type checking of the parameter.
Note: tty_struct::flow_change is kept as int because include/tty.h (tty_struct) doesn't see tty/tty.h (this enum).
And it cannot moved there because of what?

It's possible, but would make the constants "public". I hoped for forward enum declaration, but since enum is no longer a simple int, the compilation then fails with incomplete type of tty_struct.

thanks,

Signature · 2 lines

js
suse labs

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:53 UTC #21 lore raw

PATCH 13/15 tty: early return from send_break() on TTY_DRIVER_HARDWARE_BREAK

In reply to Jiri Slaby (SUSE)

If the driver sets TTY_DRIVER_HARDWARE_BREAK, we leave ops->break_ctl() to the driver and return from send_break(). But we do it using a local variable and keep the code flowing through the end of the function. Instead, do 'return' immediately with the ops->break_ctl()'s return value.

This way, we don't have to stuff the 'else' branch of the 'if' with the software break handling. And we can re-indent the function too.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/tty_io.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

drivers/tty/tty_io.c +17 ~~-15~~

index 2ed12ca7c832..87bb5094e0bb 100644
@@ -2475,22 +2475,24 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
 		return 0;
 
 	if (tty->driver->flags & TTY_DRIVER_HARDWARE_BREAK)
-		retval = tty->ops->break_ctl(tty, duration);
-	else {
-		/* Do the work ourselves */
-		if (tty_write_lock(tty, false) < 0)
-			return -EINTR;
-		retval = tty->ops->break_ctl(tty, -1);
-		if (retval)
-			goto out;
-		if (!signal_pending(current))
-			msleep_interruptible(duration);
-		retval = tty->ops->break_ctl(tty, 0);
+		return tty->ops->break_ctl(tty, duration);
+
+	/* Do the work ourselves */
+	if (tty_write_lock(tty, false) < 0)
+		return -EINTR;
+
+	retval = tty->ops->break_ctl(tty, -1);
+	if (retval)
+		goto out;
+	if (!signal_pending(current))
+		msleep_interruptible(duration);
+	retval = tty->ops->break_ctl(tty, 0);
 out:
-		tty_write_unlock(tty);
-		if (signal_pending(current))
-			retval = -EINTR;
-	}
+	tty_write_unlock(tty);
+
+	if (signal_pending(current))
+		retval = -EINTR;
+
 	return retval;
 }

Signature · 1 line

2.42.0

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:53 UTC #22 lore raw

PATCH 14/15 tty: don't check for signal_pending() in send_break()

In reply to Jiri Slaby (SUSE)

msleep_interruptible() will check on its own. So no need to do the check in send_break() before calling the above.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/tty_io.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/tty/tty_io.c +1 -2

index 87bb5094e0bb..24833b31b81c 100644
@@ -2484,8 +2484,7 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
 	retval = tty->ops->break_ctl(tty, -1);
 	if (retval)
 		goto out;
-	if (!signal_pending(current))
-		msleep_interruptible(duration);
+	msleep_interruptible(duration);
 	retval = tty->ops->break_ctl(tty, 0);
 out:
 	tty_write_unlock(tty);

Signature · 1 line

2.42.0

Ilpo Järvinen

2023-09-19 10:15 UTC #23 lore raw

Re: [PATCH 14/15] tty: don't check for signal_pending() in send_break()

Quoted 1 line · Jiri Slaby (SUSE)

msleep_interruptible() will check on its own. So no need to do the check

For clarity:

... will check !signal_pending() on its own.

Quoted 22 lines

in send_break() before calling the above.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/tty_io.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/tty/tty_io.c +1 -2

index 87bb5094e0bb..24833b31b81c 100644
@@ -2484,8 +2484,7 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
 	retval = tty->ops->break_ctl(tty, -1);
 	if (retval)
 		goto out;
-	if (!signal_pending(current))
-		msleep_interruptible(duration);
+	msleep_interruptible(duration);
 	retval = tty->ops->break_ctl(tty, 0);
 out:
 	tty_write_unlock(tty);

Signature · 1 line

i.

Jiri Slaby (SUSE)

author maintainer

2023-09-19 08:53 UTC #24 lore raw

PATCH 15/15 tty: use 'if' in send_break() instead of 'goto'

In reply to Jiri Slaby (SUSE)

Now, the "jumped-over" code is simple enough to be put inside an 'if'. Do so to make it 'goto'-less.

Signed-off-by Jiri Slaby (SUSE) <[email protected]>

---
 drivers/tty/tty_io.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

drivers/tty/tty_io.c +4 -5

index 24833b31b81c..378257c4c41a 100644
@@ -2482,11 +2482,10 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
 		return -EINTR;
 
 	retval = tty->ops->break_ctl(tty, -1);
-	if (retval)
-		goto out;
-	msleep_interruptible(duration);
-	retval = tty->ops->break_ctl(tty, 0);
-out:
+	if (!retval) {
+		msleep_interruptible(duration);
+		retval = tty->ops->break_ctl(tty, 0);
+	}
 	tty_write_unlock(tty);
 
 	if (signal_pending(current))

Signature · 1 line

2.42.0

Ilpo Järvinen

Reviewed-by on the v1 series

2023-09-19 10:17 UTC #25 lore raw

In reply to Jiri Slaby (SUSE)

Quoted 22 lines · Jiri Slaby (SUSE)

This is a collection of random fixes for tty I did while crawling through the code. Mostly done for readability and understandability. No behavior change intended (except for Documentation fixes).
Jiri Slaby (SUSE) (15):
tty: n_tty: use 'retval' instead of 'c'
tty: n_tty: rename and retype 'retval' in n_tty_ioctl()
tty: n_tty: use min3() in copy_from_read_buf()
tty: n_tty: invert the condition in copy_from_read_buf()
tty: n_tty: use do-while in n_tty_check_{,un}throttle()
tty: switch tty_{,un}throttle_safe() to return a bool
tty: invert return values of tty_{,un}throttle_safe()
tty: fix up and plug in tty_ioctl kernel-doc
tty: fix kernel-doc for functions in tty.h
tty: stop using ndash in kernel-doc
tty: tty_buffer: use bool for 'restart' in
tty_buffer_unlock_exclusive()
tty: convert THROTTLE constants into enum
tty: early return from send_break() on TTY_DRIVER_HARDWARE_BREAK
tty: don't check for signal_pending() in send_break()
tty: use 'if' in send_break() instead of 'goto'

Quoted 14 lines

 Documentation/driver-api/tty/index.rst     |   1 +
 Documentation/driver-api/tty/tty_ioctl.rst |  10 +
 drivers/tty/n_tty.c                        |  77 ++++---
 drivers/tty/tty.h                          |  13 +-
 drivers/tty/tty_buffer.c                   |   5 +-
 drivers/tty/tty_io.c                       |  36 ++--
 drivers/tty/tty_ioctl.c                    | 234 ++++++++++-----------
 drivers/tty/tty_port.c                     |   6 +-
 drivers/tty/vt/consolemap.c                |   2 +-
 drivers/tty/vt/vc_screen.c                 |   4 +-
 drivers/tty/vt/vt.c                        |   4 +-
 include/linux/tty.h                        |  25 +--
 12 files changed, 209 insertions(+), 208 deletions(-)
 create mode 100644 Documentation/driver-api/tty/tty_ioctl.rst

For this whole series except the patches I commented on:

Reviewed-by Ilpo Järvinen <[email protected]>

...If you make the amendments I requested, please add the tag also to the patches I commented on.

Signature · 1 line

i.

Greg Kroah-Hartman

maintainer

2023-09-19 10:21 UTC #26 lore raw

Quoted 39 lines · Ilpo Järvinen

Quoted 22 lines · Jiri Slaby (SUSE)
This is a collection of random fixes for tty I did while crawling through the code. Mostly done for readability and understandability. No behavior change intended (except for Documentation fixes).
Jiri Slaby (SUSE) (15):
tty: n_tty: use 'retval' instead of 'c'
tty: n_tty: rename and retype 'retval' in n_tty_ioctl()
tty: n_tty: use min3() in copy_from_read_buf()
tty: n_tty: invert the condition in copy_from_read_buf()
tty: n_tty: use do-while in n_tty_check_{,un}throttle()
tty: switch tty_{,un}throttle_safe() to return a bool
tty: invert return values of tty_{,un}throttle_safe()
tty: fix up and plug in tty_ioctl kernel-doc
tty: fix kernel-doc for functions in tty.h
tty: stop using ndash in kernel-doc
tty: tty_buffer: use bool for 'restart' in
tty_buffer_unlock_exclusive()
tty: convert THROTTLE constants into enum
tty: early return from send_break() on TTY_DRIVER_HARDWARE_BREAK
tty: don't check for signal_pending() in send_break()
tty: use 'if' in send_break() instead of 'goto'
Quoted 14 lines
 Documentation/driver-api/tty/index.rst     |   1 +
 Documentation/driver-api/tty/tty_ioctl.rst |  10 +
 drivers/tty/n_tty.c                        |  77 ++++---
 drivers/tty/tty.h                          |  13 +-
 drivers/tty/tty_buffer.c                   |   5 +-
 drivers/tty/tty_io.c                       |  36 ++--
 drivers/tty/tty_ioctl.c                    | 234 ++++++++++-----------
 drivers/tty/tty_port.c                     |   6 +-
 drivers/tty/vt/consolemap.c                |   2 +-
 drivers/tty/vt/vc_screen.c                 |   4 +-
 drivers/tty/vt/vt.c                        |   4 +-
 include/linux/tty.h                        |  25 +--
 12 files changed, 209 insertions(+), 208 deletions(-)
 create mode 100644 Documentation/driver-api/tty/tty_ioctl.rst

Quoted 6 lines

For this whole series except the patches I commented on:
Reviewed-by Ilpo Järvinen <[email protected]>
...If you make the amendments I requested, please add the tag also to the patches I commented on.

That's horrible, b4 will now just add this reviewed-by to all of them when I run it :(

greg k-h

Ilpo Järvinen

2023-09-19 10:56 UTC #27 lore raw

Quoted 42 lines · Greg Kroah-Hartman

Quoted 41 lines · Ilpo Järvinen
Quoted 22 lines · Jiri Slaby (SUSE)
This is a collection of random fixes for tty I did while crawling through the code. Mostly done for readability and understandability. No behavior change intended (except for Documentation fixes).
Jiri Slaby (SUSE) (15):
tty: n_tty: use 'retval' instead of 'c'
tty: n_tty: rename and retype 'retval' in n_tty_ioctl()
tty: n_tty: use min3() in copy_from_read_buf()
tty: n_tty: invert the condition in copy_from_read_buf()
tty: n_tty: use do-while in n_tty_check_{,un}throttle()
tty: switch tty_{,un}throttle_safe() to return a bool
tty: invert return values of tty_{,un}throttle_safe()
tty: fix up and plug in tty_ioctl kernel-doc
tty: fix kernel-doc for functions in tty.h
tty: stop using ndash in kernel-doc
tty: tty_buffer: use bool for 'restart' in
tty_buffer_unlock_exclusive()
tty: convert THROTTLE constants into enum
tty: early return from send_break() on TTY_DRIVER_HARDWARE_BREAK
tty: don't check for signal_pending() in send_break()
tty: use 'if' in send_break() instead of 'goto'
Quoted 14 lines
 Documentation/driver-api/tty/index.rst     |   1 +
 Documentation/driver-api/tty/tty_ioctl.rst |  10 +
 drivers/tty/n_tty.c                        |  77 ++++---
 drivers/tty/tty.h                          |  13 +-
 drivers/tty/tty_buffer.c                   |   5 +-
 drivers/tty/tty_io.c                       |  36 ++--
 drivers/tty/tty_ioctl.c                    | 234 ++++++++++-----------
 drivers/tty/tty_port.c                     |   6 +-
 drivers/tty/vt/consolemap.c                |   2 +-
 drivers/tty/vt/vc_screen.c                 |   4 +-
 drivers/tty/vt/vt.c                        |   4 +-
 include/linux/tty.h                        |  25 +--
 12 files changed, 209 insertions(+), 208 deletions(-)
 create mode 100644 Documentation/driver-api/tty/tty_ioctl.rst
For this whole series except the patches I commented on:

Quoted 7 lines

Quoted 4 lines
Reviewed-by Ilpo Järvinen <[email protected]>
...If you make the amendments I requested, please add the tag also to the patches I commented on.
That's horrible, b4 will now just add this reviewed-by to all of them when I run it :(

Okay, I'll try to avoid it in the future.

Signature · 1 line

i.