[PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[John Ogness]

If DMA is active during a shutdown, a delayed restore of the
registers may be pending. The restore must be performed after
the DMA is stopped, otherwise the delayed restore remains
pending and will fire upon the first DMA TX complete of a
totally different serial session.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_omap.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 5b39892..25f6255 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -657,9 +657,15 @@ static void omap_8250_shutdown(struct uart_port *port)
 	up->ier = 0;
 	serial_out(up, UART_IER, 0);
 
-	if (up->dma)
+	if (up->dma) {
 		serial8250_release_dma(up);
 
+		if (priv->delayed_restore) {
+			priv->delayed_restore = 0;
+			omap8250_restore_regs(up);
+		}
+	}
+
 	/*
 	 * Disable break condition and FIFOs
 	 */
-- 
1.7.10.4

Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[Peter Hurley]

Hi John,

On 07/30/2015 06:54 PM, John Ogness wrote:
> If DMA is active during a shutdown, a delayed restore of the
> registers may be pending. The restore must be performed after
> the DMA is stopped, otherwise the delayed restore remains
> pending and will fire upon the first DMA TX complete of a
> totally different serial session.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/8250/8250_omap.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 5b39892..25f6255 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -657,9 +657,15 @@ static void omap_8250_shutdown(struct uart_port *port)
>  	up->ier = 0;
>  	serial_out(up, UART_IER, 0);
>  
> -	if (up->dma)
> +	if (up->dma) {
>  		serial8250_release_dma(up);
>  
> +		if (priv->delayed_restore) {
> +			priv->delayed_restore = 0;
> +			omap8250_restore_regs(up);
> +		}

I was never really a fan of the deferred set_termios();
I think it's more appropriate to wait for tx dma to
complete in omap_8250_set_termios().

Regards,
Peter Hurley


> +	}
> +
>  	/*
>  	 * Disable break condition and FIFOs
>  	 */
> 

Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[Sebastian Andrzej Siewior]

* Peter Hurley | 2015-07-30 20:51:10 [-0400]:

>Hi John,
Hi Peter,

>I was never really a fan of the deferred set_termios();
>I think it's more appropriate to wait for tx dma to
>complete in omap_8250_set_termios().

So you want something like this? This was only compile + boot tested
(without triggering the corner case) and I know that 8250.h piece has to
go in a separated patch (as requested in 2/3 of this series). Just checking
if this is what you had in mind.

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index c43f74c53cd9..a407757dcecc 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -42,9 +42,9 @@ struct uart_8250_dma {
 	size_t			rx_size;
 	size_t			tx_size;
 
-	unsigned char		tx_running:1;
-	unsigned char		tx_err: 1;
-	unsigned char		rx_running:1;
+	unsigned char		tx_running;
+	unsigned char		tx_err;
+	unsigned char		rx_running;
 };
 
 struct old_serial_port {
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index d9a37191a1ae..12249125a218 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -100,9 +100,9 @@ struct omap8250_priv {
 	u8 wer;
 	u8 xon;
 	u8 xoff;
-	u8 delayed_restore;
 	u16 quot;
 
+	wait_queue_head_t termios_wait;
 	bool is_suspending;
 	int wakeirq;
 	int wakeups_enabled;
@@ -256,18 +256,6 @@ static void omap8250_update_mdr1(struct uart_8250_port *up,
 static void omap8250_restore_regs(struct uart_8250_port *up)
 {
 	struct omap8250_priv *priv = up->port.private_data;
-	struct uart_8250_dma	*dma = up->dma;
-
-	if (dma && dma->tx_running) {
-		/*
-		 * TCSANOW requests the change to occur immediately however if
-		 * we have a TX-DMA operation in progress then it has been
-		 * observed that it might stall and never complete. Therefore we
-		 * delay DMA completes to prevent this hang from happen.
-		 */
-		priv->delayed_restore = 1;
-		return;
-	}
 
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 	serial_out(up, UART_EFR, UART_EFR_ECB);
@@ -309,6 +297,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
 	up->port.ops->set_mctrl(&up->port, up->port.mctrl);
 }
 
+static void omap_8250_dma_tx_complete(void *param);
 /*
  * OMAP can use "CLK / (16 or 13) / div" for baud rate. And then we have have
  * some differences in how we want to handle flow control.
@@ -322,6 +311,7 @@ static void omap_8250_set_termios(struct uart_port *port,
 	struct omap8250_priv *priv = up->port.private_data;
 	unsigned char cval = 0;
 	unsigned int baud;
+	unsigned int complete_dma = 0;
 
 	switch (termios->c_cflag & CSIZE) {
 	case CS5:
@@ -473,6 +463,25 @@ static void omap_8250_set_termios(struct uart_port *port,
 		if (termios->c_iflag & IXANY)
 			up->mcr |= UART_MCR_XONANY;
 	}
+
+	if (up->dma && up->dma->tx_running) {
+		struct uart_8250_dma	*dma = up->dma;
+
+		/*
+		 * TCSANOW requests the change to occur immediately however if
+		 * we have a TX-DMA operation in progress then it has been
+		 * observed that it might stall and never complete. Therefore we
+		 * wait until DMA completes to prevent this hang from happen.
+		 */
+
+		dma->tx_running = 2;
+
+		spin_unlock_irq(&up->port.lock);
+		wait_event(priv->termios_wait,
+			   dma->tx_running == 3);
+		spin_lock_irq(&up->port.lock);
+		complete_dma = 1;
+	}
 	omap8250_restore_regs(up);
 
 	spin_unlock_irq(&up->port.lock);
@@ -488,6 +497,8 @@ static void omap_8250_set_termios(struct uart_port *port,
 	/* Don't rewrite B0 */
 	if (tty_termios_baud_rate(termios))
 		tty_termios_encode_baud_rate(termios, baud, baud);
+	if (complete_dma)
+		omap_8250_dma_tx_complete(up);
 }
 
 /* same as 8250 except that we may have extra flow bits set in EFR */
@@ -869,17 +880,18 @@ static void omap_8250_dma_tx_complete(void *param)
 
 	spin_lock_irqsave(&p->port.lock, flags);
 
+	if (dma->tx_running == 2) {
+		dma->tx_running = 3;
+		wake_up(&priv->termios_wait);
+		goto out;
+	}
+
 	dma->tx_running = 0;
 
 	xmit->tail += dma->tx_size;
 	xmit->tail &= UART_XMIT_SIZE - 1;
 	p->port.icount.tx += dma->tx_size;
 
-	if (priv->delayed_restore) {
-		priv->delayed_restore = 0;
-		omap8250_restore_regs(p);
-	}
-
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&p->port);
 
@@ -899,7 +911,7 @@ static void omap_8250_dma_tx_complete(void *param)
 		p->ier |= UART_IER_THRI;
 		serial_port_out(&p->port, UART_IER, p->ier);
 	}
-
+out:
 	spin_unlock_irqrestore(&p->port.lock, flags);
 }
 
