[9fans] usb/kb fault ... and fix

[9fans] usb/kb fault ... and fix

[erik quanstrom]

i had a fault in usb/kb this evening, and have a proposed fix.
it also seemed like a good time to address the goto soup in repeatproc.
by moving the timer into its own proc, it was possible to turn
repeatproc into a case statement.  the downside is there's
a tiny bit of busy work going on between the timer and repeat proc.

minooka; apatch/diffemail
email
	quanstro@quanstro.net
readme
	as there was no interlock for the repeat process shutdown, it was possible
	to free the channel before the repeat process had a chance to shut down,
	so if a repeating keyboard were unplugged, a crash could result.  this was
	seen as in this
	bootes          152    0:00   0:00     4096K Broken   usbd [kbd repeat]
	with this console output:
	
	chula# ````````````````````````````kb: /dev/usb/ep8.1: read: crc/timeout error
	kb: exiting
	`152: usbd: bootes: fault addr=0xcafebac2 kpc=0x20854e
	usbd 152: suicide: sys: trap: fault read addr=0xcafebac2 pc=0x20854e
	
removed
	
files
	/sys/src/cmd/usb/kb/kb.c	kb.c

/sys/src/cmd/usb/kb/kb.c	kb.c
kb.c.orig:19,32 - kb.c:19,50
  
  enum
  {
- 	Awakemsg= 0xdeaddead,
- 	Diemsg	= 0xbeefbeef,
+ 	Stoprpt	= -2,
+ 	Tick	= -3,
+ 	Exiting	= -4,
+ 
+ 	Msec	= 1000*1000,		/* msec per ns */
+ 
  	Dwcidle	= 8,
  };
  
  typedef struct KDev KDev;
+ typedef struct Kbd Kbd;
+ typedef struct Mouse Mouse;
  typedef struct Kin Kin;
  
+ struct Kbd
+ {
+ 	Channel*	repeatc;
+ 	Channel*	exitc;
+ 	long		nproc;
+ };
+ 
+ struct Mouse
+ {
+ 	int	accel;		/* only for mouse */
+ };
+ 
  struct KDev
  {
  	Dev*	dev;		/* usb device*/
kb.c.orig:33,42 - kb.c:51,60
  	Dev*	ep;		/* endpoint to get events */
  	Kin*	in;		/* used to send events to kernel */
  	int	idle;		/* min time between reports (× 4ms) */
- 	Channel*repeatc;	/* only for keyboard */
- 	int	accel;		/* only for mouse */
  	int	bootp;		/* has associated keyboard */
  	int	debug;
+ 	Kbd;
+ 	Mouse;
  	HidRepTempl templ;
  	int	(*ptrvals)(KDev *kd, Chain *ch, int *px, int *py, int *pb);
  };
kb.c.orig:212,219 - kb.c:230,244
  		fprint(2, "kb: fatal: %s\n", sts);
  	else
  		fprint(2, "kb: exiting\n");
- 	if(kd->repeatc != nil)
- 		nbsendul(kd->repeatc, Diemsg);
+ 	if(kd->repeatc != nil){
+ 		chanclose(kd->repeatc);
+ 		for(; kd->nproc != 0; kd->nproc--)
+ 			recvul(kd->exitc);
+ 		chanfree(kd->repeatc);
+ 		chanfree(kd->exitc);
+ 		kd->repeatc = nil;
+ 		kd->exitc = nil;
+ 	}
  	dev = kd->dev;
  	kd->dev = nil;
  	if(kd->ep != nil)
kb.c.orig:392,398 - kb.c:417,423
  static void
  stoprepeat(KDev *f)
  {
- 	sendul(f->repeatc, Awakemsg);
+ 	sendul(f->repeatc, Stoprpt);
  }
  
  static void
kb.c.orig:432,478 - kb.c:457,518
  }
  
  static void
+ repeattimerproc(void *a)
+ {
+ 	Channel *c;
+ 	KDev *f;
+ 
+ 	threadsetname("kbd reptimer");
+ 	f = a;
+ 	c = f->repeatc;
+ 
+ 	for(;;){
+ 		sleep(30);
+ 		if(sendul(c, Tick) == -1)
+ 			break;
+ 	}
+ 	sendul(f->exitc, Exiting);
+ 	threadexits("aborted");
+ }
+ 
+ static void
  repeatproc(void* a)
  {
  	KDev *f;
- 	Channel *repeatc;
- 	ulong l, t, i;
- 	uchar esc1, sc;
+ 	Channel *c;
+ 	int code;
+ 	uint l;
+ 	uvlong timeout;
  
  	threadsetname("kbd repeat");
- 	/*
- 	 * too many jumps here.
- 	 * Rewrite instead of debug, if needed.
- 	 */
  	f = a;
- 	repeatc = f->repeatc;
- 	l = Awakemsg;
- Repeat:
- 	if(l == Diemsg)
- 		goto Abort;
- 	while(l == Awakemsg)
- 		l = recvul(repeatc);
- 	if(l == Diemsg)
- 		goto Abort;
- 	esc1 = l >> 8;
- 	sc = l;
- 	t = 500;
+ 	c = f->repeatc;
+ 
+ 	timeout = 0;
+ 	code = -1;
  	for(;;){
- 		for(i = 0; i < t; i += 5){
- 			if(l = nbrecvul(repeatc))
- 				goto Repeat;
- 			sleep(5);
+ 		switch(l = recvul(c)){
+ 		default:
+ 			code = l;
+ 			timeout = nsec() + 500*Msec;
+ 			break;
+ 		case ~0ul:
+ 			sendul(f->exitc, Exiting);
+ 			threadexits("aborted");
+ 			break;
+ 		case Stoprpt:
+ 			code = -1;
+ 			break;
+ 		case Tick:
+ 			if(code == -1 || nsec() < timeout)
+ 				continue;
+ 			putscan(f, (uchar)(code>>8), (uchar)code);
+ 			timeout = nsec() + 30*Msec;
+ 			break;
  		}
- 		putscan(f, esc1, sc);
- 		t = 30;
  	}
- Abort:
- 	chanfree(repeatc);
- 	threadexits("aborted");
- 
  }
  
- 
  #define hasesc1(sc)	(((sc) >= 0x47) || ((sc) == 0x38))
  
  static void
kb.c.orig:560,570 - kb.c:600,617
  	if(f->ep->maxpkt < 3 || f->ep->maxpkt > sizeof buf)
  		kbfatal(f, "weird maxpkt");
  
+ 	f->exitc = chancreate(sizeof(ulong), 0);
+ 	if(f->exitc == nil)
+ 		kbfatal(f, "chancreate failed");
  	f->repeatc = chancreate(sizeof(ulong), 0);
- 	if(f->repeatc == nil)
+ 	if(f->repeatc == nil){
+ 		chanfree(f->exitc);
  		kbfatal(f, "chancreate failed");
+ 	}
  
+ 	f->nproc = 2;
  	proccreate(repeatproc, f, Stack);
+ 	proccreate(repeattimerproc, f, Stack);
  	memset(lbuf, 0, sizeof lbuf);
  	dk = nerrs = 0;
  	for(;;){