[PATCH v2 1/2] tty: Convert tty_buffer flags to bool

[PATCH v2 1/2] tty: Convert tty_buffer flags to bool

[Ilpo Järvinen]

The struct tty_buffer has flags which is only used for storing TTYB_NORMAL.
There is also a few quite confusing operations for checking the presense
of TTYB_NORMAL. Simplify things by converting flags to bool.

Despite the name remaining the same, the meaning of "flags" is altered
slightly by this change. Previously it referred to flags of the buffer
(only TTYB_NORMAL being used as a flag). After this change, flags tell
whether the buffer contains/should be allocated with flags array along
with character data array. It is much more suitable name that
TTYB_NORMAL was for this purpose, thus the name remains.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

v2:
- Make it more obvious why flags is not renamed (both in kerneldoc
  comment and commit message).

 drivers/tty/tty_buffer.c   | 28 ++++++++++++++--------------
 include/linux/tty_buffer.h |  5 +----
 include/linux/tty_flip.h   |  4 ++--
 3 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 5e287dedce01..b408d830fcbc 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -107,7 +107,7 @@ static void tty_buffer_reset(struct tty_buffer *p, size_t size)
 	p->commit = 0;
 	p->lookahead = 0;
 	p->read = 0;
-	p->flags = 0;
+	p->flags = true;
 }
 
 /**
@@ -249,7 +249,7 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
  * __tty_buffer_request_room	-	grow tty buffer if needed
  * @port: tty port
  * @size: size desired
- * @flags: buffer flags if new buffer allocated (default = 0)
+ * @flags: buffer has to store flags along character data
  *
  * Make at least @size bytes of linear space available for the tty buffer.
  *
@@ -260,19 +260,19 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
  * Returns: the size we managed to find.
  */
 static int __tty_buffer_request_room(struct tty_port *port, size_t size,
-				     int flags)
+				     bool flags)
 {
 	struct tty_bufhead *buf = &port->buf;
 	struct tty_buffer *b, *n;
 	int left, change;
 
 	b = buf->tail;
-	if (b->flags & TTYB_NORMAL)
+	if (!b->flags)
 		left = 2 * b->size - b->used;
 	else
 		left = b->size - b->used;
 
-	change = (b->flags & TTYB_NORMAL) && (~flags & TTYB_NORMAL);
+	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);
@@ -300,7 +300,7 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,
 
 int tty_buffer_request_room(struct tty_port *port, size_t size)
 {
-	return __tty_buffer_request_room(port, size, 0);
+	return __tty_buffer_request_room(port, size, true);
 }
 EXPORT_SYMBOL_GPL(tty_buffer_request_room);
 