@@ -1216,6 +1228,7 @@ static int omap8250_probe(struct platform_device *pdev)
 			priv->omap8250_dma.rx_size = RX_TRIGGER;
 			priv->omap8250_dma.rxconf.src_maxburst = RX_TRIGGER;
 			priv->omap8250_dma.txconf.dst_maxburst = TX_TRIGGER;
+			init_waitqueue_head(&priv->termios_wait);
 
 			if (of_machine_is_compatible("ti,am33xx"))
 				priv->habit |= OMAP_DMA_TX_KICK;


Sebastian

Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[Peter Hurley]

[ +cc Heikki ]

Hi Sebastian,

On 08/03/2015 12:09 PM, Sebastian Andrzej Siewior wrote:
> * Peter Hurley | 2015-07-30 20:51:10 [-0400]:
> 
>> Hi John,
> Hi Peter,
> 
>> I was never really a fan of the deferred set_termios();
>> I think it's more appropriate to wait for tx dma to
>> complete in omap_8250_set_termios().
> 
> So you want something like this? This was only compile + boot tested
> (without triggering the corner case) and I know that 8250.h piece has to
> go in a separated patch (as requested in 2/3 of this series). Just checking
> if this is what you had in mind.
> 
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index c43f74c53cd9..a407757dcecc 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -42,9 +42,9 @@ struct uart_8250_dma {
>  	size_t			rx_size;
>  	size_t			tx_size;
>  
> -	unsigned char		tx_running:1;
> -	unsigned char		tx_err: 1;
> -	unsigned char		rx_running:1;
> +	unsigned char		tx_running;
> +	unsigned char		tx_err;
> +	unsigned char		rx_running;
>  };

This part is ok.

