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

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

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]

Hi Takashi.

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.

Thanks to Krysztof Helt for pinpointing this problem.

Signed-off-by: Rene Herman <rene.herman@gmail.com>

Rene.

[-- Attachment #2: mce_down.diff --]
[-- Type: text/plain, Size: 2226 bytes --]

diff --git a/sound/isa/ad1848/ad1848_lib.c b/sound/isa/ad1848/ad1848_lib.c
index 8094282..dcd4435 100644
--- a/sound/isa/ad1848/ad1848_lib.c
+++ b/sound/isa/ad1848/ad1848_lib.c
@@ -227,16 +227,15 @@ static void snd_ad1848_mce_down(struct snd_ad1848 *chip)
 		spin_unlock_irqrestore(&chip->reg_lock, flags);
 		return;
 	}
-	/* calibration process */
 
-	for (timeout = 500; timeout > 0 && (snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) == 0; timeout--);
-	if ((snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) == 0) {
-		snd_printd("mce_down - auto calibration time out (1)\n");
-		spin_unlock_irqrestore(&chip->reg_lock, flags);
-		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.
+         */
+        msleep(1);
 #if 0
-	printk("(2) timeout = %i, jiffies = %li\n", timeout, jiffies);
+	printk("(2) jiffies = %li\n", jiffies);
 #endif
 	time = HZ / 4;
 	while (snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) {
diff --git a/sound/isa/cs423x/cs4231_lib.c b/sound/isa/cs423x/cs4231_lib.c
index 914d77b..9e8e0f1 100644
--- a/sound/isa/cs423x/cs4231_lib.c
+++ b/sound/isa/cs423x/cs4231_lib.c
@@ -346,16 +346,14 @@ void snd_cs4231_mce_down(struct snd_cs4231 *chip)
 	}
 	snd_cs4231_busy_wait(chip);
 
-	/* calibration process */
-
-	for (timeout = 500; timeout > 0 && (snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) == 0; timeout--)
-		udelay(10);
-	if ((snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) == 0) {
-		snd_printd("cs4231_mce_down - auto calibration time out (1)\n");
-		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.
+	 */
+	msleep(1);
 #if 0
-	printk("(2) timeout = %i, jiffies = %li\n", timeout, jiffies);
+	printk("(2) jiffies = %li\n", jiffies);
 #endif
 	/* in 10 ms increments, check condition, up to 250 ms */
 	timeout = 25;

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Takashi Iwai]

At Mon, 10 Sep 2007 23:40:34 +0200,
Krzysztof Helt wrote:
> 
> On Mon, 10 Sep 2007 20:29:21 +0200
> Rene Herman <rene.herman@gmail.com> wrote:
> > 
> > Thanks to Krysztof Helt for pinpointing this problem.
> > 
> 
> Please change it to Krzysztof Helt

Sorry, too late, already committed to the public tree...


> > diff --git a/sound/isa/cs423x/cs4231_lib.c b/sound/isa/cs423x/cs4231_lib.c
> > index 914d77b..9e8e0f1 100644
> > --- a/sound/isa/cs423x/cs4231_lib.c
> > +++ b/sound/isa/cs423x/cs4231_lib.c
> > @@ -346,16 +346,14 @@ void snd_cs4231_mce_down(struct snd_cs4231 *chip)
> >  	}
> >  	snd_cs4231_busy_wait(chip);
> >  
> > -	/* calibration process */
> > -
> 
> The snd_cs4231_busy_wait(chip) is not needed any more here.

Care to create a patch?

The latest tree can be accessed via hg.alsa-project.org (in case
hg-mirror is out of sync).


thanks,

Takashi

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Krzysztof Helt]

On Mon, 10 Sep 2007 20:29:21 +0200
Rene Herman <rene.herman@gmail.com> wrote:
> 
> Thanks to Krysztof Helt for pinpointing this problem.
> 

Please change it to Krzysztof Helt


> diff --git a/sound/isa/cs423x/cs4231_lib.c b/sound/isa/cs423x/cs4231_lib.c
> index 914d77b..9e8e0f1 100644
> --- a/sound/isa/cs423x/cs4231_lib.c
> +++ b/sound/isa/cs423x/cs4231_lib.c
> @@ -346,16 +346,14 @@ void snd_cs4231_mce_down(struct snd_cs4231 *chip)
>  	}
>  	snd_cs4231_busy_wait(chip);
>  
> -	/* calibration process */
> -

The snd_cs4231_busy_wait(chip) is not needed any more here.

Regards,
Krzysztof

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Rene Herman]

On 09/10/2007 11:40 PM, Krzysztof Helt wrote:

> On Mon, 10 Sep 2007 20:29:21 +0200
> Rene Herman <rene.herman@gmail.com> wrote:
>> Thanks to Krysztof Helt for pinpointing this problem.
>>
> 
> Please change it to Krzysztof Helt

Oh, very sorry.

>> diff --git a/sound/isa/cs423x/cs4231_lib.c b/sound/isa/cs423x/cs4231_lib.c
>> index 914d77b..9e8e0f1 100644
>> --- a/sound/isa/cs423x/cs4231_lib.c
>> +++ b/sound/isa/cs423x/cs4231_lib.c
>> @@ -346,16 +346,14 @@ void snd_cs4231_mce_down(struct snd_cs4231 *chip)
>>  	}
>>  	snd_cs4231_busy_wait(chip);
>>  
>> -	/* calibration process */
>> -
> 
> The snd_cs4231_busy_wait(chip) is not needed any more here.

I believe it is. I glanced over the CS4232 datasheet (note how cs4232 uses 
snd-cs4332-lib) and it adds a point 4 (see page 53):

* Wait until 80h NOT returned

I didn't read closely, so feel free to point out if I'm misinterpreting that.

Rene.

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Rene Herman]

On 09/10/2007 11:43 PM, Rene Herman wrote:

> On 09/10/2007 11:40 PM, Krzysztof Helt wrote:

>> The snd_cs4231_busy_wait(chip) is not needed any more here.
> 
> I believe it is. I glanced over the CS4232 datasheet (note how cs4232 
> uses snd-cs4332-lib) and it adds a point 4 (see page 53):

I need to stop this annoying replying-to-myself, but my fingers aren't 
cooperating. snd-cs4231-lib.

> * Wait until 80h NOT returned
> 
> I didn't read closely, so feel free to point out if I'm misinterpreting 
> that.

Rene.

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Krzysztof Helt]

On 9/10/07, Rene Herman <rene.herman@gmail.com> wrote:
> On 09/10/2007 11:40 PM, Krzysztof Helt wrote:
>
> > On Mon, 10 Sep 2007 20:29:21 +0200
> > Rene Herman <rene.herman@gmail.com> wrote:
> >> Thanks to Krysztof Helt for pinpointing this problem.
> >>
> >
> > Please change it to Krzysztof Helt
>
> Oh, very sorry.
>
> >> diff --git a/sound/isa/cs423x/cs4231_lib.c b/sound/isa/cs423x/cs4231_lib.c
> >> index 914d77b..9e8e0f1 100644
> >> --- a/sound/isa/cs423x/cs4231_lib.c
> >> +++ b/sound/isa/cs423x/cs4231_lib.c
> >> @@ -346,16 +346,14 @@ void snd_cs4231_mce_down(struct snd_cs4231 *chip)
> >>      }
> >>      snd_cs4231_busy_wait(chip);
> >>
> >> -    /* calibration process */
> >> -
> >
> > The snd_cs4231_busy_wait(chip) is not needed any more here.
>
> I believe it is. I glanced over the CS4232 datasheet (note how cs4232 uses
> snd-cs4332-lib) and it adds a point 4 (see page 53):
>
> * Wait until 80h NOT returned
>

I think it is not needed to way for it with this 1ms delay.

Anyway, do you care to change this long snd_cs4231_busy_wait(chip) to
the snd_cs4231_wait(chip) used in other functions to wait for ready
bit?

I suppose the comment and code:

 /* huh.. looks like this sequence is proper for CS4231A chip (GUS MAX) */
    for (timeout = 5; timeout > 0; timeout--)
        cs4231_inb(chip, CS4231P(REGSEL));


was needed to fix what we fixed (delay checking bits until they are
set). My aim is to completely eliminate snd_cs4231_busy_wait() and
replace it with just snd_cs4231_wait().

Regards,
Krzysztof

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Rene Herman]

On 09/11/2007 10:56 AM, Krzysztof Helt wrote:

> Anyway, do you care to change this long snd_cs4231_busy_wait(chip) to
> the snd_cs4231_wait(chip) used in other functions to wait for ready
> bit?

That is, only drop the "huh?" bit? Yes, that should be okay. It looks a 
little off and I have a large testing pool if need be (CS4248, CS4231(A), 
CS4232/5/6/7/8 and 9)

Rene.

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Trent Piepho]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3295 bytes --]

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.

[-- Attachment #2: Type: TEXT/PLAIN, Size: 3186 bytes --]

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@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)));

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Krzysztof Helt]

