[permalink] [raw]

Subject: Re: [PATCH 10/11] serial: make uart_console_write->putchar()'s character a char

Hi Jiri,

I love your patch! Yet something to improve:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v5.17-rc1 next-20220124]
[cannot apply to shawnguo/for-next davem-sparc/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Jiri-Slaby/TTY-patches-for-5-18/20220124-151758
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: arc-randconfig-r043-20220124 (https://download.01.org/0day-ci/archive/20220125/[email protected]/config)
compiler: arc-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/45520d4f6db241db0b77f41162adba4a9977956c
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jiri-Slaby/TTY-patches-for-5-18/20220124-151758
git checkout 45520d4f6db241db0b77f41162adba4a9977956c
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash drivers/tty/serial/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

drivers/tty/serial/omap-serial.c: In function 'serial_omap_console_write':
>> drivers/tty/serial/omap-serial.c:1272:49: error: passing argument 4 of 'uart_console_write' from incompatible pointer type [-Werror=incompatible-pointer-types]
1272 | uart_console_write(&up->port, s, count, serial_omap_console_putchar);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| void (*)(struct uart_port *, int)
In file included from drivers/tty/serial/omap-serial.c:30:
include/linux/serial_core.h:402:32: note: expected 'void (*)(struct uart_port *, char)' but argument is of type 'void (*)(struct uart_port *, int)'
402 | void (*putchar)(struct uart_port *, char));
| ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors

vim +/uart_console_write +1272 drivers/tty/serial/omap-serial.c

b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1248
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1249 static void
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1250 serial_omap_console_write(struct console *co, const char *s,
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1251 unsigned int count)
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1252 {
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1253 struct uart_omap_port *up = serial_omap_console_ports[co->index];
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1254 unsigned long flags;
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1255 unsigned int ier;
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1256 int locked = 1;
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1257
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1258 local_irq_save(flags);
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1259 if (up->port.sysrq)
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1260 locked = 0;
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1261 else if (oops_in_progress)
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1262 locked = spin_trylock(&up->port.lock);
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1263 else
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1264 spin_lock(&up->port.lock);
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1265
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1266 /*
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1267 * First save the IER then disable the interrupts
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1268 */
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1269 ier = serial_in(up, UART_IER);
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1270 serial_out(up, UART_IER, 0);
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1271
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 @1272 uart_console_write(&up->port, s, count, serial_omap_console_putchar);
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1273
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1274 /*
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1275 * Finally, wait for transmitter to become empty
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1276 * and restore the IER
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1277 */
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1278 wait_for_xmitr(up);
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1279 serial_out(up, UART_IER, ier);
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1280 /*
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1281 * The receive handling will happen properly because the
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1282 * receive ready bit will still be set; it is not cleared
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1283 * on read. However, modem control will not, we must
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1284 * call it if we have saved something in the saved flags
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1285 * while processing with interrupts off.
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1286 */
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1287 if (up->msr_saved_flags)
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1288 check_modem_status(up);
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1289
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1290 if (locked)
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1291 spin_unlock(&up->port.lock);
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1292 local_irq_restore(flags);
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1293 }
b612633b592807 drivers/serial/omap-serial.c Govindraj.R 2010-09-27 1294

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

2022-01-26 20:19:14

by Jiri Slaby

[permalink] [raw]

Subject: Re: [PATCH 10/11] serial: make uart_console_write->putchar()'s character a char

On 24. 01. 22, 15:30, kernel test robot wrote:
> Hi Jiri,
>
> I love your patch! Yet something to improve:

Please skip this patch. I wonder why the bot didn't catch it in my queue
already.

> [auto build test ERROR on tty/tty-testing]
> [also build test ERROR on v5.17-rc1 next-20220124]
> [cannot apply to shawnguo/for-next davem-sparc/master]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/0day-ci/linux/commits/Jiri-Slaby/TTY-patches-for-5-18/20220124-151758
> base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
> config: microblaze-randconfig-r006-20220124 (https://download.01.org/0day-ci/archive/20220124/[email protected]/config)
> compiler: microblaze-linux-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://github.com/0day-ci/linux/commit/45520d4f6db241db0b77f41162adba4a9977956c
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review Jiri-Slaby/TTY-patches-for-5-18/20220124-151758
> git checkout 45520d4f6db241db0b77f41162adba4a9977956c
> # save the config file to linux build tree
> mkdir build_dir
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/tty/serial/
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
> All errors (new ones prefixed by >>):
>
> drivers/tty/serial/sprd_serial.c: In function 'sprd_console_write':
>>> drivers/tty/serial/sprd_serial.c:1007:44: error: passing argument 4 of 'uart_console_write' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 1007 | uart_console_write(port, s, count, sprd_console_putchar);
> | ^~~~~~~~~~~~~~~~~~~~
> | |
> | void (*)(struct uart_port *, int)
> In file included from drivers/tty/serial/sprd_serial.c:18:
> include/linux/serial_core.h:402:32: note: expected 'void (*)(struct uart_port *, char)' but argument is of type 'void (*)(struct uart_port *, int)'
> 402 | void (*putchar)(struct uart_port *, char));
> | ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
>
>
> vim +/uart_console_write +1007 drivers/tty/serial/sprd_serial.c
>
> b7396a38fb28db Chunyan Zhang 2015-01-28 992
> b7396a38fb28db Chunyan Zhang 2015-01-28 993 static void sprd_console_write(struct console *co, const char *s,
> b7396a38fb28db Chunyan Zhang 2015-01-28 994 unsigned int count)
> b7396a38fb28db Chunyan Zhang 2015-01-28 995 {
> b7396a38fb28db Chunyan Zhang 2015-01-28 996 struct uart_port *port = &sprd_port[co->index]->port;
> b7396a38fb28db Chunyan Zhang 2015-01-28 997 int locked = 1;
> b7396a38fb28db Chunyan Zhang 2015-01-28 998 unsigned long flags;
> b7396a38fb28db Chunyan Zhang 2015-01-28 999
> b7396a38fb28db Chunyan Zhang 2015-01-28 1000 if (port->sysrq)
> b7396a38fb28db Chunyan Zhang 2015-01-28 1001 locked = 0;
> b7396a38fb28db Chunyan Zhang 2015-01-28 1002 else if (oops_in_progress)
> b7396a38fb28db Chunyan Zhang 2015-01-28 1003 locked = spin_trylock_irqsave(&port->lock, flags);
> b7396a38fb28db Chunyan Zhang 2015-01-28 1004 else
> b7396a38fb28db Chunyan Zhang 2015-01-28 1005 spin_lock_irqsave(&port->lock, flags);
> b7396a38fb28db Chunyan Zhang 2015-01-28 1006
> b7396a38fb28db Chunyan Zhang 2015-01-28 @1007 uart_console_write(port, s, count, sprd_console_putchar);
> b7396a38fb28db Chunyan Zhang 2015-01-28 1008
> b7396a38fb28db Chunyan Zhang 2015-01-28 1009 /* wait for transmitter to become empty */
> b7396a38fb28db Chunyan Zhang 2015-01-28 1010 wait_for_xmitr(port);
> b7396a38fb28db Chunyan Zhang 2015-01-28 1011
> b7396a38fb28db Chunyan Zhang 2015-01-28 1012 if (locked)
> b7396a38fb28db Chunyan Zhang 2015-01-28 1013 spin_unlock_irqrestore(&port->lock, flags);
> b7396a38fb28db Chunyan Zhang 2015-01-28 1014 }
> b7396a38fb28db Chunyan Zhang 2015-01-28 1015
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/[email protected]

--
js
suse labs

2022-01-26 21:26:55

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH 10/11] serial: make uart_console_write->putchar()'s character a char

