[alsa-devel] [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load
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.
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;
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
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
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.
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.
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
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.
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.
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
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.
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
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.
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.
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.
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.
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
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 */
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
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));
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
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>
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...)
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
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>
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);
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
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.
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
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);
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.
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
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
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
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
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.
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
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.
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
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_l...
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.
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.
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
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;}- }
- 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
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;}- }
- 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.
participants (4)
-
Krzysztof Helt -
Rene Herman -
Takashi Iwai -
Trent Piepho