linux-mips
[Top] [All Lists]

Re: [PATCH] zs: Move to the serial subsystem

To: Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH] zs: Move to the serial subsystem
From: Russell King <rmk+lkml@arm.linux.org.uk>
Date: Wed, 30 May 2007 11:09:35 +0100
Cc: "Maciej W. Rozycki" <macro@linux-mips.org>, linux-mips@linux-mips.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Andy Whitcroft <apw@shadowen.org>
In-reply-to: <20070530011224.bf36d2df.akpm@linux-foundation.org>
Mail-followup-to: Andrew Morton <akpm@linux-foundation.org>, "Maciej W. Rozycki" <macro@linux-mips.org>, linux-mips@linux-mips.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Andy Whitcroft <apw@shadowen.org>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <Pine.LNX.4.64N.0705291258390.14456@blysk.ds.pg.gda.pl> <20070530011224.bf36d2df.akpm@linux-foundation.org>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.4.2.1i
On Wed, May 30, 2007 at 01:12:24AM -0700, Andrew Morton wrote:
> > +                   if (status & (Rx_SYS | Rx_BRK))
> > +                           icount->brk++;
> > +                   else if (status & FRM_ERR)
> > +                           icount->frame++;
> > +                   else if (status & PAR_ERR)
> > +                           icount->parity++;
> 
> FRM_ERR and PAR_ERR are mutually exclusive, and cannot be set if either
> Rx_SYS or Rx_BRK are set?

That's actually fairly normal.  A break condition is by definition
a framing error, and possibly a parity error as well.  Also, a break
condition is not an error per-se.

Also, if you do add in the associated framing or parity errors, you're
likely to get different results from different hardware - some hardware
mask off the framing and parity errors when detecting a break condition.
Others don't.

> > +/*
> > + * Finally, routines used to initialize the serial port.
> > + */
> > +static int zs_startup(struct uart_port *uport)
> > +{
> > +   struct zs_port *zport = to_zport(uport);
> > +   struct zs_scc *scc = zport->scc;
> > +   unsigned long flags;
> > +   int ret;
> > +
> > +   if (!scc->irq_guard) {
> > +           ret = request_irq(zport->port.irq, zs_interrupt,
> > +                             IRQF_SHARED, "scc", scc);
> > +           if (ret) {
> > +                   printk(KERN_ERR "zs: can't get irq %d\n",
> > +                          zport->port.irq);
> > +                   return ret;
> > +           }
> > +   }
> > +   scc->irq_guard++;
> 
> The ->irq_guard handling looks a little racy?
> 
> Perhaps higher-level locks prevent this.  If so, a comment explaining this
> would be reassuring.

Does look racy if "scc" is shared between several ports.  The locking
here is only per-port, so this is racy.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

<Prev in Thread] Current Thread [Next in Thread>