[PATCH 00/10] tty: tty_buffer: cleanup

[PATCH 00/10] tty: tty_buffer: cleanup

[Jiri Slaby (SUSE)]

This is another part (say part II.) of the previous type unification
across the tty layer[1]. This time, in tty_buffer. Apart from type
changes, this series contains a larger set of refactoring of the code.
Namely, unification of byte stuffing into the tty buffers into a single
function.

[1] https://lore.kernel.org/all/20230810091510.13006-1-jirislaby@kernel.org/

Jiri Slaby (SUSE) (10):
  tty: tty_buffer: switch data type to u8
  tty: tty_buffer: use struct_size() in tty_buffer_alloc()
  tty: tty_buffer: unify tty_insert_flip_string_{fixed_flag,flags}()
  tty: tty_buffer: warn if losing flags in
    __tty_insert_flip_string_flags()
  tty: tty_buffer: switch insert functions to size_t
  tty: tty_buffer: let tty_prepare_flip_string() return size_t
  tty: tty_buffer: use __tty_insert_flip_string_flags() in
    tty_insert_flip_char()
  tty: tty_buffer: better types in __tty_buffer_request_room()
  tty: tty_buffer: initialize variables in initializers already
  tty: tty_buffer: invert conditions in __tty_buffer_request_room()

 Documentation/driver-api/tty/tty_buffer.rst |   7 +-
 drivers/tty/tty_buffer.c                    | 169 ++++++--------------
 include/linux/tty_buffer.h                  |   4 +-
 include/linux/tty_flip.h                    |  64 ++++++--
 4 files changed, 111 insertions(+), 133 deletions(-)

-- 
2.41.0

[PATCH 01/10] tty: tty_buffer: switch data type to u8

[Jiri Slaby (SUSE)]

There is no reason to have tty_buffer::data typed as unsigned long.
Switch to u8, but preserve the ulong alignment using __aligned.

This allows for the cast removal from char_buf_ptr(). And for use of
struct_size() in the allocation in tty_buffer_alloc() -- in the next
patch.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 include/linux/tty_buffer.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/tty_buffer.h b/include/linux/tty_buffer.h
index e45cba81d0e9..31125e3be3c5 100644
--- a/include/linux/tty_buffer.h
+++ b/include/linux/tty_buffer.h
@@ -19,12 +19,12 @@ struct tty_buffer {
 	unsigned int read;
 	bool flags;
 	/* Data points here */
-	unsigned long data[];
+	u8 data[] __aligned(sizeof(unsigned long));
 };
 
 static inline u8 *char_buf_ptr(struct tty_buffer *b, unsigned int ofs)
 {
-	return ((u8 *)b->data) + ofs;
+	return b->data + ofs;
 }
 
 static inline u8 *flag_buf_ptr(struct tty_buffer *b, unsigned int ofs)
-- 
2.41.0

[PATCH 02/10] tty: tty_buffer: use struct_size() in tty_buffer_alloc()

[Jiri Slaby (SUSE)]

Now, that tty_buffer::data has the right type, use struct_size() for
size calculation. struct_size() makes the code less error-prone and more
readable.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/tty_buffer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 684d099cbe11..c94df1a2d7f8 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -177,8 +177,7 @@ static struct tty_buffer *tty_buffer_alloc(struct tty_port *port, size_t size)
 	 */
 	if (atomic_read(&port->buf.mem_used) > port->buf.mem_limit)
 		return NULL;
-	p = kmalloc(sizeof(struct tty_buffer) + 2 * size,
-		    GFP_ATOMIC | __GFP_NOWARN);
+	p = kmalloc(struct_size(p, data, 2 * size), GFP_ATOMIC | __GFP_NOWARN);
 	if (p == NULL)
 		return NULL;
 
-- 
2.41.0

[PATCH 03/10] tty: tty_buffer: unify tty_insert_flip_string_{fixed_flag,flags}()

[Jiri Slaby (SUSE)]

They both do the same except for flags. One mem-copies the flags from
the caller, the other mem-sets to one flag given by the caller. This can
be unified with a simple if in the unified function.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 Documentation/driver-api/tty/tty_buffer.rst |  6 +-
 drivers/tty/tty_buffer.c                    | 70 +++++----------------
 include/linux/tty_flip.h                    | 46 ++++++++++++--
 3 files changed, 61 insertions(+), 61 deletions(-)

diff --git a/Documentation/driver-api/tty/tty_buffer.rst b/Documentation/driver-api/tty/tty_buffer.rst
index a39d4781e0d2..774dc119c312 100644
--- a/Documentation/driver-api/tty/tty_buffer.rst
+++ b/Documentation/driver-api/tty/tty_buffer.rst
@@ -15,10 +15,12 @@ Flip Buffer Management
 ======================
 
 .. kernel-doc:: drivers/tty/tty_buffer.c
-   :identifiers: tty_prepare_flip_string tty_insert_flip_string_fixed_flag
-           tty_insert_flip_string_flags __tty_insert_flip_char
+   :identifiers: tty_prepare_flip_string __tty_insert_flip_char
            tty_flip_buffer_push tty_ldisc_receive_buf
 
+.. kernel-doc:: include/linux/tty_flip.h
+   :identifiers: tty_insert_flip_string_fixed_flag tty_insert_flip_string_flags
+
 ----
 
 Other Functions
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index c94df1a2d7f8..94a88dc05a54 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -303,82 +303,42 @@ int tty_buffer_request_room(struct tty_port *port, size_t size)
 }
 EXPORT_SYMBOL_GPL(tty_buffer_request_room);
 
-/**
- * tty_insert_flip_string_fixed_flag - add characters to the tty buffer
- * @port: tty port
- * @chars: characters
- * @flag: flag value for each character
- * @size: size
- *
- * Queue a series of bytes to the tty buffering. All the characters passed are
- * marked with the supplied flag.
- *
- * Returns: the number added.
- */
-int tty_insert_flip_string_fixed_flag(struct tty_port *port, const u8 *chars,
-				      u8 flag, size_t size)
+int __tty_insert_flip_string_flags(struct tty_port *port, const u8 *chars,
+				   const u8 *flags, bool mutable_flags,
+				   size_t size)
 {
+	bool need_flags = mutable_flags || flags[0] != TTY_NORMAL;
 	int copied = 0;
-	bool flags = flag != TTY_NORMAL;
 
 	do {
 		int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
-		int space = __tty_buffer_request_room(port, goal, flags);
+		int space = __tty_buffer_request_room(port, goal, need_flags);
 		struct tty_buffer *tb = port->buf.tail;
 
 		if (unlikely(space == 0))
 			break;
-		memcpy(char_buf_ptr(tb, tb->used), chars, space);
-		if (tb->flags)
-			memset(flag_buf_ptr(tb, tb->used), flag, space);
-		tb->used += space;
-		copied += space;
-		chars += space;
-		/* There is a small chance that we need to split the data over
-		 * several buffers. If this is the case we must loop.
-		 */
-	} while (unlikely(size > copied));
-	return copied;
-}
-EXPORT_SYMBOL(tty_insert_flip_string_fixed_flag);
 
-/**
- * tty_insert_flip_string_flags	-	add characters to the tty buffer
- * @port: tty port
- * @chars: characters
- * @flags: flag bytes
- * @size: size
- *
- * Queue a series of bytes to the tty buffering. For each character the flags
- * array indicates the status of the character.
- *
- * Returns: the number added.
- */
-int tty_insert_flip_string_flags(struct tty_port *port, const u8 *chars,
-				 const u8 *flags, size_t size)
-{
-	int copied = 0;
+		memcpy(char_buf_ptr(tb, tb->used), chars, space);
 
-	do {
-		int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
-		int space = tty_buffer_request_room(port, goal);
-		struct tty_buffer *tb = port->buf.tail;
+		if (mutable_flags) {
+			memcpy(flag_buf_ptr(tb, tb->used), flags, space);
+			flags += space;
+		} else if (tb->flags) {
+			memset(flag_buf_ptr(tb, tb->used), flags[0], space);
+		}
 
-		if (unlikely(space == 0))
-			break;
-		memcpy(char_buf_ptr(tb, tb->used), chars, space);
-		memcpy(flag_buf_ptr(tb, tb->used), flags, space);
 		tb->used += space;
 		copied += space;
 		chars += space;
-		flags += space;
+
 		/* There is a small chance that we need to split the data over
 		 * several buffers. If this is the case we must loop.
 		 */
 	} while (unlikely(size > copied));
+
 	return copied;
 }