>  struct old_serial_port {
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index d9a37191a1ae..12249125a218 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -100,9 +100,9 @@ struct omap8250_priv {
>  	u8 wer;
>  	u8 xon;
>  	u8 xoff;
> -	u8 delayed_restore;
>  	u16 quot;
>  
> +	wait_queue_head_t termios_wait;
>  	bool is_suspending;
>  	int wakeirq;
>  	int wakeups_enabled;
> @@ -256,18 +256,6 @@ static void omap8250_update_mdr1(struct uart_8250_port *up,
>  static void omap8250_restore_regs(struct uart_8250_port *up)
>  {
>  	struct omap8250_priv *priv = up->port.private_data;
> -	struct uart_8250_dma	*dma = up->dma;
> -
> -	if (dma && dma->tx_running) {
> -		/*
> -		 * TCSANOW requests the change to occur immediately however if
> -		 * we have a TX-DMA operation in progress then it has been
> -		 * observed that it might stall and never complete. Therefore we
> -		 * delay DMA completes to prevent this hang from happen.
> -		 */
> -		priv->delayed_restore = 1;
> -		return;
> -	}
>  
>  	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>  	serial_out(up, UART_EFR, UART_EFR_ECB);
> @@ -309,6 +297,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
>  	up->port.ops->set_mctrl(&up->port, up->port.mctrl);
>  }
>  
> +static void omap_8250_dma_tx_complete(void *param);
>  /*
>   * OMAP can use "CLK / (16 or 13) / div" for baud rate. And then we have have
>   * some differences in how we want to handle flow control.
> @@ -322,6 +311,7 @@ static void omap_8250_set_termios(struct uart_port *port,
>  	struct omap8250_priv *priv = up->port.private_data;
>  	unsigned char cval = 0;
>  	unsigned int baud;
> +	unsigned int complete_dma = 0;
>  
>  	switch (termios->c_cflag & CSIZE) {
>  	case CS5:
> @@ -473,6 +463,25 @@ static void omap_8250_set_termios(struct uart_port *port,
>  		if (termios->c_iflag & IXANY)
>  			up->mcr |= UART_MCR_XONANY;
>  	}
> +
> +	if (up->dma && up->dma->tx_running) {
> +		struct uart_8250_dma	*dma = up->dma;
> +
> +		/*
> +		 * TCSANOW requests the change to occur immediately however if
> +		 * we have a TX-DMA operation in progress then it has been
> +		 * observed that it might stall and never complete. Therefore we
> +		 * wait until DMA completes to prevent this hang from happen.
> +		 */
> +
> +		dma->tx_running = 2;
> +
> +		spin_unlock_irq(&up->port.lock);
> +		wait_event(priv->termios_wait,
> +			   dma->tx_running == 3);

Doesn't the dmaengine api offer a race-free way to wait for pending tx dma
to complete?

Maybe we could wrap that in the 8250 dma api?

Regards,
Peter Hurley

> +		spin_lock_irq(&up->port.lock);
> +		complete_dma = 1;
> +	}
>  	omap8250_restore_regs(up);
>  
>  	spin_unlock_irq(&up->port.lock);
> @@ -488,6 +497,8 @@ static void omap_8250_set_termios(struct uart_port *port,
>  	/* Don't rewrite B0 */
>  	if (tty_termios_baud_rate(termios))
>  		tty_termios_encode_baud_rate(termios, baud, baud);
> +	if (complete_dma)
> +		omap_8250_dma_tx_complete(up);
>  }
>  
>  /* same as 8250 except that we may have extra flow bits set in EFR */
> @@ -869,17 +880,18 @@ static void omap_8250_dma_tx_complete(void *param)
>  
>  	spin_lock_irqsave(&p->port.lock, flags);
>  
> +	if (dma->tx_running == 2) {
> +		dma->tx_running = 3;
> +		wake_up(&priv->termios_wait);
> +		goto out;
> +	}
> +
>  	dma->tx_running = 0;
>  
>  	xmit->tail += dma->tx_size;
>  	xmit->tail &= UART_XMIT_SIZE - 1;
>  	p->port.icount.tx += dma->tx_size;
>  
> -	if (priv->delayed_restore) {
> -		priv->delayed_restore = 0;
> -		omap8250_restore_regs(p);
> -	}
> -
>  	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>  		uart_write_wakeup(&p->port);
>  
> @@ -899,7 +911,7 @@ static void omap_8250_dma_tx_complete(void *param)
>  		p->ier |= UART_IER_THRI;
>  		serial_port_out(&p->port, UART_IER, p->ier);
>  	}
> -
> +out:
>  	spin_unlock_irqrestore(&p->port.lock, flags);
>  }
>  
> @@ -1216,6 +1228,7 @@ static int omap8250_probe(struct platform_device *pdev)
>  			priv->omap8250_dma.rx_size = RX_TRIGGER;
>  			priv->omap8250_dma.rxconf.src_maxburst = RX_TRIGGER;
>  			priv->omap8250_dma.txconf.dst_maxburst = TX_TRIGGER;
> +			init_waitqueue_head(&priv->termios_wait);
>  
>  			if (of_machine_is_compatible("ti,am33xx"))
>  				priv->habit |= OMAP_DMA_TX_KICK;
> 
> 
> Sebastian
> 

Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[Sebastian Andrzej Siewior]

On 08/03/2015 06:34 PM, Peter Hurley wrote:
> Hi Sebastian,

Hi Peter,

