[alsa-devel] [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

Trent Piepho xyzzy at speakeasy.org
Mon Sep 17 23:04:21 CEST 2007


On Mon, 10 Sep 2007, Rene Herman wrote:
> When the ad1848/cs2431 is first being initialized, auto-calibration may not
> be set causing a timeout waiting for it in snd_ad1848/cs4231_mce_down().
>
> This has no dire consequences other than an alarming printk, but since what
> we need to wait for is for the calibration to _finish_, let's just check for
> that instead.
>
> The early chips need a slight delay (as commented -- 5 sample periods) to be
> sure that _if_ calibration is going to happen, it has started when we check
> While the CS4231A datasheet implies it'll happen immediately on downing MCE,
> some testing is showing that there's a window there as well, so just do the
> delay everywhere.

I don't think this code is doing what you think it does.

First, you call msleep(1) while holding reg_lock with interrupts disabled.
That's sleeping while atomic.  You should drop the lock first, or use
mdelay().

Second, schedule_timeout() returns immediately unless you have set the task
state to TASK_UNINTERRUPTIBLE or TASK_INTERRUPTIBLE.  I don't see anywhere
where this is done, so the 250ms delay is in fact a busy loop.  The call to
schedule_timeout() appears to be quite pointless.

This would explain what is happening when Krzysztof said, "The waiting loop
was passed 340 times after mce was down with msleep(1)." 340 loops at 250 ms
per loop is 85 seconds.  That would mean a call to hw_params() takes 85
seconds, and open(), with three mce_downs, would take over four minutes!

I looked at the ad1848 datasheet, and it says the auto-calibration should take
about 384 samples (or ~128, which I think is the time it takes ACI to go low
when auto-calibration is not on).  That would be 70 ms at the high end and
typically more like 3 ms.  A 250 ms polling interval seems to be quite long.
That would be at least 3/4 second for open(), and 1/4 second for hw_params().
With OSS emulation calling hw_params() many times, all the extra delay would
be easily noticeable.  Of course you do not notice it now because you are not
waiting 250 ms, but busy looping.

The wait for the init bit to be off after the check for ACI seems unneeded
too.  snd_ad1848_in(), called while waiting for ACI to go low, does the exact
same thing when it calls snd_ad1848_wait().  So the INIT bit is already off at
this point.

Here is a patch to fix all these problems and simplify the code in the
process.  I picked a value of 3 ms for the polling interval.  Obviously, there
is a trade-off in selecting the value.  The smaller the delay, the less wasted
time from when ACI goes low to when it is detected.  But it also means more
iterations of the loop, and thus more scheduling and interrupt disabling.

I think a good way to pick a value, is to try select the smallest value such
that 90% of the time the loop will finish on the first check.  It seems like 3
or maybe 4 would do that.

If you're not concerned much about efficiency, and just want to detect ACI low
as soon as possible, you'd use 1 ms.  Or if you want to get more complicated,
delay 3 ms on the first iteration, and 1 ms thereafter.  Or to be even smarter
than that, have a lookup table from the sample rate setting to the delay
value, so that the delay can be larger for slower sampling rates.  But that
hardly seems worth the effort.
-------------- next part --------------
ad1848: fix schedule while atomic, simplify MCE down code



The function snd_ad1848_mce_down() called msleep() while holding a spinlock

with IRQs disabled.



A polling loop to check for ACI to go down was more convoluted than it needed

to be.  New loop should be more efficient and it a lot simpler.



A polling loop to check for the device to leaving INIT mode is removed.  The

device must have already left init for the previous ACI loop to have finished.



Signed-off-by: Trent Piepho <xyzzy at speakeasy.org>

diff -r 50502c13d2cf isa/ad1848/ad1848_lib.c
--- a/isa/ad1848/ad1848_lib.c	Mon Sep 17 19:08:32 2007 +0200
+++ b/isa/ad1848/ad1848_lib.c	Mon Sep 17 12:57:05 2007 -0700
@@ -205,7 +205,6 @@ static void snd_ad1848_mce_down(struct s
 {
 	unsigned long flags;
 	int timeout;
-	unsigned long end_time;
 
 	spin_lock_irqsave(&chip->reg_lock, flags);
 	for (timeout = 5; timeout > 0; timeout--)
@@ -231,39 +230,28 @@ static void snd_ad1848_mce_down(struct s
 		return;
 	}
 
-	/*
-	 * Wait for (possible -- during init auto-calibration may not be set)
-	 * calibration process to start. Needs upto 5 sample periods on AD1848
-	 * which at the slowest possible rate of 5.5125 kHz means 907 us.
+	/* 
+	 * Wait for auto-calibration (AC) process to finish, i.e. ACI to go
+	 * low.  It may take up to 5 sample periods (at most 907 us @ 5.5125
+	 * kHz) for the process to _start_, so it is important to wait at least
+	 * that long before checking.  Otherwise we might think AC has finished
+	 * when it has in fact not begun.  It should take 128 (no AC) or 384
+	 * (AC) cycles for ACI to drop.  This gives a range of at most 70 ms
+	 * with a more typical value of just under 3 ms.  With a 3 ms wait, we
+	 * will probably finish on the first loop.
 	 */
-	msleep(1);
-
-	snd_printdd("(2) jiffies = %lu\n", jiffies);
-
-	end_time = jiffies + msecs_to_jiffies(250);
-	while (snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) {
-		spin_unlock_irqrestore(&chip->reg_lock, flags);
-		if (time_after(jiffies, end_time)) {
-			snd_printk(KERN_ERR "mce_down - auto calibration time out (2)\n");
-			return;
-		}
-		msleep(1);
+	/* give up at ~1/4 sec at 3 ms per loop */
+ 	for(timeout = 85; timeout > 0; timeout--) {
+		msleep(3);
 		spin_lock_irqsave(&chip->reg_lock, flags);
-	}
-
-	snd_printdd("(3) jiffies = %lu\n", jiffies);
-
-	end_time = jiffies + msecs_to_jiffies(100);
-	while (inb(AD1848P(chip, REGSEL)) & AD1848_INIT) {
-		spin_unlock_irqrestore(&chip->reg_lock, flags);
-		if (time_after(jiffies, end_time)) {
-			snd_printk(KERN_ERR "mce_down - auto calibration time out (3)\n");
-			return;
-		}
-		msleep(1);
-		spin_lock_irqsave(&chip->reg_lock, flags);
-	}
-	spin_unlock_irqrestore(&chip->reg_lock, flags);
+		if (snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS)
+			break;
+	}
+	spin_unlock_irqrestore(&chip->reg_lock, flags);
+	if (!timeout) {
+		snd_printk(KERN_ERR "mce_down - auto calibration time out (2)\n");
+		return;
+	}
 
 	snd_printdd("(4) jiffies = %lu\n", jiffies);
 	snd_printd("mce_down - exit = 0x%x\n", inb(AD1848P(chip, REGSEL)));


More information about the Alsa-devel mailing list