Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1199002 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2015-08-03 18:10 +0200
    Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown Peter Hurley <peter@hurleysoftware.com> - 2015-08-03 18:40 +0200
      Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2015-08-03 19:00 +0200
        Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown Peter Hurley <peter@hurleysoftware.com> - 2015-08-03 21:40 +0200
          Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2015-08-04 14:00 +0200

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

* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1199028

[ +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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199044

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199224

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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199716

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web