>>  struct old_serial_port {
>> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
>> index d9a37191a1ae..12249125a218 100644
>> --- a/drivers/tty/serial/8250/8250_omap.c
>> +++ b/drivers/tty/serial/8250/8250_omap.c
>> @@ -100,9 +100,9 @@ struct omap8250_priv {
>>  	u8 wer;
>>  	u8 xon;
>>  	u8 xoff;
>> -	u8 delayed_restore;
>>  	u16 quot;
>>  
>> +	wait_queue_head_t termios_wait;
>>  	bool is_suspending;
>>  	int wakeirq;
>>  	int wakeups_enabled;
>> @@ -256,18 +256,6 @@ static void omap8250_update_mdr1(struct uart_8250_port *up,
>>  static void omap8250_restore_regs(struct uart_8250_port *up)
>>  {
>>  	struct omap8250_priv *priv = up->port.private_data;
>> -	struct uart_8250_dma	*dma = up->dma;
>> -
>> -	if (dma && dma->tx_running) {
>> -		/*
>> -		 * TCSANOW requests the change to occur immediately however if
>> -		 * we have a TX-DMA operation in progress then it has been
>> -		 * observed that it might stall and never complete. Therefore we
>> -		 * delay DMA completes to prevent this hang from happen.
>> -		 */
>> -		priv->delayed_restore = 1;
>> -		return;
>> -	}
>>  
>>  	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>>  	serial_out(up, UART_EFR, UART_EFR_ECB);
>> @@ -309,6 +297,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
>>  	up->port.ops->set_mctrl(&up->port, up->port.mctrl);
>>  }
>>  
>> +static void omap_8250_dma_tx_complete(void *param);
>>  /*
>>   * OMAP can use "CLK / (16 or 13) / div" for baud rate. And then we have have
>>   * some differences in how we want to handle flow control.
>> @@ -322,6 +311,7 @@ static void omap_8250_set_termios(struct uart_port *port,
>>  	struct omap8250_priv *priv = up->port.private_data;
>>  	unsigned char cval = 0;
>>  	unsigned int baud;
>> +	unsigned int complete_dma = 0;
>>  
>>  	switch (termios->c_cflag & CSIZE) {
>>  	case CS5:
>> @@ -473,6 +463,25 @@ static void omap_8250_set_termios(struct uart_port *port,
>>  		if (termios->c_iflag & IXANY)
>>  			up->mcr |= UART_MCR_XONANY;
>>  	}
>> +
>> +	if (up->dma && up->dma->tx_running) {
>> +		struct uart_8250_dma	*dma = up->dma;
>> +
>> +		/*
>> +		 * TCSANOW requests the change to occur immediately however if
>> +		 * we have a TX-DMA operation in progress then it has been
>> +		 * observed that it might stall and never complete. Therefore we
>> +		 * wait until DMA completes to prevent this hang from happen.
>> +		 */
>> +
>> +		dma->tx_running = 2;
>> +
>> +		spin_unlock_irq(&up->port.lock);
>> +		wait_event(priv->termios_wait,
>> +			   dma->tx_running == 3);
> 
> Doesn't the dmaengine api offer a race-free way to wait for pending tx dma
> to complete?

Not that I know of. You still need to ensure that once that DMA
completed, nobody triggers another TX transfer before you do what you
planned. This is ensures by the tx_running != 0 and the spin lock.

> Maybe we could wrap that in the 8250 dma api?

You mean a function in 8250-dma API which does what I did just here
with the wait_event() and the wake_up in the callback? That way I could
move the termios_wait into the dma struct instead of keeping in the
omap specific part. I am also not sure if OMAP is the only one that may
hang here or the other people just didn't notice it yet.

> Regards,
> Peter Hurley
> 
>> +		spin_lock_irq(&up->port.lock);
>> +		complete_dma = 1;
>> +	}
>>  	omap8250_restore_regs(up);
>>  
>>  	spin_unlock_irq(&up->port.lock);
>> @@ -488,6 +497,8 @@ static void omap_8250_set_termios(struct uart_port *port,
>>  	/* Don't rewrite B0 */
>>  	if (tty_termios_baud_rate(termios))
>>  		tty_termios_encode_baud_rate(termios, baud, baud);
>> +	if (complete_dma)
>> +		omap_8250_dma_tx_complete(up);
>>  }
>>  
>>  /* same as 8250 except that we may have extra flow bits set in EFR */
>> @@ -869,17 +880,18 @@ static void omap_8250_dma_tx_complete(void *param)
>>  
>>  	spin_lock_irqsave(&p->port.lock, flags);
>>  
>> +	if (dma->tx_running == 2) {
>> +		dma->tx_running = 3;
>> +		wake_up(&priv->termios_wait);
>> +		goto out;
>> +	}
>> +
>>  	dma->tx_running = 0;
>>  
>>  	xmit->tail += dma->tx_size;
>>  	xmit->tail &= UART_XMIT_SIZE - 1;
>>  	p->port.icount.tx += dma->tx_size;
>>  
>> -	if (priv->delayed_restore) {
>> -		priv->delayed_restore = 0;
>> -		omap8250_restore_regs(p);
>> -	}
>> -
>>  	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>>  		uart_write_wakeup(&p->port);
>>  
>> @@ -899,7 +911,7 @@ static void omap_8250_dma_tx_complete(void *param)
>>  		p->ier |= UART_IER_THRI;
>>  		serial_port_out(&p->port, UART_IER, p->ier);
>>  	}
>> -
>> +out:
>>  	spin_unlock_irqrestore(&p->port.lock, flags);
>>  }
>>  
>> @@ -1216,6 +1228,7 @@ static int omap8250_probe(struct platform_device *pdev)
>>  			priv->omap8250_dma.rx_size = RX_TRIGGER;
>>  			priv->omap8250_dma.rxconf.src_maxburst = RX_TRIGGER;
>>  			priv->omap8250_dma.txconf.dst_maxburst = TX_TRIGGER;
>> +			init_waitqueue_head(&priv->termios_wait);
>>  
>>  			if (of_machine_is_compatible("ti,am33xx"))
>>  				priv->habit |= OMAP_DMA_TX_KICK;
>>

Sebastian

Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[Peter Hurley]

