Re: [PATCH v2 13/13] tty: pruss SUART driver

[Subhasish Ghosh]

I agree with the Codingstyle which needs to be fixed and would appreciate 
more feedback on it.

I have also gotten rid of the semaphore completely, please let me know what 
you feel of this implementation:
I have tested this without any problem.
What I am basically doing below is that, I am getting the data from the circ 
buff and then using the interrupt handler to
pump out the data. As the circ buff empties, I am accepting another request 
from the TTY.
Are you noticing any problems with this:

diff --git a/drivers/tty/serial/da8xx_pruss/pruss_suart.c 
b/drivers/tty/serial/da8xx_pruss/pruss_suart.c
index d222e2e..edc3863 100644
--- a/drivers/tty/serial/da8xx_pruss/pruss_suart.c
+++ b/drivers/tty/serial/da8xx_pruss/pruss_suart.c
@@ -72,7 +72,7 @@ struct suart_fifo {
struct omapl_pru_suart {
       struct uart_port port[NR_SUART];
       struct device *dev; /* pdev->dev */
-       struct semaphore port_sem[NR_SUART];
+       bool tx_empty[NR_SUART];
       struct clk *clk_mcasp;
       struct suart_fifo suart_fifo_addr[NR_SUART];
       const struct firmware *fw;
@@ -122,13 +122,10 @@ static void omapl_pru_tx_chars(struct omapl_pru_suart 
*soft_uart, u32 uart_no)
       if (!(suart_get_duplex(soft_uart, uart_no) & ePRU_SUART_HALF_TX))
               return;

-       if (down_trylock(&soft_uart->port_sem[uart_no]))
-               return;
-
       if (uart_circ_empty(xmit) ||
                       uart_tx_stopped(&soft_uart->port[uart_no])) {
               pruss_suart_stop_tx(&soft_uart->port[uart_no]);
-               up(&soft_uart->port_sem[uart_no]);
+               soft_uart->tx_empty[uart_no] = true;
               return;
       }

@@ -259,7 +256,6 @@ static irqreturn_t pruss_suart_interrupt(s32 irq, void 
*dev_id)
                       pru_intr_clr_isrstatus(dev, uart_num, PRU_TX_INTR);
                       pru_softuart_clr_tx_status(dev, 
&soft_uart->suart_hdl
                                                [port->line]);
-                       up(&soft_uart->port_sem[port->line]);
                       omapl_pru_tx_chars(soft_uart, port->line);
               }
       } while (txrx_flag & (PRU_RX_INTR | PRU_TX_INTR));
@@ -294,7 +290,10 @@ static void pruss_suart_start_tx(struct uart_port 
*port)

       suart_intr_setmask(dev, soft_uart->suart_hdl[port->line].uart_num,
                          PRU_TX_INTR, CHN_TXRX_IE_MASK_CMPLT);
-       omapl_pru_tx_chars(soft_uart, port->line);
+       if (soft_uart->tx_empty[port->line] == true) {
+               soft_uart->tx_empty[port->line] = false;
+               omapl_pru_tx_chars(soft_uart, port->line);
+       }
}


--------------------------------------------------
From: "Arnd Bergmann" <arnd@xxxxxxxx>
Sent: Saturday, February 19, 2011 12:21 AM
To: <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>
Cc: "Thomas Gleixner" <tglx@xxxxxxxxxxxxx>; "Alan Cox" 
<alan@xxxxxxxxxxxxxxxxxxx>; <sachi@xxxxxxxxxxxxxxxxxxxx>; 
<davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx>; "Subhasish Ghosh" 
<subhasish@xxxxxxxxxxxxxxxxxxxx>; "Greg Kroah-Hartman" <gregkh@xxxxxxx>; 
<nsekhar@xxxxxx>; "open list" <linux-kernel@xxxxxxxxxxxxxxx>; 
<m-watkins@xxxxxx>
Subject: Re: [PATCH v2 13/13] tty: pruss SUART driver

> On Friday 18 February 2011 19:23:49 Thomas Gleixner wrote:
> > On Fri, 18 Feb 2011, Alan Cox wrote:
> >
> > > On Fri, 18 Feb 2011 19:17:38 +0530
> > > "Subhasish Ghosh" <subhasish@xxxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > > Hello,
> > > >
> > > > Regarding the semaphore to mutex migration.
> > > > We are using down_trylock in interrupt context,
> > > > mutex_trylock cannot be used in interrupt context, so we cannot use 
> > > > mutex in
> > > > our driver.
> > >
> > > Then you probably need to rework your locking. Best bet might be to fix
> > > all the other stuff and report the driver, and people can think about 
> > > the
> > > locking problem.
> >
> > That semaphore is utterly useless to begin with. There are more
> > serious locking problems than this one. Non serialized calls to
> > suart_intr_clrmask/suart_intr_setmask are the most obvious ones.
> >
> > Aside of that the code is complete unreadable.
>
> I think it mostly suffers from the same problem as the CAN driver
> I commented on earlier: One of the files (pruss_suart_api.c) was
> clearly not written with Linux as the target, and the other files
> try to work around this by wrapping a Linux driver around it.
>
> The suart_api HAL stuff clearly needs to go away, so that the rest
> can be rewritten into a proper device driver.
>
> Arnd 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/