GTA01 shared UART vs. flow control (bug #788)

[Mike (mwester)]

Proposed simple solutions - comments welcome, even more welcome would be an 
explicit statement of which solution will be accepted into the 2.6.24 kernel 
for the GTA01.

I propose four variants to fix this problem in a manner that will resolve 
the objections presented by the earlier patches.  Based on the excellent 
feedback on this topic last time it was discussed, I restate the problem in 
a different way:

--> A problem exists in the serial driver wherein the kernel can hang 
indefinitely if hardware flow-control is asserted on a serial port that is 
also serving as the console for the device.

By way of background, I did look at a random selection of other serial 
drivers to see how the console is handled in event of flow control.  It 
seems that in general, flow control is simply ignored by the drivers for 
console output -- resulting in console output being transmitted regardless 
of flow control in the case where the driver handles flow control.  The 
general structure is that kernel console output will busy-wait on the serial 
port until the UART accepts the next character (clearly a "chatty" console 
is a waste of CPU cyles).  So on most drivers with dumb ports, console 
output will not lock up the kernel -- it will transmit a character despite 
flow control being asserted, and presumably the device on the other end that 
asserted flow control in the first place will drop the console output and 
report a buffer overrun or some other error.  So clearly, there is no kernel 
guarantee that flow control is honored in the first place.

---> Option 1:
We avoid the problem entirely by pro-actively dropping the character to be 
transmitted if flow-control is enabled:

 s3c24xx_serial_console_putchar(struct uart_port *port, int ch)
 {
        unsigned int ufcon = rd_regl(cons_uart, S3C2410_UFCON);
+       unsigned int umcon = rd_regl(cons_uart, S3C2410_UMCON);
+
+       /* If auto HW flow control enabled, don't risk transmitting. */
+       if (umcon & S3C2410_UMCOM_AFC)
+               return;
+
        while (!s3c24xx_serial_console_txrdy(port, ufcon))
                barrier();
        wr_regb(cons_uart, S3C2410_UTXH, ch);
 }

Ideally we would only do this is flow-control is *asserted*, but when 
hardware flow-control is enabled, there doesn't seem to be a good way to 
determine this.  It could also be argued that we could check if the FIFO has 
space left before we just drop the character, but given that the FIFO is far 
smaller than most kernel printk() messages, that just delays the inevitable.

---> Option 2:
We attempt to duplicate the behavior of the typical "dumb" serial driver, 
and transmit anyway by temporarily disabling auto-flow-control mode:

 s3c24xx_serial_console_putchar(struct uart_port *port, int ch)
 {
        unsigned int ufcon = rd_regl(cons_uart, S3C2410_UFCON);
+       unsigned int umcon = rd_regl(cons_uart, S3C2410_UMCON);
+
+       /* If auto HW flow control enabled, temporarily turn it off */
+       if (umcon & S3C2410_UMCOM_AFC)
+               wr_regl(port, S3C2410_UMCON, (umcon & !S3C2410_UMCOM_AFC));
+
        while (!s3c24xx_serial_console_txrdy(port, ufcon))
                barrier();
        wr_regb(cons_uart, S3C2410_UTXH, ch);
+
+       if (umcon & S3C2410_UMCOM_AFC)
+               wr_regl(port, S3C2410_UMCON, umcon);
 }

This may be a better solution, but I'm just not sure if the extra complexity 
is worth it.  Once particular problem is that by doing this, we've not 
*really* sent the console character, we've let the first character in the 
FIFO transmit -- and there's no guarantee in the general case that the first 
character is console output.  Also, I'm concerned about timing issues here; 
there's no locking anywhere in the driver -- is there a possibility that bad 
timing could result in an IRQ being generated as the FIFO empties that would 
trigger other code in this driver to fill it up again?

---> Option 3:
 Simply disallow auto HW flow-control entirely if the port is used as a 
console.  The code is similar to the above; we simply test if the AFC is 
enabled, and if so we disable it and leave it that way.  This option 
duplicates the behavior of the patch I submitted for bug #788, but in a far 
less intrusive and far simpler manner.

---> Option 4:
  Add a timer to Option 1 or 3.  This was proposed in earlier discussions. 
If we hard-code a value for the timer, this is not a difficult thing to do. 
However, after examining what other serial drivers do with regard to flow 
control (i.e. they ignore it), I wonder if the added complexity is really 
worthwhile.  Also, if the value is to be configurable (kconfig?), the patch 
really does become intrusive once again.


I welcome comments, and hope we can agree on a solution that we can 
implement in the 2.6.24 kernel for the neo.

Regards,
Mike (mwester)