On 08/03/2015 12:54 PM, Sebastian Andrzej Siewior wrote:
> On 08/03/2015 06:34 PM, Peter Hurley wrote:
>> Hi Sebastian,
> 
> Hi Peter,
> 
>>>  struct old_serial_port {
>>> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
>>> index d9a37191a1ae..12249125a218 100644
>>> --- a/drivers/tty/serial/8250/8250_omap.c
>>> +++ b/drivers/tty/serial/8250/8250_omap.c
>>> @@ -100,9 +100,9 @@ struct omap8250_priv {
>>>  	u8 wer;
>>>  	u8 xon;
>>>  	u8 xoff;
>>> -	u8 delayed_restore;
>>>  	u16 quot;
>>>  
>>> +	wait_queue_head_t termios_wait;
>>>  	bool is_suspending;
>>>  	int wakeirq;
>>>  	int wakeups_enabled;
>>> @@ -256,18 +256,6 @@ static void omap8250_update_mdr1(struct uart_8250_port *up,
>>>  static void omap8250_restore_regs(struct uart_8250_port *up)
>>>  {
>>>  	struct omap8250_priv *priv = up->port.private_data;
>>> -	struct uart_8250_dma	*dma = up->dma;
>>> -
>>> -	if (dma && dma->tx_running) {
>>> -		/*
>>> -		 * TCSANOW requests the change to occur immediately however if
>>> -		 * we have a TX-DMA operation in progress then it has been
>>> -		 * observed that it might stall and never complete. Therefore we
>>> -		 * delay DMA completes to prevent this hang from happen.
>>> -		 */
>>> -		priv->delayed_restore = 1;
>>> -		return;
>>> -	}
>>>  
>>>  	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>>>  	serial_out(up, UART_EFR, UART_EFR_ECB);
>>> @@ -309,6 +297,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
>>>  	up->port.ops->set_mctrl(&up->port, up->port.mctrl);
>>>  }
>>>  
>>> +static void omap_8250_dma_tx_complete(void *param);
>>>  /*
>>>   * OMAP can use "CLK / (16 or 13) / div" for baud rate. And then we have have
>>>   * some differences in how we want to handle flow control.
>>> @@ -322,6 +311,7 @@ static void omap_8250_set_termios(struct uart_port *port,
>>>  	struct omap8250_priv *priv = up->port.private_data;
>>>  	unsigned char cval = 0;
>>>  	unsigned int baud;
>>> +	unsigned int complete_dma = 0;
>>>  
>>>  	switch (termios->c_cflag & CSIZE) {
>>>  	case CS5:
>>> @@ -473,6 +463,25 @@ static void omap_8250_set_termios(struct uart_port *port,
>>>  		if (termios->c_iflag & IXANY)
>>>  			up->mcr |= UART_MCR_XONANY;
>>>  	}
>>> +
>>> +	if (up->dma && up->dma->tx_running) {
>>> +		struct uart_8250_dma	*dma = up->dma;
>>> +
>>> +		/*
>>> +		 * TCSANOW requests the change to occur immediately however if
>>> +		 * we have a TX-DMA operation in progress then it has been
>>> +		 * observed that it might stall and never complete. Therefore we
>>> +		 * wait until DMA completes to prevent this hang from happen.
>>> +		 */
>>> +
>>> +		dma->tx_running = 2;
>>> +
>>> +		spin_unlock_irq(&up->port.lock);
>>> +		wait_event(priv->termios_wait,
>>> +			   dma->tx_running == 3);
>>
>> Doesn't the dmaengine api offer a race-free way to wait for pending tx dma
>> to complete?
> 
> Not that I know of. You still need to ensure that once that DMA
> completed, nobody triggers another TX transfer before you do what you
> planned. This is ensures by the tx_running != 0 and the spin lock.
> 
>> Maybe we could wrap that in the 8250 dma api?
> 
> You mean a function in 8250-dma API which does what I did just here
> with the wait_event() and the wake_up in the callback? That way I could
> move the termios_wait into the dma struct instead of keeping in the
> omap specific part. I am also not sure if OMAP is the only one that may
> hang here or the other people just didn't notice it yet.

Exactly; and we need to fix DMA wrt x_char anyway.

Going back to the dmaengine api, I think something like this might work
(as a first approximation):

	dma_sync_wait(dma->txchan, dma->tx_cookie);
	dmaengine_pause(dma->txchan);

	/* remainder of set_termios */

	dmaengine_resume(dma->txchan);

We could require 8250 core dma to support pause/resume.


>>> +		spin_lock_irq(&up->port.lock);
>>> +		complete_dma = 1;
>>> +	}
>>>  	omap8250_restore_regs(up);
>>>  
>>>  	spin_unlock_irq(&up->port.lock);
>>> @@ -488,6 +497,8 @@ static void omap_8250_set_termios(struct uart_port *port,
>>>  	/* Don't rewrite B0 */
>>>  	if (tty_termios_baud_rate(termios))
>>>  		tty_termios_encode_baud_rate(termios, baud, baud);
>>> +	if (complete_dma)
>>> +		omap_8250_dma_tx_complete(up);
>>>  }
>>>  
>>>  /* same as 8250 except that we may have extra flow bits set in EFR */
>>> @@ -869,17 +880,18 @@ static void omap_8250_dma_tx_complete(void *param)
>>>  
>>>  	spin_lock_irqsave(&p->port.lock, flags);
>>>  
>>> +	if (dma->tx_running == 2) {
>>> +		dma->tx_running = 3;
>>> +		wake_up(&priv->termios_wait);
>>> +		goto out;
>>> +	}
>>> +
>>>  	dma->tx_running = 0;
>>>  
>>>  	xmit->tail += dma->tx_size;
>>>  	xmit->tail &= UART_XMIT_SIZE - 1;
>>>  	p->port.icount.tx += dma->tx_size;
>>>  
>>> -	if (priv->delayed_restore) {
>>> -		priv->delayed_restore = 0;
>>> -		omap8250_restore_regs(p);
>>> -	}
>>> -
>>>  	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>>>  		uart_write_wakeup(&p->port);
>>>  
>>> @@ -899,7 +911,7 @@ static void omap_8250_dma_tx_complete(void *param)
>>>  		p->ier |= UART_IER_THRI;
>>>  		serial_port_out(&p->port, UART_IER, p->ier);
>>>  	}
>>> -
>>> +out:
>>>  	spin_unlock_irqrestore(&p->port.lock, flags);
>>>  }
>>>  
>>> @@ -1216,6 +1228,7 @@ static int omap8250_probe(struct platform_device *pdev)
>>>  			priv->omap8250_dma.rx_size = RX_TRIGGER;
>>>  			priv->omap8250_dma.rxconf.src_maxburst = RX_TRIGGER;
>>>  			priv->omap8250_dma.txconf.dst_maxburst = TX_TRIGGER;
>>> +			init_waitqueue_head(&priv->termios_wait);
>>>  
>>>  			if (of_machine_is_compatible("ti,am33xx"))
>>>  				priv->habit |= OMAP_DMA_TX_KICK;
>>>
> 
> Sebastian
> 

Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[Sebastian Andrzej Siewior]

