[PATCH] ARM: imx serial driver: fix resume

[PATCH] ARM: imx serial driver: fix resume

[Daniel Mack]

From: Volker Ernst <volker.ernst@txtr.com>

I just came across a bug in the IMX31 serial driver which is still
present in the newest kernels and which prevents successful
resume-operation for the IMX31 serial ports.

What happens is that in "drivers/serial/imx.c" on resume function
"serial_imx_resume" gets called. This function in turn calls
"uart_resume_port" (in the generic serial driver "serial_core.c"),
which in turn calls "imx_start_tx" in "imx.c" (in case the SIO-port
was really suspended) which in turn calls "imx_transmit_buffer".

However calling "imx_transmit_buffer" with an empty TX-fifo (as is
usually the case) will result in the serial port starting to transmit
(actually the old [already sent] tx-buffer), as there is no check if
the tx-buffer is empty before starting to feed tx-fifo-data to the
serial port hardware.

Signed-off-by: Volker Ernst <volker.ernst@txtr.com>
Cc: Daniel Mack <daniel@caiaq.de>
Cc: Andy Green <andy@warmcat.com>
---

Volker did all the work on this, he just doesn't want to push his great
contributions upstream, so I do it for him :)


 drivers/serial/imx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/serial/imx.c b/drivers/serial/imx.c
index 66ecc7a..0170119 100644
--- a/drivers/serial/imx.c
+++ b/drivers/serial/imx.c
@@ -328,13 +328,13 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
 	struct circ_buf *xmit = &sport->port.state->xmit;
 
 	while (!(readl(sport->port.membase + UTS) & UTS_TXFULL)) {
+		if (uart_circ_empty(xmit))
+			break;
 		/* send xmit->buf[xmit->tail]
 		 * out the port here */
 		writel(xmit->buf[xmit->tail], sport->port.membase + URTX0);
 		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
 		sport->port.icount.tx++;
-		if (uart_circ_empty(xmit))
-			break;
 	}
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
-- 
1.7.0.4

[PATCH] ARM: imx serial driver: fix resume

[Andrew Morton]

On Wed,  6 Oct 2010 18:57:16 +0200
Daniel Mack <daniel@caiaq.de> wrote:

> From: Volker Ernst <volker.ernst@txtr.com>
> 
> I just came across a bug in the IMX31 serial driver which is still
> present in the newest kernels and which prevents successful
> resume-operation for the IMX31 serial ports.
> 
> What happens is that in "drivers/serial/imx.c" on resume function
> "serial_imx_resume" gets called. This function in turn calls
> "uart_resume_port" (in the generic serial driver "serial_core.c"),
> which in turn calls "imx_start_tx" in "imx.c" (in case the SIO-port
> was really suspended) which in turn calls "imx_transmit_buffer".
> 
> However calling "imx_transmit_buffer" with an empty TX-fifo (as is
> usually the case) will result in the serial port starting to transmit
> (actually the old [already sent] tx-buffer), as there is no check if
> the tx-buffer is empty before starting to feed tx-fifo-data to the
> serial port hardware.
> 
> Signed-off-by: Volker Ernst <volker.ernst@txtr.com>
> Cc: Daniel Mack <daniel@caiaq.de>

This should have been Signed-off-by:, as you were on the patch delivery
path.

I made that change to my copy of the patch, thanks.

> Cc: Andy Green <andy@warmcat.com>

[PATCH] ARM: imx serial driver: fix resume

[Uwe Kleine-König]

Hello Daniel (and Volker),

