Re: [PATCH next v2 1/4] serial: 8250: Split out IER from rs485_start_tx()

[Petr Mladek <pmladek@suse.com>]

On Wed 2024-09-18 17:10:53, John Ogness wrote:
> On 2024-09-17, Petr Mladek <pmladek@suse.com> wrote:
> > Sigh, I am trying to review this patch but I am not familiar with the
> > code. Feel free to ignore me when the questions are completely off.
> 
> I appreciate you researching where the code came from. I made my changes
> based on what I see the code doing now.
> 
> >> --- a/drivers/tty/serial/8250/8250_port.c
> >> +++ b/drivers/tty/serial/8250/8250_port.c
> >>  void serial8250_em485_start_tx(struct uart_8250_port *up)
> >>  {
> >>  	unsigned char mcr = serial8250_in_MCR(up);
> >>  
> >> +	/*
> >> +	 * Some chips set the UART_LSR_DR bit even when UART_IER_RDI is
> >> +	 * disabled, so explicitly mask it.
> >> +	 */
> >>  	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> >> -		serial8250_stop_rx(&up->port);
> >> +		up->port.read_status_mask &= ~UART_LSR_DR;
> >
> > This change is related to disabling UART_IER_RDI but we do not longer
> > disable it in this code path.
> 
> Correct. It will be disabled in the new wrapper
> serial8250_em485_start_tx(). For the console write() callback, RDI is
> already being disabled (IER is cleared). It will not use the wrapper.
> 
> > Why do we need to do it here, please?
> 
> Because the console write() callback also needs to clear LSR_DR. That
> part of the callback needs to stay.
> 
> > Why is it needed only in the em485-specific path, please?
> 
> Only RS485 deals with controlling TX/RX directions.
> 
> > On one hand, the comment talks about UART_LSR_DR and UART_IER_RDI
> > so seems to be relater.
> 
> I do not know if the LSR_DR modify is strictly necessary. I am just
> preserving the existing behavior (and related comment). The disabling of
> IER_RDI will still happen (via wrapper or explicitly as in the console
> write() callback).
> 
> >>  static bool start_tx_rs485(struct uart_port *port)
> >>  {
> >> @@ -1585,7 +1600,7 @@ static bool start_tx_rs485(struct uart_port *port)
> >>  	if (em485->tx_stopped) {
> >>  		em485->tx_stopped = false;
> >>  
> >> -		up->rs485_start_tx(up);
> >> +		serial8250_rs485_start_tx(up);
> >
> > If I get this correctly then this keeps the existing behavior when
> >
> >     up->rs485_start_tx == serial8250_em485_start_tx
> 
> Correct.
> 
> > Is this always the case, please?
> 
> Yes.
> 
> > Can start_tx_rs485() be called for the 8250_bcm2835aux.c driver?
> 
> Yes.

IMHO, the answer "Yes" to both last questions can't be valid.
The 8250_bcm2835aux driver does:

static int bcm2835aux_serial_probe(struct platform_device *pdev)
{
	[...]
	up.rs485_start_tx = bcm2835aux_rs485_start_tx;
	[...]
}

As a result, the 1st "Yes" was not correct:

	up->rs485_start_tx != serial8250_em485_start_tx

and this patch would change the behavior for the 8250_bcm2835aux driver.
Before, start_tx_rs485() called directly:

	up->rs485_start_tx(up);

Newly, it would call:

	void serial8250_rs485_start_tx(struct uart_8250_port *up)
	{
		if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
			serial8250_stop_rx(&up->port);

		up->rs485_start_tx(up);
	}

It means that it could call serial8250_stop_rx() even when it was not
called by the original code.

And SER_RS485_RX_DURING_TX seems to be checked even in
drivers/tty/serial/8250/8250_bcm2835aux.c. So, it looks like it
might be (un)set even for this driver.

Or is this code path prevented in start_tx_rs485()? I mean that
em485->tx_stopped could never be true for the 8250_bcm2835aux
driver?

But I see

	static int bcm2835aux_serial_probe(struct platform_device *pdev)
	{
		[...]
		up.port.rs485_config = serial8250_em485_config;
		up.port.rs485_supported = serial8250_em485_supported;
		[...]
	}

=> It looks like even bcm2835aux driver could have the em485 thing.
   But it obviously wanted to something special in
   up->rs485_start_tx().

It looks to me that the change might either cause regression.
Or it would deserve a comment unless the validity is obvious for people
familiar with the code.

Best Regards,
Petr