On Wed, Jan 26, 2022 at 08:26:48AM +0100, Jiri Slaby wrote:
> On 24. 01. 22, 15:30, kernel test robot wrote:
> > Hi Jiri,
> >
> > I love your patch! Yet something to improve:
>
> Please skip this patch. I wonder why the bot didn't catch it in my queue
> already.

Now skipped. Thanks for all of the other patches, now queued up.

greg k-h

2022-01-26 22:27:26

by Maciej W. Rozycki

[permalink] [raw]

Subject: Re: [PATCH 10/11] serial: make uart_console_write->putchar()'s character a char

On Mon, 24 Jan 2022, Jiri Slaby wrote:

> diff --git a/drivers/tty/serial/dz.c b/drivers/tty/serial/dz.c
> index e9edabc5a211..3493e201d67f 100644
> --- a/drivers/tty/serial/dz.c
> +++ b/drivers/tty/serial/dz.c
> @@ -802,7 +802,7 @@ static void __init dz_init_ports(void)
> * restored. Welcome to the world of PDP-11!
> * -------------------------------------------------------------------
> */
> -static void dz_console_putchar(struct uart_port *uport, int ch)
> +static void dz_console_putchar(struct uart_port *uport, char ch)
> {
> struct dz_port *dport = to_dport(uport);
> unsigned long flags;

Hmm, this is unsafe, because on the MIPS target the lone `char' type is
signed and therefore a call to `->putchar' will see `ch' sign-extended
from bit #7 to the width of the argument register used. Which means that
if a character is sent to the console that has its bit #7 set, then the
call to:

dz_out(dport, DZ_TDR, ch);

i.e.:

static void dz_out(struct dz_port *dport, unsigned offset, u16 value)

will send a value to DZ_TDR with bits #15:8 set to all-ones. And bits
#11:8 there are the BREAK control bits, active high, for serial lines #3:0
respectively.

We could handle this with a preparatory change by calling:

dz_out(dport, DZ_TDR, ch & 0xffu);

instead, but perhaps `->putchar' should simply take `unsigned char' or
maybe even `u8' as its third argument?

> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index c58cc142d23f..68e62703eaa6 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -399,7 +399,7 @@ int uart_set_options(struct uart_port *port, struct console
> *co, int baud,
> struct tty_driver *uart_console_device(struct console *co, int *index);
> void uart_console_write(struct uart_port *port, const char *s,
> unsigned int count,
> - void (*putchar)(struct uart_port *, int));
> + void (*putchar)(struct uart_port *, char));
>
> /*
> * Port/driver registration/removal

I.e.:

void (*putchar)(struct uart_port *, unsigned char));

I can see we get it right already with:

unsigned char x_char; /* xon/xoff char */

and for `dz_transmit_chars' we have:

unsigned char tmp;
[...]
tmp = xmit->buf[xmit->tail];
xmit->tail = (xmit->tail + 1) & (DZ_XMIT_SIZE - 1);
dz_out(dport, DZ_TDR, tmp);

(because `struct circ_buf' is generic and not limited to unsigned buffer
contents interpretation; it's not clear to me if that has been intended
though).

Maciej

2022-01-27 14:18:53

by Jiri Slaby

[permalink] [raw]

Subject: Re: [PATCH 10/11] serial: make uart_console_write->putchar()'s character a char

On 26. 01. 22, 18:57, Maciej W. Rozycki wrote:
> On Mon, 24 Jan 2022, Jiri Slaby wrote:
>
>> diff --git a/drivers/tty/serial/dz.c b/drivers/tty/serial/dz.c
>> index e9edabc5a211..3493e201d67f 100644
>> --- a/drivers/tty/serial/dz.c
>> +++ b/drivers/tty/serial/dz.c
>> @@ -802,7 +802,7 @@ static void __init dz_init_ports(void)
>> * restored. Welcome to the world of PDP-11!
>> * -------------------------------------------------------------------
>> */
>> -static void dz_console_putchar(struct uart_port *uport, int ch)
>> +static void dz_console_putchar(struct uart_port *uport, char ch)
>> {
>> struct dz_port *dport = to_dport(uport);
>> unsigned long flags;
>
> Hmm, this is unsafe, because on the MIPS target the lone `char' type is
> signed and therefore a call to `->putchar' will see `ch' sign-extended
> from bit #7 to the width of the argument register used. Which means that
> if a character is sent to the console that has its bit #7 set, then the
> call to:
>
> dz_out(dport, DZ_TDR, ch);
>
> i.e.:
>
> static void dz_out(struct dz_port *dport, unsigned offset, u16 value)
>
> will send a value to DZ_TDR with bits #15:8 set to all-ones. And bits
> #11:8 there are the BREAK control bits, active high, for serial lines #3:0
> respectively.
>
> We could handle this with a preparatory change by calling:
>
> dz_out(dport, DZ_TDR, ch & 0xffu);
>
> instead, but perhaps `->putchar' should simply take `unsigned char' or
> maybe even `u8' as its third argument?
>
>> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
>> index c58cc142d23f..68e62703eaa6 100644
>> --- a/include/linux/serial_core.h
>> +++ b/include/linux/serial_core.h
>> @@ -399,7 +399,7 @@ int uart_set_options(struct uart_port *port, struct console
>> *co, int baud,
>> struct tty_driver *uart_console_device(struct console *co, int *index);
>> void uart_console_write(struct uart_port *port, const char *s,
>> unsigned int count,
>> - void (*putchar)(struct uart_port *, int));
>> + void (*putchar)(struct uart_port *, char));
>>
>> /*
>> * Port/driver registration/removal
>
> I.e.:
>
> void (*putchar)(struct uart_port *, unsigned char));
>
> I can see we get it right already with:
>
> unsigned char x_char; /* xon/xoff char */
>
> and for `dz_transmit_chars' we have:
>
> unsigned char tmp;
> [...]
> tmp = xmit->buf[xmit->tail];
> xmit->tail = (xmit->tail + 1) & (DZ_XMIT_SIZE - 1);
> dz_out(dport, DZ_TDR, tmp);
>
> (because `struct circ_buf' is generic and not limited to unsigned buffer
> contents interpretation; it's not clear to me if that has been intended
> though).

Thanks, good point. I was considering unsigned char and concluded not go
that path right now as the rest of the console world uses char --
starting from the printk code. OTOH the whole uart world uses 'unsigned
char' except that circ_buf. But I am phasing circ_buf out in favor of
kfifo+unsigned-char anyway.

So let me switch the whole uart (the console code in particular) to an
unsigned world. Meaning printk will pass char, uart will receive it as
unsigned char and will keep it unsigned in all cases.

thanks,
--
js
suse labs