On Mon, 17 Sep 2007 14:04:21 -0700 (PDT)
Trent Piepho <xyzzy@speakeasy.org> wrote:

> 
> 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().
> 
Good catch. If we are going after cs4231_lib, we can drop lock in loops
of the mce_down function (the driver only reads registers).

> 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.
> 

Please check out current alsa-kernel tree. The ad1848 now waits as the cs4231 does.
Loops have 1ms delay and there is one 1ms delay before the first loop.

> 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!
> 

No, the loop never had the 250ms delay. The whole loop (all iterations) was 250ms delay.

> 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.  

The smallest value for ad1848 is over 7ms (from experiments). 
It is about 384 samples at 48kHz.

> A 250 ms polling interval seems to be quite long.

The interval was 10ms in the original cs4231 and no delay in ad1848.

> 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.
> 

Good catch again. Just look into the "Changing sample rates" section.
Maybe waiting loop for the init bit off should be the first?

> 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.
> 

We may drop interrupts disabling in waiting loops, because inside them the
driver only reads registers.

> 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.
> 

This is not optimal from the latency point of view. The 3ms delay inside the loop
means that one must wait either 3 or 6 or 9 ms. Event if the real hardware answers
correctly after 4ms.

> 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.

The whole point of sleep before loop is to make this first iteration outside the loop
so it is not obscured by any special condition.

Take into account that this driver is used also for ad1847, cs4248 and cmi8330.
This means that the perfect delays for the ad1848 may be not optimal for all of them.

I would like to see the patch which fixes this locked msleep and maybe even remove
locks from inside loops.

Any improvement is welcome,
Krzysztof

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Rene Herman]

On 09/17/2007 11:04 PM, Trent Piepho wrote:

> 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().

Yes, fuckup. Apologies -- had an mdelay(1) in there originally.

> 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.

That mce_down code was changed over the last week by Krzysztof, myself and 
Takashi so not sure what version you've been looking at, but the (original) 
version that the quoted patch was against didn't use schedule_timeout, but a 
timeout based sleeping loop for cs4231 and schedule_timeout_interruptible() 
for ad1848 which sets the state itself.

(and since then, it has been changed to use a timeout based sleeping loop 
for ad1848 as well by Krzysztof which I see is the version your patch is 
against, so not sure what you mean).

[ ... ]

> 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.

Oh, quite. Didn't change it from the original though -- this code is used 
by a number of different chips so need to be careful. 250 seems a little 
silly yes, but given that it's (now) at a 1ms granularity it's okay as far 
as I'm concerned.

[ ... ]

> 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.

Ack.

> Here is a patch to fix all these problems and simplify the code in the
> process.

I looked at it, but am not sure what version it is against. It seems to 
msleep(3) still under lock. As you said, the minimal fix right now is to 
bracket that initial msleep(1) delay with a spin_unlock_irqrestore / 
spin_lock_irqsave pair or just make it be mdelay(1). Could you do that 
against current HG and then do other possible changes incrementally on top?

Rene.

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Trent Piepho]

On Mon, 17 Sep 2007, Krzysztof Helt wrote:
> On Mon, 17 Sep 2007 14:04:21 -0700 (PDT)
> Trent Piepho <xyzzy@speakeasy.org> wrote:
>
> >
> > 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().
> >
> Good catch. If we are going after cs4231_lib, we can drop lock in loops
> of the mce_down function (the driver only reads registers).

It looks like one needs to have the lock to access (read or write) any of
the "indirect" registers.  For example, in order to read from
AD1848_TEST_INIT, one must first write the index into the REGSEL register,
then read from the REG register.  If another thread tried to concurrently
read or write a different indirect register, it would modify the index.

> > 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!
> >
>
> No, the loop never had the 250ms delay. The whole loop (all iterations) was 250ms delay.

That's what I said.  It obviously didn't have a 250 ms delay, or open()
would take four minutes.  Note that the delay was not 250 ms total either,
it would loop forever until ACI went down.

for(time = 250; time > 0 ; ) time = schedule_timeout(time);

Unless you set the task state first, that line is an infinite busy loop.

> > 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.
>
> The smallest value for ad1848 is over 7ms (from experiments).
> It is about 384 samples at 48kHz.

You said before that it took 3 jiffies on a 1000 Hz kernel.  I guess
auto-calibration is always on when MCE goes down?  It looks like it should
only take 128 samples when MCE goes down and auto-calibration isn't done.

> > 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.
> >
>
> Good catch again. Just look into the "Changing sample rates" section.
> Maybe waiting loop for the init bit off should be the first?

snd_ad1848_in() already calls snd_ad1848_wait(), so it seems like it
would be redundant.

> > 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.
> >
>
> We may drop interrupts disabling in waiting loops, because inside them the
> driver only reads registers.

If reg_lock is ever acquired from within an interrupt handler, you must
disabled interrupts when you acquire it.  If it's never acquired from
within an interrupt handler, then you never need to disable interrupts.

> > 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.
>
> This is not optimal from the latency point of view. The 3ms delay inside the loop
> means that one must wait either 3 or 6 or 9 ms. Event if the real hardware answers
> correctly after 4ms.

Obviously from a latency point of view the optimal value would be to poll
as fast as possible.  Which is the worst from an efficiency point of view.
So you make a trade off.  If 90% of the time it's done by 2.9 ms, then
waiting 3 ms means you have very little extra latency 90% of the time and
only poll once 90% of the time.

> > 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.
>
> The whole point of sleep before loop is to make this first iteration outside the loop
> so it is not obscured by any special condition.

I'm well aware of that.  By delaying first, then checking, it's no longer
necessary to have an extra delay before the loop.

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Rene Herman]

On 09/18/2007 02:17 AM, Rene Herman wrote:

>> 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.
> 
> That mce_down code was changed over the last week by Krzysztof, myself 
> and Takashi so not sure what version you've been looking at, but the 
> (original) version that the quoted patch was against didn't use 
> schedule_timeout, but a timeout based sleeping loop for cs4231 and 
> schedule_timeout_interruptible() for ad1848 which sets the state itself.

Oh. This discrepency is caused by the fact that I work against the kernel 
and only check ALSA HG every once in a while. Too infrequently it seems as 
the _interruptible was recently (and yes, wrongly) removed from ALSA:

http://hg.alsa-project.org/alsa-kernel/rev/1768363a5f1e

It's still there in 2.6.22.x which I run. The setup has been changed around 
in the meantime again anyway in this case but I guess I'll make a point of 
working against HG more directly.

Rene.

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Trent Piepho]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1761 bytes --]

On Tue, 18 Sep 2007, Rene Herman wrote:
> > 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.
>
> That mce_down code was changed over the last week by Krzysztof, myself and
> Takashi so not sure what version you've been looking at, but the (original)

I originally wrote that email in response to the email I replied to, and
with respect to the code that was in it:
http://article.gmane.org/gmane.linux.alsa.devel/48528

There were more patches before I actually sent the email, so I based my
patch on what was in ALSA Hg at the time.

> version that the quoted patch was against didn't use schedule_timeout, but a
> timeout based sleeping loop for cs4231 and schedule_timeout_interruptible()
> for ad1848 which sets the state itself.

You can see the version in Hg after your patch was applied, and it
uses schedule_timeout()
http://hg.alsa-project.org/alsa-kernel/file/f3f37d068eae/isa/ad1848/ad1848_lib.c

It wasn't until changeset 3675ae62becf, 7 days later (and after I wrote that
email), that it was changed.

> > Here is a patch to fix all these problems and simplify the code in the
> > process.
>
> I looked at it, but am not sure what version it is against. It seems to

Current Hg.

> msleep(3) still under lock. As you said, the minimal fix right now is to

I seem to have deleted the necessary unlock call before I sent the patch.  One
of the tests is inverted too.  Here is a new version which fixes all that.  The
other problem you pointed at out in the current code about mistaken timeouts is
fixed as well.