-EXPORT_SYMBOL(tty_insert_flip_string_flags);
+EXPORT_SYMBOL(__tty_insert_flip_string_flags);
 
 /**
  * __tty_insert_flip_char   -	add one character to the tty buffer
diff --git a/include/linux/tty_flip.h b/include/linux/tty_flip.h
index d33aed2172c7..8b781f709605 100644
--- a/include/linux/tty_flip.h
+++ b/include/linux/tty_flip.h
@@ -10,14 +10,52 @@ struct tty_ldisc;
 int tty_buffer_set_limit(struct tty_port *port, int limit);
 unsigned int tty_buffer_space_avail(struct tty_port *port);
 int tty_buffer_request_room(struct tty_port *port, size_t size);
-int tty_insert_flip_string_flags(struct tty_port *port, const u8 *chars,
-				 const u8 *flags, size_t size);
-int tty_insert_flip_string_fixed_flag(struct tty_port *port, const u8 *chars,
-				      u8 flag, size_t size);
+int __tty_insert_flip_string_flags(struct tty_port *port, const u8 *chars,
+				   const u8 *flags, bool mutable_flags,
+				   size_t size);
 int tty_prepare_flip_string(struct tty_port *port, u8 **chars, size_t size);
 void tty_flip_buffer_push(struct tty_port *port);
 int __tty_insert_flip_char(struct tty_port *port, u8 ch, u8 flag);
 
+/**
+ * tty_insert_flip_string_fixed_flag - add characters to the tty buffer
+ * @port: tty port
+ * @chars: characters
+ * @flag: flag value for each character
+ * @size: size
+ *
+ * Queue a series of bytes to the tty buffering. All the characters passed are
+ * marked with the supplied flag.
+ *
+ * Returns: the number added.
+ */
+static inline int tty_insert_flip_string_fixed_flag(struct tty_port *port,
+						    const u8 *chars, u8 flag,
+						    size_t size)
+{
+	return __tty_insert_flip_string_flags(port, chars, &flag, false, size);
+}
+
+/**
+ * tty_insert_flip_string_flags - add characters to the tty buffer
+ * @port: tty port
+ * @chars: characters
+ * @flags: flag bytes
+ * @size: size
+ *
+ * Queue a series of bytes to the tty buffering. For each character the flags
+ * array indicates the status of the character.
+ *
+ * Returns: the number added.
+ */
+static inline int tty_insert_flip_string_flags(struct tty_port *port,
+					       const u8 *chars, const u8 *flags,
+					       size_t size)
+{
+	return __tty_insert_flip_string_flags(port, chars, flags, true, size);
+}
+
+
 static inline int tty_insert_flip_char(struct tty_port *port, u8 ch, u8 flag)
 {
 	struct tty_buffer *tb = port->buf.tail;
-- 
2.41.0

[PATCH 04/10] tty: tty_buffer: warn if losing flags in __tty_insert_flip_string_flags()

[Jiri Slaby (SUSE)]

And add a WARN_ON_ONCE(need_flags) to make sure we are not losing flags
in __tty_insert_flip_string_flags().

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/tty_buffer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 94a88dc05a54..c101b4ab737e 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -325,6 +325,9 @@ int __tty_insert_flip_string_flags(struct tty_port *port, const u8 *chars,
 			flags += space;
 		} else if (tb->flags) {
 			memset(flag_buf_ptr(tb, tb->used), flags[0], space);
+		} else {
+			/* tb->flags should be available once requested */
+			WARN_ON_ONCE(need_flags);
 		}
 
 		tb->used += space;
-- 
2.41.0

[PATCH 05/10] tty: tty_buffer: switch insert functions to size_t

[Jiri Slaby (SUSE)]

All the functions accept size_t as a size argument. They finally return
the same size (or less). It is quite unexpected that they return a
signed value and can confuse users to check for negative values.

Instead, return the same size_t as accepted to make clear we return
values >= 0, where zero in fact means failure.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/tty_buffer.c | 12 ++++++------
 include/linux/tty_flip.h | 24 ++++++++++++------------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index c101b4ab737e..598891e53031 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -303,16 +303,16 @@ int tty_buffer_request_room(struct tty_port *port, size_t size)
 }
 EXPORT_SYMBOL_GPL(tty_buffer_request_room);
 