@@ -320,17 +320,17 @@ int tty_insert_flip_string_fixed_flag(struct tty_port *port,
 		const unsigned char *chars, char flag, size_t size)
 {
 	int copied = 0;
+	bool flags = flag != TTY_NORMAL;
 
 	do {
 		int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
-		int flags = (flag == TTY_NORMAL) ? TTYB_NORMAL : 0;
 		int space = __tty_buffer_request_room(port, goal, 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 & TTYB_NORMAL)
+		if (tb->flags)
 			memset(flag_buf_ptr(tb, tb->used), flag, space);
 		tb->used += space;
 		copied += space;
@@ -393,13 +393,13 @@ EXPORT_SYMBOL(tty_insert_flip_string_flags);
 int __tty_insert_flip_char(struct tty_port *port, unsigned char ch, char flag)
 {
 	struct tty_buffer *tb;
-	int flags = (flag == TTY_NORMAL) ? TTYB_NORMAL : 0;
+	bool flags = flag != TTY_NORMAL;
 
 	if (!__tty_buffer_request_room(port, 1, flags))
 		return 0;
 
 	tb = port->buf.tail;
-	if (~tb->flags & TTYB_NORMAL)
+	if (tb->flags)
 		*flag_buf_ptr(tb, tb->used) = flag;
 	*char_buf_ptr(tb, tb->used++) = ch;
 
@@ -424,13 +424,13 @@ EXPORT_SYMBOL(__tty_insert_flip_char);
 int tty_prepare_flip_string(struct tty_port *port, unsigned char **chars,
 		size_t size)
 {
-	int space = __tty_buffer_request_room(port, size, TTYB_NORMAL);
+	int space = __tty_buffer_request_room(port, size, false);
 
 	if (likely(space)) {
 		struct tty_buffer *tb = port->buf.tail;
 
 		*chars = char_buf_ptr(tb, tb->used);
-		if (~tb->flags & TTYB_NORMAL)
+		if (tb->flags)
 			memset(flag_buf_ptr(tb, tb->used), TTY_NORMAL, space);
 		tb->used += space;
 	}
@@ -492,7 +492,7 @@ static void lookahead_bufs(struct tty_port *port, struct tty_buffer *head)
 			unsigned char *p, *f = NULL;
 
 			p = char_buf_ptr(head, head->lookahead);
-			if (~head->flags & TTYB_NORMAL)
+			if (head->flags)
 				f = flag_buf_ptr(head, head->lookahead);
 
 			port->client_ops->lookahead_buf(port, p, f, count);
@@ -509,7 +509,7 @@ receive_buf(struct tty_port *port, struct tty_buffer *head, int count)
 	const char *f = NULL;
 	int n;
 
-	if (~head->flags & TTYB_NORMAL)
+	if (head->flags)
 		f = flag_buf_ptr(head, head->read);
 
 	n = port->client_ops->receive_buf(port, p, f, count);
diff --git a/include/linux/tty_buffer.h b/include/linux/tty_buffer.h
index 1796648c2907..6ceb2789e6c8 100644
--- a/include/linux/tty_buffer.h
+++ b/include/linux/tty_buffer.h
@@ -17,14 +17,11 @@ struct tty_buffer {
 	int commit;
 	int lookahead;		/* Lazy update on recv, can become less than "read" */
 	int read;
-	int flags;
+	bool flags;
 	/* Data points here */
 	unsigned long data[];
 };
 
-/* Values for .flags field of tty_buffer */
-#define TTYB_NORMAL	1	/* buffer has no flags buffer */
-
 static inline unsigned char *char_buf_ptr(struct tty_buffer *b, int ofs)
 {
 	return ((unsigned char *)b->data) + ofs;
diff --git a/include/linux/tty_flip.h b/include/linux/tty_flip.h
index 483d41cbcbb7..bfaaeee61a05 100644
--- a/include/linux/tty_flip.h
+++ b/include/linux/tty_flip.h
@@ -25,9 +25,9 @@ static inline int tty_insert_flip_char(struct tty_port *port,
 	struct tty_buffer *tb = port->buf.tail;
 	int change;
 
-	change = (tb->flags & TTYB_NORMAL) && (flag != TTY_NORMAL);
+	change = !tb->flags && (flag != TTY_NORMAL);
 	if (!change && tb->used < tb->size) {
-		if (~tb->flags & TTYB_NORMAL)
+		if (tb->flags)
 			*flag_buf_ptr(tb, tb->used) = flag;
 		*char_buf_ptr(tb, tb->used++) = ch;
 		return 1;
-- 
2.30.2

[PATCH v2 2/2] tty: Cleanup tty buffer align mask

[Ilpo Järvinen]

Don't use decimal for mask. Don't use literal for aligning.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/tty_buffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index b408d830fcbc..2df86ed90574 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -21,7 +21,7 @@
 #include "tty.h"
 
 #define MIN_TTYB_SIZE	256
-#define TTYB_ALIGN_MASK	255
+#define TTYB_ALIGN_MASK	0xff
 
 /*
  * Byte threshold to limit memory consumption for flip buffers.
@@ -37,7 +37,7 @@
  * logic this must match.
  */
 
-#define TTY_BUFFER_PAGE	(((PAGE_SIZE - sizeof(struct tty_buffer)) / 2) & ~0xFF)
+#define TTY_BUFFER_PAGE	(((PAGE_SIZE - sizeof(struct tty_buffer)) / 2) & ~TTYB_ALIGN_MASK)
 
 /**
  * tty_buffer_lock_exclusive	-	gain exclusive access to buffer
-- 
2.30.2

Re: [PATCH v2 1/2] tty: Convert tty_buffer flags to bool

[Greg KH]

On Wed, Oct 19, 2022 at 01:55:03PM +0300, Ilpo Järvinen wrote:
> The struct tty_buffer has flags which is only used for storing TTYB_NORMAL.
> There is also a few quite confusing operations for checking the presense
> of TTYB_NORMAL. Simplify things by converting flags to bool.
> 
> Despite the name remaining the same, the meaning of "flags" is altered
> slightly by this change. Previously it referred to flags of the buffer
> (only TTYB_NORMAL being used as a flag). After this change, flags tell
> whether the buffer contains/should be allocated with flags array along
> with character data array. It is much more suitable name that
> TTYB_NORMAL was for this purpose, thus the name remains.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> 
> v2:
> - Make it more obvious why flags is not renamed (both in kerneldoc
>   comment and commit message).
> 
>  drivers/tty/tty_buffer.c   | 28 ++++++++++++++--------------
>  include/linux/tty_buffer.h |  5 +----
>  include/linux/tty_flip.h   |  4 ++--
>  3 files changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 5e287dedce01..b408d830fcbc 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -107,7 +107,7 @@ static void tty_buffer_reset(struct tty_buffer *p, size_t size)
>  	p->commit = 0;
>  	p->lookahead = 0;
>  	p->read = 0;
> -	p->flags = 0;
> +	p->flags = true;
>  }
>  
>  /**
> @@ -249,7 +249,7 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
>   * __tty_buffer_request_room	-	grow tty buffer if needed
>   * @port: tty port
>   * @size: size desired
> - * @flags: buffer flags if new buffer allocated (default = 0)
> + * @flags: buffer has to store flags along character data
>   *
>   * Make at least @size bytes of linear space available for the tty buffer.
>   *
> @@ -260,19 +260,19 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
>   * Returns: the size we managed to find.
>   */
>  static int __tty_buffer_request_room(struct tty_port *port, size_t size,
> -				     int flags)
> +				     bool flags)
>  {
>  	struct tty_bufhead *buf = &port->buf;
>  	struct tty_buffer *b, *n;
>  	int left, change;
>  
>  	b = buf->tail;
> -	if (b->flags & TTYB_NORMAL)
> +	if (!b->flags)
>  		left = 2 * b->size - b->used;
>  	else
>  		left = b->size - b->used;
>  
> -	change = (b->flags & TTYB_NORMAL) && (~flags & TTYB_NORMAL);
> +	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);
> @@ -300,7 +300,7 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,
>  
>  int tty_buffer_request_room(struct tty_port *port, size_t size)
>  {
> -	return __tty_buffer_request_room(port, size, 0);
> +	return __tty_buffer_request_room(port, size, true);

Did this logic just get inverted?

Maybe it's the jet-lag, but this feels like it's not correct anymore.

Maybe a commet up above where you calculate "left" would make more sense
as to what is going on?

thanks,

greg k-h

Re: [PATCH v2 1/2] tty: Convert tty_buffer flags to bool

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4725 bytes --]

On Thu, 3 Nov 2022, Greg KH wrote:

> On Wed, Oct 19, 2022 at 01:55:03PM +0300, Ilpo Järvinen wrote:
> > The struct tty_buffer has flags which is only used for storing TTYB_NORMAL.
> > There is also a few quite confusing operations for checking the presense
> > of TTYB_NORMAL. Simplify things by converting flags to bool.
> > 
> > Despite the name remaining the same, the meaning of "flags" is altered
> > slightly by this change. Previously it referred to flags of the buffer
> > (only TTYB_NORMAL being used as a flag). After this change, flags tell
> > whether the buffer contains/should be allocated with flags array along
> > with character data array. It is much more suitable name that
> > TTYB_NORMAL was for this purpose, thus the name remains.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> > 
> > v2:
> > - Make it more obvious why flags is not renamed (both in kerneldoc
> >   comment and commit message).
> > 
> >  drivers/tty/tty_buffer.c   | 28 ++++++++++++++--------------
> >  include/linux/tty_buffer.h |  5 +----
> >  include/linux/tty_flip.h   |  4 ++--
> >  3 files changed, 17 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> > index 5e287dedce01..b408d830fcbc 100644
> > --- a/drivers/tty/tty_buffer.c
> > +++ b/drivers/tty/tty_buffer.c
> > @@ -107,7 +107,7 @@ static void tty_buffer_reset(struct tty_buffer *p, size_t size)
> >  	p->commit = 0;
> >  	p->lookahead = 0;
> >  	p->read = 0;
> > -	p->flags = 0;
> > +	p->flags = true;
> >  }
> >  
> >  /**
> > @@ -249,7 +249,7 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
> >   * __tty_buffer_request_room	-	grow tty buffer if needed
> >   * @port: tty port
> >   * @size: size desired
> > - * @flags: buffer flags if new buffer allocated (default = 0)
> > + * @flags: buffer has to store flags along character data
> >   *
> >   * Make at least @size bytes of linear space available for the tty buffer.
> >   *
> > @@ -260,19 +260,19 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
> >   * Returns: the size we managed to find.
> >   */
> >  static int __tty_buffer_request_room(struct tty_port *port, size_t size,
> > -				     int flags)
> > +				     bool flags)
> >  {
> >  	struct tty_bufhead *buf = &port->buf;
> >  	struct tty_buffer *b, *n;
> >  	int left, change;
> >  
> >  	b = buf->tail;
> > -	if (b->flags & TTYB_NORMAL)
> > +	if (!b->flags)
> >  		left = 2 * b->size - b->used;
> >  	else
> >  		left = b->size - b->used;
> >  
> > -	change = (b->flags & TTYB_NORMAL) && (~flags & TTYB_NORMAL);
> > +	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);
> > @@ -300,7 +300,7 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,
> >  
> >  int tty_buffer_request_room(struct tty_port *port, size_t size)
> >  {
> > -	return __tty_buffer_request_room(port, size, 0);
> > +	return __tty_buffer_request_room(port, size, true);
> 
> Did this logic just get inverted?
>
> Maybe it's the jet-lag, but this feels like it's not correct anymore.

As you can see, the old way is sooo confusing :-). I'll admit I stumbled 
myself with this same default thing first. It's even more confusing than 
the other places.

This check is true when flag bytes are present / required to be present:
	(~flags & TTYB_NORMAL)
It's very very confusing way to check such condition due to layered 
reverse logic.

With old code, the per character flag bytes won't be there in the buffer 
if TTYB_NORMAL is present. Thus, the old default of 0 means 
__tty_buffer_request_room will allocate room for those flag bytes.

If you think about it carefully, the old code passed 0. Therefore, ~0 & 
TTYB_NORMAL is going to be true. After my change true is passed and true 
matches to the original code.

So the logic was not inverted. I just cleared those layered reverse logic 
traps the original had which makes my patch look it's inverting things.

I really appreciate you took your time to find out this little detail
from it! This is far from a simple change because of how trappy the old
way of doing things is.

> Maybe a commet up above where you calculate "left" would make more sense
> as to what is going on?

Do you mean you want me to add a comment there? I don't see any 
pre-existing comments that you could be pointing me to.


Should I resubmit it since you probably dropped the patch?


While doing this cleanup, I realized there would likely be room for 
some improvements which would avoid allocing a new tty_buffer. I'll 
probably look into those at some point.


-- 
 i.

Re: [PATCH v2 1/2] tty: Convert tty_buffer flags to bool

[Greg KH]

On Thu, Nov 03, 2022 at 12:11:26PM +0200, Ilpo Järvinen wrote:
> On Thu, 3 Nov 2022, Greg KH wrote:
> 
> > On Wed, Oct 19, 2022 at 01:55:03PM +0300, Ilpo Järvinen wrote:
> > > The struct tty_buffer has flags which is only used for storing TTYB_NORMAL.
> > > There is also a few quite confusing operations for checking the presense
> > > of TTYB_NORMAL. Simplify things by converting flags to bool.
> > > 
> > > Despite the name remaining the same, the meaning of "flags" is altered
> > > slightly by this change. Previously it referred to flags of the buffer
> > > (only TTYB_NORMAL being used as a flag). After this change, flags tell
> > > whether the buffer contains/should be allocated with flags array along
> > > with character data array. It is much more suitable name that
> > > TTYB_NORMAL was for this purpose, thus the name remains.
> > > 
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > ---
> > > 
> > > v2:
> > > - Make it more obvious why flags is not renamed (both in kerneldoc
> > >   comment and commit message).
> > > 
> > >  drivers/tty/tty_buffer.c   | 28 ++++++++++++++--------------
> > >  include/linux/tty_buffer.h |  5 +----
> > >  include/linux/tty_flip.h   |  4 ++--
> > >  3 files changed, 17 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> > > index 5e287dedce01..b408d830fcbc 100644
> > > --- a/drivers/tty/tty_buffer.c
> > > +++ b/drivers/tty/tty_buffer.c
> > > @@ -107,7 +107,7 @@ static void tty_buffer_reset(struct tty_buffer *p, size_t size)
> > >  	p->commit = 0;
> > >  	p->lookahead = 0;
> > >  	p->read = 0;
> > > -	p->flags = 0;
> > > +	p->flags = true;
> > >  }
> > >  
> > >  /**
> > > @@ -249,7 +249,7 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
> > >   * __tty_buffer_request_room	-	grow tty buffer if needed
> > >   * @port: tty port
> > >   * @size: size desired
> > > - * @flags: buffer flags if new buffer allocated (default = 0)
> > > + * @flags: buffer has to store flags along character data
> > >   *
> > >   * Make at least @size bytes of linear space available for the tty buffer.
> > >   *
> > > @@ -260,19 +260,19 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
> > >   * Returns: the size we managed to find.
> > >   */
> > >  static int __tty_buffer_request_room(struct tty_port *port, size_t size,
> > > -				     int flags)
> > > +				     bool flags)
> > >  {
> > >  	struct tty_bufhead *buf = &port->buf;
> > >  	struct tty_buffer *b, *n;
> > >  	int left, change;
> > >  
> > >  	b = buf->tail;
> > > -	if (b->flags & TTYB_NORMAL)
> > > +	if (!b->flags)
> > >  		left = 2 * b->size - b->used;
> > >  	else
> > >  		left = b->size - b->used;
> > >  
> > > -	change = (b->flags & TTYB_NORMAL) && (~flags & TTYB_NORMAL);
> > > +	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);
> > > @@ -300,7 +300,7 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,
> > >  
> > >  int tty_buffer_request_room(struct tty_port *port, size_t size)
> > >  {
> > > -	return __tty_buffer_request_room(port, size, 0);
> > > +	return __tty_buffer_request_room(port, size, true);
> > 
> > Did this logic just get inverted?
> >
> > Maybe it's the jet-lag, but this feels like it's not correct anymore.
> 
> As you can see, the old way is sooo confusing :-). I'll admit I stumbled 
> myself with this same default thing first. It's even more confusing than 
> the other places.
> 
> This check is true when flag bytes are present / required to be present:
> 	(~flags & TTYB_NORMAL)
> It's very very confusing way to check such condition due to layered 
> reverse logic.
> 
> With old code, the per character flag bytes won't be there in the buffer 
> if TTYB_NORMAL is present. Thus, the old default of 0 means 
> __tty_buffer_request_room will allocate room for those flag bytes.
> 
> If you think about it carefully, the old code passed 0. Therefore, ~0 & 
> TTYB_NORMAL is going to be true. After my change true is passed and true 
> matches to the original code.
> 
> So the logic was not inverted. I just cleared those layered reverse logic 
> traps the original had which makes my patch look it's inverting things.
> 
> I really appreciate you took your time to find out this little detail
> from it! This is far from a simple change because of how trappy the old
> way of doing things is.
> 
> > Maybe a commet up above where you calculate "left" would make more sense
> > as to what is going on?
> 
> Do you mean you want me to add a comment there? I don't see any 
> pre-existing comments that you could be pointing me to.
> 
> 
> Should I resubmit it since you probably dropped the patch?

No need, I took is as-is now, thanks.

greg k-h