linux-kernel - Re: [PATCH tty-next v4 5/6] serial: 8250: Switch to nbcon console

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <844j2lqxly.fsf@jogness.linutronix.de>
Date: Mon, 30 Dec 2024 11:28:17 +0106
From: John Ogness <john.ogness@...utronix.de>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby
 <jirislaby@...nel.org>, Petr Mladek <pmladek@...e.com>, Sergey Senozhatsky
 <senozhatsky@...omium.org>, Steven Rostedt <rostedt@...dmis.org>, Thomas
 Gleixner <tglx@...utronix.de>, Esben Haabendal <esben@...nix.com>,
 linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org, Matt Turner
 <mattst88@...il.com>, Tony Lindgren <tony@...mide.com>, Arnd Bergmann
 <arnd@...db.de>, Niklas Schnelle <schnelle@...ux.ibm.com>, Serge Semin
 <fancer.lancer@...il.com>
Subject: Re: [PATCH tty-next v4 5/6] serial: 8250: Switch to nbcon console

On 2024-12-29, Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:
>> -static void fifo_wait_for_lsr(struct uart_8250_port *up, unsigned int count)
>> +static int fifo_wait_for_lsr(struct uart_8250_port *up,
>> +			      struct nbcon_write_context *wctxt,
>> +			      unsigned int count)
>>  {
>>  	unsigned int i;
>>  
>>  	for (i = 0; i < count; i++) {
>> +		if (!nbcon_can_proceed(wctxt))
>> +			return -EPERM;
>> +
>>  		if (wait_for_lsr(up, UART_LSR_THRE))
>> -			return;
>> +			return 0;
>>  	}
>> +
>> +	return -ETIMEDOUT;
>>  }
>
> ...
>
>>  static void serial8250_console_fifo_write(struct uart_8250_port *up,
>> -					  const char *s, unsigned int count)
>> +					  struct nbcon_write_context *wctxt)
>>  {
>> -	const char *end = s + count;
>>  	unsigned int fifosize = up->tx_loadsz;
>>  	struct uart_port *port = &up->port;
>> +	const char *s = wctxt->outbuf;
>> +	const char *end = s + wctxt->len;
>>  	unsigned int tx_count = 0;
>>  	bool cr_sent = false;
>>  	unsigned int i;
>>  
>>  	while (s != end) {
>>  		/* Allow timeout for each byte of a possibly full FIFO */
>> -		fifo_wait_for_lsr(up, fifosize);
>> +		if (fifo_wait_for_lsr(up, wctxt, fifosize) == -EPERM)
>> +			return;
>
> Perhaps it was already discussed and I forgot about it or hadn't read,
> but I don't get how per-byte check for NBCON permission can help if there
> is something already in the HW FIFO?
>
> I mean in _this_ case wouldn't be enough to check FIFO to drain and only after
> that check the permission?

If we did that and other CPUs had taken over printing, then this CPU
would possibly busy-wait the maximum timeout because the FIFO keeps
getting refilled by the other CPUs. If this CPU knows that it has lost
ownership then it can abort waiting and trying to write ASAP.

The CPU taking over will perform the necessary waiting for the FIFO to
drain what this CPU had wrote into the FIFO.

>
>>  		for (i = 0; i < fifosize && s != end; ++i) {

That being said, this loop is not checking for lost ownership between
bytes. I will add an nbcon_can_proceed() check here as well. If
ownership was lost, this CPU must not continue writing into the FIFO.

	for (i = 0; i < fifosize && s != end && nbcon_can_proceed(wctxt); ++i) {

>>  			if (*s == '\n' && !cr_sent) {
>
>>  	 * Allow timeout for each byte written since the caller will only wait
>>  	 * for UART_LSR_BOTH_EMPTY using the timeout of a single character
>>  	 */
>> -	fifo_wait_for_lsr(up, tx_count);
>> +	fifo_wait_for_lsr(up, wctxt, tx_count);
>> +}
>
>
> ...
>
>> +	if (up->msr_saved_flags) {
>> +		/*
>> +		 * For atomic, it must be deferred to irq_work because this
>> +		 * may be a context that does not permit waking up tasks.
>> +		 */
>> +		if (is_atomic)
>> +			irq_work_queue(&up->modem_status_work);
>> +		else
>> +			serial8250_modem_status(up);
>
> Hmm... Shouldn't this be part of _modem_status() for all drivers using this call?

I am not sure I follow what you are saying.

Only NBCON drivers (that have implemented the ->write_atomic() callback)
will print from any context. OTOH legacy console drivers do not print
directly from scheduling or NMI contexts and instead defer to a printk
irq_work.

As we convert more console drivers to NBCON, it might make sense to move
the irq_work into the generic struct uart_port and possibly consolidate
the _modem_status() implementations. They seem to be mostly copy/paste
code.

>> +	bool			console_line_ended;	/* line fully output */
>
> Sorry, I didn't get the comment. Do you meant "line is fully printed out
> [to HW]"?

'\n' was the most recent byte written to the TX register by the console
driver.

Tracking this is necessary during takeovers so that the CPU taking over
can politely add an extra newline for cleaner output.

The wording for this comment was suggested by Petr Mladek. Please
suggest an acceptable alternative if you require something more concise.

John