-int __tty_insert_flip_string_flags(struct tty_port *port, const u8 *chars,
-				   const u8 *flags, bool mutable_flags,
-				   size_t size)
+size_t __tty_insert_flip_string_flags(struct tty_port *port, const u8 *chars,
+				      const u8 *flags, bool mutable_flags,
+				      size_t size)
 {
 	bool need_flags = mutable_flags || flags[0] != TTY_NORMAL;
-	int copied = 0;
+	size_t copied = 0;
 
 	do {
-		int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
-		int space = __tty_buffer_request_room(port, goal, need_flags);
+		size_t goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
+		size_t space = __tty_buffer_request_room(port, goal, need_flags);
 		struct tty_buffer *tb = port->buf.tail;
 
 		if (unlikely(space == 0))
diff --git a/include/linux/tty_flip.h b/include/linux/tty_flip.h
index 8b781f709605..569747364ae5 100644
--- a/include/linux/tty_flip.h
+++ b/include/linux/tty_flip.h
@@ -10,9 +10,9 @@ struct tty_ldisc;
 int tty_buffer_set_limit(struct tty_port *port, int limit);
 unsigned int tty_buffer_space_avail(struct tty_port *port);
 int tty_buffer_request_room(struct tty_port *port, size_t size);
-int __tty_insert_flip_string_flags(struct tty_port *port, const u8 *chars,
-				   const u8 *flags, bool mutable_flags,
-				   size_t size);
+size_t __tty_insert_flip_string_flags(struct tty_port *port, const u8 *chars,
+				      const u8 *flags, bool mutable_flags,
+				      size_t size);
 int tty_prepare_flip_string(struct tty_port *port, u8 **chars, size_t size);
 void tty_flip_buffer_push(struct tty_port *port);
 int __tty_insert_flip_char(struct tty_port *port, u8 ch, u8 flag);
@@ -29,9 +29,9 @@ int __tty_insert_flip_char(struct tty_port *port, u8 ch, u8 flag);
  *
  * Returns: the number added.
  */
-static inline int tty_insert_flip_string_fixed_flag(struct tty_port *port,
-						    const u8 *chars, u8 flag,
-						    size_t size)
+static inline size_t tty_insert_flip_string_fixed_flag(struct tty_port *port,
+						       const u8 *chars, u8 flag,
+						       size_t size)
 {
 	return __tty_insert_flip_string_flags(port, chars, &flag, false, size);
 }
@@ -48,15 +48,15 @@ static inline int tty_insert_flip_string_fixed_flag(struct tty_port *port,
  *
  * Returns: the number added.
  */
-static inline int tty_insert_flip_string_flags(struct tty_port *port,
-					       const u8 *chars, const u8 *flags,
-					       size_t size)
+static inline size_t tty_insert_flip_string_flags(struct tty_port *port,
+						  const u8 *chars,
+						  const u8 *flags, size_t size)
 {
 	return __tty_insert_flip_string_flags(port, chars, flags, true, size);
 }
 
 
-static inline int tty_insert_flip_char(struct tty_port *port, u8 ch, u8 flag)
+static inline size_t tty_insert_flip_char(struct tty_port *port, u8 ch, u8 flag)
 {
 	struct tty_buffer *tb = port->buf.tail;
 	int change;
@@ -71,8 +71,8 @@ static inline int tty_insert_flip_char(struct tty_port *port, u8 ch, u8 flag)
 	return __tty_insert_flip_char(port, ch, flag);
 }
 
-static inline int tty_insert_flip_string(struct tty_port *port,
-		const u8 *chars, size_t size)
+static inline size_t tty_insert_flip_string(struct tty_port *port,
+					    const u8 *chars, size_t size)
 {
 	return tty_insert_flip_string_fixed_flag(port, chars, TTY_NORMAL, size);
 }
-- 
2.41.0

[PATCH 06/10] tty: tty_buffer: let tty_prepare_flip_string() return size_t

[Jiri Slaby (SUSE)]

The same as in the previous patch, tty_prepare_flip_string() accepts
size_t as an size argument. It returns the same size (or less). It is
unexpected that it returns a signed value and can confuse users to check
for negative values.

Instead, return the same size_t as accepted to make clear we return
values >= 0, where zero in fact means failure.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/tty_buffer.c | 5 +++--
 include/linux/tty_flip.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 598891e53031..4f84466498f7 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -383,9 +383,9 @@ EXPORT_SYMBOL(__tty_insert_flip_char);
  * Returns: the length available and buffer pointer (@chars) to the space which
  * is now allocated and accounted for as ready for normal characters.
  */
-int tty_prepare_flip_string(struct tty_port *port, u8 **chars, size_t size)
+size_t tty_prepare_flip_string(struct tty_port *port, u8 **chars, size_t size)
 {
-	int space = __tty_buffer_request_room(port, size, false);
+	size_t space = __tty_buffer_request_room(port, size, false);
 
 	if (likely(space)) {
 		struct tty_buffer *tb = port->buf.tail;
@@ -395,6 +395,7 @@ int tty_prepare_flip_string(struct tty_port *port, u8 **chars, size_t size)
 			memset(flag_buf_ptr(tb, tb->used), TTY_NORMAL, space);
 		tb->used += space;
 	}
+
 	return space;
 }
 EXPORT_SYMBOL_GPL(tty_prepare_flip_string);
diff --git a/include/linux/tty_flip.h b/include/linux/tty_flip.h
index 569747364ae5..efd03d9c11f8 100644
--- a/include/linux/tty_flip.h
+++ b/include/linux/tty_flip.h
@@ -13,7 +13,7 @@ int tty_buffer_request_room(struct tty_port *port, size_t size);
 size_t __tty_insert_flip_string_flags(struct tty_port *port, const u8 *chars,
 				      const u8 *flags, bool mutable_flags,
 				      size_t size);
-int tty_prepare_flip_string(struct tty_port *port, u8 **chars, size_t size);
+size_t tty_prepare_flip_string(struct tty_port *port, u8 **chars, size_t size);
 void tty_flip_buffer_push(struct tty_port *port);
 int __tty_insert_flip_char(struct tty_port *port, u8 ch, u8 flag);
 
-- 
2.41.0

[PATCH 07/10] tty: tty_buffer: use __tty_insert_flip_string_flags() in tty_insert_flip_char()

[Jiri Slaby (SUSE)]

Use __tty_insert_flip_string_flags() for the slow path of
tty_insert_flip_char(). The former is generic enough, so there is no
reason to reimplement the injection once again.

So now we have a single function stuffing into tty buffers.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 Documentation/driver-api/tty/tty_buffer.rst |  3 ++-
 drivers/tty/tty_buffer.c                    | 26 ---------------------
 include/linux/tty_flip.h                    | 12 +++++++---
 3 files changed, 11 insertions(+), 30 deletions(-)

diff --git a/Documentation/driver-api/tty/tty_buffer.rst b/Documentation/driver-api/tty/tty_buffer.rst
index 774dc119c312..4b5ca1776d4f 100644
--- a/Documentation/driver-api/tty/tty_buffer.rst
+++ b/Documentation/driver-api/tty/tty_buffer.rst
@@ -15,11 +15,12 @@ Flip Buffer Management
 ======================
 
 .. kernel-doc:: drivers/tty/tty_buffer.c
-   :identifiers: tty_prepare_flip_string __tty_insert_flip_char
+   :identifiers: tty_prepare_flip_string
            tty_flip_buffer_push tty_ldisc_receive_buf
 
 .. kernel-doc:: include/linux/tty_flip.h
    :identifiers: tty_insert_flip_string_fixed_flag tty_insert_flip_string_flags
+           tty_insert_flip_char
 
 ----
 
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 4f84466498f7..e162318d6c31 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -343,32 +343,6 @@ size_t __tty_insert_flip_string_flags(struct tty_port *port, const u8 *chars,
 }
 EXPORT_SYMBOL(__tty_insert_flip_string_flags);
 
-/**
- * __tty_insert_flip_char   -	add one character to the tty buffer
- * @port: tty port
- * @ch: character
- * @flag: flag byte
- *
- * Queue a single byte @ch to the tty buffering, with an optional flag. This is
- * the slow path of tty_insert_flip_char().
- */
-int __tty_insert_flip_char(struct tty_port *port, u8 ch, u8 flag)
-{
-	struct tty_buffer *tb;
-	bool flags = flag != TTY_NORMAL;
-
-	if (!__tty_buffer_request_room(port, 1, flags))
-		return 0;
-
-	tb = port->buf.tail;
-	if (tb->flags)
-		*flag_buf_ptr(tb, tb->used) = flag;
-	*char_buf_ptr(tb, tb->used++) = ch;
-
-	return 1;
-}
-EXPORT_SYMBOL(__tty_insert_flip_char);
-
 /**
  * tty_prepare_flip_string	-	make room for characters
  * @port: tty port
diff --git a/include/linux/tty_flip.h b/include/linux/tty_flip.h
index efd03d9c11f8..af4fce98f64e 100644
--- a/include/linux/tty_flip.h
+++ b/include/linux/tty_flip.h
@@ -15,7 +15,6 @@ size_t __tty_insert_flip_string_flags(struct tty_port *port, const u8 *chars,
 				      size_t size);
 size_t tty_prepare_flip_string(struct tty_port *port, u8 **chars, size_t size);
 void tty_flip_buffer_push(struct tty_port *port);
-int __tty_insert_flip_char(struct tty_port *port, u8 ch, u8 flag);
 
 /**
  * tty_insert_flip_string_fixed_flag - add characters to the tty buffer
@@ -55,7 +54,14 @@ static inline size_t tty_insert_flip_string_flags(struct tty_port *port,
 	return __tty_insert_flip_string_flags(port, chars, flags, true, size);
 }
 
-
+/**
+ * tty_insert_flip_char - add one character to the tty buffer
+ * @port: tty port
+ * @ch: character
+ * @flag: flag byte
+ *
+ * Queue a single byte @ch to the tty buffering, with an optional flag.
+ */
 static inline size_t tty_insert_flip_char(struct tty_port *port, u8 ch, u8 flag)
 {
 	struct tty_buffer *tb = port->buf.tail;
@@ -68,7 +74,7 @@ static inline size_t tty_insert_flip_char(struct tty_port *port, u8 ch, u8 flag)
 		*char_buf_ptr(tb, tb->used++) = ch;
 		return 1;
 	}
-	return __tty_insert_flip_char(port, ch, flag);
+	return __tty_insert_flip_string_flags(port, &ch, &flag, false, 1);
 }
 
 static inline size_t tty_insert_flip_string(struct tty_port *port,
-- 
2.41.0

[PATCH 08/10] tty: tty_buffer: better types in __tty_buffer_request_room()

[Jiri Slaby (SUSE)]

* use bool for 'change' as it holds a result of a boolean.
* use size_t for 'left', so it is the same as 'size' which it is
  compared to. Both are supposed to contain an unsigned value.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/tty_buffer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index e162318d6c31..414bb7f9155f 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -263,7 +263,8 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,
 {
 	struct tty_bufhead *buf = &port->buf;
 	struct tty_buffer *b, *n;
-	int left, change;
+	size_t left;
+	bool change;
 
 	b = buf->tail;
 	if (!b->flags)
-- 
2.41.0

[PATCH 09/10] tty: tty_buffer: initialize variables in initializers already

[Jiri Slaby (SUSE)]

It makes the code both more compact, and more understandable.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/tty_buffer.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 414bb7f9155f..44c0adaec850 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -262,17 +262,10 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,
 				     bool flags)
 {
 	struct tty_bufhead *buf = &port->buf;
-	struct tty_buffer *b, *n;
-	size_t left;
-	bool change;
+	struct tty_buffer *n, *b = buf->tail;
+	size_t left = (b->flags ? 1 : 2) * b->size - b->used;
+	bool change = !b->flags && flags;
 
-	b = buf->tail;
-	if (!b->flags)
-		left = 2 * b->size - b->used;
-	else
-		left = b->size - b->used;
-
-	change = !b->flags && flags;
 	if (change || left < size) {
 		/* This is the slow path - looking for new buffers to use */
 		n = tty_buffer_alloc(port, size);
-- 
2.41.0

[PATCH 10/10] tty: tty_buffer: invert conditions in __tty_buffer_request_room()

[Jiri Slaby (SUSE)]

We are used to handle "bad" states in the 'if's in the kernel. Refactor
(invert the two conditions in) __tty_buffer_request_room(), so that the
code returns from the fast paths immediately instead of postponing to
the heavy end of the function.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/tty_buffer.c | 44 ++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 44c0adaec850..5f6d0cf67571 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -266,28 +266,28 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,
 	size_t left = (b->flags ? 1 : 2) * b->size - b->used;
 	bool change = !b->flags && flags;
 
-	if (change || left < size) {
-		/* This is the slow path - looking for new buffers to use */
-		n = tty_buffer_alloc(port, size);
-		if (n != NULL) {
-			n->flags = flags;
-			buf->tail = n;
-			/*
-			 * Paired w/ acquire in flush_to_ldisc() and lookahead_bufs()
-			 * ensures they see all buffer data.
-			 */
-			smp_store_release(&b->commit, b->used);
-			/*
-			 * Paired w/ acquire in flush_to_ldisc() and lookahead_bufs()
-			 * ensures the latest commit value can be read before the head
-			 * is advanced to the next buffer.
-			 */
-			smp_store_release(&b->next, n);
-		} else if (change)
-			size = 0;
-		else
-			size = left;
-	}
+	if (!change && left >= size)
+		return size;
+
+	/* This is the slow path - looking for new buffers to use */
+	n = tty_buffer_alloc(port, size);
+	if (n == NULL)
+		return change ? 0 : left;
+
+	n->flags = flags;
+	buf->tail = n;
+	/*
+	 * Paired w/ acquire in flush_to_ldisc() and lookahead_bufs()
+	 * ensures they see all buffer data.
+	 */
+	smp_store_release(&b->commit, b->used);
+	/*
+	 * Paired w/ acquire in flush_to_ldisc() and lookahead_bufs()
+	 * ensures the latest commit value can be read before the head
+	 * is advanced to the next buffer.
+	 */
+	smp_store_release(&b->next, n);
+
 	return size;
 }
 
-- 
2.41.0

Re: [PATCH 09/10] tty: tty_buffer: initialize variables in initializers already

[Greg KH]

On Wed, Aug 16, 2023 at 12:55:29PM +0200, Jiri Slaby (SUSE) wrote:
> It makes the code both more compact, and more understandable.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> ---
>  drivers/tty/tty_buffer.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 414bb7f9155f..44c0adaec850 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -262,17 +262,10 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,
>  				     bool flags)
>  {
>  	struct tty_bufhead *buf = &port->buf;
> -	struct tty_buffer *b, *n;
> -	size_t left;
> -	bool change;
> +	struct tty_buffer *n, *b = buf->tail;
> +	size_t left = (b->flags ? 1 : 2) * b->size - b->used;

That's understandable?  Hah!

I'll take it, but ick, the original:

> +	bool change = !b->flags && flags;
>  
> -	b = buf->tail;
> -	if (!b->flags)
> -		left = 2 * b->size - b->used;
> -	else
> -		left = b->size - b->used;

Is much more readable.

Well kind of, it's all a mess :)

thanks,

greg k-h

Re: [PATCH 00/10] tty: tty_buffer: cleanup

[Greg KH]

On Wed, Aug 16, 2023 at 12:55:20PM +0200, Jiri Slaby (SUSE) wrote:
> This is another part (say part II.) of the previous type unification
> across the tty layer[1]. This time, in tty_buffer. Apart from type
> changes, this series contains a larger set of refactoring of the code.
> Namely, unification of byte stuffing into the tty buffers into a single
> function.
> 
> [1] https://lore.kernel.org/all/20230810091510.13006-1-jirislaby@kernel.org/
> 
> Jiri Slaby (SUSE) (10):
>   tty: tty_buffer: switch data type to u8
>   tty: tty_buffer: use struct_size() in tty_buffer_alloc()
>   tty: tty_buffer: unify tty_insert_flip_string_{fixed_flag,flags}()
>   tty: tty_buffer: warn if losing flags in
>     __tty_insert_flip_string_flags()
>   tty: tty_buffer: switch insert functions to size_t
>   tty: tty_buffer: let tty_prepare_flip_string() return size_t
>   tty: tty_buffer: use __tty_insert_flip_string_flags() in
>     tty_insert_flip_char()
>   tty: tty_buffer: better types in __tty_buffer_request_room()
>   tty: tty_buffer: initialize variables in initializers already
>   tty: tty_buffer: invert conditions in __tty_buffer_request_room()
> 
>  Documentation/driver-api/tty/tty_buffer.rst |   7 +-
>  drivers/tty/tty_buffer.c                    | 169 ++++++--------------
>  include/linux/tty_buffer.h                  |   4 +-
>  include/linux/tty_flip.h                    |  64 ++++++--
>  4 files changed, 111 insertions(+), 133 deletions(-)
> 
> -- 
> 2.41.0
> 

Nice work, thanks, all now queued up.

greg k-h