ARM i.MX serial: fix tx buffer overflows
diff mbox series

Message ID 20070105155144.GD5838@localhost.localdomain
State New, archived
Headers show
Series
  • ARM i.MX serial: fix tx buffer overflows
Related show

Commit Message

Sascha Hauer Jan. 5, 2007, 3:51 p.m. UTC
This patch fixes occasional tx buffer overflows in the i.MX serial
driver which came from the fact that space in the buffer was checked
after sending the first byte. Also, fifosize is 32 bytes, not 8.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de

---
 drivers/serial/imx.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)


-
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/

Comments

Pavel Pisa Jan. 5, 2007, 6:01 p.m. UTC | #1
On Friday 05 January 2007 16:51, Sascha Hauer wrote:
> This patch fixes occasional tx buffer overflows in the i.MX serial
> driver which came from the fact that space in the buffer was checked
> after sending the first byte. Also, fifosize is 32 bytes, not 8.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de

Acked-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>

Hello Sascha,

I have tested your change on 2.6.19-rt14 kernel
which I have on hand there. It is only very short
test, but I have not noticed any problems.

In the fact, I think, that it is possible, that
I have noticed some mentioned problem during
my CPU-Freq testing on RT kernels before.
I have noticed some problems sometimes with BusyBox
shell history editing during frequency throttling.
May it be, that RT+changed frequency invoked the problem.

I have some other small fix for i.MX uart there.
Can you add it into patch, or should I send it
as separate one-liner?

If RTS interrupt is caused by RTS senzing logic inside
i.MX UART module the IRQ type cannot be set.

It applies only for interrupts going through GPIO layer.
The problem has been noticed by Konstantin Kletschke
some time ago.

  No IRQF_TRIGGER set_type function for IRQ 26 (MPU)

I would not change type to fixed 0, because it could
be possible to use different GPIO MX1 pin for RTS
in the theory. On the other hand it is only for documentation
purposes now, because RTS read code would have to be adjusted
in such case.

Health and success wishes
in year 2007 from

                     Pavel Pisa

---

 drivers/serial/imx.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6.19-rt/drivers/serial/imx.c
