[PATCH 00/17] tty: small cleanups and fixes

[PATCH 00/17] tty: small cleanups and fixes

[Jiri Slaby (SUSE)]

This is a series to fix/clean up some obvious issues I revealed during
u8+size_t conversions (to be posted later).

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Jan Kara <jack@suse.com>
Cc: Laurentiu Tudor <laurentiu.tudor@nxp.com>
Cc: linux-alpha@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-usb@vger.kernel.org
Cc: Matt Turner <mattst88@gmail.com>
Cc: netdev@vger.kernel.org
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>

Jiri Slaby (SUSE) (17):
  tty: deprecate tty_write_message()
  tty: remove unneeded mbz from tiocsti()
  tty: fix tty_operations types in documentation
  tty: move locking docs out of Returns for functions in tty.h
  tty: amiserial: return from receive_chars() without goto
  tty: amiserial: use bool and rename overrun flag in receive_chars()
  tty: ehv_bytecha: use memcpy_and_pad() in local_ev_byte_channel_send()
  tty: goldfish: drop unneeded temporary variables
  tty: hso: don't emit load/unload info to the log
  tty: hso: don't initialize global serial_table
  tty: hvc_console: use flexible array for outbuf
  tty: nozomi: remove unused debugging DUMP()
  tty: srmcons: use 'buf' directly in srmcons_do_write()
  tty: srmcons: use 'count' directly in srmcons_do_write()
  tty: srmcons: make srmcons_do_write() return void
  tty: srmcons: switch need_cr to bool
  tty: srmcons: make 'str_cr' const and non-array

 arch/alpha/kernel/srmcons.c   | 29 +++++++++++++----------------
 drivers/net/usb/hso.c         | 11 -----------
 drivers/tty/amiserial.c       | 10 ++++------
 drivers/tty/ehv_bytechan.c    |  7 +++++--
 drivers/tty/goldfish.c        |  7 ++-----
 drivers/tty/hvc/hvc_console.c |  4 +---
 drivers/tty/hvc/hvc_console.h |  2 +-
 drivers/tty/nozomi.c          | 18 ------------------
 drivers/tty/tty_io.c          |  8 ++++++--
 include/linux/tty.h           | 12 +++++++-----
 include/linux/tty_driver.h    |  5 ++---
 11 files changed, 41 insertions(+), 72 deletions(-)

-- 
2.42.1

[PATCH 01/17] tty: deprecate tty_write_message()

[Jiri Slaby (SUSE)]

tty_write_message() has only one user: quotas. In particular, there the
use depends on CONFIG_PRINT_QUOTA_WARNING. And that is deprecated and
marked as BROKEN already too.

So make tty_write_message() dependent on that very config option. This
action in fact drops tty_write_message() from the vmlinux binary. Good
riddance.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Jan Kara <jack@suse.com>
---
 drivers/tty/tty_io.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 06414e43e0b5..ee5a90f9adb5 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1047,6 +1047,7 @@ static ssize_t iterate_tty_write(struct tty_ldisc *ld, struct tty_struct *tty,
 	return ret;
 }
 
+#ifdef CONFIG_PRINT_QUOTA_WARNING
 /**
  * tty_write_message - write a message to a certain tty, not just the console.
  * @tty: the destination tty_struct
@@ -1057,6 +1058,8 @@ static ssize_t iterate_tty_write(struct tty_ldisc *ld, struct tty_struct *tty,
  * needed.
  *
  * We must still hold the BTM and test the CLOSING flag for the moment.
+ *
+ * This function is DEPRECATED, do not use in new code.
  */
 void tty_write_message(struct tty_struct *tty, char *msg)
 {
@@ -1069,6 +1072,7 @@ void tty_write_message(struct tty_struct *tty, char *msg)
 		tty_write_unlock(tty);
 	}
 }
+#endif
 
 static ssize_t file_tty_write(struct file *file, struct kiocb *iocb, struct iov_iter *from)
 {
-- 
2.42.1

Re: [PATCH 01/17] tty: deprecate tty_write_message()

[Jan Kara]

On Tue 21-11-23 10:22:42, Jiri Slaby (SUSE) wrote:
> tty_write_message() has only one user: quotas. In particular, there the
> use depends on CONFIG_PRINT_QUOTA_WARNING. And that is deprecated and
> marked as BROKEN already too.
> 
> So make tty_write_message() dependent on that very config option. This
> action in fact drops tty_write_message() from the vmlinux binary. Good
> riddance.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Jan Kara <jack@suse.com>

Sure, that was indeed a hack. Feel free to add:

Acked-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  drivers/tty/tty_io.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 06414e43e0b5..ee5a90f9adb5 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1047,6 +1047,7 @@ static ssize_t iterate_tty_write(struct tty_ldisc *ld, struct tty_struct *tty,
>  	return ret;
>  }
>  
> +#ifdef CONFIG_PRINT_QUOTA_WARNING
>  /**
>   * tty_write_message - write a message to a certain tty, not just the console.
>   * @tty: the destination tty_struct
> @@ -1057,6 +1058,8 @@ static ssize_t iterate_tty_write(struct tty_ldisc *ld, struct tty_struct *tty,
>   * needed.
>   *
>   * We must still hold the BTM and test the CLOSING flag for the moment.
> + *
> + * This function is DEPRECATED, do not use in new code.
>   */
>  void tty_write_message(struct tty_struct *tty, char *msg)
>  {
> @@ -1069,6 +1072,7 @@ void tty_write_message(struct tty_struct *tty, char *msg)
>  		tty_write_unlock(tty);
>  	}
>  }
> +#endif
>  
>  static ssize_t file_tty_write(struct file *file, struct kiocb *iocb, struct iov_iter *from)
>  {
> -- 
> 2.42.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 02/17] tty: remove unneeded mbz from tiocsti()

[Jiri Slaby (SUSE)]

'mbz' in tiocsti() is used only to pass TTY_NORMAL to
tty_ldisc_ops::receive_buf(). But that can be achieved easier by simply
passing NULL to ::receive_buf().

