Re: [9front] ahci led: reset spins.

Re: [9front] ahci led: reset spins.

[ori]

> On one of my machines, there was aa fairly constant load from the
> ialed kproc, because it was stuck in a loop resetting the ahci
> controller. The controller never came back.

Oh, and obviously, if anyone has ideas on how to convince the
controller to come back, that would be even better :)

Re: [9front] ahci led: reset spins.

[ori]

> the whole code in ledkproc() looks rather suspect to me.
> 
> for example, the map[] is NCtlr sized, so i would
> assume that the index is the controller number,
> but later, we index into it by the driver number.

Yes. What was going on there seemed odd.  especially
the 'j += c->enctype' bit. I was planning on asking
wtf was going on with it on IRC before trying to
make changes, since it wasn't part of my current
problem.

> theres also a microdelay() loop in blink() that
> could be spinning forever. theres no reason to use
> delay() or microdelay() as well as we run on behalf
> of a kproc so we can properly sleep and not burn
> cpu cycles.

This diff definitely looks better, and works on my
machines -- thanks!

> 
> diff -r 874554781a81 sys/src/9/pc/sdiahci.c
> --- a/sys/src/9/pc/sdiahci.c	Sun Nov 17 19:04:38 2019 +0100
> +++ b/sys/src/9/pc/sdiahci.c	Sun Nov 17 21:04:51 2019 +0100
> @@ -1215,14 +1215,19 @@
>  ahciencreset(Ctlr *c)
>  {
>  	Ahba *h;
> +	int i;
>  
>  	if(c->enctype == Eesb)
>  		return 0;
>  	h = c->hba;
>  	h->emctl |= Emrst;
> -	while(h->emctl & Emrst)
> -		delay(1);
> -	return 0;
> +	for(i = 0; i < 1000; i++){
> +		if((h->emctl & Emrst) == 0)
> +			return 0;
> +		esleep(1);
> +	}
> +	print("%s: ahciencreset: hung ctlr\n", Tname(c));
> +	return -1;
>  }
>  
>  /*
> @@ -1301,9 +1306,11 @@
>  		return 0;
>  	c = d->ctlr;
>  	h = c->hba;
> +
>  	/* ensure last message has been transmitted */
> -	while(h->emctl & Tmsg)
> -		microdelay(1);
> +	if(h->emctl & Tmsg)
> +		return -1;
> +
>  	switch(c->enctype){
>  	default:
>  		panic("%s: bad led type %d", dnam(d), c->enctype);
> @@ -1403,30 +1410,34 @@
>  	memset(map, 0, sizeof map);
>  	for(i = 0; i < niactlr; i++)
>  		if(iactlr[i].enctype != 0){
> -			ahciencreset(iactlr + i);
> +			if(ahciencreset(iactlr + i) == -1)
> +				continue;
>  			map[i] = 1;
>  			j++;
>  		}
>  	if(j == 0)
>  		pexit("no work", 1);
>  	for(i = 0; i < niadrive; i++){
> -		iadrive[i]->nled = 3;		/* hardcoded */
> -		if(iadrive[i]->ctlr->enctype == Eesb)
> -			iadrive[i]->nled = 3;
> -		iadrive[i]->ledbits = -1;
> +		d = iadrive[i];
> +		d->nled = 3;		/* hardcoded */
> +		if(d->ctlr->enctype == Eesb)
> +			d->nled = 3;
> +		d->ledbits = -1;
>  	}
>  	for(i = 0; ; i++){
>  		t0 = Ticks;
>  		for(j = 0; j < niadrive; ){
> -			c = iadrive[j]->ctlr;
> -			if(map[j] == 0)
> -				j += c->enctype;
> -			else if(c->enctype == Eesb){
> +			d = iadrive[j];
> +			c = d->ctlr;
> +			if(map[c - iactlr] == 0)
> +				j++;
> +			else
> +			if(c->enctype == Eesb){
>  				blinkesb(c, i);
>  				j += c->ndrive;
>  			}else{
> -				d = iadrive[j++];
>  				blink(d, i);
> +				j++;
>  			}
>  		}
>  		t1 = Ticks;
> 
> --
> cinap

Re: [9front] ahci led: reset spins.

[cinap_lenrek]

the whole code in ledkproc() looks rather suspect to me.

for example, the map[] is NCtlr sized, so i would
assume that the index is the controller number,
but later, we index into it by the driver number.

theres also a microdelay() loop in blink() that
could be spinning forever. theres no reason to use
delay() or microdelay() as well as we run on behalf
of a kproc so we can properly sleep and not burn
cpu cycles.

diff -r 874554781a81 sys/src/9/pc/sdiahci.c
--- a/sys/src/9/pc/sdiahci.c	Sun Nov 17 19:04:38 2019 +0100
+++ b/sys/src/9/pc/sdiahci.c	Sun Nov 17 21:04:51 2019 +0100
@@ -1215,14 +1215,19 @@
 ahciencreset(Ctlr *c)
 {
 	Ahba *h;
+	int i;
 
 	if(c->enctype == Eesb)
 		return 0;
 	h = c->hba;
 	h->emctl |= Emrst;
-	while(h->emctl & Emrst)
-		delay(1);
-	return 0;
+	for(i = 0; i < 1000; i++){
+		if((h->emctl & Emrst) == 0)
+			return 0;
+		esleep(1);
+	}
+	print("%s: ahciencreset: hung ctlr\n", Tname(c));
+	return -1;
 }
 
 /*
@@ -1301,9 +1306,11 @@
 		return 0;
 	c = d->ctlr;
 	h = c->hba;
+
 	/* ensure last message has been transmitted */
-	while(h->emctl & Tmsg)
-		microdelay(1);
+	if(h->emctl & Tmsg)
+		return -1;
+
 	switch(c->enctype){
 	default:
 		panic("%s: bad led type %d", dnam(d), c->enctype);
@@ -1403,30 +1410,34 @@
 	memset(map, 0, sizeof map);
 	for(i = 0; i < niactlr; i++)
 		if(iactlr[i].enctype != 0){
-			ahciencreset(iactlr + i);
+			if(ahciencreset(iactlr + i) == -1)
+				continue;
 			map[i] = 1;
 			j++;
 		}
 	if(j == 0)
 		pexit("no work", 1);
 	for(i = 0; i < niadrive; i++){
-		iadrive[i]->nled = 3;		/* hardcoded */
-		if(iadrive[i]->ctlr->enctype == Eesb)
-			iadrive[i]->nled = 3;
-		iadrive[i]->ledbits = -1;
+		d = iadrive[i];
+		d->nled = 3;		/* hardcoded */
+		if(d->ctlr->enctype == Eesb)
+			d->nled = 3;
+		d->ledbits = -1;
 	}
 	for(i = 0; ; i++){
 		t0 = Ticks;
 		for(j = 0; j < niadrive; ){
-			c = iadrive[j]->ctlr;
-			if(map[j] == 0)
-				j += c->enctype;
-			else if(c->enctype == Eesb){
+			d = iadrive[j];
+			c = d->ctlr;
+			if(map[c - iactlr] == 0)
+				j++;
+			else
+			if(c->enctype == Eesb){
 				blinkesb(c, i);
 				j += c->ndrive;
 			}else{
-				d = iadrive[j++];
 				blink(d, i);
+				j++;
 			}
 		}
 		t1 = Ticks;

--
cinap