[-- Attachment #2: Type: TEXT/PLAIN, Size: 656 bytes --]

ad1848: Fix msleep while atomic

Simplest fix.

Signed-off-by: Trent Piepho <xyzzy@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 19:19:52 2007 -0700
@@ -236,7 +236,9 @@ static void snd_ad1848_mce_down(struct s
 	 * calibration process to start. Needs upto 5 sample periods on AD1848
 	 * which at the slowest possible rate of 5.5125 kHz means 907 us.
 	 */
+	spin_unlock_irqrestore(&chip->reg_lock, flags);
 	msleep(1);
+	spin_lock_irqsave(&chip->reg_lock, flags);
 
 	snd_printdd("(2) jiffies = %lu\n", jiffies);
 

[-- Attachment #3: Type: TEXT/PLAIN, Size: 3981 bytes --]

ad1848: simplify MCE down code

The 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 is a lot simpler.  The
old loop checked for a timeout before checking for ACI down, which could
result in an erroneous timeout.  It's only a failure if the timeout expires
_and_ ACI is still high.  There is nothing wrong with the timeout expiring
while the task is sleeping if ACI went low.

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@speakeasy.org>

diff -r ec96283a273f isa/ad1848/ad1848_lib.c
--- a/isa/ad1848/ad1848_lib.c	Mon Sep 17 19:20:48 2007 -0700
+++ b/isa/ad1848/ad1848_lib.c	Mon Sep 17 19:22:36 2007 -0700
@@ -203,9 +203,8 @@ static void snd_ad1848_mce_up(struct snd
 
 static void snd_ad1848_mce_down(struct snd_ad1848 *chip)
 {
-	unsigned long flags;
-	int timeout;
-	unsigned long end_time;
+	unsigned long flags, timeout;
+	int regsel;
 
 	spin_lock_irqsave(&chip->reg_lock, flags);
 	for (timeout = 5; timeout > 0; timeout--)
@@ -222,50 +221,34 @@ static void snd_ad1848_mce_down(struct s
 #endif
 
 	chip->mce_bit &= ~AD1848_MCE;
-	timeout = inb(AD1848P(chip, REGSEL));
-	outb(chip->mce_bit | (timeout & 0x1f), AD1848P(chip, REGSEL));
-	if (timeout == 0x80)
+	regsel = inb(AD1848P(chip, REGSEL));
+	outb(chip->mce_bit | (regsel & 0x1f), AD1848P(chip, REGSEL));
+	if (regsel == 0x80)
 		snd_printk(KERN_WARNING "mce_down [0x%lx]: serious init problem - codec still busy\n", chip->port);
-	if ((timeout & AD1848_MCE) == 0) {
+	if ((regsel & AD1848_MCE) == 0) {
 		spin_unlock_irqrestore(&chip->reg_lock, flags);
 		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 wait of at most 70 ms with a more
+	 * typical value of 3-9 ms.
 	 */
-	spin_unlock_irqrestore(&chip->reg_lock, flags);
-	msleep(1);
-	spin_lock_irqsave(&chip->reg_lock, flags);
-
-	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) {
+	timeout = jiffies + msecs_to_jiffies(250);
+	do {
 		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);
 		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);
+		regsel = snd_ad1848_in(chip, AD1848_TEST_INIT);
+	} while ((regsel & AD1848_CALIB_IN_PROGRESS) && time_before(jiffies, timeout));
+	spin_unlock_irqrestore(&chip->reg_lock, flags);
+	if (regsel & AD1848_CALIB_IN_PROGRESS)
+		snd_printk(KERN_ERR "mce_down - auto calibration time out (2)\n");
 
 	snd_printdd("(4) jiffies = %lu\n", jiffies);
 	snd_printd("mce_down - exit = 0x%x\n", inb(AD1848P(chip, REGSEL)));

[-- Attachment #4: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 1745 bytes --]

On 09/18/2007 03:57 AM, Rene Herman wrote:

> Oh. This discrepency is caused by the fact that I work against the 
> kernel and only check ALSA HG every once in a while. Too infrequently it 
> seems as the _interruptible was recently (and yes, wrongly) removed from 
> ALSA:
> 
> http://hg.alsa-project.org/alsa-kernel/rev/1768363a5f1e
> 
> It's still there in 2.6.22.x which I run. The setup has been changed 
> around in the meantime again anyway in this case but I guess I'll make a 
> point of working against HG more directly.

And here's the others in that changeset, against current hg. As far as I'm 
aware, the only purpose of the _interruptible versions is bailing out when 
signal_pending(current) and if we're simply looping around without checking 
anyway, they might as well be schedule_timeout_uninterruptible(), also known 
as msleep().

Given that delaying for a jiffy generally doesn't make a great deal of sense 
given variable frequency, they might actually want to be msleep(1) directly 
but I didn't do that.

This also adds a barrier() to one of the /core/seq/seq_instr.c ones which as 
far as I can see is needed to keep the compiler from optimising the check 
away. For the other instances there, the spin_lock functions serve as a 
barrier already I believe but please check me on that -- I'm inexperienced 
at that level.

The wavefront_synth one was strange. It was originally (when still using 
_interruptible) in the absence of signals basically just doing a single:

	msleep(jiffies_to_msecs(timeout));

with the dev->irq_ok check only happening when woken up by a signal which I 
assume was not so much intended. This now just does a sleeping loop.

Otherwise,

Signed-off-by: Rene Herman <rene.herman@gmail.com>


[-- Attachment #2: schedule_timeout-fixes.diff --]
[-- Type: text/plain, Size: 4474 bytes --]

diff -r 0028e39ead78 core/seq/seq_instr.c
--- a/core/seq/seq_instr.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/core/seq/seq_instr.c	Tue Sep 18 06:20:25 2007 +0200
@@ -109,7 +109,7 @@ void snd_seq_instr_list_free(struct snd_
 			spin_lock_irqsave(&list->lock, flags);
 			while (instr->use) {
 				spin_unlock_irqrestore(&list->lock, flags);
-				schedule_timeout(1);
+				schedule_timeout_uninterruptible(1);
 				spin_lock_irqsave(&list->lock, flags);
 			}				
 			spin_unlock_irqrestore(&list->lock, flags);
@@ -198,8 +198,10 @@ int snd_seq_instr_list_free_cond(struct 
 		while (flist) {
 			instr = flist;
 			flist = instr->next;
-			while (instr->use)
-				schedule_timeout(1);
+			while (instr->use) {
+				schedule_timeout_uninterruptible(1);
+				barrier();
+			}
 			if (snd_seq_instr_free(instr, atomic)<0)
 				snd_printk(KERN_WARNING "instrument free problem\n");
 			instr = next;
@@ -555,7 +557,7 @@ static int instr_free(struct snd_seq_kin
 					   SNDRV_SEQ_INSTR_NOTIFY_REMOVE);
 		while (instr->use) {
 			spin_unlock_irqrestore(&list->lock, flags);
-			schedule_timeout(1);
+			schedule_timeout_uninterruptible(1);
 			spin_lock_irqsave(&list->lock, flags);
 		}				
 		spin_unlock_irqrestore(&list->lock, flags);
diff -r 0028e39ead78 isa/sscape.c
--- a/isa/sscape.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/isa/sscape.c	Tue Sep 18 06:20:25 2007 +0200
@@ -428,7 +428,7 @@ static int host_startup_ack(struct sound
 		unsigned long flags;
 		unsigned char x;
 
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 
 		spin_lock_irqsave(&s->lock, flags);
 		x = inb(HOST_DATA_IO(s->io_base));
diff -r 0028e39ead78 isa/wavefront/wavefront_synth.c
--- a/isa/wavefront/wavefront_synth.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/isa/wavefront/wavefront_synth.c	Tue Sep 18 06:20:25 2007 +0200
@@ -1768,7 +1768,7 @@ snd_wavefront_interrupt_bits (int irq)
 
 static void __devinit
 wavefront_should_cause_interrupt (snd_wavefront_t *dev, 
-				  int val, int port, int timeout)
+				  int val, int port, unsigned long timeout)
 
 {
 	wait_queue_t wait;
@@ -1779,11 +1779,9 @@ wavefront_should_cause_interrupt (snd_wa
 	dev->irq_ok = 0;
 	outb (val,port);
 	spin_unlock_irq(&dev->irq_lock);
-	while (1) {
-		if ((timeout = schedule_timeout(timeout)) == 0)
-			return;
-		if (dev->irq_ok)
-			return;
+	while (!dev->irq_ok && time_before(jiffies, timeout)) {
+		schedule_timeout_uninterruptible(1);
+		barrier();
 	}
 }
 
diff -r 0028e39ead78 pci/hda/hda_intel.c
--- a/pci/hda/hda_intel.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/pci/hda/hda_intel.c	Tue Sep 18 06:20:25 2007 +0200
@@ -555,7 +555,7 @@ static unsigned int azx_rirb_get_respons
 		}
 		if (!chip->rirb.cmds)
 			return chip->rirb.res; /* the last value */
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 	} while (time_after_eq(timeout, jiffies));
 
 	if (chip->msi) {
diff -r 0028e39ead78 pci/via82xx.c
--- a/pci/via82xx.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/pci/via82xx.c	Tue Sep 18 06:20:25 2007 +0200
@@ -2090,7 +2090,7 @@ static int snd_via82xx_chip_init(struct 
 		pci_read_config_byte(chip->pci, VIA_ACLINK_STAT, &pval);
 		if (pval & VIA_ACLINK_C00_READY) /* primary codec ready */
 			break;
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 	} while (time_before(jiffies, end_time));
 
 	if ((val = snd_via82xx_codec_xread(chip)) & VIA_REG_AC97_BUSY)
@@ -2109,7 +2109,7 @@ static int snd_via82xx_chip_init(struct 
 			chip->ac97_secondary = 1;
 			goto __ac97_ok2;
 		}
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 	} while (time_before(jiffies, end_time));
 	/* This is ok, the most of motherboards have only one codec */
 
diff -r 0028e39ead78 pci/via82xx_modem.c
--- a/pci/via82xx_modem.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/pci/via82xx_modem.c	Tue Sep 18 06:20:25 2007 +0200
@@ -983,7 +983,7 @@ static int snd_via82xx_chip_init(struct 
 		pci_read_config_byte(chip->pci, VIA_ACLINK_STAT, &pval);
 		if (pval & VIA_ACLINK_C00_READY) /* primary codec ready */
 			break;
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 	} while (time_before(jiffies, end_time));
 
 	if ((val = snd_via82xx_codec_xread(chip)) & VIA_REG_AC97_BUSY)
@@ -1001,7 +1001,7 @@ static int snd_via82xx_chip_init(struct 
 			chip->ac97_secondary = 1;
 			goto __ac97_ok2;
 		}
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 	} while (time_before(jiffies, end_time));
 	/* This is ok, the most of motherboards have only one codec */
 

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Rene Herman]

On 09/18/2007 04:32 AM, Trent Piepho wrote:

> You can see the version in Hg after your patch was applied, and it uses
> schedule_timeout()

Yes, as said, that was a mismatch between the 2.6.22.x I was looking at and 
the patch that removed the _interruptible() from a few months go already. 
Also explains some of the mis-communication with Krzysztof earlier...

> I seem to have deleted the necessary unlock call before I sent the patch.
> One of the tests is inverted too. Here is a new version which fixes all 
> that. The other problem you pointed at out in the current code about 
> mistaken timeouts is fixed as well.

Yes, I agree with this, looking much better now. The removal of that last 
INIT checking loop _might_ cause a timing difference if earlier we just 
timed out in snd_ad1848_wait() but in that case, we're sol already anyway 
and who cares. Comment is good as well. Maybe s/should/could/ in:

+ * has in fact not begun. It should take 128 (no AC) or 384 (AC) cycles
+ * for ACI to drop. This gives a wait of at most 70 ms with a more
+ * typical value of 3-9 ms.

By the way, as to the cs4231 mirror image -- the CS4248 was the only chip 
that I didn't experience the autocalibration "(1)" timeout on using 
cs4231_lib which seems to indicate that for the CS423x chips, ACI never 
comes up at all when not auto-calibrating. Just for info, this same code 
handles that just fine.

I just now tested this on a CS4231A driven by ad1848_lib -- is working fine.

Acked-by: Rene Herman <rene.herman@gmail.com>

Rene.

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Krzysztof Helt]

On Mon, 17 Sep 2007 18:18:27 -0700 (PDT)
Trent Piepho <xyzzy@speakeasy.org> wrote:


> It looks like one needs to have the lock to access (read or write) any of
> the "indirect" registers.  For example, in order to read from
> AD1848_TEST_INIT, one must first write the index into the REGSEL register,
> then read from the REG register.  If another thread tried to concurrently
> read or write a different indirect register, it would modify the index.
> 

Right.

> >
> > No, the loop never had the 250ms delay. The whole loop (all iterations) was 250ms delay.
> 
> That's what I said.  It obviously didn't have a 250 ms delay, or open()
> would take four minutes.  Note that the delay was not 250 ms total either,
> it would loop forever until ACI went down.
> 
> for(time = 250; time > 0 ; ) time = schedule_timeout(time);
> 
> Unless you set the task state first, that line is an infinite busy loop.
> 

This may be not true. If you compare schedule_timeout() with cpu_relax()
you will see that schedule_timeout() is slower. I am not expert on this,
but I think that the schedule_timeout() switches tasks and returns
which is not busy waiting (like userland sleep(0) function does).
It would make it close to the cpu_relax().

 This could be original intention of Jeff Garzik's change.

> > > 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.
> >
> > The smallest value for ad1848 is over 7ms (from experiments).
> > It is about 384 samples at 48kHz.
> 
> You said before that it took 3 jiffies on a 1000 Hz kernel.  I guess
> auto-calibration is always on when MCE goes down?  It looks like it should
> only take 128 samples when MCE goes down and auto-calibration isn't done.
> 

It was for CS4231 chip which is faster. On the CS4231 it is 3 jiffies. You confused
these two chips here.

> snd_ad1848_in() already calls snd_ad1848_wait(), so it seems like it
> would be redundant.
> 

The whole waiting for the busy bit is redundant because any next operation (in or out)
will wait for it anyway.

> Obviously from a latency point of view the optimal value would be to poll
> as fast as possible.  Which is the worst from an efficiency point of view.

No, you can poll at maximum speed and call cpu_relax() after each poll.
This will provide the least latency with little overhead. It is probably
overkill if waiting is in the range 7 to 70 ms (it will be thousand of polls).

> So you make a trade off.  If 90% of the time it's done by 2.9 ms, then
> waiting 3 ms means you have very little extra latency 90% of the time and
> only poll once 90% of the time.
> 

The waiting of 3ms does not mean that you do it 90% of time. It depends
on frequency so if someone plays 44kHz files (which gives delay above 3ms) it will
be penalized by additional 2ms (6ms instead of 4ms) of latency 100% of time.

> > The whole point of sleep before loop is to make this first iteration outside the loop
> > so it is not obscured by any special condition.
> 
> I'm well aware of that.  By delaying first, then checking, it's no longer
> necessary to have an extra delay before the loop.

The original idea was to make this first waiting longer than loop granularity.
7ms before the loop than 1ms granularity inside the loop.
Rene Herman was against it so he changed it to be the same as 1ms inside
the loop.

Regards,
Krzysztof

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Krzysztof Helt]

On Mon, 17 Sep 2007 19:32:11 -0700 (PDT)
Trent Piepho <xyzzy@speakeasy.org> wrote:

> Simplest fix.
> 
> Signed-off-by: Trent Piepho <xyzzy@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 19:19:52 2007 -0700
> @@ -236,7 +236,9 @@ static void snd_ad1848_mce_down(struct s
>  	 * calibration process to start. Needs upto 5 sample periods on AD1848
>  	 * which at the slowest possible rate of 5.5125 kHz means 907 us.
>  	 */
> +	spin_unlock_irqrestore(&chip->reg_lock, flags);
>  	msleep(1);
> +	spin_lock_irqsave(&chip->reg_lock, flags);
>  
>  	snd_printdd("(2) jiffies = %lu\n", jiffies);
>  

Acked-by: Krzysztof Helt <krzysztof.h1@wp.pl>

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Krzysztof Helt]

On Mon, 17 Sep 2007 19:32:11 -0700 (PDT)
Trent Piepho <xyzzy@speakeasy.org> wrote:

> 
> The 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 is a lot simpler.  The
> old loop checked for a timeout before checking for ACI down, which could
> result in an erroneous timeout.  It's only a failure if the timeout expires
> _and_ ACI is still high.  There is nothing wrong with the timeout expiring
> while the task is sleeping if ACI went low.
> 

This was already fixed by Rene's patch. You only simplified it.

> 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@speakeasy.org>
> 
> diff -r ec96283a273f isa/ad1848/ad1848_lib.c
> --- a/isa/ad1848/ad1848_lib.c	Mon Sep 17 19:20:48 2007 -0700
> +++ b/isa/ad1848/ad1848_lib.c	Mon Sep 17 19:22:36 2007 -0700
> @@ -203,9 +203,8 @@ static void snd_ad1848_mce_up(struct snd
>  
>  static void snd_ad1848_mce_down(struct snd_ad1848 *chip)
>  {
> -	unsigned long flags;
> -	int timeout;
> -	unsigned long end_time;
> +	unsigned long flags, timeout;
> +	int regsel;
>  
>  	spin_lock_irqsave(&chip->reg_lock, flags);
>  	for (timeout = 5; timeout > 0; timeout--)
> @@ -222,50 +221,34 @@ static void snd_ad1848_mce_down(struct s
>  #endif
>  
>  	chip->mce_bit &= ~AD1848_MCE;
> -	timeout = inb(AD1848P(chip, REGSEL));
> -	outb(chip->mce_bit | (timeout & 0x1f), AD1848P(chip, REGSEL));
> -	if (timeout == 0x80)
> +	regsel = inb(AD1848P(chip, REGSEL));
> +	outb(chip->mce_bit | (regsel & 0x1f), AD1848P(chip, REGSEL));
> +	if (regsel == 0x80)
>  		snd_printk(KERN_WARNING "mce_down [0x%lx]: serious init problem - codec still busy\n", chip->port);
> -	if ((timeout & AD1848_MCE) == 0) {
> +	if ((regsel & AD1848_MCE) == 0) {
>  		spin_unlock_irqrestore(&chip->reg_lock, flags);
>  		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 wait of at most 70 ms with a more
> +	 * typical value of 3-9 ms.
>  	 */
> -	spin_unlock_irqrestore(&chip->reg_lock, flags);
> -	msleep(1);
> -	spin_lock_irqsave(&chip->reg_lock, flags);
> -
> -	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) {
> +	timeout = jiffies + msecs_to_jiffies(250);
> +	do {
>  		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);
>  		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);
> +		regsel = snd_ad1848_in(chip, AD1848_TEST_INIT);
> +	} while ((regsel & AD1848_CALIB_IN_PROGRESS) && time_before(jiffies, timeout));

Break this long line. You may calculate "regsel & AD1848_CALIB_IN_PROGRESS"
inside the loop and use it in the condition outside the loop too.

Regards,
Krzysztof

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Rene Herman]

On 09/18/2007 10:02 AM, Krzysztof Helt wrote:

>> +	timeout = jiffies + msecs_to_jiffies(250);
>> +	do {
>>  		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);
>>  		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);
>> +		regsel = snd_ad1848_in(chip, AD1848_TEST_INIT);
>> +	} while ((regsel & AD1848_CALIB_IN_PROGRESS) && time_before(jiffies, timeout));
> 
> Break this long line. You may calculate "regsel & AD1848_CALIB_IN_PROGRESS"
> inside the loop and use it in the condition outside the loop too.

Or just break out directly with a goto:

		regsel = snd_ad1848_in();
		if (!(regsel & AD1848_CALIB_IN_PROGRESS))
			goto out;
	while (time_before(jiffies, timeout));

	snd_printk(KERN_ERR "mce_down - auto calibration time out (2)\n")
out:
	spin_unlock_irqrestore()
	snd_printd()
	return;
}

Have grown to like those best generally -= falling off a timeout-loop means 
you've timed out, and if not, you jump over the error handling for that.

But I'm quite sure we'll be able to get religious over that. We're four 
people patching the same little function over and over again, so we're 
pretty daft anyway :-)

Rene.

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 192 bytes --]

On 09/18/2007 06:24 AM, Rene Herman wrote:

> And here's the others in that changeset, against current hg. As far as

One more inside isa/.

Signed-off-by: Rene Herman <rene.herman@gmail.com>

[-- Attachment #2: schedule_timeout-fixes-one-more.diff --]
[-- Type: text/plain, Size: 384 bytes --]

diff -r 0028e39ead78 isa/sscape.c
--- a/isa/sscape.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/isa/sscape.c	Tue Sep 18 13:01:55 2007 +0200
@@ -401,7 +401,7 @@ static int obp_startup_ack(struct sounds
 		unsigned long flags;
 		unsigned char x;
 
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 
 		spin_lock_irqsave(&s->lock, flags);
 		x = inb(HOST_DATA_IO(s->io_base));

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Takashi Iwai]

At Tue, 18 Sep 2007 06:24:04 +0200,
Rene Herman wrote:
> 
> On 09/18/2007 03:57 AM, Rene Herman wrote:
> 
> > Oh. This discrepency is caused by the fact that I work against the 
> > kernel and only check ALSA HG every once in a while. Too infrequently it 
> > seems as the _interruptible was recently (and yes, wrongly) removed from 
> > ALSA:
> > 
> > http://hg.alsa-project.org/alsa-kernel/rev/1768363a5f1e
> > 
> > It's still there in 2.6.22.x which I run. The setup has been changed 
> > around in the meantime again anyway in this case but I guess I'll make a 
> > point of working against HG more directly.
> 
> And here's the others in that changeset, against current hg. As far as I'm 
> aware, the only purpose of the _interruptible versions is bailing out when 
> signal_pending(current) and if we're simply looping around without checking 
> anyway, they might as well be schedule_timeout_uninterruptible(), also known 
> as msleep().
> 
> Given that delaying for a jiffy generally doesn't make a great deal of sense 
> given variable frequency, they might actually want to be msleep(1) directly 
> but I didn't do that.
> 
> This also adds a barrier() to one of the /core/seq/seq_instr.c ones which as 
> far as I can see is needed to keep the compiler from optimising the check 
> away. For the other instances there, the spin_lock functions serve as a 
> barrier already I believe but please check me on that -- I'm inexperienced 
> at that level.
> 
> The wavefront_synth one was strange. It was originally (when still using 
> _interruptible) in the absence of signals basically just doing a single:
> 
> 	msleep(jiffies_to_msecs(timeout));
> 
> with the dev->irq_ok check only happening when woken up by a signal which I 
> assume was not so much intended. This now just does a sleeping loop.
> 
> Otherwise,
> 
> Signed-off-by: Rene Herman <rene.herman@gmail.com>

Let's leave seq_instr as is.  It's a never-working concept.  The only
user is OPL3, and this should be rewritten using hwdep.  Then we'll
get rid of this whole code chunk.

The other changes look good to me.  But, honestly, I couldn't follow
all the pathes you sent in the right order.  So, could you guys make a
series of patches to be applied to HG tree?  That'll be really helpful
for review, too.


Thanks,

Takashi

[ PATCH 1/4] alsa-kernel: schedule_timeout() fixes

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

On 09/18/2007 01:54 PM, Takashi Iwai wrote:

> The other changes look good to me.  But, honestly, I couldn't follow all
> the pathes you sent in the right order.  So, could you guys make a series
> of patches to be applied to HG tree?  That'll be really helpful for
> review, too.

Ofcourse. These schedule_timeout() fixes are not dependent on anything else. 
  I audited alsa-kernel for this schedule_timeout() issue, and this and next 
two messages do all I found.

===

alsa-kernel: schedule_timeout() fixes

Fix schedule_timeout() use in alsa-kernel. Mostly just

	schedule_timeout(1) --> schedule_timeout_uninterruptible(1)

The wavefront_synth one fixes the surrounding loop as well. In ymfpci_main, 
delete a superfluous set_current_state() and in soc/soc-dapm.c replace an 
_interruptible with _uninterruptible in some debug code; it's not waiting 
for signals.

Signed-off-by: Rene Herman <rene.herman>

[-- Attachment #2: schedule_timeout-alsa_kernel.diff --]
[-- Type: text/plain, Size: 4576 bytes --]

diff -r 0028e39ead78 isa/sscape.c
--- a/isa/sscape.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/isa/sscape.c	Tue Sep 18 14:53:57 2007 +0200
@@ -401,7 +401,7 @@ static int obp_startup_ack(struct sounds
 		unsigned long flags;
 		unsigned char x;
 
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 
 		spin_lock_irqsave(&s->lock, flags);
 		x = inb(HOST_DATA_IO(s->io_base));
@@ -428,7 +428,7 @@ static int host_startup_ack(struct sound
 		unsigned long flags;
 		unsigned char x;
 
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 
 		spin_lock_irqsave(&s->lock, flags);
 		x = inb(HOST_DATA_IO(s->io_base));
diff -r 0028e39ead78 isa/wavefront/wavefront_synth.c
--- a/isa/wavefront/wavefront_synth.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/isa/wavefront/wavefront_synth.c	Tue Sep 18 14:53:57 2007 +0200
@@ -1768,7 +1768,7 @@ snd_wavefront_interrupt_bits (int irq)
 
 static void __devinit
 wavefront_should_cause_interrupt (snd_wavefront_t *dev, 
-				  int val, int port, int timeout)
+				  int val, int port, unsigned long timeout)
 
 {
 	wait_queue_t wait;
@@ -1779,11 +1779,9 @@ wavefront_should_cause_interrupt (snd_wa
 	dev->irq_ok = 0;
 	outb (val,port);
 	spin_unlock_irq(&dev->irq_lock);
-	while (1) {
-		if ((timeout = schedule_timeout(timeout)) == 0)
-			return;
-		if (dev->irq_ok)
-			return;
+	while (!dev->irq_ok && time_before(jiffies, timeout)) {
+		schedule_timeout_uninterruptible(1);
+		barrier();
 	}
 }
 
diff -r 0028e39ead78 pci/hda/hda_intel.c
--- a/pci/hda/hda_intel.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/pci/hda/hda_intel.c	Tue Sep 18 14:53:57 2007 +0200
@@ -555,7 +555,7 @@ static unsigned int azx_rirb_get_respons
 		}
 		if (!chip->rirb.cmds)
 			return chip->rirb.res; /* the last value */
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 	} while (time_after_eq(timeout, jiffies));
 
 	if (chip->msi) {
diff -r 0028e39ead78 pci/via82xx.c
--- a/pci/via82xx.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/pci/via82xx.c	Tue Sep 18 14:53:57 2007 +0200
@@ -2090,7 +2090,7 @@ static int snd_via82xx_chip_init(struct 
 		pci_read_config_byte(chip->pci, VIA_ACLINK_STAT, &pval);
 		if (pval & VIA_ACLINK_C00_READY) /* primary codec ready */
 			break;
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 	} while (time_before(jiffies, end_time));
 
 	if ((val = snd_via82xx_codec_xread(chip)) & VIA_REG_AC97_BUSY)
@@ -2109,7 +2109,7 @@ static int snd_via82xx_chip_init(struct 
 			chip->ac97_secondary = 1;
 			goto __ac97_ok2;
 		}
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 	} while (time_before(jiffies, end_time));
 	/* This is ok, the most of motherboards have only one codec */
 
diff -r 0028e39ead78 pci/via82xx_modem.c
--- a/pci/via82xx_modem.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/pci/via82xx_modem.c	Tue Sep 18 14:53:57 2007 +0200
@@ -983,7 +983,7 @@ static int snd_via82xx_chip_init(struct 
 		pci_read_config_byte(chip->pci, VIA_ACLINK_STAT, &pval);
 		if (pval & VIA_ACLINK_C00_READY) /* primary codec ready */
 			break;
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 	} while (time_before(jiffies, end_time));
 
 	if ((val = snd_via82xx_codec_xread(chip)) & VIA_REG_AC97_BUSY)
@@ -1001,7 +1001,7 @@ static int snd_via82xx_chip_init(struct 
 			chip->ac97_secondary = 1;
 			goto __ac97_ok2;
 		}
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 	} while (time_before(jiffies, end_time));
 	/* This is ok, the most of motherboards have only one codec */
 
diff -r 0028e39ead78 pci/ymfpci/ymfpci_main.c
--- a/pci/ymfpci/ymfpci_main.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/pci/ymfpci/ymfpci_main.c	Tue Sep 18 14:53:57 2007 +0200
@@ -84,7 +84,6 @@ static int snd_ymfpci_codec_ready(struct
 	do {
 		if ((snd_ymfpci_readw(chip, reg) & 0x8000) == 0)
 			return 0;
-		set_current_state(TASK_UNINTERRUPTIBLE);
 		schedule_timeout_uninterruptible(1);
 	} while (time_before(jiffies, end_time));
 	snd_printk(KERN_ERR "codec_ready: codec %i is not ready [0x%x]\n", secondary, snd_ymfpci_readw(chip, reg));
diff -r 0028e39ead78 soc/soc-dapm.c
--- a/soc/soc-dapm.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/soc/soc-dapm.c	Tue Sep 18 14:53:57 2007 +0200
@@ -63,7 +63,7 @@
 #define POP_DEBUG 0
 #if POP_DEBUG
 #define POP_TIME 500 /* 500 msecs - change if pop debug is too fast */
-#define pop_wait(time) schedule_timeout_interruptible(msecs_to_jiffies(time))
+#define pop_wait(time) schedule_timeout_uninterruptible(msecs_to_jiffies(time))
 #define pop_dbg(format, arg...) printk(format, ## arg); pop_wait(POP_TIME)
 #else
 #define pop_dbg(format, arg...)

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 2/4] alsa-kernel: schedule_timeout() fix for kernel/drivers/input/touchscreen/ucb1400_ts.c

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

On 09/18/2007 01:54 PM, Takashi Iwai wrote:

> The other changes look good to me.  But, honestly, I couldn't follow all
> the pathes you sent in the right order.  So, could you guys make a series
> of patches to be applied to HG tree?  That'll be really helpful for
> review, too.

Only split of from the rest since I'm not in fact sure what/why that 
drivers/input stuff is inside alsa-kernel.

===

alsa-kernel: schedule_timeout() fix for ucb1400_ts.c

ucb14ts_ts.c is doing a (manual) schedule_timeout_uninterruptible, but is 
not actually checking for pending signals. An _uninterruptible() one will do 
then.

Signed-off-by: Rene Herman <rene.herman>

[-- Attachment #2: schedule_timeout-ucb1400_ts.diff --]
[-- Type: text/plain, Size: 516 bytes --]

diff -r 0028e39ead78 kernel/drivers/input/touchscreen/ucb1400_ts.c
--- a/kernel/drivers/input/touchscreen/ucb1400_ts.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/kernel/drivers/input/touchscreen/ucb1400_ts.c	Tue Sep 18 14:51:04 2007 +0200
@@ -130,8 +130,7 @@ static unsigned int ucb1400_adc_read(str
 		if (val & UCB_ADC_DAT_VALID)
 			break;
 		/* yield to other processes */
-		set_current_state(TASK_INTERRUPTIBLE);
-		schedule_timeout(1);
+		schedule_timeout_uninterruptible(1);
 	}
 
 	return UCB_ADC_DAT_VALUE(val);

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]

On 09/18/2007 01:54 PM, Takashi Iwai wrote:

> Let's leave seq_instr as is.  It's a never-working concept.  The only
> user is OPL3, and this should be rewritten using hwdep.  Then we'll
> get rid of this whole code chunk.

Well, if you insist, but thought I'd submit is seperately once again. The 
schedule_timeout() calls in there are so-so, will simply not schedule, but that

	while (instr->use)
		schedule_timeout(1);

loop is dangerous. There's nothing that I can see that's stopping the 
compiler from turning this into an infinite loop:

	if (instr->use)
		while (1)
			schedule_timeout(1);

I'll admit I have no idea where this code ends up, but if it's _ever_ used 
this seems to not be good.

Ofcourse, feel free to drop on the floor if you really do insist.

Signed-off-by: Rene Herman <rene.herman@gmail.com>

[-- Attachment #2: schedule_timeout-seq_instr.diff --]
[-- Type: text/plain, Size: 1230 bytes --]

diff -r 0028e39ead78 core/seq/seq_instr.c
--- a/core/seq/seq_instr.c	Tue Sep 18 00:52:38 2007 +0200
+++ b/core/seq/seq_instr.c	Tue Sep 18 14:49:04 2007 +0200
@@ -109,7 +109,7 @@ void snd_seq_instr_list_free(struct snd_
 			spin_lock_irqsave(&list->lock, flags);
 			while (instr->use) {
 				spin_unlock_irqrestore(&list->lock, flags);
-				schedule_timeout(1);
+				schedule_timeout_uninterruptible(1);
 				spin_lock_irqsave(&list->lock, flags);
 			}				
 			spin_unlock_irqrestore(&list->lock, flags);
@@ -198,8 +198,10 @@ int snd_seq_instr_list_free_cond(struct 
 		while (flist) {
 			instr = flist;
 			flist = instr->next;
-			while (instr->use)
-				schedule_timeout(1);
+			while (instr->use) {
+				schedule_timeout_uninterruptible(1);
+				barrier();
+			}
 			if (snd_seq_instr_free(instr, atomic)<0)
 				snd_printk(KERN_WARNING "instrument free problem\n");
 			instr = next;
@@ -555,7 +557,7 @@ static int instr_free(struct snd_seq_kin
 					   SNDRV_SEQ_INSTR_NOTIFY_REMOVE);
 		while (instr->use) {
 			spin_unlock_irqrestore(&list->lock, flags);
-			schedule_timeout(1);
+			schedule_timeout_uninterruptible(1);
 			spin_lock_irqsave(&list->lock, flags);
 		}				
 		spin_unlock_irqrestore(&list->lock, flags);

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 2/4] alsa-kernel: schedule_timeout() fix for kernel/drivers/input/touchscreen/ucb1400_ts.c

[Takashi Iwai]

At Tue, 18 Sep 2007 15:49:29 +0200,
Rene Herman wrote:
> 
> On 09/18/2007 01:54 PM, Takashi Iwai wrote:
> 
> > The other changes look good to me.  But, honestly, I couldn't follow all
> > the pathes you sent in the right order.  So, could you guys make a series
> > of patches to be applied to HG tree?  That'll be really helpful for
> > review, too.
> 
> Only split of from the rest since I'm not in fact sure what/why that 
> drivers/input stuff is inside alsa-kernel.

This code is in ALSA HG tree just because it refers AC97 interface.
When the ac97 side is changed, we might need to touch it.

But, this change has nothing to do with the sound, so I think it'd be
better to pass it to the input subsystem maintainer.


thanks,

Takashi

Re: [PATCH 2/4] alsa-kernel: schedule_timeout() fix for kernel/drivers/input/touchscreen/ucb1400_ts.c

[Rene Herman]

On 09/18/2007 03:58 PM, Takashi Iwai wrote:

> At Tue, 18 Sep 2007 15:49:29 +0200,
> Rene Herman wrote:

>> Only split of from the rest since I'm not in fact sure what/why that 
>> drivers/input stuff is inside alsa-kernel.
> 
> This code is in ALSA HG tree just because it refers AC97 interface.
> When the ac97 side is changed, we might need to touch it.
> 
> But, this change has nothing to do with the sound, so I think it'd be
> better to pass it to the input subsystem maintainer.

Will do, thanks.

Rene.

[PATCH 3/4] alsa-kernel: schedule_timeout() fix for core/seq/seq_instr.c

[Rene Herman]

On 09/18/2007 03:57 PM, Rene Herman wrote:

Subject was supposed to be as above.

> Well, if you insist, but thought I'd submit is seperately once again. 
> The schedule_timeout() calls in there are so-so, will simply not 
> schedule, but that
> 
>     while (instr->use)
>         schedule_timeout(1);
> 
> loop is dangerous. There's nothing that I can see that's stopping the 
> compiler from turning this into an infinite loop:
> 
>     if (instr->use)
>         while (1)
>             schedule_timeout(1);
> 
> I'll admit I have no idea where this code ends up, but if it's _ever_ 
> used this seems to not be good.
> 
> Ofcourse, feel free to drop on the floor if you really do insist.
> 
> Signed-off-by: Rene Herman <rene.herman@gmail.com>

Rene.

[PATCH 4/4] alsa-driver: schedule_timeout() fixes.

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

On 09/18/2007 01:54 PM, Takashi Iwai wrote:

> The other changes look good to me.  But, honestly, I couldn't follow all
> the pathes you sent in the right order.  So, could you guys make a series
> of patches to be applied to HG tree?  That'll be really helpful for
> review, too.

Two more for alsa-drivers. Just for completeness, but feel free to drop on 
the floor as well. I'm not hugely sure about that "if (!signal_pending)" in 
the msnd_pinnacle one (so just keep it interruptible) and the other two are 
just replacinging HZ calculations.

Speaking about that msnd one -- I believe I've seen that driver sitting in 
isa/ in alsa-drivers ages ago already. Is someone still working on that? 
(no, I don't have the hardware).

Signed-off-by: Rene Herman <rene.herman@gmail.com>

[-- Attachment #2: schedule_timeout-alsa_driver.diff --]
[-- Type: text/plain, Size: 1674 bytes --]

diff -r 3fefacd5d76c isa/msnd/msnd_pinnacle.c
--- a/isa/msnd/msnd_pinnacle.c	Mon Sep 17 19:04:40 2007 +0200
+++ b/isa/msnd/msnd_pinnacle.c	Tue Sep 18 16:21:50 2007 +0200
@@ -239,10 +239,8 @@ static void dsp_write_flush(void)
 		&dev.writeflush,
 		get_play_delay_jiffies(dev.DAPF.len));*/
 	clear_bit(F_WRITEFLUSH, &dev.flags);
-	if (!signal_pending(current)) {
-		current->state = TASK_INTERRUPTIBLE;
-		schedule_timeout(get_play_delay_jiffies( dev.play_period_bytes));
-	}
+	if (!signal_pending(current)) 
+		schedule_timeout_interruptible(get_play_delay_jiffies(dev.play_period_bytes));
 	clear_bit(F_WRITING, &dev.flags);
 }
 
@@ -691,8 +689,7 @@ static int __init snd_msnd_calibrate_adc
 		       & ~0x0001, dev.SMA + SMA_wCurrHostStatusFlags);
 	if (snd_msnd_send_word(&dev, 0, 0, HDEXAR_CAL_A_TO_D) == 0 &&
 	    snd_msnd_send_dsp_cmd_chk(&dev, HDEX_AUX_REQ) == 0) {
-		current->state = TASK_INTERRUPTIBLE;
-		schedule_timeout(HZ / 3);
+		schedule_timeout_interruptible(msecs_to_jiffiies(333));
 		return 0;
 	}
 	printk(KERN_WARNING LOGNAME ": ADC calibration failed\n");
diff -r 3fefacd5d76c pci/asihpi/hpios_linux_kernel.c
--- a/pci/asihpi/hpios_linux_kernel.c	Mon Sep 17 19:04:40 2007 +0200
+++ b/pci/asihpi/hpios_linux_kernel.c	Tue Sep 18 16:21:50 2007 +0200
@@ -38,8 +38,7 @@ and the task receives a signal.
 and the task receives a signal.
 Setting the state to UNINTERRUPTIBLE stops it from returning early.
 */
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		schedule_timeout((HZ * dwNumMicroSec + (HZ - 1)) / 1000000);
+		schedule_timeout_uninterruptible(usecs_to_jiffies(dwNumMicroSec));
 	} else if (dwNumMicroSec <= 2000)
 		udelay(dwNumMicroSec);
 	else

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Takashi Iwai]

At Tue, 18 Sep 2007 15:57:41 +0200,
Rene Herman wrote:
> 
> On 09/18/2007 01:54 PM, Takashi Iwai wrote:
> 
> > Let's leave seq_instr as is.  It's a never-working concept.  The only
> > user is OPL3, and this should be rewritten using hwdep.  Then we'll
> > get rid of this whole code chunk.
> 
> Well, if you insist, but thought I'd submit is seperately once again. The 
> schedule_timeout() calls in there are so-so, will simply not schedule, but that
> 
> 	while (instr->use)
> 		schedule_timeout(1);
> 
> loop is dangerous. There's nothing that I can see that's stopping the 
> compiler from turning this into an infinite loop:
> 
> 	if (instr->use)
> 		while (1)
> 			schedule_timeout(1);
> 
> I'll admit I have no idea where this code ends up, but if it's _ever_ used 
> this seems to not be good.

Fair enough, I merged it.


Takashi

Re: [ PATCH 1/4] alsa-kernel: schedule_timeout() fixes

[Takashi Iwai]

At Tue, 18 Sep 2007 15:38:45 +0200,
Rene Herman wrote:
> 
> On 09/18/2007 01:54 PM, Takashi Iwai wrote:
> 
> > The other changes look good to me.  But, honestly, I couldn't follow all
> > the pathes you sent in the right order.  So, could you guys make a series
> > of patches to be applied to HG tree?  That'll be really helpful for
> > review, too.
> 
> Ofcourse. These schedule_timeout() fixes are not dependent on anything else. 
>   I audited alsa-kernel for this schedule_timeout() issue, and this and next 
> two messages do all I found.
> 
> ===
> 
> alsa-kernel: schedule_timeout() fixes
> 
> Fix schedule_timeout() use in alsa-kernel. Mostly just
> 
> 	schedule_timeout(1) --> schedule_timeout_uninterruptible(1)
> 
> The wavefront_synth one fixes the surrounding loop as well. In ymfpci_main, 
> delete a superfluous set_current_state() and in soc/soc-dapm.c replace an 
> _interruptible with _uninterruptible in some debug code; it's not waiting 
> for signals.
> 
> Signed-off-by: Rene Herman <rene.herman>

Applied now to HG tree.

(BTW, could you cut the thread if you post patches?  When a post with
a patch to be merged is burried in a long thread, it's hard to find.)


Thanks,

Takashi

Re: [PATCH 4/4] alsa-driver: schedule_timeout() fixes.

[Takashi Iwai]

At Tue, 18 Sep 2007 16:34:46 +0200,
Rene Herman wrote:
> 
> On 09/18/2007 01:54 PM, Takashi Iwai wrote:
> 
> > The other changes look good to me.  But, honestly, I couldn't follow all
> > the pathes you sent in the right order.  So, could you guys make a series
> > of patches to be applied to HG tree?  That'll be really helpful for
> > review, too.
> 
> Two more for alsa-drivers. Just for completeness, but feel free to drop on 
> the floor as well. I'm not hugely sure about that "if (!signal_pending)" in 
> the msnd_pinnacle one (so just keep it interruptible) and the other two are 
> just replacinging HZ calculations.
> 
> Speaking about that msnd one -- I believe I've seen that driver sitting in 
> isa/ in alsa-drivers ages ago already. Is someone still working on that? 
> (no, I don't have the hardware).
> 
> Signed-off-by: Rene Herman <rene.herman@gmail.com>

Got a compiler error.

alsa-driver/isa/msnd/msnd_pinnacle.c: In function ‘snd_msnd_calibrate_adc’:
alsa-driver/isa/msnd/msnd_pinnacle.c:692: error: implicit declaration of function ‘msecs_to_jiffiies’


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 4/4] alsa-driver: schedule_timeout() fixes.

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]

On 09/18/2007 06:44 PM, Takashi Iwai wrote:

>> Speaking about that msnd one -- I believe I've seen that driver sitting in 
>> isa/ in alsa-drivers ages ago already. Is someone still working on that? 
>> (no, I don't have the hardware).
>>
>> Signed-off-by: Rene Herman <rene.herman@gmail.com>
> 
> Got a compiler error.
> 
> alsa-driver/isa/msnd/msnd_pinnacle.c: In function ‘snd_msnd_calibrate_adc’:
> alsa-driver/isa/msnd/msnd_pinnacle.c:692: error: implicit declaration of function ‘msecs_to_jiffiies’

What an amazingly perfect example of the problem with noisy builds ;-/

For me, it's a warning, buried just above a string of other warnings:

   CC [M]  /home/rene/src/alsa/alsa-driver/isa/msnd/msnd.o
   CC [M]  /home/rene/src/alsa/alsa-driver/isa/msnd/msnd_pinnacle.o
/home/rene/src/alsa/alsa-driver/isa/msnd/msnd_pinnacle.c: In function 
'snd_msnd_calibrate_adc':
/home/rene/src/alsa/alsa-driver/isa/msnd/msnd_pinnacle.c:692: warning: 
implicit declaration of function 'msecs_to_jiffiies'
include/asm/io.h: In function 'memcpy_toio':
include/asm/io.h:208: warning: passing argument 1 of '__memcpy' discards 
qualifiers from pointer target type
include/asm/io.h: In function 'memset_io':
include/asm/io.h:200: warning: passing argument 1 of 
'__constant_c_and_count_memset' discards qualifiers from pointer target type
include/asm/io.h:200: warning: passing argument 1 of '__constant_c_memset' 
discards qualifiers from pointer target type
include/asm/io.h:200: warning: passing argument 1 of '__memset_generic' 
discards qualifiers from pointer target type
include/asm/io.h:200: warning: passing argument 1 of '__memset_generic' 
discards qualifiers from pointer target type
   CC [M]  /home/rene/src/alsa/alsa-driver/isa/msnd/msnd_pinnacle_mixer.o
   CC [M]  /home/rene/src/alsa/alsa-driver/isa/msnd/msnd_midi.o
   LD [M]  /home/rene/src/alsa/alsa-driver/isa/msnd/snd-msnd-pinnacle.o
   CC [M]  /home/rene/src/alsa/alsa-driver/isa/opti9xx/miro.o

(and I'm still getting used to this mercurial environment and just building 
everything with ./hgcompile meaning it's a lot of output).

Anyways, s/jiffiies/jiffies/ and sorry for not noticing. Updated patch 
attached. Changelog:

===
alsa-driver: use schedule_timeout_{,un}interruptible.

Replace 3 open-coded implementations of schedule_timout_{,un}interruptible 
and use {u,m}secs_to_jiffies.

Signed-off-by: Rene Herman <rene.herman@gmail.com>
===

Rene.

[-- Attachment #2: schedule_timeout-alsa_driver.diff --]
[-- Type: text/plain, Size: 1673 bytes --]

diff -r 3fefacd5d76c isa/msnd/msnd_pinnacle.c
--- a/isa/msnd/msnd_pinnacle.c	Mon Sep 17 19:04:40 2007 +0200
+++ b/isa/msnd/msnd_pinnacle.c	Tue Sep 18 18:51:39 2007 +0200
@@ -239,10 +239,8 @@ static void dsp_write_flush(void)
 		&dev.writeflush,
 		get_play_delay_jiffies(dev.DAPF.len));*/
 	clear_bit(F_WRITEFLUSH, &dev.flags);
-	if (!signal_pending(current)) {
-		current->state = TASK_INTERRUPTIBLE;
-		schedule_timeout(get_play_delay_jiffies( dev.play_period_bytes));
-	}
+	if (!signal_pending(current)) 
+		schedule_timeout_interruptible(get_play_delay_jiffies(dev.play_period_bytes));
 	clear_bit(F_WRITING, &dev.flags);
 }
 
@@ -691,8 +689,7 @@ static int __init snd_msnd_calibrate_adc
 		       & ~0x0001, dev.SMA + SMA_wCurrHostStatusFlags);
 	if (snd_msnd_send_word(&dev, 0, 0, HDEXAR_CAL_A_TO_D) == 0 &&
 	    snd_msnd_send_dsp_cmd_chk(&dev, HDEX_AUX_REQ) == 0) {
-		current->state = TASK_INTERRUPTIBLE;
-		schedule_timeout(HZ / 3);
+		schedule_timeout_interruptible(msecs_to_jiffies(333));
 		return 0;
 	}
 	printk(KERN_WARNING LOGNAME ": ADC calibration failed\n");
diff -r 3fefacd5d76c pci/asihpi/hpios_linux_kernel.c
--- a/pci/asihpi/hpios_linux_kernel.c	Mon Sep 17 19:04:40 2007 +0200
+++ b/pci/asihpi/hpios_linux_kernel.c	Tue Sep 18 18:51:39 2007 +0200
@@ -38,8 +38,7 @@ and the task receives a signal.
 and the task receives a signal.
 Setting the state to UNINTERRUPTIBLE stops it from returning early.
 */
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		schedule_timeout((HZ * dwNumMicroSec + (HZ - 1)) / 1000000);
+		schedule_timeout_uninterruptible(usecs_to_jiffies(dwNumMicroSec));
 	} else if (dwNumMicroSec <= 2000)
 		udelay(dwNumMicroSec);
 	else

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 4/4] alsa-driver: schedule_timeout() fixes.

[Rene Herman]

On 09/18/2007 07:05 PM, Rene Herman wrote:

> Anyways, s/jiffiies/jiffies/ and sorry for not noticing. Updated patch 
> attached.

(and hope you don't hugely mind the Base64; that's a years old Thunderbird 
bug: if the text-format is UTF8, which it was now due to GCCs stupid use of 
UTF8 tickmarks in its output, Thunderbird automatically uses Base64 for 
attachments for some reason).

Rene.

Re: [PATCH 4/4] alsa-driver: schedule_timeout() fixes.

[Takashi Iwai]

At Tue, 18 Sep 2007 19:09:01 +0200,
Rene Herman wrote:
> 
> On 09/18/2007 07:05 PM, Rene Herman wrote:
> 
> > Anyways, s/jiffiies/jiffies/ and sorry for not noticing. Updated patch 
> > attached.
> 
> (and hope you don't hugely mind the Base64; that's a years old Thunderbird 
> bug: if the text-format is UTF8, which it was now due to GCCs stupid use of 
> UTF8 tickmarks in its output, Thunderbird automatically uses Base64 for 
> attachments for some reason).

It's fine with me.

I applied the new patch.  Thanks!


Takashi

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Trent Piepho]

On Tue, 18 Sep 2007, Krzysztof Helt wrote:
> On Mon, 17 Sep 2007 18:18:27 -0700 (PDT)
> Trent Piepho <xyzzy@speakeasy.org> wrote:
> > for(time = 250; time > 0 ; ) time = schedule_timeout(time);
> >
> > Unless you set the task state first, that line is an infinite busy loop.
>
> This may be not true. If you compare schedule_timeout() with cpu_relax()
> you will see that schedule_timeout() is slower. I am not expert on this,
> but I think that the schedule_timeout() switches tasks and returns
> which is not busy waiting (like userland sleep(0) function does).
> It would make it close to the cpu_relax().

I had tested it and that wasn't the case, but I didn't have more runable tasks
than CPUs while testing.  I think if I had done that, you'd be right and a task
switch might occur.

On the other hand, cpu_relax() is totally different.  It's basically a hint
to the processor to lower the clock speed/power consumption or give
resources to a sibling virtual processor in hyper-threading.  i.e., it's ok
to call cpu_relax() from an interrupt or while atomic.

Might be a good idea in the non-sleeping busy loops, like the one in
snd_ad1848_wait(), but I bet the udelay() already does that.

> The original idea was to make this first waiting longer than loop granularity.
> 7ms before the loop than 1ms granularity inside the loop.
> Rene Herman was against it so he changed it to be the same as 1ms inside
> the loop.

With the delay time being dependent on sample rate, and with different
chips being considerably faster than others, picking a good value for the
first iteration is complicated.  If one could avoid 70 ms of busy looping
in the kernel by doing so, then that might be worth it.  But we don't busy
loop, just poll once per ms (even less when HZ<1000), so it seems that
there is very little to gain here to justify the extra complexity.

> > The 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 is a lot simpler.  The
> > old loop checked for a timeout before checking for ACI down, which could
> > result in an erroneous timeout.  It's only a failure if the timeout expires
> > _and_ ACI is still high.  There is nothing wrong with the timeout expiring
> > while the task is sleeping if ACI went low.
> >
>
> This was already fixed by Rene's patch. You only simplified it.

No, that problem was still there.  If you look at the code patched:
-     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");

If the CPU is interrupted between the spin_unlock_irqrestore() and the next
line, a timeout will be detected, even though ACI may have already gone low.

> > +     } while ((regsel & AD1848_CALIB_IN_PROGRESS) && time_before(jiffies, timeout));
> Break this long line. You may calculate "regsel & AD1848_CALIB_IN_PROGRESS"

Good idea, I've done that.

Re: [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Rene Herman]

On 09/19/2007 03:27 AM, Trent Piepho wrote:

> On the other hand, cpu_relax() is totally different.  It's basically a
> hint to the processor to lower the clock speed/power consumption or give 
> resources to a sibling virtual processor in hyper-threading.  i.e., it's
> ok to call cpu_relax() from an interrupt or while atomic.
> 
> Might be a good idea in the non-sleeping busy loops, like the one in 
> snd_ad1848_wait(), but I bet the udelay() already does that.

It does (when using the TSC).

>> The original idea was to make this first waiting longer than loop
>> granularity. 7ms before the loop than 1ms granularity inside the loop. 
>> Rene Herman was against it so he changed it to be the same as 1ms
>> inside the loop.

Well, at that time, it still was first the 1 and then a full 250 -- or at 
least in my 2.6.22.x schedule_timeout_interruptible() version that actually 
scheduled...

> With the delay time being dependent on sample rate, and with different 
> chips being considerably faster than others, picking a good value for the
> first iteration is complicated.  If one could avoid 70 ms of busy
> looping in the kernel by doing so, then that might be worth it.  But we
> don't busy loop, just poll once per ms (even less when HZ<1000), so it
> seems that there is very little to gain here to justify the extra
> complexity.

Quite -- I plan on being around sound/isa/ for some time and am as such also 
already concerned with maintainability. If there's one thing I do know, it's 
that obviousness is an extremely important factor in that. So if the 
datasheet and hardware say 5 sample periods are enough, so be they...

>> Break this long line. You may calculate "regsel &
>> AD1848_CALIB_IN_PROGRESS"
> 
> Good idea, I've done that.

No goto? :-)

Rene.