diff mbox series

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

Message ID 20240913140538.221708-2-john.ogness@linutronix.de
State New
Headers show
Series convert 8250 to nbcon | expand

Commit Message

John Ogness Sept. 13, 2024, 2:05 p.m. UTC
Move IER handling out of rs485_start_tx() callback and into a new
wrapper serial8250_rs485_start_tx(). Replace all callback call sites
with wrapper, except for the console write() callback, where it is
inappropriate to modify IER.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250.h      |  2 ++
 drivers/tty/serial/8250/8250_port.c | 27 +++++++++++++++++++++------
 2 files changed, 23 insertions(+), 6 deletions(-)

Comments

Petr Mladek Sept. 17, 2024, 2:48 p.m. UTC | #1
On Fri 2024-09-13 16:11:35, John Ogness wrote:
> Move IER handling out of rs485_start_tx() callback and into a new
> wrapper serial8250_rs485_start_tx(). Replace all callback call sites
> with wrapper, except for the console write() callback, where it is
> inappropriate to modify IER.

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.

> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1370,7 +1370,6 @@ static void serial8250_stop_rx(struct uart_port *port)
>  	serial8250_rpm_get(up);
>  
>  	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
> -	up->port.read_status_mask &= ~UART_LSR_DR;
>  	serial_port_out(port, UART_IER, up->ier);
>  
>  	serial8250_rpm_put(up);
> @@ -1543,16 +1542,20 @@ static inline void __start_tx(struct uart_port *port)
>   *
>   * Generic callback usable by 8250 uart drivers to start rs485 transmission.
>   * Assumes that setting the RTS bit in the MCR register means RTS is high.
> - * (Some chips use inverse semantics.)  Further assumes that reception is
> - * stoppable by disabling the UART_IER_RDI interrupt. (Some chips set the
> - * UART_LSR_DR bit even when UART_IER_RDI is disabled, foiling this approach.)
> + * (Some chips use inverse semantics.)
> + * It does not disable RX interrupts. Use the wrapper function
> + * serial8250_rs485_start_tx() if that is also needed.
>   */
>  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.

Why do we need to do it here, please?
Why is it needed only in the em485-specific path, please?


I tried to understand the code and am in doubts:

On one hand, the comment talks about UART_LSR_DR and UART_IER_RDI
so seems to be relater.

But the "Some chips set..." comment has been added by the commit
058bc104f7ca5c83d81 ("serial: 8250: Generalize rs485 software emulation").
And I do not see any explanation why it was added in this code path
even though UART_LSR_DR and UART_IER_RDI were manipulated in
serial8250_stop_rx() which can be called also in other code
paths via uport->ops->stop_rx().

Also the comment suggests that this fixes a bug in some chips but
the line has been added into 1.1.60 back in 2007.

--- a/drivers/char/ChangeLog
+++ b/drivers/char/ChangeLog
@@ -1,3 +1,28 @@
+Sat Oct 29 18:17:34 1994  Theodore Y. Ts'o  (tytso@rt-11)
+
+	* serial.c (rs_ioctl, get_lsr_info): Added patch suggested by Arne
+		Riiber so that user mode programs can tell when the
+		transmitter shift register is empty.
+
+Thu Oct 27 23:14:29 1994  Theodore Y. Ts'o  (tytso@rt-11)
+
+	* tty_ioctl.c (wait_until_sent): Added debugging printk statements
+		(under the #ifdef TTY_DEBUG_WAIT_UNTL_SENT)  
+
+	* serial.c (rs_interrupt, rs_interrupt_single, receive_chars,
+		change_speed, rs_close): rs_close now disables receiver
+		interrupts when closing the serial port.  This allows the
+		serial port to close quickly when Linux and a modem (or a
+		mouse) are engaged in an echo war; when closing the serial
+		port, we now first stop listening to incoming characters,
+		and *then* wait for the transmit buffer to drain.  
+
+		In order to make this change, the info->read_status_mask
+		is now used to control what bits of the line status
+		register are looked at in the interrupt routine in all
+		cases; previously it was only used in receive_chars to
+		select a few of the status bits.
+
--- a/drivers/char/serial.c
+++ b/drivers/char/serial.c
[...]
@@ -1780,6 +1830,15 @@ static void rs_close(struct tty_struct *tty, struct file * filp)
 		info->normal_termios = *tty->termios;
 	if (info->flags & ASYNC_CALLOUT_ACTIVE)
 		info->callout_termios = *tty->termios;
+	/*
+	 * At this point we stop accepting input.  To do this, we
+	 * disable the receive line status interrupts, and tell the
+	 * interrut driver to stop checking the data ready bit in the
+	 * line status register.
+	 */
+	info->IER &= ~UART_IER_RLSI;
+	serial_out(info, UART_IER, info->IER);
+	info->read_status_mask &= ~UART_LSR_DR;
 	if (info->flags & ASYNC_INITIALIZED) {
 		wait_until_sent(tty, 3000); /* 30 seconds timeout */
 		/*

    => It looks like it was not a fix for a "buggy chips". It looks
       like it was part of the design.

>  
>  	if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>  		mcr |= UART_MCR_RTS;
> @@ -1562,6 +1565,18 @@ void serial8250_em485_start_tx(struct uart_8250_port *up)
>  }
>  EXPORT_SYMBOL_GPL(serial8250_em485_start_tx);
>  
> +/**
> + * serial8250_rs485_start_tx() - stop rs485 reception, enable transmission
> + * @up: uart 8250 port
> + */
> +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);
> +}
> +
>  /* Returns false, if start_tx_timer was setup to defer TX start */
>  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

