On 01/20/2015 07:11 AM, Orson Zhai wrote:

Hi, Peter,

Thank you for reviewing our code!
Some discussion below.

On 2015?01?16? 23:20, Peter Hurley wrote:

quoted

On 01/16/2015 05:00 AM, Chunyan Zhang wrote:

quoted

Add a full sc9836-uart driver for SC9836 SoC which is based on the
spreadtrum sharkl64 platform.
This driver also support earlycon.
This patch also replaced the spaces between the macros and their
values with the tabs in serial_core.h

The locking doesn't look correct. Specific notations below.

quoted

quoted

+static inline void sprd_tx(int irq, void *dev_id)
+{
+    struct uart_port *port = dev_id;
+    struct circ_buf *xmit = &port->state->xmit;
+    int count;
+
+    if (port->x_char) {
+        serial_out(port, SPRD_TXD, port->x_char);
+        port->icount.tx++;
+        port->x_char = 0;
+        return;
+    }
+
+    if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
+        sprd_stop_tx(port);
+        return;
+    }
+
+    count = THLD_TX_EMPTY;
+    do {
+        serial_out(port, SPRD_TXD, xmit->buf[xmit->tail]);
+        xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
+        port->icount.tx++;
+        if (uart_circ_empty(xmit))
+            break;
+    } while (--count > 0);
+
+    if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+        uart_write_wakeup(port);
+
+    if (uart_circ_empty(xmit))
+        sprd_stop_tx(port);
+}
+
+/* this handles the interrupt from one port */
+static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
+{
+    struct uart_port *port = (struct uart_port *)dev_id;
+    unsigned int ims;

Why does your isr not have to take port->lock ?

The original consideration is the registers are accessed by isr only.
Interrupt will not be nested because of gic chip driver protection.
So there is not other thread will race on it.
Does this make sense?

The xmit->buf[] and its indexes could be accessed concurrently.
For example,

CPU 0                                      | CPU 1
                                           |
sprd_handle_irq                            | uart_flush_buffer
  sprd_tx                                  |   spin_lock_irqsave
    ...                                    |
    count = 64                             |
    do {                                   |     xmit->tail = 0
      serial_out(xmit->buf[xmit->tail])    |

      whoops - what byte did this just output?

I'm sure there's many more possible races, perhaps with worse
outcomes than just 1 bad byte output.

quoted

quoted

+    ims = serial_in(port, SPRD_IMSR);
+
+    if (!ims)
+        return IRQ_NONE;
+
+    serial_out(port, SPRD_ICLR, ~0);
+
+    if (ims & (SPRD_IMSR_RX_FIFO_FULL |
+        SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT))
+        sprd_rx(irq, port);
+
+    if (ims & SPRD_IMSR_TX_FIFO_EMPTY)
+        sprd_tx(irq, port);
+
+    return IRQ_HANDLED;
+}

[...]

quoted

quoted

+static void sprd_console_putchar(struct uart_port *port, int ch)
+{
+    wait_for_xmitr(port);
+    serial_out(port, SPRD_TXD, ch);
+}
+
+static void sprd_console_write(struct console *co, const char *s,
+                      unsigned int count)
+{
+    struct uart_port *port = &sprd_port[co->index]->port;
+    int ien;
+    int locked = 1;
+
+    if (oops_in_progress)
+        locked = spin_trylock(&port->lock);
+    else
+        spin_lock(&port->lock);

If you do need to take the port->lock in your isr, then you need to
disable local irq here.

You mean to use spin_lock_irqsave()?

We do disable irq below....

But not before an irq could happen with the spin_lock already taken.

printk
  ...
  sprd_console_write
    spin_lock
       <IRQ>
       sprd_handle_irq
         spin_lock

    ** DEADLOCK **

[
  Note: some drivers assume that console->write() will always be
  called with local interrupts disabled. This is a bad idea and I
  have warned those driver authors when this has come up before.
]

Also, since you handle sysrq in your isr the above needs to check
for non-zero port->sysrq and _not_ attempt the spinlock because
the isr will already have it; for example,

<IRQ>
sprd_handle_irq
  spin_lock
    sprd_rx
      ...
      uart_handle_syrq_char
        handle_sysrq
          __handle_sysrq
            printk
              ...
              sprd_console_write
                spin_lock

    ** DEADLOCK **

Regards,
Peter Hurley

quoted

quoted

+    /* save the IEN then disable the interrupts */
+    ien = serial_in(port, SPRD_IEN);
+    serial_out(port, SPRD_IEN, 0x0);

Here, we disable port IEN register.

quoted

quoted

+
+    uart_console_write(port, s, count, sprd_console_putchar);
+
+    /* wait for transmitter to become empty and restore the IEN */
+    wait_for_xmitr(port);
+    serial_out(port, SPRD_IEN, ien);
+    if (locked)
+        spin_unlock(&port->lock);
+}