On 08/03/2015 09:32 PM, Peter Hurley wrote:

>> You mean a function in 8250-dma API which does what I did just here
>> with the wait_event() and the wake_up in the callback? That way I could
>> move the termios_wait into the dma struct instead of keeping in the
>> omap specific part. I am also not sure if OMAP is the only one that may
>> hang here or the other people just didn't notice it yet.
> 
> Exactly; and we need to fix DMA wrt x_char anyway.
> 
> Going back to the dmaengine api, I think something like this might work
> (as a first approximation):
> 
> 	dma_sync_wait(dma->txchan, dma->tx_cookie);
> 	dmaengine_pause(dma->txchan);
> 
> 	/* remainder of set_termios */
> 
> 	dmaengine_resume(dma->txchan);
> 
> We could require 8250 core dma to support pause/resume.

I would prefer the waitqueue approach.
You can't do this while holding the port lock. The lock is taken with
irqs off so may not see the transfer completing.
Why do you pause the channel? It may not work without an active
descriptor and a start without "resume" should work. Also you must
ensure that DMA's complete callback does not start another transfer if
there is something queued up (that is why I had the tx_running dance).
I am not sure if a transfer that is active and then paused will not
trigger the hang bug if we change the termios in between.

Sebastian

Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[Peter Hurley]

Hi Sebastian,

On 08/04/2015 07:58 AM, Sebastian Andrzej Siewior wrote:
> On 08/03/2015 09:32 PM, Peter Hurley wrote:
> 
>>> You mean a function in 8250-dma API which does what I did just here
>>> with the wait_event() and the wake_up in the callback? That way I could
>>> move the termios_wait into the dma struct instead of keeping in the
>>> omap specific part. I am also not sure if OMAP is the only one that may
>>> hang here or the other people just didn't notice it yet.
>>
>> Exactly; and we need to fix DMA wrt x_char anyway.
>>
>> Going back to the dmaengine api, I think something like this might work
>> (as a first approximation):
>>
>> 	dma_sync_wait(dma->txchan, dma->tx_cookie);
>> 	dmaengine_pause(dma->txchan);
>>
>> 	/* remainder of set_termios */
>>
>> 	dmaengine_resume(dma->txchan);
>>
>> We could require 8250 core dma to support pause/resume.
> 
> I would prefer the waitqueue approach.
> You can't do this while holding the port lock. The lock is taken with
> irqs off so may not see the transfer completing.
> Why do you pause the channel? It may not work without an active
> descriptor and a start without "resume" should work. Also you must
> ensure that DMA's complete callback does not start another transfer if
> there is something queued up (that is why I had the tx_running dance).
> I am not sure if a transfer that is active and then paused will not
> trigger the hang bug if we change the termios in between.

I'll look at/test this this weekend, ok?

Regards,
Peter Hurley

Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[Sebastian Andrzej Siewior]

On 08/06/2015 02:27 PM, Peter Hurley wrote:
> Hi Sebastian,

Hi Peter,