Is this always the case, please?

The callback has been added by the commit 058bc104f7ca5c83d81
("serial: 8250: Generalize rs485 software emulation") because
8250_bcm2835aux.c driver needed to do something else.

Can start_tx_rs485() be called for the 8250_bcm2835aux.c driver?
Will it still work as expected?

>  
>  		if (up->port.rs485.delay_rts_before_send > 0) {
>  			em485->active_timer = &em485->start_tx_timer;

Best Regards,
Petr
John Ogness Sept. 18, 2024, 3:04 p.m. UTC | #2
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.

> Will it still work as expected?

Yes, but it does perform an extra read. And since someone added a
comment just to mention that, I assume it was important for some use
case.

John
Petr Mladek Sept. 19, 2024, 3:01 p.m. UTC | #3
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
diff mbox series

Patch

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index e5310c65cf52..6e90223ba1d6 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -236,6 +236,8 @@  void serial8250_em485_stop_tx(struct uart_8250_port *p);
 void serial8250_em485_destroy(struct uart_8250_port *p);
 extern struct serial_rs485 serial8250_em485_supported;
 
+void serial8250_rs485_start_tx(struct uart_8250_port *up);
+
 /* MCR <-> TIOCM conversion */
 static inline int serial8250_TIOCM_to_MCR(int tiocm)
 {
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 2786918aea98..ba8d9cefc451 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1370,7 +1370,6 @@  static void serial8250_stop_rx(struct uart_port *port)
 	serial8250_rpm_get(up);
 
 	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
-	up->port.read_status_mask &= ~UART_LSR_DR;
 	serial_port_out(port, UART_IER, up->ier);
 
 	serial8250_rpm_put(up);
@@ -1543,16 +1542,20 @@  static inline void __start_tx(struct uart_port *port)
  *
  * Generic callback usable by 8250 uart drivers to start rs485 transmission.
  * Assumes that setting the RTS bit in the MCR register means RTS is high.
- * (Some chips use inverse semantics.)  Further assumes that reception is
- * stoppable by disabling the UART_IER_RDI interrupt.  (Some chips set the
- * UART_LSR_DR bit even when UART_IER_RDI is disabled, foiling this approach.)
+ * (Some chips use inverse semantics.)
+ * It does not disable RX interrupts. Use the wrapper function
+ * serial8250_rs485_start_tx() if that is also needed.
  */
 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;
 
 	if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
 		mcr |= UART_MCR_RTS;
@@ -1562,6 +1565,18 @@  void serial8250_em485_start_tx(struct uart_8250_port *up)
 }
 EXPORT_SYMBOL_GPL(serial8250_em485_start_tx);
 
+/**
+ * serial8250_rs485_start_tx() - stop rs485 reception, enable transmission
+ * @up: uart 8250 port
+ */
+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);
+}
+
 /* Returns false, if start_tx_timer was setup to defer TX start */
 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 (up->port.rs485.delay_rts_before_send > 0) {
 			em485->active_timer = &em485->start_tx_timer;