[PATCH] xen: more robust serial port driver

[PATCH] xen: more robust serial port driver

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 422 bytes --]


Hi!

Attached patch adds a check if the fifo is usable before we
actually use it.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_serial.diff --]
[-- Type: text/x-diff, Size: 1736 bytes --]

diff -r 8af26fef898c xen/drivers/char/ns16550.c
--- a/xen/drivers/char/ns16550.c	Fri Jul 24 12:08:54 2009 +0100
+++ b/xen/drivers/char/ns16550.c	Tue Jul 28 16:01:48 2009 +0200
@@ -16,6 +16,7 @@
 #include <xen/serial.h>
 #include <xen/iocap.h>
 #include <asm/io.h>
+#include <xen/delay.h>
 
 /*
  * Configure serial port with a string:
@@ -115,8 +116,10 @@ static char ns_read_reg(struct ns16550 *
 
 static void ns_write_reg(struct ns16550 *uart, int reg, char c)
 {
-    if ( uart->remapped_io_base == NULL )
-        return outb(c, uart->io_base + reg);
+    if ( uart->remapped_io_base == NULL ) {
+        outb(c, uart->io_base + reg);
+        return;
+    }
     writeb(c, uart->remapped_io_base + reg);
 }
 
@@ -181,6 +184,7 @@ static void __devinit ns16550_init_preir
     unsigned int  divisor;
 
     /* I/O ports are distinguished by their size (16 bits). */
+    uart->remapped_io_base = NULL;
     if ( uart->io_base >= 0x10000 )
         uart->remapped_io_base = (char *)ioremap(uart->io_base, 8);
 
@@ -212,10 +216,15 @@ static void __devinit ns16550_init_preir
 
     /* Enable and clear the FIFOs. Set a large trigger threshold. */
     ns_write_reg(uart, FCR, FCR_ENABLE | FCR_CLRX | FCR_CLTX | FCR_TRG14);
+    udelay(100);
 
     /* Check this really is a 16550+. Otherwise we have no FIFOs. */
-    if ( (ns_read_reg(uart, IIR) & 0xc0) == 0xc0 )
-        port->tx_fifo_size = 16;
+    port->tx_fifo_size = 1;
+    if ( (ns_read_reg(uart, IIR) & 0xc0) == 0xc0 ) {
+        /* We have a FIFO. Check if we have a *working* FIFO. */
+        if ( (ns_read_reg(uart, FCR) & FCR_TRG14) == FCR_TRG14 )
+            port->tx_fifo_size = 16;
+    }
 }
 
 static void __devinit ns16550_init_postirq(struct serial_port *port)

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] xen: more robust serial port driver

[Keir Fraser]

On 28/07/2009 15:07, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

> 
> Attached patch adds a check if the fifo is usable before we
> actually use it.

I count that at least the first two chunks and the initialisation of
tx_fifo_size are unnecessary. Might this be the case for the udelay(100) as
well? And what kinds of systems have these broken UARTs that half-advertise
a broken/non-existent FIFO?

 -- Keir

Re: [PATCH] xen: more robust serial port driver

[Christoph Egger]

On Tuesday 28 July 2009 16:15:42 Keir Fraser wrote:
> On 28/07/2009 15:07, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> > Attached patch adds a check if the fifo is usable before we
> > actually use it.
>
> I count that at least the first two chunks and the initialisation of
> tx_fifo_size are unnecessary.

First hunk:

    It looks suspicious returning a value of type void.

Second hunk and initialization of tx_fifo_size:

    If you are sure these initializations are duplicates, then they are
    unnecessary.

> Might this be the case for the udelay(100) as 
> well?

No. This ensures, that the write takes effect before the read happens.

> And what kinds of systems have these broken UARTs that half-advertise 
> a broken/non-existent FIFO?

The original ns16550 has a broken FIFO. The ns16450 has no FIFO.
There are simulators which simulate those old things instead of a ns16550a.

Christoph

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] xen: more robust serial port driver

[Keir Fraser]

On 28/07/2009 15:47, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

>> Might this be the case for the udelay(100) as
>> well?
> 
> No. This ensures, that the write takes effect before the read happens.

Can a read really overtake a write in this case? Seriously?

>> And what kinds of systems have these broken UARTs that half-advertise
>> a broken/non-existent FIFO?
> 
> The original ns16550 has a broken FIFO. The ns16450 has no FIFO.
> There are simulators which simulate those old things instead of a ns16550a.

Hm, well, okay, the actual one line core of this patch actually seems okay
to me. It's all the rest I want to drop on the floor.

 -- Keir

Re: [PATCH] xen: more robust serial port driver

[Christoph Egger]

On Tuesday 28 July 2009 16:58:41 Keir Fraser wrote:
> On 28/07/2009 15:47, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> >> Might this be the case for the udelay(100) as
> >> well?
> >
> > No. This ensures, that the write takes effect before the read happens.
>
> Can a read really overtake a write in this case? Seriously?

In real world this can happen on embedded devices, Xen doesn't support.
In simulators, things may differ from real hw.

> >> And what kinds of systems have these broken UARTs that half-advertise
> >> a broken/non-existent FIFO?
> >
> > The original ns16550 has a broken FIFO. The ns16450 has no FIFO.
> > There are simulators which simulate those old things instead of a
> > ns16550a.
>
> Hm, well, okay, the actual one line core of this patch actually seems okay
> to me. It's all the rest I want to drop on the floor.

Oh, I just noticed that there's a missing '__init' in the function definition
of check_existence. Please add it. Tnx.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632