Subject: Re: port-i386/1082: /sys/dev/isa/com.c bugfixes and improvements
To: None <felix@Simplex.NL>
From: Charles M. Hannum <mycroft@ai.mit.edu>
List: netbsd-bugs
Date: 06/04/1995 07:38:54
A few comments:

1) Your patch is against a fairly old com.c.

2) Your modified comdiag() doesn't reset sc_errors, so you will not
observe any further overflows.

3) There are a few places that can be faster; e.g. by insuring that a
value is cached in a register.  Also, comeint() can simply go away.

4) Your change to comparam() is slightly wrong; the second arg to
l_modem() in that case should always be 0.  Otherwise, TS_CARR_ON may
be incorrectly set when turning off MDMBUF.

5) The way you use `comevents' seems more complicated (and thus
slower) than necessary.

Enclosed is a new set of diffs against the -current com.c.  I can't
test this very easily, so it would be nice if someone else did.


Index: com.c
===================================================================
RCS file: /a/cvsroot/src/sys/dev/isa/com.c,v
retrieving revision 1.56
diff -c -2 -r1.56 com.c
*** com.c	1995/06/01 21:26:51	1.56
--- com.c	1995/06/04 11:32:41
***************
*** 63,66 ****
--- 63,69 ----
  #include <dev/ic/ns16550.h>
  
+ #define	COM_IBUFSIZE	(2 * 256)
+ #define	COM_IHIGHWATER	((3 * COM_IBUFSIZE) / 4)
+ 
  struct com_softc {
  	struct device sc_dev;
***************
*** 69,72 ****
--- 72,78 ----
  
  	int sc_overflows;
+ 	int sc_floods;
+ 	int sc_errors;
+ 
  	int sc_iobase;
  	u_char sc_hwflags;
***************
*** 80,83 ****
--- 86,92 ----
  #define	COM_SW_MDMBUF	0x08
  	u_char sc_msr, sc_mcr;
+ 
+ 	u_char *sc_ibuf, *sc_ibufp, *sc_ibufhigh, *sc_ibufend;
+ 	u_char sc_ibufs[2][COM_IBUFSIZE];
  };
  
***************
*** 88,91 ****
--- 97,101 ----
  void comdiag __P((void *));
  int comintr __P((void *));
+ void compoll __P((void *));
  int comparam __P((struct tty *, struct termios *));
  void comstart __P((struct tty *));
***************
*** 103,106 ****
--- 113,118 ----
  int	comconsinit;
  int	commajor;
+ int	comsopen = 0;
+ int	comevents = 0;
  
  #ifdef KGDB
***************
*** 263,268 ****
  		return ENXIO;
  
- 	s = spltty();
- 
  	if (!sc->sc_tty)
  		tp = sc->sc_tty = ttymalloc();
--- 275,278 ----
***************
*** 287,293 ****
--- 297,313 ----
  		tp->t_lflag = TTYDEF_LFLAG;
  		tp->t_ispeed = tp->t_ospeed = comdefaultrate;
+ 
+ 		s = spltty();
+ 
  		comparam(tp, &tp->t_termios);
  		ttsetwater(tp);
  
+ 		if (comsopen++ == 0)
+ 			timeout(compoll, NULL, 1);
+ 
+ 		sc->sc_ibufp = sc->sc_ibuf = sc->sc_ibufs[0];
+ 		sc->sc_ibufhigh = sc->sc_ibuf + COM_IHIGHWATER;
+ 		sc->sc_ibufend = sc->sc_ibuf + COM_IBUFSIZE;
+ 
  		iobase = sc->sc_iobase;
  		/* Set the FIFO threshold based on the receive speed. */
***************
*** 297,302 ****
  			    (tp->t_ispeed <= 1200 ? FIFO_TRIGGER_1 : FIFO_TRIGGER_8));
  		/* flush any pending I/O */
! 		(void) inb(iobase + com_lsr);
! 		(void) inb(iobase + com_data);
  		/* you turn me on, baby */
  		sc->sc_mcr = MCR_DTR | MCR_RTS;
--- 317,322 ----
  			    (tp->t_ispeed <= 1200 ? FIFO_TRIGGER_1 : FIFO_TRIGGER_8));
  		/* flush any pending I/O */
! 		while (inb(iobase + com_lsr) & LSR_RXRDY)
! 			(void) inb(iobase + com_data);
  		/* you turn me on, baby */
  		sc->sc_mcr = MCR_DTR | MCR_RTS;
***************
*** 314,320 ****
  			tp->t_state &= ~TS_CARR_ON;
  	} else if (tp->t_state&TS_XCLUDE && p->p_ucred->cr_uid != 0) {
- 		splx(s);
  		return EBUSY;
! 	}
  
  	/* wait for carrier if necessary */
--- 334,340 ----
  			tp->t_state &= ~TS_CARR_ON;
  	} else if (tp->t_state&TS_XCLUDE && p->p_ucred->cr_uid != 0) {
  		return EBUSY;
! 	} else
! 		s = spltty();
  
  	/* wait for carrier if necessary */
***************
*** 347,364 ****
  	struct tty *tp = sc->sc_tty;
  	int iobase = sc->sc_iobase;
  
  	(*linesw[tp->t_line].l_close)(tp, flag);
! #ifdef KGDB
! 	/* do not disable interrupts if debugging */
! 	if (kgdb_dev != makedev(commajor, unit))
! #endif
! 	{
! 		bic(iobase + com_cfcr, CFCR_SBREAK);
! 		outb(iobase + com_ier, 0);
! 		if (tp->t_cflag & HUPCL &&
! 		    (sc->sc_swflags & COM_SW_SOFTCAR) == 0)
! 			/* XXX perhaps only clear DTR */
! 			outb(iobase + com_mcr, 0);
! 	}
  	ttyclose(tp);
  #ifdef notyet /* XXXX */
--- 367,385 ----
  	struct tty *tp = sc->sc_tty;
  	int iobase = sc->sc_iobase;
+ 	int s;
  
  	(*linesw[tp->t_line].l_close)(tp, flag);
! 	s = spltty();
! 	bic(iobase + com_cfcr, CFCR_SBREAK);
! 	outb(iobase + com_ier, 0);
! 	if (tp->t_cflag & HUPCL &&
! 	    (sc->sc_swflags & COM_SW_SOFTCAR) == 0) {
! 		/* XXX perhaps only clear DTR */
! 		outb(iobase + com_mcr, 0);
! 	}
! 	tp->t_state &= ~(TS_BUSY | TS_FLUSH);
! 	if (--comsopen == 0)
! 		untimeout(compoll, NULL);
! 	splx(s);
  	ttyclose(tp);
  #ifdef notyet /* XXXX */
***************
*** 447,454 ****
  		break;
  	case TIOCSDTR:
! 		outb(iobase + com_mcr, sc->sc_mcr |= (MCR_DTR | MCR_RTS));
  		break;
  	case TIOCCDTR:
! 		outb(iobase + com_mcr, sc->sc_mcr &= ~(MCR_DTR | MCR_RTS));
  		break;
  	case TIOCMSET:
--- 468,477 ----
  		break;
  	case TIOCSDTR:
! 		outb(iobase + com_mcr, sc->sc_mcr |=
! 		    (tp->t_cflag & CRTSCTS) ? MCR_DTR : (MCR_DTR | MCR_RTS));
  		break;
  	case TIOCCDTR:
! 		outb(iobase + com_mcr, sc->sc_mcr &=
! 		    (tp->t_cflag & CRTSCTS) ? ~MCR_DTR : ~(MCR_DTR | MCR_RTS));
  		break;
  	case TIOCMSET:
***************
*** 537,540 ****
--- 560,564 ----
  	int ospeed = comspeed(t->c_ospeed);
  	u_char cfcr;
+ 	tcflag_t oldcflag;
  	int s;
  
***************
*** 604,639 ****
  	}
  
- 	/*
- 	 * If CTS is off and CRTSCTS is changed, we must toggle TS_TTSTOP.
- 	 * XXX should be done at tty layer.
- 	 */
- 	if ((sc->sc_msr & MSR_CTS) == 0 &&
- 	    (tp->t_cflag & CRTSCTS) != (t->c_cflag & CRTSCTS)) {
- 		if ((t->c_cflag & CRTSCTS) == 0) {
- 			tp->t_state &= ~TS_TTSTOP;
- 			(*linesw[tp->t_line].l_start)(tp);
- 		} else
- 			tp->t_state |= TS_TTSTOP;
- 	}
- 
- 	/*
- 	 * If DCD is off and MDMBUF is changed, we must toggle TS_TTSTOP.
- 	 * XXX should be done at tty layer.
- 	 */
- 	if ((sc->sc_swflags & COM_SW_SOFTCAR) == 0 &&
- 	    (sc->sc_msr & MSR_DCD) == 0 &&
- 	    (tp->t_cflag & MDMBUF) != (t->c_cflag & MDMBUF)) {
- 		if ((t->c_cflag & MDMBUF) == 0) {
- 			tp->t_state &= ~TS_TTSTOP;
- 			(*linesw[tp->t_line].l_start)(tp);
- 		} else
- 			tp->t_state |= TS_TTSTOP;
- 	}
- 
  	/* and copy to tty */
  	tp->t_ispeed = t->c_ispeed;
  	tp->t_ospeed = t->c_ospeed;
  	tp->t_cflag = t->c_cflag;
  
  	splx(s);
  	return 0;
--- 628,649 ----
  	}
  
  	/* and copy to tty */
  	tp->t_ispeed = t->c_ispeed;
  	tp->t_ospeed = t->c_ospeed;
+ 	oldcflag = tp->t_cflag;
  	tp->t_cflag = t->c_cflag;
  
+ 	/*
+ 	 * If DCD is off and MDMBUF is changed, ask the tty layer if we should
+ 	 * stop the device.
+ 	 */
+ 	if ((sc->sc_msr & MSR_DCD) == 0 &&
+ 	    (sc->sc_swflags & COM_SW_SOFTCAR) == 0 &&
+ 	    (oldcflag & MDMBUF) != (tp->t_cflag & MDMBUF) &&
+ 	    (*linesw[tp->t_line].l_modem)(tp, 0) == 0) {
+ 		outb(iobase + com_mcr,
+ 		    (tp->t_cflag & CRTSCTS) ? ~MCR_DTR : ~(MCR_DTR | MCR_RTS));
+ 	}
+ 
  	splx(s);
  	return 0;
***************
*** 651,658 ****
  	if (tp->t_state & (TS_TTSTOP | TS_BUSY))
  		goto out;
- #if 0 /* XXXX I think this is handled adequately by commint() and comparam(). */
  	if (tp->t_cflag & CRTSCTS && (sc->sc_mcr & MSR_CTS) == 0)
  		goto out;
- #endif
  	if (tp->t_outq.c_cc <= tp->t_lowat) {
  		if (tp->t_state & TS_ASLEEP) {
--- 661,666 ----
***************
*** 693,733 ****
  }
  
- static inline void
- comeint(sc, stat)
- 	struct com_softc *sc;
- 	int stat;
- {
- 	struct tty *tp = sc->sc_tty;
- 	int iobase = sc->sc_iobase;
- 	int c;
- 
- 	c = inb(iobase + com_data);
- #ifdef DDB
- 	if ((stat & LSR_BI) && (sc->sc_dev.dv_unit == comconsole)) {
- 		Debugger();
- 		return;
- 	}
- #endif
- 	if ((tp->t_state & TS_ISOPEN) == 0) {
- #ifdef KGDB
- 		/* we don't care about parity errors */
- 		if (((stat & (LSR_BI | LSR_FE | LSR_PE)) == LSR_PE) &&
- 		    kgdb_dev == makedev(commajor, unit) && c == FRAME_END)
- 			kgdb_connect(0); /* trap into kgdb */
- #endif
- 		return;
- 	}
- 	if (stat & (LSR_BI | LSR_FE))
- 		c |= TTY_FE;
- 	else if (stat & LSR_PE)
- 		c |= TTY_PE;
- 	if (stat & LSR_OE) {
- 		if (sc->sc_overflows++ == 0)
- 			timeout(comdiag, sc, 60 * hz);
- 	}
- 	/* XXXX put in FIFO and process later */
- 	(*linesw[tp->t_line].l_rint)(c, tp);
- }
-  
  void
  comdiag(arg)
--- 701,704 ----
***************
*** 735,749 ****
  {
  	struct com_softc *sc = arg;
! 	int overflows;
  	int s;
  
  	s = spltty();
  	overflows = sc->sc_overflows;
  	sc->sc_overflows = 0;
  	splx(s);
  
! 	if (overflows)
! 		log(LOG_WARNING, "%s: %d silo overflow%s\n",
! 		    sc->sc_dev.dv_xname, overflows, overflows == 1 ? "" : "s");
  }
  
--- 706,795 ----
  {
  	struct com_softc *sc = arg;
! 	int overflows, floods;
  	int s;
  
  	s = spltty();
+ 	sc->sc_errors = 0;
  	overflows = sc->sc_overflows;
  	sc->sc_overflows = 0;
+ 	floods = sc->sc_floods;
+ 	sc->sc_floods = 0;
  	splx(s);
  
! 	log(LOG_WARNING, "%s: %d silo overflow%s, %d ibuf overflow%s\n",
! 	    sc->sc_dev.dv_xname,
! 	    overflows, overflows == 1 ? "" : "s",
! 	    floods, floods == 1 ? "" : "s");
! }
! 
! void
! compoll(arg)
! 	void *arg;
! {
! 	int unit;
! 	struct com_softc *sc;
! 	struct tty *tp;
! 	register u_char *ibufp;
! 	u_char *ibufend;
! 	register int c;
! 	int s;
! 	static int lsrmap[8] = {
! 		0,      TTY_PE,
! 		TTY_FE, TTY_PE|TTY_FE,
! 		TTY_FE, TTY_PE|TTY_FE,
! 		TTY_FE, TTY_PE|TTY_FE
! 	};
! 
! 	s = spltty();
! 	if (comevents == 0) {
! 		splx(s);
! 		goto out;
! 	}
! 	comevents = 0;
! 	splx(s);
! 
! 	for (unit = 0; unit < comcd.cd_ndevs; unit++) {
! 		sc = comcd.cd_devs[unit];
! 		if (sc == 0 || sc->sc_ibufp == sc->sc_ibuf)
! 			continue;
! 
! 		tp = sc->sc_tty;
! 
! 		s = spltty();
! 
! 		ibufp = sc->sc_ibuf;
! 		ibufend = sc->sc_ibufp;
! 
! 		sc->sc_ibufp = sc->sc_ibuf = (ibufp == sc->sc_ibufs[0]) ?
! 					     sc->sc_ibufs[1] : sc->sc_ibufs[0];
! 		sc->sc_ibufhigh = sc->sc_ibuf + COM_IHIGHWATER;
! 		sc->sc_ibufend = sc->sc_ibuf + COM_IBUFSIZE;
! 
! 		if (tp == 0 || (tp->t_state & TS_ISOPEN) == 0) {
! 			splx(s);
! 			continue;
! 		}
! 
! 		if ((tp->t_cflag & CRTSCTS) != 0 &&
! 		    (sc->sc_mcr & MCR_RTS) == 0)
! 			outb(sc->sc_iobase + com_mcr, sc->sc_mcr |= MCR_RTS);
! 
! 		splx(s);
! 
! 		while (ibufp < ibufend) {
! 			c = *ibufp++;
! 			if (*ibufp & LSR_OE) {
! 				sc->sc_overflows++;
! 				if (sc->sc_errors++ == 0)
! 					timeout(comdiag, sc, 60 * hz);
! 			}
! 			/* This is ugly, but fast. */
! 			c |= lsrmap[(*ibufp++ & (LSR_BI|LSR_FE|LSR_PE)) >> 2];
! 			(*linesw[tp->t_line].l_rint)(c, tp);
! 		}
! 	}
! 
! out:
! 	timeout(compoll, NULL, 1);
  }
  
***************
*** 766,784 ****
  
  		if (lsr & LSR_RCV_MASK) {
! 			/* XXXX put in FIFO and process later */
  			do {
! 				if (lsr & (LSR_BI|LSR_FE|LSR_PE|LSR_OE))
! 					comeint(sc, lsr);
! 				else {
! 					data = inb(iobase + com_data);
! 					if (tp->t_state & TS_ISOPEN)
! 						(*linesw[tp->t_line].l_rint)(data, tp);
! #ifdef KGDB
! 					else {
! 						if (kgdb_dev == makedev(commajor, unit) &&
! 						    data == FRAME_END)
! 							kgdb_connect(0);
! 					}
  #endif
  				}
  				lsr = inb(iobase + com_lsr);
--- 812,838 ----
  
  		if (lsr & LSR_RCV_MASK) {
! 			register u_char *p = sc->sc_ibuf;
! 
! 			comevents = 1;
  			do {
! #ifdef DDB
! 				if ((lsr & LSR_BI) != 0 &&
! 				    sc->sc_dev.dv_unit == comconsole) {
! 					Debugger();
! 					continue;
! 				}
  #endif
+ 				data = inb(iobase + com_data);
+ 				if (p >= sc->sc_ibufend) {
+ 					sc->sc_floods++;
+ 					if (sc->sc_errors++ == 0)
+ 						timeout(comdiag, sc, 60 * hz);
+ 				} else {
+ 					*p++ = data;
+ 					*p++ = lsr;
+ 					if (p == sc->sc_ibufhigh &&
+ 					    (tp->t_cflag & CRTSCTS) != 0)
+ 						outb(iobase + com_mcr,
+ 						     sc->sc_mcr &= ~MCR_RTS);
  				}
  				lsr = inb(iobase + com_lsr);
***************
*** 802,819 ****
  			delta = msr ^ sc->sc_msr;
  			sc->sc_msr = msr;
! 			if (delta & MSR_DCD && (sc->sc_swflags & COM_SW_SOFTCAR) == 0) {
! 				if (msr & MSR_DCD)
! 					(void)(*linesw[tp->t_line].l_modem)(tp, 1);
! 				else if ((*linesw[tp->t_line].l_modem)(tp, 0) == 0)
! 					outb(iobase + com_mcr,
! 					     sc->sc_mcr &= ~(MCR_DTR | MCR_RTS));
  			}
! 			if (delta & MSR_CTS && (tp->t_cflag & CRTSCTS) != 0) {
  				/* the line is up and we want to do rts/cts flow control */
! 				if (msr & MSR_CTS) {
! 					tp->t_state &= ~TS_TTSTOP;
! 					(*linesw[tp->t_line].l_start)(tp);
! 				} else
! 					tp->t_state |= TS_TTSTOP;
  			}
  		}
--- 856,869 ----
  			delta = msr ^ sc->sc_msr;
  			sc->sc_msr = msr;
! 			if ((delta & MSR_DCD) != 0 &&
! 			    (sc->sc_swflags & COM_SW_SOFTCAR) == 0 &&
! 			    (*linesw[tp->t_line].l_modem)(tp, (msr & MSR_DCD) != 0) == 0) {
! 				outb(iobase + com_mcr,
! 				    (tp->t_cflag & CRTSCTS) ? ~MCR_DTR : ~(MCR_DTR | MCR_RTS));
  			}
! 			if ((delta & msr & MSR_CTS) != 0 &&
! 			    (tp->t_cflag & CRTSCTS) != 0) {
  				/* the line is up and we want to do rts/cts flow control */
! 				(*linesw[tp->t_line].l_start)(tp);
  			}
  		}