===================================================================
--- linux-2.6.19-rt.orig/drivers/serial/imx.c
+++ linux-2.6.19-rt/drivers/serial/imx.c
@@ -403,7 +403,8 @@ static int imx_startup(struct uart_port 
 	if (retval) goto error_out2;
 
 	retval = request_irq(sport->rtsirq, imx_rtsint,
-			     IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			     (sport->rtsirq < IMX_IRQS) ? 0 :
+			       IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
 			     DRIVER_NAME, sport);
 	if (retval) goto error_out3;
 




-
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/
Konstantin Kletschke Jan. 29, 2007, 2:47 p.m. UTC | #2
Am 2007-01-05 19:01 +0100 schrieb Pavel Pisa:

> It applies only for interrupts going through GPIO layer.
> The problem has been noticed by Konstantin Kletschke
> some time ago.
> 
>   No IRQF_TRIGGER set_type function for IRQ 26 (MPU)

Yes. I reported this also.

>  drivers/serial/imx.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.19-rt/drivers/serial/imx.c
> ===================================================================
> +++ linux-2.6.19-rt/drivers/serial/imx.c
> @@ -403,7 +403,8 @@ static int imx_startup(struct uart_port 
>  	if (retval) goto error_out2;
>  
>  	retval = request_irq(sport->rtsirq, imx_rtsint,
> -			     IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> +			     (sport->rtsirq < IMX_IRQS) ? 0 :
> +			       IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
>  			     DRIVER_NAME, sport);
>  	if (retval) goto error_out3;


Okay, this indeed fixes my

No IRQF_TRIGGER set_type function for IRQ 26 (MPU)

here!

Regards, Konsti
Russell King Jan. 29, 2007, 3:37 p.m. UTC | #3
On Mon, Jan 29, 2007 at 03:47:59PM +0100, Konstantin Kletschke wrote:
> Am 2007-01-05 19:01 +0100 schrieb Pavel Pisa:
> 
> > It applies only for interrupts going through GPIO layer.
> > The problem has been noticed by Konstantin Kletschke
> > some time ago.
> > 
> >   No IRQF_TRIGGER set_type function for IRQ 26 (MPU)
> 
> Yes. I reported this also.
> 
> >  drivers/serial/imx.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6.19-rt/drivers/serial/imx.c
> > ===================================================================
> > +++ linux-2.6.19-rt/drivers/serial/imx.c
> > @@ -403,7 +403,8 @@ static int imx_startup(struct uart_port 
> >  	if (retval) goto error_out2;
> >  
> >  	retval = request_irq(sport->rtsirq, imx_rtsint,
> > -			     IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> > +			     (sport->rtsirq < IMX_IRQS) ? 0 :
> > +			       IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> >  			     DRIVER_NAME, sport);
> >  	if (retval) goto error_out3;
> 
> 
> Okay, this indeed fixes my
> 
> No IRQF_TRIGGER set_type function for IRQ 26 (MPU)

Is it really worth adding additional code to shut up this (imho) silly
warning?  It's just adding needless complexity to drivers.

What happens if a driver is used on multiple platforms, some of which
support trigger setting and others which don't?  Are we going to end
up with a large #ifdef in every driver?
Konstantin Kletschke Jan. 29, 2007, 4:43 p.m. UTC | #4
Am 2007-01-29 15:37 +0000 schrieb Russell King:

> Is it really worth adding additional code to shut up this (imho) silly
> warning?  It's just adding needless complexity to drivers.

As I pointed out in 
http://lists.arm.linux.org.uk/pipermail/linux-arm-kernel/2006-November/037192.html
the console gets flooded with this "warning" to unusable state sometimes.

> What happens if a driver is used on multiple platforms, some of which
> support trigger setting and others which don't?  Are we going to end
> up with a large #ifdef in every driver?

I don't know exactly. But in addition to the fact, that this warning
floods my console to unusable state I am used to sell software without
warnings. If there are warnings my boss some things are broken.

Konsti

Patch
diff mbox series

Index: linux-2.6.20-rc3/drivers/serial/imx.c
===================================================================
--- linux-2.6.20-rc3.orig/drivers/serial/imx.c
+++ linux-2.6.20-rc3/drivers/serial/imx.c
@@ -154,7 +154,7 @@  static inline void imx_transmit_buffer(s
 {
 	struct circ_buf *xmit = &sport->port.info->xmit;
 
-	do {
+	while (!(UTS((u32)sport->port.membase) & UTS_TXFULL)) {
 		/* send xmit->buf[xmit->tail]
 		 * out the port here */
 		URTX0((u32)sport->port.membase) = xmit->buf[xmit->tail];
@@ -163,7 +163,7 @@  static inline void imx_transmit_buffer(s
 		sport->port.icount.tx++;
 		if (uart_circ_empty(xmit))
 			break;
-	} while (!(UTS((u32)sport->port.membase) & UTS_TXFULL));
+	}
 
 	if (uart_circ_empty(xmit))
 		imx_stop_tx(&sport->port);
@@ -178,8 +178,7 @@  static void imx_start_tx(struct uart_por
 
 	UCR1((u32)sport->port.membase) |= UCR1_TXMPTYEN;
 
-	if(UTS((u32)sport->port.membase) & UTS_TXEMPTY)
-		imx_transmit_buffer(sport);
+	imx_transmit_buffer(sport);
 }
 
 static irqreturn_t imx_rtsint(int irq, void *dev_id)
@@ -678,7 +677,7 @@  static struct imx_port imx_ports[] = {
 		.mapbase	= IMX_UART1_BASE, /* FIXME */
 		.irq		= UART1_MINT_RX,
 		.uartclk	= 16000000,
-		.fifosize	= 8,
+		.fifosize	= 32,
 		.flags		= UPF_BOOT_AUTOCONF,
 		.ops		= &imx_pops,
 		.line		= 0,
@@ -694,7 +693,7 @@  static struct imx_port imx_ports[] = {
 		.mapbase	= IMX_UART2_BASE, /* FIXME */
 		.irq		= UART2_MINT_RX,
 		.uartclk	= 16000000,
-		.fifosize	= 8,
+		.fifosize	= 32,
 		.flags		= UPF_BOOT_AUTOCONF,
 		.ops		= &imx_pops,
 		.line		= 1,