On Wed, Oct 06, 2010 at 06:57:16PM +0200, Daniel Mack wrote:
> From: Volker Ernst <volker.ernst@txtr.com>
> 
> I just came across a bug in the IMX31 serial driver which is still
> present in the newest kernels and which prevents successful
> resume-operation for the IMX31 serial ports.
> 
> What happens is that in "drivers/serial/imx.c" on resume function
> "serial_imx_resume" gets called. This function in turn calls
> "uart_resume_port" (in the generic serial driver "serial_core.c"),
> which in turn calls "imx_start_tx" in "imx.c" (in case the SIO-port
> was really suspended) which in turn calls "imx_transmit_buffer".
> 
> However calling "imx_transmit_buffer" with an empty TX-fifo (as is
> usually the case) will result in the serial port starting to transmit
> (actually the old [already sent] tx-buffer), as there is no check if
> the tx-buffer is empty before starting to feed tx-fifo-data to the
> serial port hardware.
> 
> Signed-off-by: Volker Ernst <volker.ernst@txtr.com>
> Cc: Daniel Mack <daniel@caiaq.de>
> Cc: Andy Green <andy@warmcat.com>
> ---
> 
> Volker did all the work on this, he just doesn't want to push his great
> contributions upstream, so I do it for him :)
> 
> 
>  drivers/serial/imx.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/serial/imx.c b/drivers/serial/imx.c
> index 66ecc7a..0170119 100644
> --- a/drivers/serial/imx.c
> +++ b/drivers/serial/imx.c
> @@ -328,13 +328,13 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
>  	struct circ_buf *xmit = &sport->port.state->xmit;
>  
>  	while (!(readl(sport->port.membase + UTS) & UTS_TXFULL)) {
> +		if (uart_circ_empty(xmit))
> +			break;
As readl is a hardware access and uart_circ_empty "only" needs RAM, I
wonder if using

	while (!uart_circ_empty(xmit) &&
		!(readl(sport->port.membase + UTS) & UTS_TXFULL)) {

would be a tad smarter.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] ARM: imx serial driver: fix resume

[Uwe Kleine-König]

Hallo Daniel und Volker,

On Wed, Oct 06, 2010 at 06:57:16PM +0200, Daniel Mack wrote:
> From: Volker Ernst <volker.ernst@txtr.com>
> 
> I just came across a bug in the IMX31 serial driver which is still
> present in the newest kernels and which prevents successful
> resume-operation for the IMX31 serial ports.
> 
> What happens is that in "drivers/serial/imx.c" on resume function
> "serial_imx_resume" gets called. This function in turn calls
> "uart_resume_port" (in the generic serial driver "serial_core.c"),
> which in turn calls "imx_start_tx" in "imx.c" (in case the SIO-port
> was really suspended) which in turn calls "imx_transmit_buffer".
> 
> However calling "imx_transmit_buffer" with an empty TX-fifo (as is
> usually the case) will result in the serial port starting to transmit
> (actually the old [already sent] tx-buffer), as there is no check if
> the tx-buffer is empty before starting to feed tx-fifo-data to the
> serial port hardware.
> 
> Signed-off-by: Volker Ernst <volker.ernst@txtr.com>
> Cc: Daniel Mack <daniel@caiaq.de>
> Cc: Andy Green <andy@warmcat.com>
> ---
> 
> Volker did all the work on this, he just doesn't want to push his great
> contributions upstream, so I do it for him :)
> 
> 
>  drivers/serial/imx.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/serial/imx.c b/drivers/serial/imx.c
> index 66ecc7a..0170119 100644
> --- a/drivers/serial/imx.c
> +++ b/drivers/serial/imx.c
> @@ -328,13 +328,13 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
>  	struct circ_buf *xmit = &sport->port.state->xmit;
>  
>  	while (!(readl(sport->port.membase + UTS) & UTS_TXFULL)) {
> +		if (uart_circ_empty(xmit))
> +			break;
>  		/* send xmit->buf[xmit->tail]
>  		 * out the port here */
>  		writel(xmit->buf[xmit->tail], sport->port.membase + URTX0);
>  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>  		sport->port.icount.tx++;
> -		if (uart_circ_empty(xmit))
> -			break;
>  	}
>  
>  	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
I needed that patch now, too, though in a different situation.  I havn't
investigated the details yet, but I think the problem is that
imx_transmit_buffer is called, too, when using handshaking and the other
side starts to be ready.

You can count that as an Acked-by: me, preferably with the check added
to the while condition as suggested in a different mail of this thread.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] ARM: imx serial driver: fix resume

[Daniel Mack]

On Tue, Oct 12, 2010 at 07:56:58PM +0200, Uwe Kleine-K?nig wrote:
> On Wed, Oct 06, 2010 at 06:57:16PM +0200, Daniel Mack wrote:
> > From: Volker Ernst <volker.ernst@txtr.com>
> > 
> > I just came across a bug in the IMX31 serial driver which is still
> > present in the newest kernels and which prevents successful
> > resume-operation for the IMX31 serial ports.
> > 
> > What happens is that in "drivers/serial/imx.c" on resume function
> > "serial_imx_resume" gets called. This function in turn calls
> > "uart_resume_port" (in the generic serial driver "serial_core.c"),
> > which in turn calls "imx_start_tx" in "imx.c" (in case the SIO-port
> > was really suspended) which in turn calls "imx_transmit_buffer".
> > 
> > However calling "imx_transmit_buffer" with an empty TX-fifo (as is
> > usually the case) will result in the serial port starting to transmit
> > (actually the old [already sent] tx-buffer), as there is no check if
> > the tx-buffer is empty before starting to feed tx-fifo-data to the
> > serial port hardware.
> > 
> > Signed-off-by: Volker Ernst <volker.ernst@txtr.com>
> > Cc: Daniel Mack <daniel@caiaq.de>
> > Cc: Andy Green <andy@warmcat.com>
> > ---
> > 
> > Volker did all the work on this, he just doesn't want to push his great
> > contributions upstream, so I do it for him :)
> > 
> > 
> >  drivers/serial/imx.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/serial/imx.c b/drivers/serial/imx.c
> > index 66ecc7a..0170119 100644
> > --- a/drivers/serial/imx.c
> > +++ b/drivers/serial/imx.c
> > @@ -328,13 +328,13 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
> >  	struct circ_buf *xmit = &sport->port.state->xmit;
> >  
> >  	while (!(readl(sport->port.membase + UTS) & UTS_TXFULL)) {
> > +		if (uart_circ_empty(xmit))
> > +			break;
> >  		/* send xmit->buf[xmit->tail]
> >  		 * out the port here */
> >  		writel(xmit->buf[xmit->tail], sport->port.membase + URTX0);
> >  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> >  		sport->port.icount.tx++;
> > -		if (uart_circ_empty(xmit))
> > -			break;
> >  	}
> >  
> >  	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> I needed that patch now, too, though in a different situation.  I havn't
> investigated the details yet, but I think the problem is that
> imx_transmit_buffer is called, too, when using handshaking and the other
> side starts to be ready.
> 
> You can count that as an Acked-by: me, preferably with the check added
> to the while condition as suggested in a different mail of this thread.

