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

[Jiri Slaby <jirislaby@kernel.org>]

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