Re: [PATCH v6 1/6] serial: 8250: make saved LSR larger

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2619 bytes --]

On Mon, 13 Jun 2022, Jiri Slaby wrote:

> On 13. 06. 22, 9:52, Ilpo Järvinen wrote:
> > DW flags address received as BIT(8) in LSR. In order to not lose that
> > on read, enlarge lsr_saved_flags to u16.
> > 
> > Adjust lsr/status variables and related call chains which used unsigned
> > char type previously to unsigned int. Technically, some of these type
> > conversion would not be needed but it doesn't hurt to be consistent.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ...
> > --- a/include/linux/serial_8250.h
> > +++ b/include/linux/serial_8250.h
> > @@ -119,7 +119,7 @@ struct uart_8250_port {
> >   	 * be immediately processed.
> >   	 */
> >   #define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
> > -	unsigned char		lsr_saved_flags;
> > +	u16			lsr_saved_flags;
> >   #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
> >   	unsigned char		msr_saved_flags;
> >   @@ -170,8 +170,8 @@ extern void serial8250_do_set_divisor(struct uart_port
> > *port, unsigned int baud,
> >   				      unsigned int quot_frac);
> >   extern int fsl8250_handle_irq(struct uart_port *port);
> >   int serial8250_handle_irq(struct uart_port *port, unsigned int iir);
> > -unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char
> > lsr);
> > -void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr);
> > +unsigned int serial8250_rx_chars(struct uart_8250_port *up, unsigned int
> > lsr);
> > +void serial8250_read_char(struct uart_8250_port *up, unsigned int lsr);
> 
> It looks odd to have
>   u16 lsr_saved_flags
> in the struct and
>   unsigned int lsr
> here and there. You wrote:
>   Technically, some of these type conversion would not be needed
>   but it doesn't hurt to be consistent
> But it looks like you actually made them a bit inconsistent.

Those were actually meant to discuss on different things. u16 is the 
oldest part of change and the reason why it is only u16 is that I
didn't need more than that to store the bits used for the mask.

That "consistent" part was written to note that there are case which check 
only e.g. TEMT flag. As TEMT is within first 8 bits, it doesn't absolutely 
need more than unsigned char but I enlarged those types regardless.
I agree with you though the wording doesn't convey meaning exclusive to 
those cases.

> So why not use u16 for everyone?

If that consistency is necessary, I'd be more inclined to make the ones in 
the uart_8250_port unsigned int instead. The reason is that it would then 
align better to read/ignore_status_mask that are already unsigned int. 
Would it be ok or do you still prefer u16?

-- 
 i.