[PATCH 1/3] serial: 8250: move rx_running out of the bitfield

[PATCH 1/3] serial: 8250: move rx_running out of the bitfield

[Sebastian Andrzej Siewior]

From: John Ogness <john.ogness@linutronix.de>

That bitfield is modified by read + or + write operation. If someone
sets any of the other two bits it might render the lock useless.

While at it, remove other bitfields as well to avoid more such
errors.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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 {
-- 
2.5.0

[PATCH 2/3] serial: 8250_omap: check how many bytes were injected

[Sebastian Andrzej Siewior]

The function tty_insert_flip_string() returns an int and as such it
might fail. So the result is that I kindly asked to insert 48 bytes and
the function only insterted 32.
I have no idea what to do with the remaining 16 so I think dropping them
is the only option. I also increase the buf_overrun counter so userpace
has a clue that we lost bytes.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_omap.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 2ac63c8bd946..933f7ef2c004 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -738,6 +738,7 @@ static void __dma_rx_do_complete(struct uart_8250_port *p, bool error)
 	struct dma_tx_state     state;
 	int                     count;
 	unsigned long		flags;
+	int			ret;
 
 	dma_sync_single_for_cpu(dma->rxchan->device->dev, dma->rx_addr,
 				dma->rx_size, DMA_FROM_DEVICE);
@@ -753,8 +754,10 @@ static void __dma_rx_do_complete(struct uart_8250_port *p, bool error)
 
 	count = dma->rx_size - state.residue;
 
-	tty_insert_flip_string(tty_port, dma->rx_buf, count);
-	p->port.icount.rx += count;
+	ret = tty_insert_flip_string(tty_port, dma->rx_buf, count);
+
+	p->port.icount.rx += ret;
+	p->port.icount.buf_overrun += count - ret;
 unlock:
 	spin_unlock_irqrestore(&priv->rx_dma_lock, flags);
 
-- 
2.5.0

Re: [PATCH 2/3] serial: 8250_omap: check how many bytes were injected

[Sekhar Nori]

On Friday 14 August 2015 09:31 PM, Sebastian Andrzej Siewior wrote:
> The function tty_insert_flip_string() returns an int and as such it
> might fail. So the result is that I kindly asked to insert 48 bytes and
> the function only insterted 32.
> I have no idea what to do with the remaining 16 so I think dropping them
> is the only option. I also increase the buf_overrun counter so userpace
> has a clue that we lost bytes.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Tested with wilink BT module on TI's DRA7 EVM.

Tested-by: Sekhar Nori <nsekhar@ti.com>

Thanks,
Sekhar

Re: [PATCH 2/3] serial: 8250_omap: check how many bytes were injected

[Peter Hurley]

On 08/14/2015 12:01 PM, Sebastian Andrzej Siewior wrote:
> The function tty_insert_flip_string() returns an int and as such it
> might fail. So the result is that I kindly asked to insert 48 bytes and
> the function only insterted 32.
> I have no idea what to do with the remaining 16 so I think dropping them
> is the only option. I also increase the buf_overrun counter so userpace
> has a clue that we lost bytes.

No objection to the patch but I'm curious whether this is something you've
actually observed and under what circumstances.

Regards,
Peter Hurley


> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/tty/serial/8250/8250_omap.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 2ac63c8bd946..933f7ef2c004 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -738,6 +738,7 @@ static void __dma_rx_do_complete(struct uart_8250_port *p, bool error)
>  	struct dma_tx_state     state;
>  	int                     count;
>  	unsigned long		flags;
> +	int			ret;
>  
>  	dma_sync_single_for_cpu(dma->rxchan->device->dev, dma->rx_addr,
>  				dma->rx_size, DMA_FROM_DEVICE);
> @@ -753,8 +754,10 @@ static void __dma_rx_do_complete(struct uart_8250_port *p, bool error)
>  
>  	count = dma->rx_size - state.residue;
>  
> -	tty_insert_flip_string(tty_port, dma->rx_buf, count);
> -	p->port.icount.rx += count;
> +	ret = tty_insert_flip_string(tty_port, dma->rx_buf, count);
> +
> +	p->port.icount.rx += ret;
> +	p->port.icount.buf_overrun += count - ret;
>  unlock:
>  	spin_unlock_irqrestore(&priv->rx_dma_lock, flags);
>  
> 

Re: [PATCH 2/3] serial: 8250_omap: check how many bytes were injected

[Sekhar Nori]

On Wednesday 26 August 2015 06:14 PM, Peter Hurley wrote:
> On 08/14/2015 12:01 PM, Sebastian Andrzej Siewior wrote:
>> The function tty_insert_flip_string() returns an int and as such it
>> might fail. So the result is that I kindly asked to insert 48 bytes and
>> the function only insterted 32.
>> I have no idea what to do with the remaining 16 so I think dropping them
>> is the only option. I also increase the buf_overrun counter so userpace
>> has a clue that we lost bytes.
> 
> No objection to the patch but I'm curious whether this is something you've
> actually observed and under what circumstances.

This was observed while doing a UART internal loopback test at 3Mbaud on
TI's DRA7 EVM.

Thanks,
Sekhar

[RFC 3/3] serial: 8250_omap: try to avoid IER_RDI with DMA

[Sebastian Andrzej Siewior]

It has been observed on DRA7-evm that the UART triggers the interrupt and
reading IIR says IIR_NO_INT. It seems that we receive data via RX-DMA
but the interrupt is triggered anyway. I have hardly observed it on
AM335x and not in *that* quantity. On DRA7-evm with continuous transfers
at 3MBaud and CPU running at 1.5Ghz it is possible that the IRQ-core will
close the UART interrupt after "some time" with "nobody cared".

I've seen that by not enabling IER_RDI those spurious interrupts are not
triggered. Also it seems that DMA and RDI cause the "timeout interrupt"
which does not allow RX-DMA to be scheduled even if the FIFO reached the
programmed RX threshold. However without RDI we don't get a notification
if we have less than RX threshold bytes in the FIFO.
This is where we have the rx_dma_wd timer. After programming the RX-DMA
transfer wait HZ / 4 or 250ms for it to complete. If it does not
complete in that time span we cancel the DMA transfer and enable RDI.
RDI will trigger an UART interrupt in case we have bytes in the FIFO.
Once we read bytes manually from the FIFO we enable RX-DMA again
(without RDI) with the same 250ms timeout.

One downside with this approach is that latency sensitive protocols that
transfer less than 48 bytes will have to wait 250ms to complete.

Is there maybe a user interface where one could set small or bulk transfers?

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_omap.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 383f143b6b36..68d03553617b 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -113,6 +113,7 @@ struct omap8250_priv {
 	struct uart_8250_dma omap8250_dma;
 	spinlock_t rx_dma_lock;
 	bool rx_dma_broken;
+	struct timer_list rx_dma_wd;
 };
 
 static u32 uart_read(struct uart_8250_port *up, u32 reg)
@@ -599,6 +600,7 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 	return IRQ_RETVAL(ret);
 }
 
+static void omap8250_rx_dma_wd(unsigned long data);
 static int omap_8250_startup(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
@@ -627,6 +629,10 @@ static int omap_8250_startup(struct uart_port *port)
 			dev_warn_ratelimited(port->dev,
 					     "failed to request DMA\n");
 			up->dma = NULL;
+		} else {
+			init_timer(&priv->rx_dma_wd);
+			priv->rx_dma_wd.function = omap8250_rx_dma_wd;
+			priv->rx_dma_wd.data = (unsigned long) up;
 		}
 	}
 
@@ -635,7 +641,9 @@ static int omap_8250_startup(struct uart_port *port)
 	if (ret < 0)
 		goto err;
 
-	up->ier = UART_IER_RLSI | UART_IER_RDI;
+	up->ier = UART_IER_RLSI;
+	if (!up->dma->rxchan)
+		up->ier |= UART_IER_RDI;
 	serial_out(up, UART_IER, up->ier);
 
 #ifdef CONFIG_PM
@@ -670,6 +678,8 @@ static void omap_8250_shutdown(struct uart_port *port)
 	if (up->dma)
 		up->dma->rx_dma(up, UART_IIR_RX_TIMEOUT);
 
+	del_timer_sync(&priv->rx_dma_wd);
+
 	pm_runtime_get_sync(port->dev);
 
 	serial_out(up, UART_OMAP_WER, 0);
@@ -846,6 +856,9 @@ static int omap_8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
 	if (dma->rx_running)
 		goto out;
 
+	if (p->ier & UART_IER_RDI)
+		goto out;
+
 	desc = dmaengine_prep_slave_single(dma->rxchan, dma->rx_addr,
 					   dma->rx_size, DMA_DEV_TO_MEM,
 					   DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -864,6 +877,7 @@ static int omap_8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
 				   dma->rx_size, DMA_FROM_DEVICE);
 
 	dma_async_issue_pending(dma->rxchan);
+	mod_timer(&priv->rx_dma_wd, jiffies + HZ / 4);
 out:
 	spin_unlock_irqrestore(&priv->rx_dma_lock, flags);
 	return err;
@@ -1044,6 +1058,9 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 		dma_err = omap_8250_rx_dma(up, iir);
 		if (dma_err) {
 			status = serial8250_rx_chars(up, status);
+
+			up->ier &= ~UART_IER_RDI;
+			serial_port_out(port, UART_IER, up->ier);
 			omap_8250_rx_dma(up, 0);
 		}
 	}
@@ -1069,6 +1086,20 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 	return 1;
 }
 
