Wrong GIT author ? also bugs: serial: Add uart driver for i.MX23/28

Wrong GIT author ? also bugs: serial: Add uart driver for i.MX23/28

[Alan Cox]

>     serial: Add auart driver for i.MX23/28
>     
>     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

You have a few things that need fixing, one of which is quite nasty

> + * Freescale STMP37XX/STMP378X Application UART driver
> + *
> + * Author: dmitry pervushin <dimka@embeddedalley.com>

So its signed off by one person and without a signature from the author ?
Is that intentional

And it git we have

Author:     Sascha Hauer <s.hauer@pengutronix.de>

which from the header block appears to be untrue ?

> +static void mxs_auart_rx_chars(struct mxs_auart_port *s)
> +{
> +	struct tty_struct *tty = s->port.state->port.tty;
> +	u32 stat = 0;

tty may be NULL at this point, in which case your code will try and push
bits through a NULL pointer and crash

A user can seek to make tty NULL at the right moment by judicious use of
vhangup, and a remote device can use carrier signals and bit timing to
mount an attack. See tty_port_tty_get() and/or hold the port lock over
the IRQ as other drivers do. For the uart stuff I'd hold the port lock as
the rest of the code in the uart layer sort of assumes that


> +static void mxs_auart_settermios(struct uart_port *u,
> +				 struct ktermios *termios,
> +				 struct ktermios *old)

Minor things here
- You need to set the termios bits to reflect the mode you actually set.
  So as you don't seem to support mark/space you need to clear the bit,
  ditto h/w flow control if not supported

- You also need to report back the actual baud rate


> +static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
> +{
> +	u32 istatus, istat;
> +	struct mxs_auart_port *s = context;
> +	u32 stat = readl(s->port.membase + AUART_STAT);

Your IRQ handler either needs to take the port lock or have the
underlying methods do that or their own full locking, you aren't doing
that.

Re: Wrong GIT author ? also bugs: serial: Add uart driver for i.MX23/28

[Sascha Hauer]

Hi Alan,

On Fri, Mar 18, 2011 at 11:29:20AM +0000, Alan Cox wrote:
> >     serial: Add auart driver for i.MX23/28
> >     
> >     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> You have a few things that need fixing, one of which is quite nasty
> 
> > + * Freescale STMP37XX/STMP378X Application UART driver
> > + *
> > + * Author: dmitry pervushin <dimka@embeddedalley.com>
> 
> So its signed off by one person and without a signature from the author ?
> Is that intentional
> 
> And it git we have
> 
> Author:     Sascha Hauer <s.hauer@pengutronix.de>
> 
> which from the header block appears to be untrue ?

The original author indeed is Dmitry with a lot of changes by myself.
Sorry, I didn't hink much while importing it to git, I should probably
have used --author while committing it.


> 
> > +static void mxs_auart_rx_chars(struct mxs_auart_port *s)
> > +{
> > +	struct tty_struct *tty = s->port.state->port.tty;
> > +	u32 stat = 0;
> 
> tty may be NULL at this point, in which case your code will try and push
> bits through a NULL pointer and crash
> 
> A user can seek to make tty NULL at the right moment by judicious use of
> vhangup, and a remote device can use carrier signals and bit timing to
> mount an attack. See tty_port_tty_get() and/or hold the port lock over
> the IRQ as other drivers do. For the uart stuff I'd hold the port lock as
> the rest of the code in the uart layer sort of assumes that
> 
> 
> > +static void mxs_auart_settermios(struct uart_port *u,
> > +				 struct ktermios *termios,
> > +				 struct ktermios *old)
> 
> Minor things here
> - You need to set the termios bits to reflect the mode you actually set.
>   So as you don't seem to support mark/space you need to clear the bit,
>   ditto h/w flow control if not supported
> 
> - You also need to report back the actual baud rate
> 
> 
> > +static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
> > +{
> > +	u32 istatus, istat;
> > +	struct mxs_auart_port *s = context;
> > +	u32 stat = readl(s->port.membase + AUART_STAT);
> 
> Your IRQ handler either needs to take the port lock or have the
> underlying methods do that or their own full locking, you aren't doing
> that.
> 

I will send update patches for the comments you made soon.

BTW I wasn't aware of the linux-serial list and the according MAINTAINER
entries do not exist. Should they be added?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: Wrong GIT author ? also bugs: serial: Add uart driver for i.MX23/28

[Alan Cox]

> BTW I wasn't aware of the linux-serial list and the according MAINTAINER
> entries do not exist. Should they be added?

Judging by the fact you looked in MAINTAINERS and didn't fid it - yes

Alan

Re: Wrong GIT author ? also bugs: serial: Add uart driver for i.MX23/28

[Uwe Kleine-König]

Hello,

On Fri, Mar 18, 2011 at 11:29:20AM +0000, Alan Cox wrote:
> >     serial: Add auart driver for i.MX23/28
> >     
> >     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> You have a few things that need fixing, one of which is quite nasty
I'm going to address these as Sascha is currently busy doing other
stuff.

> > + * Freescale STMP37XX/STMP378X Application UART driver
> > + *
> > + * Author: dmitry pervushin <dimka@embeddedalley.com>
> 
> So its signed off by one person and without a signature from the author ?
> Is that intentional
The base for the driver was taken from a vendor BSP and polished to be
(nearly :-) ready for mainline. I don't know if Sascha tried to contact
Dimtry (I guess not) but keeping the original author listed is IMHO OK
as is using GIT_AUTHOR=Sascha when the polishing was non-trivial.

So maybe make this read:

	Based on a driver by dmitry pervushin <dimka@embeddedalley.com>
	and modified for mainline by Sascha Hauer <...>

?
> > +static void mxs_auart_rx_chars(struct mxs_auart_port *s)
> > +{
> > +	struct tty_struct *tty = s->port.state->port.tty;
> > +	u32 stat = 0;
> 
> tty may be NULL at this point, in which case your code will try and push
> bits through a NULL pointer and crash
> 
> A user can seek to make tty NULL at the right moment by judicious use of
> vhangup, and a remote device can use carrier signals and bit timing to
> mount an attack. See tty_port_tty_get() and/or hold the port lock over
> the IRQ as other drivers do. For the uart stuff I'd hold the port lock as
> the rest of the code in the uart layer sort of assumes that
OK

> > +static void mxs_auart_settermios(struct uart_port *u,
> > +				 struct ktermios *termios,
> > +				 struct ktermios *old)
> 
> Minor things here
> - You need to set the termios bits to reflect the mode you actually set.
>   So as you don't seem to support mark/space you need to clear the bit,
>   ditto h/w flow control if not supported
> 
> - You also need to report back the actual baud rate
I don't understand how to do this and didn't find something like that in
other drivers. (I checked amba-pl011.c and imx.c.) Also
Documentation/serial/driver doesn't describe this. (It is also silent
about CMSPAR and doesn't even advise to clear unsupported bits.)
Can you be a bit more verbose here?
 
> > +static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
> > +{
> > +	u32 istatus, istat;
> > +	struct mxs_auart_port *s = context;
> > +	u32 stat = readl(s->port.membase + AUART_STAT);
> 
> Your IRQ handler either needs to take the port lock or have the
> underlying methods do that or their own full locking, you aren't doing
> that.
OK, below you can find my current wip patch (yet untested on real hardware).
(If/when this looks OK for you and the above issue is addressed I will
create a proper patch.)

Best regards
Uwe
 
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 7e02c9c..3b4c96a 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -1,17 +1,14 @@
 /*
  * Freescale STMP37XX/STMP378X Application UART driver
  *
- * Author: dmitry pervushin <dimka@embeddedalley.com>
+ * Based on a driver by dmitry pervushin <dimka@embeddedalley.com>
+ * and modified for mainline by Sascha Hauer <s.hauer@pengutronix.de>
  *
  * Copyright 2008-2010 Freescale Semiconductor, Inc.
  * Copyright 2008 Embedded Alley Solutions, Inc All Rights Reserved.
  *
  * The code contained herein is licensed under the GNU General Public
- * License. You may obtain a copy of the GNU General Public License
- * Version 2 or later at the following locations:
- *
- * http://www.opensource.org/licenses/gpl-license.html
- * http://www.gnu.org/copyleft/gpl.html
+ * License; version 2 or later.
  */
 
 #include <linux/kernel.h>
@@ -286,6 +283,9 @@ static void mxs_auart_settermios(struct uart_port *u,
 {
 	u32 bm, ctrl, ctrl2, div;
 	unsigned int cflag, baud;
+	unsigned long flags;
+
+	spin_lock_irqsave(&u->lock, flags);
 
 	cflag = termios->c_cflag;
 
@@ -303,11 +303,9 @@ static void mxs_auart_settermios(struct uart_port *u,
 	case CS7:
 		bm = 2;
 		break;
-	case CS8:
+	default: /* CS8 */
 		bm = 3;
 		break;
-	default:
-		return;
 	}
 
 	ctrl |= AUART_LINECTRL_WLEN(bm);
@@ -368,13 +366,23 @@ static void mxs_auart_settermios(struct uart_port *u,
 
 	writel(ctrl, u->membase + AUART_LINECTRL);
 	writel(ctrl2, u->membase + AUART_CTRL2);
+
+	/* clear unsupported flags */
+	termios->c_cflag &= ~CMSPAR;
+
+	spin_unlock_irqrestore(&u->lock, flags);
 }
 
 static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 {
 	u32 istatus, istat;
 	struct mxs_auart_port *s = context;
-	u32 stat = readl(s->port.membase + AUART_STAT);
+	unsigned long flags;
+	u32 stat;
+       
+	spin_lock_irqsave(&s->port.lock, flags);
+
+	stat = readl(s->port.membase + AUART_STAT);
 
 	istatus = istat = readl(s->port.membase + AUART_INTR);
 
@@ -401,6 +409,8 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 		| AUART_INTR_CTSMIS),
 			s->port.membase + AUART_INTR_CLR);
 
+	spin_unlock_irqrestore(&s->port.lock, flags);
+
 	return IRQ_HANDLED;
 }
 
-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: Wrong GIT author ? also bugs: serial: Add uart driver for i.MX23/28

[Alan Cox]

> I don't understand how to do this and didn't find something like that in
> other drivers. (I checked amba-pl011.c and imx.c.) Also
> Documentation/serial/driver doesn't describe this. (It is also silent
> about CMSPAR and doesn't even advise to clear unsupported bits.)
> Can you be a bit more verbose here?

8250.c is the best place to look.

Basically on return from your routine the _hardware_ bits of the termios
struct should be the ones set.

So eg if you only supported 8bit characters you'd wipe the CSx bits and
set CS8.

For the speed there is a helper so you probably want

        /* Don't rewrite B0 */
        if (tty_termios_baud_rate(termios))
                tty_termios_encode_baud_rate(termios, baud, baud);

> @@ -286,6 +283,9 @@ static void mxs_auart_settermios(struct uart_port *u,
>  {
>  	u32 bm, ctrl, ctrl2, div;
>  	unsigned int cflag, baud;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&u->lock, flags);

Termios you don't need the lock - just the IRQ handler and you still
really want the tty_port_tty_get()/tty_kref_put() if that wasn't already
dealt with.

(You may not need it but its easier to put in than prove with the serial
layer at the moment)

Re: Wrong GIT author ? also bugs: serial: Add uart driver for i.MX23/28

[Uwe Kleine-König]

On Tue, Mar 29, 2011 at 10:33:35AM +0100, Alan Cox wrote:
> > I don't understand how to do this and didn't find something like that in
> > other drivers. (I checked amba-pl011.c and imx.c.) Also
> > Documentation/serial/driver doesn't describe this. (It is also silent
> > about CMSPAR and doesn't even advise to clear unsupported bits.)
> > Can you be a bit more verbose here?
> 
> 8250.c is the best place to look.
> 
> Basically on return from your routine the _hardware_ bits of the termios
> struct should be the ones set.
> 
> So eg if you only supported 8bit characters you'd wipe the CSx bits and
> set CS8.
> 
> For the speed there is a helper so you probably want
> 
>         /* Don't rewrite B0 */
>         if (tty_termios_baud_rate(termios))
>                 tty_termios_encode_baud_rate(termios, baud, baud);
OK, I will review the function to assert that the other bits are updated
accordingly.

> > @@ -286,6 +283,9 @@ static void mxs_auart_settermios(struct uart_port *u,
> >  {
> >  	u32 bm, ctrl, ctrl2, div;
> >  	unsigned int cflag, baud;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&u->lock, flags);
> 
> Termios you don't need the lock - just the IRQ handler and you still
> really want the tty_port_tty_get()/tty_kref_put() if that wasn't already
> dealt with.
If I look at 8250.c spin_lock_irqsave seems OK :-)

To be honest, it felt much better when a simpler driver than 8250 could
be used to copy from. Documentation/serial/driver (which is IMHO very
helpful) declares amba_pl011.c to be the reference implementation. It
uses neither tty_termios_encode_baud_rate nor
tty_port_tty_get()/tty_kref_put() :-(

Anyhow, I'll do my best to implement both requests.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |