On 2024-10-25, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> 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