+static void omap8250_rx_dma_wd(unsigned long data)
+{
+	struct uart_8250_port	*up = (void *) data;
+	struct uart_port	*port = &up->port;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	omap_8250_rx_dma_flush(up);
+	up->ier |= UART_IER_RDI;
+	serial_port_out(port, UART_IER, up->ier);
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
 static bool the_no_dma_filter_fn(struct dma_chan *chan, void *param)
 {
 	return false;
-- 
2.5.0

Re: [RFC 3/3] serial: 8250_omap: try to avoid IER_RDI with DMA

[Sekhar Nori]

On Friday 14 August 2015 09:31 PM, Sebastian Andrzej Siewior wrote:
> It has been observed on DRA7-evm that the UART triggers the interrupt and
> reading IIR says IIR_NO_INT. It seems that we receive data via RX-DMA
> but the interrupt is triggered anyway. I have hardly observed it on
> AM335x and not in *that* quantity. On DRA7-evm with continuous transfers
> at 3MBaud and CPU running at 1.5Ghz it is possible that the IRQ-core will
> close the UART interrupt after "some time" with "nobody cared".
> 
> I've seen that by not enabling IER_RDI those spurious interrupts are not
> triggered. Also it seems that DMA and RDI cause the "timeout interrupt"
> which does not allow RX-DMA to be scheduled even if the FIFO reached the
> programmed RX threshold. However without RDI we don't get a notification
> if we have less than RX threshold bytes in the FIFO.
> This is where we have the rx_dma_wd timer. After programming the RX-DMA
> transfer wait HZ / 4 or 250ms for it to complete. If it does not
> complete in that time span we cancel the DMA transfer and enable RDI.
> RDI will trigger an UART interrupt in case we have bytes in the FIFO.
> Once we read bytes manually from the FIFO we enable RX-DMA again
> (without RDI) with the same 250ms timeout.
> 
> One downside with this approach is that latency sensitive protocols that
> transfer less than 48 bytes will have to wait 250ms to complete.

I guess because of this reason, wlink8 bluetooth connected to TI's DRA7
EVM failed to probe with this patch included. At the least, looks like
this patch needs some tuning.

> 
> Is there maybe a user interface where one could set small or bulk transfers?
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Thanks,
Sekhar

Re: [PATCH 1/3] serial: 8250: move rx_running out of the bitfield

[Sekhar Nori]

On Friday 14 August 2015 09:31 PM, Sebastian Andrzej Siewior wrote:
> From: John Ogness <john.ogness@linutronix.de>
> 
> That bitfield is modified by read + or + write operation. If someone
> sets any of the other two bits it might render the lock useless.
> 
> While at it, remove other bitfields as well to avoid more such
> errors.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Tested with wilink BT module on TI's DRA7 EVM.

Tested-by: Sekhar Nori <nsekhar@ti.com>

Thanks,
Sekhar

Re: [PATCH 1/3] serial: 8250: move rx_running out of the bitfield

[Peter Hurley]

On 08/26/2015 05:37 AM, Sekhar Nori wrote:
> On Friday 14 August 2015 09:31 PM, Sebastian Andrzej Siewior wrote:
>> From: John Ogness <john.ogness@linutronix.de>
>>
>> That bitfield is modified by read + or + write operation. If someone
>> sets any of the other two bits it might render the lock useless.
>>
>> While at it, remove other bitfields as well to avoid more such
>> errors.
>>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: John Ogness <john.ogness@linutronix.de>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Tested with wilink BT module on TI's DRA7 EVM.
> 
> Tested-by: Sekhar Nori <nsekhar@ti.com>

Already in Greg's tty-next tree (and 4.3-rc1 pull request), Sekhar.

Regards,
Peter Hurley

Re: [PATCH 1/3] serial: 8250: move rx_running out of the bitfield

[Sekhar Nori]

On Wednesday 26 August 2015 06:13 PM, Peter Hurley wrote:
> On 08/26/2015 05:37 AM, Sekhar Nori wrote:
>> On Friday 14 August 2015 09:31 PM, Sebastian Andrzej Siewior wrote:
>>> From: John Ogness <john.ogness@linutronix.de>
>>>
>>> That bitfield is modified by read + or + write operation. If someone
>>> sets any of the other two bits it might render the lock useless.
>>>
>>> While at it, remove other bitfields as well to avoid more such
>>> errors.
>>>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Signed-off-by: John Ogness <john.ogness@linutronix.de>
>>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>>
>> Tested with wilink BT module on TI's DRA7 EVM.
>>
>> Tested-by: Sekhar Nori <nsekhar@ti.com>
> 
> Already in Greg's tty-next tree (and 4.3-rc1 pull request), Sekhar.

Oops, no problem. Did not notice that.

Thanks,
Sekhar