> On 08/04/2015 07:58 AM, Sebastian Andrzej Siewior wrote:
>> On 08/03/2015 09:32 PM, Peter Hurley wrote:
>>
>>>> You mean a function in 8250-dma API which does what I did just here
>>>> with the wait_event() and the wake_up in the callback? That way I could
>>>> move the termios_wait into the dma struct instead of keeping in the
>>>> omap specific part. I am also not sure if OMAP is the only one that may
>>>> hang here or the other people just didn't notice it yet.
>>>
>>> Exactly; and we need to fix DMA wrt x_char anyway.
>>>
>>> Going back to the dmaengine api, I think something like this might work
>>> (as a first approximation):
>>>
>>> 	dma_sync_wait(dma->txchan, dma->tx_cookie);
>>> 	dmaengine_pause(dma->txchan);
>>>
>>> 	/* remainder of set_termios */
>>>
>>> 	dmaengine_resume(dma->txchan);
>>>
>>> We could require 8250 core dma to support pause/resume.
>>
>> I would prefer the waitqueue approach.
>> You can't do this while holding the port lock. The lock is taken with
>> irqs off so may not see the transfer completing.
>> Why do you pause the channel? It may not work without an active
>> descriptor and a start without "resume" should work. Also you must
>> ensure that DMA's complete callback does not start another transfer if
>> there is something queued up (that is why I had the tx_running dance).
>> I am not sure if a transfer that is active and then paused will not
>> trigger the hang bug if we change the termios in between.
> 
> I'll look at/test this this weekend, ok?

Sure. I'm currently re-spinning the patches so have everything in
proper pieces. While at it I will take a look at x_char.

> Regards,
> Peter Hurley
> 
Sebastian

Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[Sebastian Andrzej Siewior]

On 08/06/2015 02:31 PM, Sebastian Andrzej Siewior wrote:

Hi Peter,

>> I'll look at/test this this weekend, ok?
> 
> Sure. I'm currently re-spinning the patches so have everything in
> proper pieces. While at it I will take a look at x_char.

So now that I actually look at it. If I read this right, we never send
the x_char if the TX-DMA never fails to do its job. The comment above
uart_send_xchar() says it is high priority. What do you suggest, wait
until the transfer completes, send the x_char _or_ pause the transfer
send that byte and then send the byte?
In both cases we have to wait until for the FIFO-empty interrupt to
make sure we don't overrun that TX-FIFO.

I *think* waiting until the transfer completes would be simpler but it
is not necessarily high priority.

>> Regards,
>> Peter Hurley

Sebastian

Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[Peter Hurley]

On 08/06/2015 09:59 AM, Sebastian Andrzej Siewior wrote:
> On 08/06/2015 02:31 PM, Sebastian Andrzej Siewior wrote:
> 
> Hi Peter,
> 
>>> I'll look at/test this this weekend, ok?
>>
>> Sure. I'm currently re-spinning the patches so have everything in
>> proper pieces. While at it I will take a look at x_char.
> 
> So now that I actually look at it. If I read this right, we never send
> the x_char if the TX-DMA never fails to do its job.

That's what I saw too; almost all the dma drivers are broken wrt x_char.
The amba-pl011 driver gets it right.

> The comment above uart_send_xchar() says it is high priority.

'High' priority is meant relative to previously written data which has
not yet been sent.

> What do you suggest, wait
> until the transfer completes, send the x_char _or_ pause the transfer
> send that byte and then send the byte?

'Better' would be sending the x_char when the current dma transfer
completes. However, it will probably have /some/ impact on what line
rates software flow control can be used. Worst case @ 115Kbaud is 35ms
delay in sending.

'Best' would be pausing the dma and sending the byte. However, I'm not
even sure if this is possible on OMAP; the TRM is woefully under-documented
in that regard.

> In both cases we have to wait until for the FIFO-empty interrupt to
> make sure we don't overrun that TX-FIFO.
> 
> I *think* waiting until the transfer completes would be simpler but it
> is not necessarily high priority.

I agree; this is what we should do first because someone might want it
for backports.

Regards,
Peter Hurley

Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[Charles Manning]

On Fri, Aug 7, 2015 at 6:22 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> I agree; this is what we should do first because someone might want it
> for backports.

Got an idea how far this can be ported back?

I'm being hampered by severe performance issues on a
beagleboneblack-like device (am335x) running on 3.18 kernel.

I have a 1Mbaud link that is getting packets of about 22 bytes @800 Hz
and old omap-serial is keeping up, but killing the CPU.

I'm considering backporting 8250_omap as a get out of jail.

Opinions appreciated.

Thanks.

Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown

[John Ogness]

Hi Sebastian, Peter,

On 2015-08-03, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> * Peter Hurley | 2015-07-30 20:51:10 [-0400]:
>> I was never really a fan of the deferred set_termios();
>> I think it's more appropriate to wait for tx dma to
>> complete in omap_8250_set_termios().
>
> So you want something like this? This was only compile + boot tested
> (without triggering the corner case) [...] Just checking if this is
> what you had in mind.

I tested this against next-20160226 using an AM335x (beaglebone
black). I made sure the corner case was hit. I also ran tests of
terminating the sender or termios-setter while in the corner
case. Everything ran without any problems.

> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index d9a37191a1ae..12249125a218 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -100,9 +100,9 @@ struct omap8250_priv {
>  	u8 wer;
>  	u8 xon;
>  	u8 xoff;
> -	u8 delayed_restore;
>  	u16 quot;
>  
> +	wait_queue_head_t termios_wait;
>  	bool is_suspending;
>  	int wakeirq;
>  	int wakeups_enabled;
> @@ -256,18 +256,6 @@ static void omap8250_update_mdr1(struct uart_8250_port *up,
>  static void omap8250_restore_regs(struct uart_8250_port *up)
>  {
>  	struct omap8250_priv *priv = up->port.private_data;
> -	struct uart_8250_dma	*dma = up->dma;
> -
> -	if (dma && dma->tx_running) {
> -		/*
> -		 * TCSANOW requests the change to occur immediately however if
> -		 * we have a TX-DMA operation in progress then it has been
> -		 * observed that it might stall and never complete. Therefore we
> -		 * delay DMA completes to prevent this hang from happen.
> -		 */
> -		priv->delayed_restore = 1;
> -		return;
> -	}
>  
>  	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>  	serial_out(up, UART_EFR, UART_EFR_ECB);
> @@ -309,6 +297,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
>  	up->port.ops->set_mctrl(&up->port, up->port.mctrl);
>  }
>  
> +static void omap_8250_dma_tx_complete(void *param);
>  /*
>   * OMAP can use "CLK / (16 or 13) / div" for baud rate. And then we have have
>   * some differences in how we want to handle flow control.
> @@ -322,6 +311,7 @@ static void omap_8250_set_termios(struct uart_port *port,
>  	struct omap8250_priv *priv = up->port.private_data;
>  	unsigned char cval = 0;
>  	unsigned int baud;
> +	unsigned int complete_dma = 0;
>  
>  	switch (termios->c_cflag & CSIZE) {
>  	case CS5:
> @@ -473,6 +463,25 @@ static void omap_8250_set_termios(struct uart_port *port,
>  		if (termios->c_iflag & IXANY)
>  			up->mcr |= UART_MCR_XONANY;
>  	}
> +
> +	if (up->dma && up->dma->tx_running) {
> +		struct uart_8250_dma	*dma = up->dma;
> +
> +		/*
> +		 * TCSANOW requests the change to occur immediately however if
> +		 * we have a TX-DMA operation in progress then it has been
> +		 * observed that it might stall and never complete. Therefore we
> +		 * wait until DMA completes to prevent this hang from happen.
> +		 */
> +
> +		dma->tx_running = 2;
> +
> +		spin_unlock_irq(&up->port.lock);
> +		wait_event(priv->termios_wait,
> +			   dma->tx_running == 3);

If I comment out the wait_event() call here, I can reproduce the DMA
stalls reported by the comments.

> +		spin_lock_irq(&up->port.lock);
> +		complete_dma = 1;
> +	}
>  	omap8250_restore_regs(up);
>  
>  	spin_unlock_irq(&up->port.lock);
> @@ -488,6 +497,8 @@ static void omap_8250_set_termios(struct uart_port *port,
>  	/* Don't rewrite B0 */
>  	if (tty_termios_baud_rate(termios))
>  		tty_termios_encode_baud_rate(termios, baud, baud);
> +	if (complete_dma)
> +		omap_8250_dma_tx_complete(up);
>  }
>  
>  /* same as 8250 except that we may have extra flow bits set in EFR */
> @@ -869,17 +880,18 @@ static void omap_8250_dma_tx_complete(void *param)
>  
>  	spin_lock_irqsave(&p->port.lock, flags);
>  
> +	if (dma->tx_running == 2) {
> +		dma->tx_running = 3;
> +		wake_up(&priv->termios_wait);
> +		goto out;
> +	}
> +
>  	dma->tx_running = 0;
>  
>  	xmit->tail += dma->tx_size;
>  	xmit->tail &= UART_XMIT_SIZE - 1;
>  	p->port.icount.tx += dma->tx_size;
>  
> -	if (priv->delayed_restore) {
> -		priv->delayed_restore = 0;
> -		omap8250_restore_regs(p);
> -	}
> -
>  	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>  		uart_write_wakeup(&p->port);
>  
> @@ -899,7 +911,7 @@ static void omap_8250_dma_tx_complete(void *param)
>  		p->ier |= UART_IER_THRI;
>  		serial_port_out(&p->port, UART_IER, p->ier);
>  	}
> -
> +out:
>  	spin_unlock_irqrestore(&p->port.lock, flags);
>  }
>  
> @@ -1216,6 +1228,7 @@ static int omap8250_probe(struct platform_device *pdev)
>  			priv->omap8250_dma.rx_size = RX_TRIGGER;
>  			priv->omap8250_dma.rxconf.src_maxburst = RX_TRIGGER;
>  			priv->omap8250_dma.txconf.dst_maxburst = TX_TRIGGER;
> +			init_waitqueue_head(&priv->termios_wait);
>  
>  			if (of_machine_is_compatible("ti,am33xx"))
>  				priv->habit |= OMAP_DMA_TX_KICK;
>

John Ogness