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

[John Ogness <john.ogness@linutronix.de>]

On 2024-10-25, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> +/*
>> + * Only to be used directly by the console write callbacks, which may not
>> + * require the port lock. Use serial8250_clear_IER() instead for all other
>> + * cases.
>> + */
>> +static void __serial8250_clear_IER(struct uart_8250_port *up)
>>  {
>>  	if (up->capabilities & UART_CAP_UUE)
>>  		serial_out(up, UART_IER, UART_IER_UUE);
>
>>  		serial_out(up, UART_IER, 0);
>>  }
>>  
>> +static inline void serial8250_clear_IER(struct uart_8250_port *up)
>> +{
>> +	__serial8250_clear_IER(up);
>
> Shouldn't this have a lockdep annotation to differentiate with the
> above?

Yes, but the follow-up patch adds the annotation as a clean "revert
patch". I can add a line about that in the commit message.

>> +static void serial8250_console_byte_write(struct uart_8250_port *up,
>> +					  struct nbcon_write_context *wctxt)
>> +{
>> +	const char *s = READ_ONCE(wctxt->outbuf);
>> +	const char *end = s + READ_ONCE(wctxt->len);
>
> Is there any possibility that outbuf value be changed before we get
> the len and at the end we get the wrong pointer?

No. I was concerned about compiler optimization, since @outbuf can
become NULL. However, it can only become NULL if ownership was
transferred, and that is properly checked anyway. I will remove the
READ_ONCE() usage for v4.

>>  struct uart_8250_port {
>
>>  	u16			lsr_save_mask;
>>  #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
>>  	unsigned char		msr_saved_flags;
>> +	struct irq_work		modem_status_work;
>> +
>> +	bool			console_line_ended;	/* line fully output */
>>  
>>  	struct uart_8250_dma	*dma;
>>  	const struct uart_8250_ops *ops;
>
> Btw, have you run `pahole` on this? Perhaps there are better places
> for new members?

Indeed there are. Placing it above the MSR_SAVE_FLAGS macro will reduce
an existing 3-byte hole to 2-bytes and avoid creating a new 7-byte
hole.

Thanks.

John