Ok, thanks. However, I can not test this myself - Volker, can you try
the patch Uwe sent along some days ago?

Thanks,
Daniel

[PATCH] ARM: imx serial driver: fix resume

[Uwe Kleine-König]

On Tue, Oct 12, 2010 at 08:32:39PM +0200, Daniel Mack wrote:
> On Tue, Oct 12, 2010 at 07:56:58PM +0200, Uwe Kleine-K?nig wrote:
> > On Wed, Oct 06, 2010 at 06:57:16PM +0200, Daniel Mack wrote:
> > > From: Volker Ernst <volker.ernst@txtr.com>
> > > 
> > > I just came across a bug in the IMX31 serial driver which is still
> > > present in the newest kernels and which prevents successful
> > > resume-operation for the IMX31 serial ports.
> > > 
> > > What happens is that in "drivers/serial/imx.c" on resume function
> > > "serial_imx_resume" gets called. This function in turn calls
> > > "uart_resume_port" (in the generic serial driver "serial_core.c"),
> > > which in turn calls "imx_start_tx" in "imx.c" (in case the SIO-port
> > > was really suspended) which in turn calls "imx_transmit_buffer".
> > > 
> > > However calling "imx_transmit_buffer" with an empty TX-fifo (as is
> > > usually the case) will result in the serial port starting to transmit
> > > (actually the old [already sent] tx-buffer), as there is no check if
> > > the tx-buffer is empty before starting to feed tx-fifo-data to the
> > > serial port hardware.
> > > 
> > > Signed-off-by: Volker Ernst <volker.ernst@txtr.com>
> > > Cc: Daniel Mack <daniel@caiaq.de>
> > > Cc: Andy Green <andy@warmcat.com>
> > > ---
> > > 
> > > Volker did all the work on this, he just doesn't want to push his great
> > > contributions upstream, so I do it for him :)
> > > 
> > > 
> > >  drivers/serial/imx.c |    4 ++--
> > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/serial/imx.c b/drivers/serial/imx.c
> > > index 66ecc7a..0170119 100644
> > > --- a/drivers/serial/imx.c
> > > +++ b/drivers/serial/imx.c
> > > @@ -328,13 +328,13 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
> > >  	struct circ_buf *xmit = &sport->port.state->xmit;
> > >  
> > >  	while (!(readl(sport->port.membase + UTS) & UTS_TXFULL)) {
> > > +		if (uart_circ_empty(xmit))
> > > +			break;
> > >  		/* send xmit->buf[xmit->tail]
> > >  		 * out the port here */
> > >  		writel(xmit->buf[xmit->tail], sport->port.membase + URTX0);
> > >  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> > >  		sport->port.icount.tx++;
> > > -		if (uart_circ_empty(xmit))
> > > -			break;
> > >  	}
> > >  
> > >  	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> > I needed that patch now, too, though in a different situation.  I havn't
> > investigated the details yet, but I think the problem is that
> > imx_transmit_buffer is called, too, when using handshaking and the other
> > side starts to be ready.
> > 
> > You can count that as an Acked-by: me, preferably with the check added
> > to the while condition as suggested in a different mail of this thread.
> 
> Ok, thanks. However, I can not test this myself - Volker, can you try
> the patch Uwe sent along some days ago?
I'm currently working on an other issue with the driver that will result
in a few patches.  So I can just pick up this patch en passant and look
to get it into mainline.

Still a fixed patch or an Ack would be fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] serial/imx: check that the buffer is non-empty before sending it out

[Uwe Kleine-König]

From: Volker Ernst <volker.ernst@txtr.com>

The .start_tx callback (imx_start_tx here) isn't only called when the
buffer is non-empty.  E.g. after resume or when handshaking is enabled
and the other side starts to signal being ready.

