| Message ID | 20230810091510.13006-1-jirislaby@kernel.org |
|---|---|
| Headers | show |
| Series | tty: type unifications -- part I. | expand |
On Thu, 10 Aug 2023, Jiri Slaby (SUSE) wrote: > This makes all those 'unsigned char's an explicit 'u8'. This is part of > the continuing unification of chars and flags to be consistent u8. > > This approaches tty_port_default_receive_buf(). Flags to be next. > > Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org> > Cc: William Hubbs <w.d.hubbs@gmail.com> > Cc: Chris Brannon <chris@the-brannons.com> > Cc: Kirk Reiser <kirk@reisers.ca> > Cc: Samuel Thibault <samuel.thibault@ens-lyon.org> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Max Staudt <max@enpas.org> > Cc: Wolfgang Grandegger <wg@grandegger.com> > Cc: Marc Kleine-Budde <mkl@pengutronix.de> > 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: Dario Binacchi <dario.binacchi@amarulasolutions.com> > Cc: Andreas Koensgen <ajk@comnets.uni-bremen.de> > Cc: Jeremy Kerr <jk@codeconstruct.com.au> > Cc: Matt Johnston <matt@codeconstruct.com.au> > Cc: Liam Girdwood <lgirdwood@gmail.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: Jaroslav Kysela <perex@perex.cz> > Cc: Takashi Iwai <tiwai@suse.com> > Cc: Peter Ujfalusi <peter.ujfalusi@gmail.com> > --- > drivers/accessibility/speakup/spk_ttyio.c | 5 ++-- > drivers/input/serio/serport.c | 5 ++-- > drivers/misc/ti-st/st_core.c | 2 +- > drivers/net/can/can327.c | 2 +- > drivers/net/can/slcan/slcan-core.c | 5 ++-- > drivers/net/hamradio/6pack.c | 4 ++-- > drivers/net/hamradio/mkiss.c | 2 +- > drivers/net/mctp/mctp-serial.c | 3 +-- > drivers/net/ppp/ppp_async.c | 8 +++---- > drivers/net/ppp/ppp_synctty.c | 11 ++++----- > drivers/net/slip/slip.c | 2 +- > drivers/tty/n_gsm.c | 2 +- > drivers/tty/n_hdlc.c | 2 +- > drivers/tty/n_tty.c | 28 +++++++++++------------ > drivers/tty/tty.h | 2 +- > drivers/tty/tty_buffer.c | 21 ++++++++--------- > include/linux/tty_buffer.h | 4 ++-- > include/linux/tty_flip.h | 22 ++++++++---------- > include/linux/tty_ldisc.h | 18 +++++++-------- > sound/soc/codecs/cx20442.c | 4 ++-- > sound/soc/ti/ams-delta.c | 2 +- > 21 files changed, 73 insertions(+), 81 deletions(-) > diff --git a/include/linux/tty_buffer.h b/include/linux/tty_buffer.h > index 6ceb2789e6c8..6f2966b15093 100644 > --- a/include/linux/tty_buffer.h > +++ b/include/linux/tty_buffer.h > @@ -22,9 +22,9 @@ struct tty_buffer { > unsigned long data[]; > }; > > -static inline unsigned char *char_buf_ptr(struct tty_buffer *b, int ofs) > +static inline u8 *char_buf_ptr(struct tty_buffer *b, int ofs) > { > - return ((unsigned char *)b->data) + ofs; > + return ((u8 *)b->data) + ofs; > } Any particular reason why b->data is left unsigned long?
Thank you for simplifying this!
Reviewed-by: Max Staudt <max@enpas.org>
In case you're re-sending this series, may I ask for one empty line between the final "return;" and the new label "uart_failure:"?
Acked-by: Max Staudt <max@enpas.org>
Acked-by: Max Staudt <max@enpas.org>
Acked-by: Max Staudt <max@enpas.org>
On 11. 08. 23, 12:28, Ilpo Järvinen wrote: > On Thu, 10 Aug 2023, Jiri Slaby (SUSE) wrote: > >> This makes all those 'unsigned char's an explicit 'u8'. This is part of >> the continuing unification of chars and flags to be consistent u8. >> >> This approaches tty_port_default_receive_buf(). Flags to be next. ...>> diff --git a/include/linux/tty_buffer.h b/include/linux/tty_buffer.h >> index 6ceb2789e6c8..6f2966b15093 100644 >> --- a/include/linux/tty_buffer.h >> +++ b/include/linux/tty_buffer.h >> @@ -22,9 +22,9 @@ struct tty_buffer { >> unsigned long data[]; >> }; >> >> -static inline unsigned char *char_buf_ptr(struct tty_buffer *b, int ofs) >> +static inline u8 *char_buf_ptr(struct tty_buffer *b, int ofs) >> { >> - return ((unsigned char *)b->data) + ofs; >> + return ((u8 *)b->data) + ofs; >> } > > Any particular reason why b->data is left unsigned long? Nope :): https://git.kernel.org/pub/scm/linux/kernel/git/jirislaby/linux.git/commit/?h=devel&id=57c9d0cbe1ad69957a2092b66dc31dc1da4d1d4b I am dumb not mentioning it in 00/36, see my e-mail there. thanks,
On 11. 08. 23, 12:26, Ilpo Järvinen wrote: > 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. tl;dr https://git.kernel.org/pub/scm/linux/kernel/git/jirislaby/linux.git/commit/?h=devel&id=9abb593df5a9b9b72d13438f1862ca67936f6b66 ---- Yes, sorry, my bad -- I forgot to elaborate on why this is "part I." and what is going to be part II., III., ... So yeah, I have more in my queue which is growing a lot. I had to cut it at some point as I was losing myself in all the changes already. So I flushed this "part I.". It is only a minimalistic change in the core and necessary changes in drivers' hooks. Parts II. and on will spread this more, of course. Ideally, to every single loop in every driver ;) (in long-term). I still have a bunch of changes for tty_buffer and n_tty in my queue. As soon as I rebase on the today's -next which is already supposed to contain this part I., I will send part II. with these changes. I could have merged those II. changes to some earlier I. patches. At first, I actually did try, but the patches were growing with more and more dependencies, so I stopped this approach. Instead, I separated the changes per the core/ldisc/drivers. The parts are self-contained, despite it might look like the changes are incomplete (i.e. not everything is changed everywhere). After all, I wanted to avoid one hundred+ patches series. thanks,
On Thu, Aug 10, 2023 at 11:16 AM Jiri Slaby (SUSE) <jirislaby@kernel.org> wrote: > Data are now typed as u8. Propagate this change to > tty_operations::put_char(). > > Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org> > arch/m68k/emu/nfcon.c | 4 ++-- > drivers/tty/amiserial.c | 2 +- Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> # m68k Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> # m68k Gr{oetje,eeting}s, Geert
On Mon, 14 Aug 2023, Jiri Slaby wrote: > On 11. 08. 23, 12:26, Ilpo Järvinen wrote: > > 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. > > tl;dr > https://git.kernel.org/pub/scm/linux/kernel/git/jirislaby/linux.git/commit/?h=devel&id=9abb593df5a9b9b72d13438f1862ca67936f6b66 > > ---- > > Yes, sorry, my bad -- I forgot to elaborate on why this is "part I." and what > is going to be part II., III., ... > > So yeah, I have more in my queue which is growing a lot. I had to cut it at > some point as I was losing myself in all the changes already. So I flushed > this "part I.". It is only a minimalistic change in the core and necessary > changes in drivers' hooks. Parts II. and on will spread this more, of course. > Ideally, to every single loop in every driver ;) (in long-term). > > I still have a bunch of changes for tty_buffer and n_tty in my queue. As soon > as I rebase on the today's -next which is already supposed to contain this > part I., I will send part II. with these changes. I could have merged those > II. changes to some earlier I. patches. At first, I actually did try, but the > patches were growing with more and more dependencies, so I stopped this > approach. Instead, I separated the changes per the core/ldisc/drivers. The > parts are self-contained, despite it might look like the changes are > incomplete (i.e. not everything is changed everywhere). After all, I wanted to > avoid one hundred+ patches series. Yeah, right. Very much understandable. I realized you probably had more patches somewhere due to "Part I" designation but I couldn't check so I just noted the things that I came up during the review.
On Thu, Aug 10, 2023 at 11:39 AM Jiri Slaby (SUSE) <jirislaby@kernel.org> wrote: > Unify with the rest of the code. Use size_t for counts and ssize_t for > retval. > > Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org> > arch/m68k/emu/nfcon.c | 3 ++- > drivers/tty/amiserial.c | 2 +- Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> # m68k Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> # m68k Gr{oetje,eeting}s, Geert
On Thu, Aug 10, 2023 at 11:15:08AM +0200, Jiri Slaby (SUSE) wrote: > Unify the type of tty_operations::write() counters with the 'count' > parameter. I.e. use size_t for them. > > This includes changing constants to UL to keep min() and avoid min_t(). This patch appears to cause a warning/error on 32-bit architectures now due to this part of the change, as size_t is 'unsigned int' there: In file included from include/linux/kernel.h:27, from drivers/staging/gdm724x/gdm_tty.c:6: drivers/staging/gdm724x/gdm_tty.c: In function 'gdm_tty_write': include/linux/minmax.h:21:35: error: comparison of distinct pointer types lacks a cast [-Werror] 21 | (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1))) | ^~ include/linux/minmax.h:27:18: note: in expansion of macro '__typecheck' 27 | (__typecheck(x, y) && __no_side_effects(x, y)) | ^~~~~~~~~~~ include/linux/minmax.h:37:31: note: in expansion of macro '__safe_cmp' 37 | __builtin_choose_expr(__safe_cmp(x, y), \ | ^~~~~~~~~~ include/linux/minmax.h:68:25: note: in expansion of macro '__careful_cmp' 68 | #define min(x, y) __careful_cmp(x, y, <) | ^~~~~~~~~~~~~ drivers/staging/gdm724x/gdm_tty.c:162:38: note: in expansion of macro 'min' 162 | size_t sending_len = min(MUX_TX_MAX_SIZE, remain); | ^~~ cc1: all warnings being treated as errors > Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org> > Cc: linux-staging@lists.linux.dev > --- > drivers/staging/gdm724x/gdm_tty.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/gdm724x/gdm_tty.c b/drivers/staging/gdm724x/gdm_tty.c > index b31f2afb0286..cbaaa8fa7474 100644 > --- a/drivers/staging/gdm724x/gdm_tty.c > +++ b/drivers/staging/gdm724x/gdm_tty.c > @@ -17,9 +17,9 @@ > #define GDM_TTY_MAJOR 0 > #define GDM_TTY_MINOR 32 > > -#define WRITE_SIZE 2048 > +#define WRITE_SIZE 2048UL > > -#define MUX_TX_MAX_SIZE 2048 > +#define MUX_TX_MAX_SIZE 2048UL > > static inline bool gdm_tty_ready(struct gdm *gdm) > { > @@ -152,9 +152,8 @@ static void gdm_tty_send_complete(void *arg) > static ssize_t gdm_tty_write(struct tty_struct *tty, const u8 *buf, size_t len) > { > struct gdm *gdm = tty->driver_data; > - int remain = len; > - int sent_len = 0; > - int sending_len = 0; > + size_t remain = len; > + size_t sent_len = 0; > > if (!gdm_tty_ready(gdm)) > return -ENODEV; > @@ -163,7 +162,7 @@ static ssize_t gdm_tty_write(struct tty_struct *tty, const u8 *buf, size_t len) > return 0; > > while (1) { > - sending_len = min(MUX_TX_MAX_SIZE, remain); > + size_t sending_len = min(MUX_TX_MAX_SIZE, remain); > gdm->tty_dev->send_func(gdm->tty_dev->priv_dev, > (void *)(buf + sent_len), > sending_len, > -- > 2.41.0 >
On 15. 08. 23, 19:22, Nathan Chancellor wrote: > On Thu, Aug 10, 2023 at 11:15:08AM +0200, Jiri Slaby (SUSE) wrote: >> Unify the type of tty_operations::write() counters with the 'count' >> parameter. I.e. use size_t for them. >> >> This includes changing constants to UL to keep min() and avoid min_t(). > > This patch appears to cause a warning/error on 32-bit architectures now > due to this part of the change, as size_t is 'unsigned int' there: Right, this is my brain fart thinking ulong is the same as size_t everywhere. No, size_t is uint on 32bit. I will fix this -- kernel build bot seems to be slow -- it didn't find the issue out in my queue, nor in tty-testing. thanks,
From: Jiri Slaby > Sent: Wednesday, August 16, 2023 7:47 AM > > On 15. 08. 23, 19:22, Nathan Chancellor wrote: > > On Thu, Aug 10, 2023 at 11:15:08AM +0200, Jiri Slaby (SUSE) wrote: > >> Unify the type of tty_operations::write() counters with the 'count' > >> parameter. I.e. use size_t for them. > >> > >> This includes changing constants to UL to keep min() and avoid min_t(). > > > > This patch appears to cause a warning/error on 32-bit architectures now > > due to this part of the change, as size_t is 'unsigned int' there: > > Right, this is my brain fart thinking ulong is the same as size_t > everywhere. No, size_t is uint on 32bit. > > I will fix this -- kernel build bot seems to be slow -- it didn't find > the issue out in my queue, nor in tty-testing. 'Vote up' my patches to minmax.h that make this all work. Then it won't care provided both values have the same signedness. (or, with patch 5, are non-negative 31bit compile time constants.) Pretty much the only other patch is casting the constants to (size_t). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 16. 08. 23, 10:40, David Laight wrote: > From: Jiri Slaby >> Sent: Wednesday, August 16, 2023 7:47 AM >> >> On 15. 08. 23, 19:22, Nathan Chancellor wrote: >>> On Thu, Aug 10, 2023 at 11:15:08AM +0200, Jiri Slaby (SUSE) wrote: >>>> Unify the type of tty_operations::write() counters with the 'count' >>>> parameter. I.e. use size_t for them. >>>> >>>> This includes changing constants to UL to keep min() and avoid min_t(). >>> >>> This patch appears to cause a warning/error on 32-bit architectures now >>> due to this part of the change, as size_t is 'unsigned int' there: >> >> Right, this is my brain fart thinking ulong is the same as size_t >> everywhere. No, size_t is uint on 32bit. >> >> I will fix this -- kernel build bot seems to be slow -- it didn't find >> the issue out in my queue, nor in tty-testing. > > 'Vote up' my patches to minmax.h that make this all work. > Then it won't care provided both values have the same signedness. > (or, with patch 5, are non-negative 31bit compile time constants.) Oh yeah, that [1] looks great. Why should one care in min(4096, sizeof()) after all… So what's the current status of those? [1] https://lore.kernel.org/all/b4ce9dad748e489f9314a2dc95615033@AcuMS.aculab.com/ thanks,
From: Jiri Slaby > Sent: Wednesday, August 16, 2023 9:59 AM ... > > 'Vote up' my patches to minmax.h that make this all work. > > Then it won't care provided both values have the same signedness. > > (or, with patch 5, are non-negative 31bit compile time constants.) > > Oh yeah, that [1] looks great. Why should one care in min(4096, > sizeof()) after all… > > So what's the current status of those? Waiting... :-( The only comment is from Linus who really doesn't like the idea that min(signed_var, 4u) should be the same as min(signed_var, 4). I think he is ok with min(unsigned_var, 4) though. The min_t(u16,...) I quoted from the console buffer code is a real bug that was identified by someone else last week. Really min_t() is just an accident waiting to happen. David > > [1] > https://lore.kernel.org/all/b4ce9dad748e489f9314a2dc95615033@AcuMS.aculab.com/ > > thanks, > -- > js > suse labs - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, Aug 10, 2023 at 11:15:02AM +0200, Jiri Slaby (SUSE) wrote: ... > drivers/s390/char/con3215.c | 2 +- > drivers/s390/char/con3270.c | 2 +- > drivers/s390/char/sclp_tty.c | 6 +++--- > drivers/s390/char/sclp_vt220.c | 2 +- ... > diff --git a/drivers/s390/char/con3215.c b/drivers/s390/char/con3215.c > index 16b6f430dfd3..8bbce6a4d7f5 100644 > --- a/drivers/s390/char/con3215.c > +++ b/drivers/s390/char/con3215.c > @@ -1030,7 +1030,7 @@ static int tty3215_write(struct tty_struct *tty, const u8 *buf, int count) > /* > * Put character routine for 3215 ttys > */ > -static int tty3215_put_char(struct tty_struct *tty, unsigned char ch) > +static int tty3215_put_char(struct tty_struct *tty, u8 ch) > { > struct raw3215_info *raw = tty->driver_data; > > diff --git a/drivers/s390/char/con3270.c b/drivers/s390/char/con3270.c > index 123524bff734..6374555a0937 100644 > --- a/drivers/s390/char/con3270.c > +++ b/drivers/s390/char/con3270.c > @@ -1821,7 +1821,7 @@ static int tty3270_write(struct tty_struct *tty, const u8 *buf, int count) > /* > * Put single characters to the ttys character buffer > */ > -static int tty3270_put_char(struct tty_struct *tty, unsigned char ch) > +static int tty3270_put_char(struct tty_struct *tty, u8 ch) > { > struct tty3270 *tp; > > diff --git a/drivers/s390/char/sclp_tty.c b/drivers/s390/char/sclp_tty.c > index cc0f6a97124e..831a8c7cacc2 100644 > --- a/drivers/s390/char/sclp_tty.c > +++ b/drivers/s390/char/sclp_tty.c > @@ -48,7 +48,7 @@ static struct sclp_buffer *sclp_ttybuf; > static struct timer_list sclp_tty_timer; > > static struct tty_port sclp_port; > -static unsigned char sclp_tty_chars[SCLP_TTY_BUF_SIZE]; > +static u8 sclp_tty_chars[SCLP_TTY_BUF_SIZE]; > static unsigned short int sclp_tty_chars_count; > > struct tty_driver *sclp_tty_driver; > @@ -168,7 +168,7 @@ sclp_tty_timeout(struct timer_list *unused) > /* > * Write a string to the sclp tty. > */ > -static int sclp_tty_write_string(const unsigned char *str, int count, int may_fail) > +static int sclp_tty_write_string(const u8 *str, int count, int may_fail) > { > unsigned long flags; > void *page; > @@ -250,7 +250,7 @@ sclp_tty_write(struct tty_struct *tty, const u8 *buf, int count) > * sclp_write() without final '\n' - will be written. > */ > static int > -sclp_tty_put_char(struct tty_struct *tty, unsigned char ch) > +sclp_tty_put_char(struct tty_struct *tty, u8 ch) > { > sclp_tty_chars[sclp_tty_chars_count++] = ch; > if (ch == '\n' || sclp_tty_chars_count >= SCLP_TTY_BUF_SIZE) { > diff --git a/drivers/s390/char/sclp_vt220.c b/drivers/s390/char/sclp_vt220.c > index 44974d801c1e..e148350c1e2c 100644 > --- a/drivers/s390/char/sclp_vt220.c > +++ b/drivers/s390/char/sclp_vt220.c > @@ -579,7 +579,7 @@ sclp_vt220_close(struct tty_struct *tty, struct file *filp) > * done stuffing characters into the driver. > */ > static int > -sclp_vt220_put_char(struct tty_struct *tty, unsigned char ch) > +sclp_vt220_put_char(struct tty_struct *tty, u8 ch) > { > return __sclp_vt220_write(&ch, 1, 0, 0, 1); > } For s390 part: Acked-by: Alexander Gordeev <agordeev@linux.ibm.com> Thanks!