Re: [PATCH 00/36] tty: type unifications -- part I.

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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

On Thu, 10 Aug 2023, Jiri Slaby (SUSE) wrote:

> Currently, the tty layer ops and functions use various types for same
> things:
> * characters and flags: unsigned char, char are used on a random basis,
> * counts: int, unsigned int, size_t are used, again more-or-less
>   randomly.
> 
> This makes it rather hard to remember where each type is required and it
> also makes the code harder to follow. Also the code has to do min_t() on
> many places simply because the variables hold the same kind of data, but
> of different type.
> 
> This is the first part of the series to unify the types:
> * make characters and flags 'u8'. This is what the hardware expects and
>   what feeds the tty layer with. Since we compile with -funsigned-char,
>   char and unsigned char are the same types on all platforms. So there
>   is no actual change in type.
> * make sizes/counts 'size_t'. This is what comes from the VFS layer and
>   some tty functions already operate on this. So instead of using
>   "shorter" (in term of bytes on 64bit) unsigned int, stick to size_t
>   and promote it to most places.
> 
> More cleanup and spreading will be done in tty_buffer, n_tty, and
> likely other places later.
>
> Patches 1-8 are cleanups only. The rest (the real switch) depends on
> those. 

Yeah, very much needed change and step into the right direction!

It's a bit tedious to review all this and comment a particular subchange 
but e.g. n_tty_receive_buf_common() still seems to still have int count 
which I think fall into the same call chain about size/count (probably 
most related change is #15). Note though that it also has room which I 
think can actually become negative so it might not be as straightforward 
search and replace like some other parts are.

At least for patches #1-#14,

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

> Jiri Slaby (SUSE) (36):
>   tty: xtensa/iss: drop unneeded tty_operations hooks
>   tty: ldisc: document that ldops are optional
>   tty: remove dummy tty_ldisc_ops::poll() implementations
>   tty: n_null: remove optional ldops
>   tty: change tty_write_lock()'s ndelay parameter to bool
>   tty: tty_port: rename 'disc' to 'ld'
>   tty: drop tty_debug_wait_until_sent()
>   tty: make tty_change_softcar() more understandable
>   tty: make tty_port_client_operations operate with u8
>   tty: make counts in tty_port_client_operations hooks size_t
>   tty: switch receive_buf() counts to size_t
>   tty: switch count in tty_ldisc_receive_buf() to size_t
>   tty: can327: unify error paths in can327_ldisc_rx()
>   tty: can327, move overflow test inside can327_ldisc_rx()'s loop
>   tty: make tty_ldisc_ops::*buf*() hooks operate on size_t
>   tty: use u8 for chars
>   tty: use u8 for flags
>   misc: ti-st: make st_recv() conforming to tty_ldisc_ops::receive_buf()
>   tty: make char_buf_ptr()/flag_buf_ptr()'s offset unsigned
>   tty: tty_buffer: make all offsets unsigned
>   tty: don't pass write() to do_tty_write()
>   tty: rename and de-inline do_tty_write()
>   tty: use min() in iterate_tty_write()
>   tty: use ssize_t for iterate_tty_read() returned type
>   tty: switch size and count types in iterate_tty_read() to size_t
>   tty: use min() for size computation in iterate_tty_read()
>   tty: propagate u8 data to tty_operations::write()
>   tty: propagate u8 data to tty_operations::put_char()
>   tty: make tty_operations::write()'s count size_t
>   tty: audit: unify to u8
>   tty: ldops: unify to u8
>   tty: hvc: convert counts to size_t
>   tty: vcc: convert counts to size_t
>   tty: gdm724x: convert counts to size_t
>   tty: hso: simplify hso_serial_write()
>   tty: rfcomm: convert counts to size_t
> 
>  arch/alpha/kernel/srmcons.c               |  5 +-
>  arch/m68k/emu/nfcon.c                     |  8 +--
>  arch/sparc/include/asm/vio.h              |  2 +-
>  arch/um/drivers/line.c                    |  2 +-
>  arch/um/drivers/line.h                    |  3 +-
>  arch/xtensa/platforms/iss/console.c       | 27 +--------
>  drivers/accessibility/speakup/spk_ttyio.c |  5 +-
>  drivers/bluetooth/hci_ldisc.c             | 15 ++---
>  drivers/char/ttyprintk.c                  |  5 +-
>  drivers/input/serio/serport.c             |  8 +--
>  drivers/ipack/devices/ipoctal.c           |  7 +--
>  drivers/isdn/capi/capi.c                  |  8 +--
>  drivers/misc/bcm-vk/bcm_vk_tty.c          |  5 +-
>  drivers/misc/ti-st/st_core.c              | 11 ++--
>  drivers/misc/ti-st/st_kim.c               |  6 +-
>  drivers/mmc/core/sdio_uart.c              |  4 +-
>  drivers/net/caif/caif_serial.c            |  2 +-
>  drivers/net/can/can327.c                  | 39 ++++++-------
>  drivers/net/can/slcan/slcan-core.c        |  5 +-
>  drivers/net/hamradio/6pack.c              |  4 +-
>  drivers/net/hamradio/mkiss.c              |  4 +-
>  drivers/net/mctp/mctp-serial.c            |  5 +-
>  drivers/net/ppp/ppp_async.c               | 26 +++------
>  drivers/net/ppp/ppp_synctty.c             | 26 +++------
>  drivers/net/slip/slip.c                   |  4 +-
>  drivers/net/usb/hso.c                     | 20 +++----
>  drivers/s390/char/con3215.c               |  6 +-
>  drivers/s390/char/con3270.c               |  6 +-
>  drivers/s390/char/sclp_tty.c              | 10 ++--
>  drivers/s390/char/sclp_vt220.c            |  6 +-
>  drivers/staging/gdm724x/gdm_tty.c         | 14 ++---
>  drivers/staging/greybus/uart.c            |  3 +-
>  drivers/tty/amiserial.c                   |  4 +-
>  drivers/tty/ehv_bytechan.c                |  4 +-
>  drivers/tty/goldfish.c                    |  7 +--
>  drivers/tty/hvc/hvc_console.c             |  4 +-
>  drivers/tty/hvc/hvcs.c                    | 10 ++--
>  drivers/tty/hvc/hvsi.c                    | 14 ++---
>  drivers/tty/ipwireless/hardware.c         |  2 +-
>  drivers/tty/ipwireless/tty.c              |  4 +-
>  drivers/tty/mips_ejtag_fdc.c              |  6 +-
>  drivers/tty/moxa.c                        |  8 +--
>  drivers/tty/mxser.c                       |  4 +-
>  drivers/tty/n_gsm.c                       | 14 ++---
>  drivers/tty/n_hdlc.c                      | 12 ++--
>  drivers/tty/n_null.c                      | 25 +--------
>  drivers/tty/n_tty.c                       | 59 ++++++++++----------
>  drivers/tty/nozomi.c                      |  6 +-
>  drivers/tty/pty.c                         |  2 +-
>  drivers/tty/rpmsg_tty.c                   |  5 +-
>  drivers/tty/serdev/serdev-ttyport.c       |  4 +-
>  drivers/tty/serial/kgdb_nmi.c             |  3 +-
>  drivers/tty/serial/serial_core.c          |  5 +-
>  drivers/tty/synclink_gt.c                 | 13 ++---
>  drivers/tty/tty.h                         |  8 +--
>  drivers/tty/tty_audit.c                   |  6 +-
>  drivers/tty/tty_buffer.c                  | 35 ++++++------
>  drivers/tty/tty_io.c                      | 46 +++++++---------
>  drivers/tty/tty_ioctl.c                   | 18 ++----
>  drivers/tty/tty_port.c                    | 34 ++++++------
>  drivers/tty/ttynull.c                     |  4 +-
>  drivers/tty/vcc.c                         | 18 +++---
>  drivers/tty/vt/selection.c                |  2 +-
>  drivers/tty/vt/vt.c                       |  6 +-
>  drivers/usb/class/cdc-acm.c               |  8 +--
>  drivers/usb/gadget/function/u_serial.c    |  6 +-
>  drivers/usb/host/xhci-dbgtty.c            |  7 +--
>  drivers/usb/serial/usb-serial.c           |  5 +-
>  include/linux/ti_wilink_st.h              |  2 +-
>  include/linux/tty_buffer.h                | 18 +++---
>  include/linux/tty_driver.h                |  9 ++-
>  include/linux/tty_flip.h                  | 22 ++++----
>  include/linux/tty_ldisc.h                 | 67 +++++++++++++++--------
>  include/linux/tty_port.h                  |  7 ++-
>  net/bluetooth/rfcomm/tty.c                |  9 +--
>  net/nfc/nci/uart.c                        | 15 ++---
>  sound/soc/codecs/cx20442.c                |  4 +-
>  sound/soc/ti/ams-delta.c                  |  2 +-
>  78 files changed, 381 insertions(+), 493 deletions(-)
> 
> 

-- 
 i.