So check for an empty puffer already before sending the first character.
This prevents sending out stale (or uninitialised) data.

Signed-off-by: Volker Ernst <volker.ernst@txtr.com>
Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Andy Green <andy@warmcat.com>
[ukl: reword commit log, put check in while condition]
Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
Hello,

as I didn't sent a patch, here comes my current version.  Don't know if
it makes it easier to test for you ...

I faked Daniel's SoB as Andrew said he did, compressed the changelog a
bit and put the check in the while condition as I suggested.

Best regards
Uwe

 drivers/serial/imx.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/serial/imx.c b/drivers/serial/imx.c
index 66ecc7a..dfcf4b1 100644
--- a/drivers/serial/imx.c
+++ b/drivers/serial/imx.c
@@ -327,14 +327,13 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
 {
 	struct circ_buf *xmit = &sport->port.state->xmit;
 
-	while (!(readl(sport->port.membase + UTS) & UTS_TXFULL)) {
+	while (!uart_circ_empty(xmit) &&
+			!(readl(sport->port.membase + UTS) & UTS_TXFULL)) {
 		/* send xmit->buf[xmit->tail]
 		 * out the port here */
 		writel(xmit->buf[xmit->tail], sport->port.membase + URTX0);
 		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
 		sport->port.icount.tx++;
-		if (uart_circ_empty(xmit))
-			break;
 	}
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
-- 
1.7.2.3

[PATCH] serial/imx: check that the buffer is non-empty before sending it out

[Daniel Mack]

On Wed, Oct 13, 2010 at 11:03:57AM +0200, Uwe Kleine-K?nig wrote:
> From: Volker Ernst <volker.ernst@txtr.com>
> 
> The .start_tx callback (imx_start_tx here) isn't only called when the
> buffer is non-empty.  E.g. after resume or when handshaking is enabled
> and the other side starts to signal being ready.
> 
> So check for an empty puffer already before sending the first character.
> This prevents sending out stale (or uninitialised) data.
> 
> Signed-off-by: Volker Ernst <volker.ernst@txtr.com>
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Cc: Andy Green <andy@warmcat.com>
> [ukl: reword commit log, put check in while condition]
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> as I didn't sent a patch, here comes my current version.  Don't know if
> it makes it easier to test for you ...
> 
> I faked Daniel's SoB as Andrew said he did, compressed the changelog a
> bit and put the check in the while condition as I suggested.

FWIW, that looks good to me. Thanks for finishing the work :)


Daniel


>  drivers/serial/imx.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/serial/imx.c b/drivers/serial/imx.c
> index 66ecc7a..dfcf4b1 100644
> --- a/drivers/serial/imx.c
> +++ b/drivers/serial/imx.c
> @@ -327,14 +327,13 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
>  {
>  	struct circ_buf *xmit = &sport->port.state->xmit;
>  
> -	while (!(readl(sport->port.membase + UTS) & UTS_TXFULL)) {
> +	while (!uart_circ_empty(xmit) &&
> +			!(readl(sport->port.membase + UTS) & UTS_TXFULL)) {
>  		/* send xmit->buf[xmit->tail]
>  		 * out the port here */
>  		writel(xmit->buf[xmit->tail], sport->port.membase + URTX0);
>  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>  		sport->port.icount.tx++;
> -		if (uart_circ_empty(xmit))
> -			break;
>  	}
>  
>  	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> -- 
> 1.7.2.3
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] serial/imx: check that the buffer is non-empty before sending it out

[Greg KH]

On Thu, Oct 14, 2010 at 01:27:35PM +0200, Daniel Mack wrote:
> On Wed, Oct 13, 2010 at 11:03:57AM +0200, Uwe Kleine-K?nig wrote:
> > From: Volker Ernst <volker.ernst@txtr.com>
> > 
> > The .start_tx callback (imx_start_tx here) isn't only called when the
> > buffer is non-empty.  E.g. after resume or when handshaking is enabled
> > and the other side starts to signal being ready.
> > 
> > So check for an empty puffer already before sending the first character.
> > This prevents sending out stale (or uninitialised) data.
> > 
> > Signed-off-by: Volker Ernst <volker.ernst@txtr.com>
> > Signed-off-by: Daniel Mack <daniel@caiaq.de>
> > Cc: Andy Green <andy@warmcat.com>
> > [ukl: reword commit log, put check in while condition]
> > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > ---
> > Hello,
> > 
> > as I didn't sent a patch, here comes my current version.  Don't know if
> > it makes it easier to test for you ...
> > 
> > I faked Daniel's SoB as Andrew said he did, compressed the changelog a
> > bit and put the check in the while condition as I suggested.
> 
> FWIW, that looks good to me. Thanks for finishing the work :)

Nice, I've replaced the original patch in my tree with this one.

thanks,

greg k-h