So drop this 'mbz'.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index ee5a90f9adb5..005d91c63707 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2280,7 +2280,7 @@ static bool tty_legacy_tiocsti __read_mostly = IS_ENABLED(CONFIG_LEGACY_TIOCSTI)
  */
 static int tiocsti(struct tty_struct *tty, char __user *p)
 {
-	char ch, mbz = 0;
+	char ch;
 	struct tty_ldisc *ld;
 
 	if (!tty_legacy_tiocsti && !capable(CAP_SYS_ADMIN))
@@ -2296,7 +2296,7 @@ static int tiocsti(struct tty_struct *tty, char __user *p)
 		return -EIO;
 	tty_buffer_lock_exclusive(tty->port);
 	if (ld->ops->receive_buf)
-		ld->ops->receive_buf(tty, &ch, &mbz, 1);
+		ld->ops->receive_buf(tty, &ch, NULL, 1);
 	tty_buffer_unlock_exclusive(tty->port);
 	tty_ldisc_deref(ld);
 	return 0;
-- 
2.42.1

[PATCH 03/17] tty: fix tty_operations types in documentation

[Jiri Slaby (SUSE)]

Commits 95713967ba52 ("tty: make tty_operations::write()'s count
size_t") and dcaafbe6ee3b ("tty: propagate u8 data to
tty_operations::put_char()") changed types of characters to u8, but
omitted to fix the documentation.

Fix the latter now.

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

diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
index 18beff0cec1a..f428c1b784a2 100644
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -72,8 +72,7 @@ struct serial_struct;
  *	is closed for the last time freeing up the resources. This is
  *	actually the second part of shutdown for routines that might sleep.
  *
- * @write: ``ssize_t ()(struct tty_struct *tty, const unsigned char *buf,
- *		    size_t count)``
+ * @write: ``ssize_t ()(struct tty_struct *tty, const u8 *buf, size_t count)``
  *
  *	This routine is called by the kernel to write a series (@count) of
  *	characters (@buf) to the @tty device. The characters may come from
@@ -85,7 +84,7 @@ struct serial_struct;
  *
  *	Optional: Required for writable devices. May not sleep.
  *
- * @put_char: ``int ()(struct tty_struct *tty, unsigned char ch)``
+ * @put_char: ``int ()(struct tty_struct *tty, u8 ch)``
  *
  *	This routine is called by the kernel to write a single character @ch to
  *	the @tty device. If the kernel uses this routine, it must call the
-- 
2.42.1

[PATCH 04/17] tty: move locking docs out of Returns for functions in tty.h

[Jiri Slaby (SUSE)]

Both tty_kref_get() and tty_get_baud_rate() note about locking in their
Return kernel-doc clause. Extract this info into a separate "Locking"
paragraph -- the same as we do for other tty functions.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 include/linux/tty.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/linux/tty.h b/include/linux/tty.h
index 4b6340ac2af2..7625fc98fef3 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -393,8 +393,10 @@ extern const struct class tty_class;
  * tty_kref_get - get a tty reference
  * @tty: tty device
  *
- * Returns: 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
+ *
+ * Locking: 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)
 {
@@ -436,10 +438,10 @@ void tty_encode_baud_rate(struct tty_struct *tty, speed_t ibaud,
  * 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
  *
- * Locking: none
+ * Locking: The termios lock must be held by the caller and the terminal bit
+ * flags may be updated.
  */
 static inline speed_t tty_get_baud_rate(struct tty_struct *tty)
 {
-- 
2.42.1

Re: [PATCH 04/17] tty: move locking docs out of Returns for functions in tty.h

[Ilpo Järvinen]

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

On Tue, 21 Nov 2023, Jiri Slaby (SUSE) wrote:

> Both tty_kref_get() and tty_get_baud_rate() note about locking in their
> Return kernel-doc clause. Extract this info into a separate "Locking"
> paragraph -- the same as we do for other tty functions.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  include/linux/tty.h | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index 4b6340ac2af2..7625fc98fef3 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -393,8 +393,10 @@ extern const struct class tty_class;
>   * tty_kref_get - get a tty reference
>   * @tty: tty device
>   *
> - * Returns: 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
> + *
> + * Locking: The caller must hold sufficient locks/counts to ensure that their
> + * existing reference cannot go away.

Just noting this is a bit vaguely worded (but so is the original).

>   */
>  static inline struct tty_struct *tty_kref_get(struct tty_struct *tty)
>  {
> @@ -436,10 +438,10 @@ void tty_encode_baud_rate(struct tty_struct *tty, speed_t ibaud,
>   * 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
>   *
> - * Locking: none
> + * Locking: The termios lock must be held by the caller and the terminal bit
> + * flags may be updated.

I don't think the second part about the flags really belongs here, I'd 
keep it the description.

Other than that,

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

-- 
 i.

Re: [PATCH 04/17] tty: move locking docs out of Returns for functions in tty.h

[Jiri Slaby]

On 21. 11. 23, 10:48, Ilpo Järvinen wrote:
> On Tue, 21 Nov 2023, Jiri Slaby (SUSE) wrote:
> 
>> Both tty_kref_get() and tty_get_baud_rate() note about locking in their
>> Return kernel-doc clause. Extract this info into a separate "Locking"
>> paragraph -- the same as we do for other tty functions.
>>
>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> ---
>>   include/linux/tty.h | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/tty.h b/include/linux/tty.h
>> index 4b6340ac2af2..7625fc98fef3 100644
>> --- a/include/linux/tty.h
>> +++ b/include/linux/tty.h
...
>> @@ -436,10 +438,10 @@ void tty_encode_baud_rate(struct tty_struct *tty, speed_t ibaud,
>>    * 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
>>    *
>> - * Locking: none
>> + * Locking: The termios lock must be held by the caller and the terminal bit
>> + * flags may be updated.
> 
> I don't think the second part about the flags really belongs here, I'd
> keep it the description.

Any idea what does the part says in fact? I had not been thinking about 
the content and now I don't understand it.

thanks,
-- 
js
suse labs

Re: [PATCH 04/17] tty: move locking docs out of Returns for functions in tty.h

[Jiri Slaby]

On 22. 11. 23, 7:45, Jiri Slaby wrote:
> On 21. 11. 23, 10:48, Ilpo Järvinen wrote:
>> On Tue, 21 Nov 2023, Jiri Slaby (SUSE) wrote:
>>
>>> Both tty_kref_get() and tty_get_baud_rate() note about locking in their
>>> Return kernel-doc clause. Extract this info into a separate "Locking"
>>> paragraph -- the same as we do for other tty functions.
>>>
>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>>> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>> ---
>>>   include/linux/tty.h | 12 +++++++-----
>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/linux/tty.h b/include/linux/tty.h
>>> index 4b6340ac2af2..7625fc98fef3 100644
>>> --- a/include/linux/tty.h
>>> +++ b/include/linux/tty.h
> ...
>>> @@ -436,10 +438,10 @@ void tty_encode_baud_rate(struct tty_struct 
>>> *tty, speed_t ibaud,
>>>    * 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
>>>    *
>>> - * Locking: none
>>> + * Locking: The termios lock must be held by the caller and the 
>>> terminal bit
>>> + * flags may be updated.
>>
>> I don't think the second part about the flags really belongs here, I'd
>> keep it the description.
> 
> Any idea what does the part says in fact? I had not been thinking about 
> the content and now I don't understand it.

Maybe before:
commit 6865ff222ccab371c04afce17aec1f7d70b17dbc
Author: Jiri Slaby <jirislaby@kernel.org>
Date:   Thu Mar 7 13:12:27 2013 +0100

     TTY: do not warn about setting speed via SPD_*


tty->warned was "the terminal bit".

Let's drop that part. And we can make tty const there too.

> thanks,

-- 
js
suse labs

Re: [PATCH 04/17] tty: move locking docs out of Returns for functions in tty.h

[Ilpo Järvinen]

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

On Wed, 22 Nov 2023, Jiri Slaby wrote:

> On 22. 11. 23, 7:45, Jiri Slaby wrote:
> > On 21. 11. 23, 10:48, Ilpo Järvinen wrote:
> > > On Tue, 21 Nov 2023, Jiri Slaby (SUSE) wrote:
> > > 
> > > > Both tty_kref_get() and tty_get_baud_rate() note about locking in their
> > > > Return kernel-doc clause. Extract this info into a separate "Locking"
> > > > paragraph -- the same as we do for other tty functions.
> > > > 
> > > > Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> > > > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > ---
> > > >   include/linux/tty.h | 12 +++++++-----
> > > >   1 file changed, 7 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/include/linux/tty.h b/include/linux/tty.h
> > > > index 4b6340ac2af2..7625fc98fef3 100644
> > > > --- a/include/linux/tty.h
> > > > +++ b/include/linux/tty.h
> > ...
> > > > @@ -436,10 +438,10 @@ void tty_encode_baud_rate(struct tty_struct *tty,
> > > > speed_t ibaud,
> > > >    * 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
> > > >    *
> > > > - * Locking: none
> > > > + * Locking: The termios lock must be held by the caller and the
> > > > terminal bit
> > > > + * flags may be updated.
> > > 
> > > I don't think the second part about the flags really belongs here, I'd
> > > keep it the description.
> > 
> > Any idea what does the part says in fact? I had not been thinking about the
> > content and now I don't understand it.
> 
> Maybe before:
> commit 6865ff222ccab371c04afce17aec1f7d70b17dbc
> Author: Jiri Slaby <jirislaby@kernel.org>
> Date:   Thu Mar 7 13:12:27 2013 +0100
> 
>     TTY: do not warn about setting speed via SPD_*
> 
> 
> tty->warned was "the terminal bit".
> 
> Let's drop that part. And we can make tty const there too.

Good point. 

The commit you point to is probably unrelated though but thanks anyway 
because it gave me this idea which I think is the correct reference: I 
removed the ->c_cflag alteringin 87888fb9ac0c ("tty: Remove baudrate dead 
code & make ktermios params const"). It had become deadcode anyway long 
since (I never went through the arch headers to find how long ago).

So yes, dropping the second part seems the correct way to go.

-- 
 i.

[PATCH 05/17] tty: amiserial: return from receive_chars() without goto

[Jiri Slaby (SUSE)]

The 'out' label is just before 'return'. So return immediately and drop
both the label and the return. This makes the code more straightforward.

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

diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index 785558c65ae8..b9580bb9afd3 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -230,7 +230,7 @@ static void receive_chars(struct serial_state *info)
 	   * should be ignored.
 	   */
 	  if (status & info->ignore_status_mask)
-	    goto out;
+		  return;
 
 	  status &= info->read_status_mask;
 
@@ -258,8 +258,6 @@ static void receive_chars(struct serial_state *info)
 	if (oe == 1)
 		tty_insert_flip_char(&info->tport, 0, TTY_OVERRUN);
 	tty_flip_buffer_push(&info->tport);
-out:
-	return;
 }
 
 static void transmit_chars(struct serial_state *info)
-- 
2.42.1

[PATCH 06/17] tty: amiserial: use bool and rename overrun flag in receive_chars()

[Jiri Slaby (SUSE)]

'oe' is a yes-no flag, switch it to boolean. And rename to overrun. All
for the code to be more obvious.

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

diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index b9580bb9afd3..a80f059f77bf 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -180,7 +180,7 @@ static void receive_chars(struct serial_state *info)
 	int serdatr;
 	unsigned char ch, flag;
 	struct	async_icount *icount;
-	int oe = 0;
+	bool overrun = false;
 
 	icount = &info->icount;
 
@@ -251,11 +251,11 @@ static void receive_chars(struct serial_state *info)
 	     * reported immediately, and doesn't
 	     * affect the current character
 	     */
-	     oe = 1;
+	     overrun = true;
 	  }
 	}
 	tty_insert_flip_char(&info->tport, ch, flag);
-	if (oe == 1)
+	if (overrun)
 		tty_insert_flip_char(&info->tport, 0, TTY_OVERRUN);
 	tty_flip_buffer_push(&info->tport);
 }
-- 
2.42.1

[PATCH 07/17] tty: ehv_bytecha: use memcpy_and_pad() in local_ev_byte_channel_send()

[Jiri Slaby (SUSE)]

There is a helper for memcpy(buffer)+memset(the_rest). Use it for
simplicity.

And add a comment why we are doing the copy in the first place.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Laurentiu Tudor <laurentiu.tudor@nxp.com>
Cc: linuxppc-dev@lists.ozlabs.org
---
 drivers/tty/ehv_bytechan.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/ehv_bytechan.c b/drivers/tty/ehv_bytechan.c
index a067628e01c8..cc9f4338da60 100644
--- a/drivers/tty/ehv_bytechan.c
+++ b/drivers/tty/ehv_bytechan.c
@@ -143,9 +143,12 @@ static unsigned int local_ev_byte_channel_send(unsigned int handle,
 	char buffer[EV_BYTE_CHANNEL_MAX_BYTES];
 	unsigned int c = *count;
 
+	/*
+	 * ev_byte_channel_send() expects at least EV_BYTE_CHANNEL_MAX_BYTES
+	 * (16 B) in the buffer. Fake it using a local buffer if needed.
+	 */
 	if (c < sizeof(buffer)) {
-		memcpy(buffer, p, c);
-		memset(&buffer[c], 0, sizeof(buffer) - c);
+		memcpy_and_pad(buffer, sizeof(buffer), p, c, 0);
 		p = buffer;
 	}
 	return ev_byte_channel_send(handle, count, p);
-- 
2.42.1

[PATCH 08/17] tty: goldfish: drop unneeded temporary variables

[Jiri Slaby (SUSE)]

We can pass 'buf' directly to goldfish_tty_rw() using simple (unsigned
long) cast. There is no need to obfuscate the code by another variable
with double casts.

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

diff --git a/drivers/tty/goldfish.c b/drivers/tty/goldfish.c
index 4591f940b7a1..dccf6c6c69c6 100644
--- a/drivers/tty/goldfish.c
+++ b/drivers/tty/goldfish.c
@@ -128,16 +128,14 @@ static void goldfish_tty_rw(struct goldfish_tty *qtty,
 static void goldfish_tty_do_write(int line, const u8 *buf, unsigned int count)
 {
 	struct goldfish_tty *qtty = &goldfish_ttys[line];
-	unsigned long address = (unsigned long)(void *)buf;
 
-	goldfish_tty_rw(qtty, address, count, 1);
+	goldfish_tty_rw(qtty, (unsigned long)buf, count, 1);
 }
 
 static irqreturn_t goldfish_tty_interrupt(int irq, void *dev_id)
 {
 	struct goldfish_tty *qtty = dev_id;
 	void __iomem *base = qtty->base;
-	unsigned long address;
 	unsigned char *buf;
 	u32 count;
 
@@ -147,8 +145,7 @@ static irqreturn_t goldfish_tty_interrupt(int irq, void *dev_id)
 
 	count = tty_prepare_flip_string(&qtty->port, &buf, count);
 
-	address = (unsigned long)(void *)buf;
-	goldfish_tty_rw(qtty, address, count, 0);
+	goldfish_tty_rw(qtty, (unsigned long)buf, count, 0);
 
 	tty_flip_buffer_push(&qtty->port);
 	return IRQ_HANDLED;
-- 
2.42.1

[PATCH 09/17] tty: hso: don't emit load/unload info to the log

[Jiri Slaby (SUSE)]

It's preferred NOT to emit anything during the module load and unload
(in case the un/load was successful). So drop these prints from hso
along with global 'version'. It even contains no version after all.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: linux-usb@vger.kernel.org
Cc: netdev@vger.kernel.org
---
 drivers/net/usb/hso.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 83b8452220ec..48450fe861ad 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -363,7 +363,6 @@ static int disable_net;
 /* driver info */
 static const char driver_name[] = "hso";
 static const char tty_filename[] = "ttyHS";
-static const char *version = __FILE__ ": " MOD_AUTHOR;
 /* the usb driver itself (registered in hso_init) */
 static struct usb_driver hso_driver;
 /* serial structures */
@@ -3231,9 +3230,6 @@ static int __init hso_init(void)
 	int i;
 	int result;
 
-	/* put it in the log */
-	pr_info("%s\n", version);
-
 	/* Initialise the serial table semaphore and table */
 	for (i = 0; i < HSO_SERIAL_TTY_MINORS; i++)
 		serial_table[i] = NULL;
@@ -3285,8 +3281,6 @@ static int __init hso_init(void)
 
 static void __exit hso_exit(void)
 {
-	pr_info("unloaded\n");
-
 	tty_unregister_driver(tty_drv);
 	/* deregister the usb driver */
 	usb_deregister(&hso_driver);
-- 
2.42.1

Re: [PATCH 09/17] tty: hso: don't emit load/unload info to the log

[Jakub Kicinski]

On Tue, 21 Nov 2023 10:22:50 +0100 Jiri Slaby (SUSE) wrote:
> It's preferred NOT to emit anything during the module load and unload
> (in case the un/load was successful). So drop these prints from hso
> along with global 'version'. It even contains no version after all.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>

Acked-by: Jakub Kicinski <kuba@kernel.org>

[PATCH 10/17] tty: hso: don't initialize global serial_table

[Jiri Slaby (SUSE)]

'serial_table' is global, so there is no need to initialize it to NULLs
at the module load. Drop this unneeded for loop.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: linux-usb@vger.kernel.org
Cc: netdev@vger.kernel.org
---
 drivers/net/usb/hso.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 48450fe861ad..f088ea2ba6f3 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -3227,13 +3227,8 @@ static struct usb_driver hso_driver = {
 
 static int __init hso_init(void)
 {
-	int i;
 	int result;
 
-	/* Initialise the serial table semaphore and table */
-	for (i = 0; i < HSO_SERIAL_TTY_MINORS; i++)
-		serial_table[i] = NULL;
-
 	/* allocate our driver using the proper amount of supported minors */
 	tty_drv = tty_alloc_driver(HSO_SERIAL_TTY_MINORS, TTY_DRIVER_REAL_RAW |
 			TTY_DRIVER_DYNAMIC_DEV);
-- 
2.42.1

Re: [PATCH 10/17] tty: hso: don't initialize global serial_table

[Jakub Kicinski]

On Tue, 21 Nov 2023 10:22:51 +0100 Jiri Slaby (SUSE) wrote:
> 'serial_table' is global, so there is no need to initialize it to NULLs
> at the module load. Drop this unneeded for loop.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>

Acked-by: Jakub Kicinski <kuba@kernel.org>

[PATCH 11/17] tty: hvc_console: use flexible array for outbuf

[Jiri Slaby (SUSE)]

This means:
* move outbuf to the end of struct hvc_struct and convert from pointer
  to flexible array (the structure is smaller now)
* use struct_size() at the allocation site
* align outbuf in the struct instead of ALIGN() at kzalloc()

And apart from the above, use u8 instead of char (which are the same
thanks to -funsigned-char). The former is now preferred over the latter.

It makes the code easier to understand.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: linuxppc-dev@lists.ozlabs.org
---
 drivers/tty/hvc/hvc_console.c | 4 +---
 drivers/tty/hvc/hvc_console.h | 2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 959fae54ca39..93b613e1f176 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -922,8 +922,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
 			return ERR_PTR(err);
 	}
 
-	hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
-			GFP_KERNEL);
+	hp = kzalloc(struct_size(hp, outbuf, outbuf_size), GFP_KERNEL);
 	if (!hp)
 		return ERR_PTR(-ENOMEM);
 
@@ -931,7 +930,6 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
 	hp->data = data;
 	hp->ops = ops;
 	hp->outbuf_size = outbuf_size;
-	hp->outbuf = &((char *)hp)[ALIGN(sizeof(*hp), sizeof(long))];
 
 	tty_port_init(&hp->port);
 	hp->port.ops = &hvc_port_ops;
diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
index 9668f821db01..b718714bf399 100644
--- a/drivers/tty/hvc/hvc_console.h
+++ b/drivers/tty/hvc/hvc_console.h
@@ -37,7 +37,6 @@ struct hvc_struct {
 	spinlock_t lock;
 	int index;
 	int do_wakeup;
-	char *outbuf;
 	int outbuf_size;
 	int n_outbuf;
 	uint32_t vtermno;
@@ -48,6 +47,7 @@ struct hvc_struct {
 	struct work_struct tty_resize;
 	struct list_head next;
 	unsigned long flags;
+	u8 outbuf[] __aligned(sizeof(long));
 };
 
 /* implemented by a low level driver */
-- 
2.42.1

[PATCH 12/17] tty: nozomi: remove unused debugging DUMP()

[Jiri Slaby (SUSE)]

DUMP()'s only use is commented out. Remove the macro completely along
with this unused use.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/nozomi.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/tty/nozomi.c b/drivers/tty/nozomi.c
index 02cd40147b3a..b247341bd12f 100644
--- a/drivers/tty/nozomi.c
+++ b/drivers/tty/nozomi.c
@@ -65,24 +65,8 @@ do {							\
 #define DBG3(args...) DBG_(0x04, ##args)
 #define DBG4(args...) DBG_(0x08, ##args)
 
-/* TODO: rewrite to optimize macros... */
-
 #define TMP_BUF_MAX 256
 
-#define DUMP(buf__, len__)						\
-	do {								\
-		char tbuf[TMP_BUF_MAX] = {0};				\
-		if (len__ > 1) {					\
-			u32 data_len = min_t(u32, len__, TMP_BUF_MAX);	\
-			strscpy(tbuf, buf__, data_len);			\
-			if (tbuf[data_len - 2] == '\r')			\
-				tbuf[data_len - 2] = 'r';		\
-			DBG1("SENDING: '%s' (%d+n)", tbuf, len__);	\
-		} else {						\
-			DBG1("SENDING: '%s' (%d)", tbuf, len__);	\
-		}							\
-	} while (0)
-
 /*    Defines */
 #define NOZOMI_NAME		"nozomi"
 #define NOZOMI_NAME_TTY		"nozomi_tty"
@@ -754,8 +738,6 @@ static int send_data(enum port_type index, struct nozomi *dc)
 		return 0;
 	}
 
-	/* DUMP(buf, size); */
-
 	/* Write length + data */
 	write_mem32(addr, (u32 *) &size, 4);
 	write_mem32(addr + 4, (u32 *) dc->send_buf, size);
-- 
2.42.1

[PATCH 13/17] tty: srmcons: use 'buf' directly in srmcons_do_write()

[Jiri Slaby (SUSE)]

There is no need to have a separate iterator ('cur') through 'buf' in
srmcons_do_write(). 'buf' can be used directly which simplifies the code
a bit.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
---
 arch/alpha/kernel/srmcons.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
index d6139dbae4ac..b68c5af083cd 100644
--- a/arch/alpha/kernel/srmcons.c
+++ b/arch/alpha/kernel/srmcons.c
@@ -94,24 +94,23 @@ srmcons_do_write(struct tty_port *port, const char *buf, int count)
 	static char str_cr[1] = "\r";
 	long c, remaining = count;
 	srmcons_result result;
-	char *cur;
 	int need_cr;
 
-	for (cur = (char *)buf; remaining > 0; ) {
+	while (remaining > 0) {
 		need_cr = 0;
 		/* 
 		 * Break it up into reasonable size chunks to allow a chance
 		 * for input to get in
 		 */
 		for (c = 0; c < min_t(long, 128L, remaining) && !need_cr; c++)
-			if (cur[c] == '\n')
+			if (buf[c] == '\n')
 				need_cr = 1;
 		
 		while (c > 0) {
-			result.as_long = callback_puts(0, cur, c);
+			result.as_long = callback_puts(0, buf, c);
 			c -= result.bits.c;
 			remaining -= result.bits.c;
-			cur += result.bits.c;
+			buf += result.bits.c;
 
 			/*
 			 * Check for pending input iff a tty port was provided
-- 
2.42.1

Re: [PATCH 13/17] tty: srmcons: use 'buf' directly in srmcons_do_write()

[Richard Henderson]

On 11/21/23 03:22, Jiri Slaby (SUSE) wrote:
> There is no need to have a separate iterator ('cur') through 'buf' in
> srmcons_do_write(). 'buf' can be used directly which simplifies the code
> a bit.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> Cc: Matt Turner <mattst88@gmail.com>
> Cc: linux-alpha@vger.kernel.org
> ---
>   arch/alpha/kernel/srmcons.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 14/17] tty: srmcons: use 'count' directly in srmcons_do_write()

[Jiri Slaby (SUSE)]

Similarly to 'buf' in the previous patch, there is no need to have a
separate counter ('remaining') in srmcons_do_write(). 'count' can be
used directly which simplifies the code a bit.

Note that the type of the current count ('c') is changed from 'long' to
'size_t' so that:
1) it is prepared for the upcoming change of 'count's type, and
2) is unsigned.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
---
 arch/alpha/kernel/srmcons.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
index b68c5af083cd..8025e2a882ed 100644
--- a/arch/alpha/kernel/srmcons.c
+++ b/arch/alpha/kernel/srmcons.c
@@ -92,24 +92,24 @@ static int
 srmcons_do_write(struct tty_port *port, const char *buf, int count)
 {
 	static char str_cr[1] = "\r";
-	long c, remaining = count;
+	size_t c;
 	srmcons_result result;
 	int need_cr;
 
-	while (remaining > 0) {
+	while (count > 0) {
 		need_cr = 0;
 		/* 
 		 * Break it up into reasonable size chunks to allow a chance
 		 * for input to get in
 		 */
-		for (c = 0; c < min_t(long, 128L, remaining) && !need_cr; c++)
+		for (c = 0; c < min_t(size_t, 128U, count) && !need_cr; c++)
 			if (buf[c] == '\n')
 				need_cr = 1;
 		
 		while (c > 0) {
 			result.as_long = callback_puts(0, buf, c);
 			c -= result.bits.c;
-			remaining -= result.bits.c;
+			count -= result.bits.c;
 			buf += result.bits.c;
 
 			/*
-- 
2.42.1

Re: [PATCH 14/17] tty: srmcons: use 'count' directly in srmcons_do_write()

[Ilpo Järvinen]

On Tue, 21 Nov 2023, Jiri Slaby (SUSE) wrote:

> Similarly to 'buf' in the previous patch, there is no need to have a
> separate counter ('remaining') in srmcons_do_write(). 'count' can be
> used directly which simplifies the code a bit.
> 
> Note that the type of the current count ('c') is changed from 'long' to
> 'size_t' so that:
> 1) it is prepared for the upcoming change of 'count's type, and
> 2) is unsigned.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> Cc: Matt Turner <mattst88@gmail.com>
> Cc: linux-alpha@vger.kernel.org
> ---
>  arch/alpha/kernel/srmcons.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
> index b68c5af083cd..8025e2a882ed 100644
> --- a/arch/alpha/kernel/srmcons.c
> +++ b/arch/alpha/kernel/srmcons.c
> @@ -92,24 +92,24 @@ static int
>  srmcons_do_write(struct tty_port *port, const char *buf, int count)
>  {
>  	static char str_cr[1] = "\r";
> -	long c, remaining = count;
> +	size_t c;
>  	srmcons_result result;
>  	int need_cr;
>  
> -	while (remaining > 0) {
> +	while (count > 0) {
>  		need_cr = 0;
>  		/* 
>  		 * Break it up into reasonable size chunks to allow a chance
>  		 * for input to get in
>  		 */
> -		for (c = 0; c < min_t(long, 128L, remaining) && !need_cr; c++)
> +		for (c = 0; c < min_t(size_t, 128U, count) && !need_cr; c++)
>  			if (buf[c] == '\n')
>  				need_cr = 1;
>  		
>  		while (c > 0) {
>  			result.as_long = callback_puts(0, buf, c);
>  			c -= result.bits.c;
> -			remaining -= result.bits.c;
> +			count -= result.bits.c;
>  			buf += result.bits.c;
>  
>  			/*
> 

The patches in the series are in pretty odd order and it was not told 
anywhere here that the return value is unused by the callers. I'd just 
reorder the patches.

-- 
 i.

Re: [PATCH 14/17] tty: srmcons: use 'count' directly in srmcons_do_write()

[Richard Henderson]

On 11/21/23 09:21, Ilpo Järvinen wrote:
> On Tue, 21 Nov 2023, Jiri Slaby (SUSE) wrote:
> 
>> Similarly to 'buf' in the previous patch, there is no need to have a
>> separate counter ('remaining') in srmcons_do_write(). 'count' can be
>> used directly which simplifies the code a bit.
>>
>> Note that the type of the current count ('c') is changed from 'long' to
>> 'size_t' so that:
>> 1) it is prepared for the upcoming change of 'count's type, and
>> 2) is unsigned.
>>
>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
>> Cc: Matt Turner <mattst88@gmail.com>
>> Cc: linux-alpha@vger.kernel.org
>> ---
>>   arch/alpha/kernel/srmcons.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
>> index b68c5af083cd..8025e2a882ed 100644
>> --- a/arch/alpha/kernel/srmcons.c
>> +++ b/arch/alpha/kernel/srmcons.c
>> @@ -92,24 +92,24 @@ static int
>>   srmcons_do_write(struct tty_port *port, const char *buf, int count)
>>   {
>>   	static char str_cr[1] = "\r";
>> -	long c, remaining = count;
>> +	size_t c;
>>   	srmcons_result result;
>>   	int need_cr;
>>   
>> -	while (remaining > 0) {
>> +	while (count > 0) {
>>   		need_cr = 0;
>>   		/*
>>   		 * Break it up into reasonable size chunks to allow a chance
>>   		 * for input to get in
>>   		 */
>> -		for (c = 0; c < min_t(long, 128L, remaining) && !need_cr; c++)
>> +		for (c = 0; c < min_t(size_t, 128U, count) && !need_cr; c++)
>>   			if (buf[c] == '\n')
>>   				need_cr = 1;
>>   		
>>   		while (c > 0) {
>>   			result.as_long = callback_puts(0, buf, c);
>>   			c -= result.bits.c;
>> -			remaining -= result.bits.c;
>> +			count -= result.bits.c;
>>   			buf += result.bits.c;
>>   
>>   			/*
>>
> 
> The patches in the series are in pretty odd order and it was not told
> anywhere here that the return value is unused by the callers. I'd just
> reorder the patches.
> 

Agreed, patch 15 needs to be before patch 14.  With that,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 14/17] tty: srmcons: use 'count' directly in srmcons_do_write()

[Jiri Slaby]

On 21. 11. 23, 18:48, Richard Henderson wrote:
> On 11/21/23 09:21, Ilpo Järvinen wrote:
>> On Tue, 21 Nov 2023, Jiri Slaby (SUSE) wrote:
>>
>>> Similarly to 'buf' in the previous patch, there is no need to have a
>>> separate counter ('remaining') in srmcons_do_write(). 'count' can be
>>> used directly which simplifies the code a bit.
>>>
>>> Note that the type of the current count ('c') is changed from 'long' to
>>> 'size_t' so that:
>>> 1) it is prepared for the upcoming change of 'count's type, and
>>> 2) is unsigned.
>>>
>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>>> Cc: Richard Henderson <richard.henderson@linaro.org>
>>> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
>>> Cc: Matt Turner <mattst88@gmail.com>
>>> Cc: linux-alpha@vger.kernel.org
>>> ---
>>>   arch/alpha/kernel/srmcons.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
>>> index b68c5af083cd..8025e2a882ed 100644
>>> --- a/arch/alpha/kernel/srmcons.c
>>> +++ b/arch/alpha/kernel/srmcons.c
>>> @@ -92,24 +92,24 @@ static int
>>>   srmcons_do_write(struct tty_port *port, const char *buf, int count)
>>>   {
>>>       static char str_cr[1] = "\r";
>>> -    long c, remaining = count;
>>> +    size_t c;
>>>       srmcons_result result;
>>>       int need_cr;
>>> -    while (remaining > 0) {
>>> +    while (count > 0) {
>>>           need_cr = 0;
>>>           /*
>>>            * Break it up into reasonable size chunks to allow a chance
>>>            * for input to get in
>>>            */
>>> -        for (c = 0; c < min_t(long, 128L, remaining) && !need_cr; c++)
>>> +        for (c = 0; c < min_t(size_t, 128U, count) && !need_cr; c++)
>>>               if (buf[c] == '\n')
>>>                   need_cr = 1;
>>>
>>>           while (c > 0) {
>>>               result.as_long = callback_puts(0, buf, c);
>>>               c -= result.bits.c;
>>> -            remaining -= result.bits.c;
>>> +            count -= result.bits.c;
>>>               buf += result.bits.c;
>>>               /*
>>>
>>
>> The patches in the series are in pretty odd order and it was not told
>> anywhere here that the return value is unused by the callers. I'd just
>> reorder the patches.
>>
> 
> Agreed, patch 15 needs to be before patch 14.  With that,

Ah, sure, I reordered the three to have buf and count changes close to 
each other, but didn't realize this.

thanks,
-- 
js
suse labs

[PATCH 15/17] tty: srmcons: make srmcons_do_write() return void

[Jiri Slaby (SUSE)]

The return value of srmcons_do_write() is ignored as all characters are
pushed. So make srmcons_do_write() to return void.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
---
 arch/alpha/kernel/srmcons.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
index 8025e2a882ed..32bc098de7da 100644
--- a/arch/alpha/kernel/srmcons.c
+++ b/arch/alpha/kernel/srmcons.c
@@ -88,7 +88,7 @@ srmcons_receive_chars(struct timer_list *t)
 }
 
 /* called with callback_lock held */
-static int
+static void
 srmcons_do_write(struct tty_port *port, const char *buf, int count)
 {
 	static char str_cr[1] = "\r";
@@ -125,7 +125,6 @@ srmcons_do_write(struct tty_port *port, const char *buf, int count)
 				need_cr = 0;
 		}
 	}
-	return count;
 }
 
 static ssize_t
-- 
2.42.1

Re: [PATCH 15/17] tty: srmcons: make srmcons_do_write() return void

[Richard Henderson]

On 11/21/23 03:22, Jiri Slaby (SUSE) wrote:
> The return value of srmcons_do_write() is ignored as all characters are
> pushed. So make srmcons_do_write() to return void.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> Cc: Matt Turner <mattst88@gmail.com>
> Cc: linux-alpha@vger.kernel.org
> ---
>   arch/alpha/kernel/srmcons.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
> index 8025e2a882ed..32bc098de7da 100644
> --- a/arch/alpha/kernel/srmcons.c
> +++ b/arch/alpha/kernel/srmcons.c
> @@ -88,7 +88,7 @@ srmcons_receive_chars(struct timer_list *t)
>   }
>   
>   /* called with callback_lock held */
> -static int
> +static void
>   srmcons_do_write(struct tty_port *port, const char *buf, int count)
>   {
>   	static char str_cr[1] = "\r";
> @@ -125,7 +125,6 @@ srmcons_do_write(struct tty_port *port, const char *buf, int count)
>   				need_cr = 0;
>   		}
>   	}
> -	return count;
>   }
>   
>   static ssize_t

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 16/17] tty: srmcons: switch need_cr to bool

[Jiri Slaby (SUSE)]

'need_cr' is a flag, so type it properly to be a 'bool'. Move the
declaration into the loop too. That ensures the variable is initialized
properly even if the code was moved somehow.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
---
 arch/alpha/kernel/srmcons.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
index 32bc098de7da..c6b821afbfd3 100644
--- a/arch/alpha/kernel/srmcons.c
+++ b/arch/alpha/kernel/srmcons.c
@@ -94,17 +94,16 @@ srmcons_do_write(struct tty_port *port, const char *buf, int count)
 	static char str_cr[1] = "\r";
 	size_t c;
 	srmcons_result result;
-	int need_cr;
 
 	while (count > 0) {
-		need_cr = 0;
+		bool need_cr = false;
 		/* 
 		 * Break it up into reasonable size chunks to allow a chance
 		 * for input to get in
 		 */
 		for (c = 0; c < min_t(size_t, 128U, count) && !need_cr; c++)
 			if (buf[c] == '\n')
-				need_cr = 1;
+				need_cr = true;
 		
 		while (c > 0) {
 			result.as_long = callback_puts(0, buf, c);
@@ -122,7 +121,7 @@ srmcons_do_write(struct tty_port *port, const char *buf, int count)
 		while (need_cr) {
 			result.as_long = callback_puts(0, str_cr, 1);
 			if (result.bits.c > 0)
-				need_cr = 0;
+				need_cr = false;
 		}
 	}
 }
-- 
2.42.1

Re: [PATCH 16/17] tty: srmcons: switch need_cr to bool

[Richard Henderson]

On 11/21/23 03:22, Jiri Slaby (SUSE) wrote:
> 'need_cr' is a flag, so type it properly to be a 'bool'. Move the
> declaration into the loop too. That ensures the variable is initialized
> properly even if the code was moved somehow.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> Cc: Matt Turner <mattst88@gmail.com>
> Cc: linux-alpha@vger.kernel.org
> ---
>   arch/alpha/kernel/srmcons.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 17/17] tty: srmcons: make 'str_cr' const and non-array

[Jiri Slaby (SUSE)]

'str_cr' contains a single character: \n. There is no need to declare it
as array. Declare it as a variable, make it const and pass a pointer to
it to callback_puts().

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
---
 arch/alpha/kernel/srmcons.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
index c6b821afbfd3..a6cff61706b5 100644
--- a/arch/alpha/kernel/srmcons.c
+++ b/arch/alpha/kernel/srmcons.c
@@ -91,7 +91,7 @@ srmcons_receive_chars(struct timer_list *t)
 static void
 srmcons_do_write(struct tty_port *port, const char *buf, int count)
 {
-	static char str_cr[1] = "\r";
+	static const char str_cr = '\r';
 	size_t c;
 	srmcons_result result;
 
@@ -119,7 +119,7 @@ srmcons_do_write(struct tty_port *port, const char *buf, int count)
 		}
 
 		while (need_cr) {
-			result.as_long = callback_puts(0, str_cr, 1);
+			result.as_long = callback_puts(0, &str_cr, 1);
 			if (result.bits.c > 0)
 				need_cr = false;
 		}
-- 
2.42.1

Re: [PATCH 17/17] tty: srmcons: make 'str_cr' const and non-array

[Ilpo Järvinen]

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

On Tue, 21 Nov 2023, Jiri Slaby (SUSE) wrote:

> 'str_cr' contains a single character: \n. There is no need to declare it

Aren't \r and \n different characters?

> -	static char str_cr[1] = "\r";
> +	static const char str_cr = '\r';

Thanks for making these cleanups.

I've reviewed all the patches in this series, so if I didn't comment a 
patch or when you address my remarks, feel free to add:

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

-- 
 i.

Re: [PATCH 17/17] tty: srmcons: make 'str_cr' const and non-array

[Jiri Slaby]

On 21. 11. 23, 16:28, Ilpo Järvinen wrote:
> On Tue, 21 Nov 2023, Jiri Slaby (SUSE) wrote:
> 
>> 'str_cr' contains a single character: \n. There is no need to declare it
> 
> Aren't \r and \n different characters?

Definitely, this is a thinko -- I didn't remember properly what it 
contains when writing the log. Fixed.

> 
>> -	static char str_cr[1] = "\r";
>> +	static const char str_cr = '\r';
> 
> Thanks for making these cleanups.
> 
> I've reviewed all the patches in this series, so if I didn't comment a
> patch or when you address my remarks, feel free to add:

thanks,
-- 
js
suse labs

Re: [PATCH 17/17] tty: srmcons: make 'str_cr' const and non-array

[Richard Henderson]

On 11/21/23 03:22, Jiri Slaby (SUSE) wrote:
> 'str_cr' contains a single character: \n. There is no need to declare it

\r

> as array. Declare it as a variable, make it const and pass a pointer to
> it to callback_puts().
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> Cc: Matt Turner <mattst88@gmail.com>
> Cc: linux-alpha@vger.kernel.org
> ---
>   arch/alpha/kernel/srmcons.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
> index c6b821afbfd3..a6cff61706b5 100644
> --- a/arch/alpha/kernel/srmcons.c
> +++ b/arch/alpha/kernel/srmcons.c
> @@ -91,7 +91,7 @@ srmcons_receive_chars(struct timer_list *t)
>   static void
>   srmcons_do_write(struct tty_port *port, const char *buf, int count)
>   {
> -	static char str_cr[1] = "\r";
> +	static const char str_cr = '\r';

An array of one element is fine -- what's wrong with that?
Adding const is an improvement though.


r~

Re: [PATCH 17/17] tty: srmcons: make 'str_cr' const and non-array

[Richard Henderson]

On 11/21/23 03:22, Jiri Slaby (SUSE) wrote:
> 'str_cr' contains a single character: \n. There is no need to declare it
> as array. Declare it as a variable, make it const and pass a pointer to
> it to callback_puts().
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> Cc: Matt Turner <mattst88@gmail.com>
> Cc: linux-alpha@vger.kernel.org
> ---
>   arch/alpha/kernel/srmcons.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
> index c6b821afbfd3..a6cff61706b5 100644
> --- a/arch/alpha/kernel/srmcons.c
> +++ b/arch/alpha/kernel/srmcons.c
> @@ -91,7 +91,7 @@ srmcons_receive_chars(struct timer_list *t)
>   static void
>   srmcons_do_write(struct tty_port *port, const char *buf, int count)
>   {
> -	static char str_cr[1] = "\r";
> +	static const char str_cr = '\r';

Best to remove this entirely...

>   	size_t c;
>   	srmcons_result result;
>   
> @@ -119,7 +119,7 @@ srmcons_do_write(struct tty_port *port, const char *buf, int count)
>   		}
>   
>   		while (need_cr) {
> -			result.as_long = callback_puts(0, str_cr, 1);
> +			result.as_long = callback_puts(0, &str_cr, 1);

... and simply use "\r" here.

Logically it adds one '\0' of const data, but it is virtually certain that even more bytes 
of padding are present anyway.  As a string literal it will be placed into .rodata.str1.1 
and packed with other strings.


r~

Re: [PATCH 00/17] tty: small cleanups and fixes

[Greg KH]

On Tue, Nov 21, 2023 at 10:22:41AM +0100, Jiri Slaby (SUSE) wrote:
> This is a series to fix/clean up some obvious issues I revealed during
> u8+size_t conversions (to be posted later).

I applied most of these except the last few, as I think you were going
to reorder them, right?

thanks,

greg k-h

Re: [PATCH 00/17] tty: small cleanups and fixes

[Jiri Slaby]

On 23. 11. 23, 21:19, Greg KH wrote:
> On Tue, Nov 21, 2023 at 10:22:41AM +0100, Jiri Slaby (SUSE) wrote:
>> This is a series to fix/clean up some obvious issues I revealed during
>> u8+size_t conversions (to be posted later).
> 
> I applied most of these except the last few, as I think you were going
> to reorder them, right?

Yes, great. I will rebase and see/resend what is missing.

thanks,
-- 
js
suse labs