Current ad1848/cs4231 mce_down code

Current ad1848/cs4231 mce_down code

[Rene Herman]

By the way, while looking at the current mce_down code:

http://hg.alsa-project.org/alsa-kernel/file/0028e39ead78/isa/ad1848/ad1848_lib.c
http://hg.alsa-project.org/alsa-kernel/file/0028e39ead78/isa/cs423x/cs4231_lib.c

we have the ad1848 loops doing:

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 " [ timeout ] ")
		return;
	}
	msleep(1);
	spin_lock_irqsave(&chip->reg_lock, flags);
}

At (---) we are no longer atomic, so imagine being pre-empted there. When we 
get back, we may/will well be past end_time and although by now calibration 
has long finished, we bail out believing we've timed out.

Should it rather be this (probably with a "goto eror" type of setup, but as 
far as the logic is concerned)?

end_time = jiffies + msecs_to_jiffies(250);
while (snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) {
	if (time_after(jiffies, end_time)) {
		spin_unlock_irqrestore(&chip->reg_lock, flags);
		snd_printk(KERN_ERR " [ timeout ] ")
		return;
	}
	spin_unlock_irqrestore(&chip->reg_lock, flags);
	msleep(1);
	spin_lock_irqsave(&chip->reg_lock, flags);
}

Moreover, we aren't under lock at all in the cs4231 version (which also 
means that msleep(1) thing is in fact fine there). Can't we do that for 
ad1848 as well?

(but as said, minimal fix for now is the msleep(1) under lock fix in ad1848)

Rene.

Re: Current ad1848/cs4231 mce_down code

[Krzysztof Helt]

On Tue, 18 Sep 2007 02:53:28 +0200
Rene Herman <rene.herman@gmail.com> wrote:


> At (---) we are no longer atomic, so imagine being pre-empted there. When we 
> get back, we may/will well be past end_time and although by now calibration 
> has long finished, we bail out believing we've timed out.
> 
Good catch.

> 
> Moreover, we aren't under lock at all in the cs4231 version (which also 
> means that msleep(1) thing is in fact fine there). Can't we do that for 
> ad1848 as well?
> 

The lock is right thing to do as Trent found out. The in() call writes
to register. On the SMP setup it may backfire (cs4231 does not have
locks but sparc cs4231 has them).

Regards,
Krzysztof

Re: Current ad1848/cs4231 mce_down code

[Rene Herman]

On 09/18/2007 09:28 AM, Krzysztof Helt wrote:

>> Moreover, we aren't under lock at all in the cs4231 version (which also 
>> means that msleep(1) thing is in fact fine there). Can't we do that for 
>> ad1848 as well?
>>
> 
> The lock is right thing to do as Trent found out. The in() call writes
> to register. On the SMP setup it may backfire (cs4231 does not have
> locks but sparc cs4231 has them).

Yep, cs4231 is broken. Trent's new ad1848 version is the reference now it 
seems. I'll fix up cs4231 if he doesn't beat me to it -- I'm still messing 
with the Aztech cards so am testing cs4231 continously anyway...

Rene.