New thread for the the final version of the patches for my fixes to the ad1848
auto-calibration loop. I made a couple suggested changes. These should be ok
to apply, and should be independent of the other as yet un-applied patches
that have been posted by Rene and Krzysztof.
I'm not an expert on this chip by any means, but from looking at the code I
think there are few things that could be fixed.
The reg_lock spinlock isn't acquired from the irq handler, as the handler
doesn't use any of the indirect registers. I think this means that is isn't
necessary to use the irq disabling versions of the spin_lock functions with
reg_lock.
It does look like there is a different SMP race condition wrt the irq handler.
>From snd_ad1848_interrupt():
578 if ((chip->mode & AD1848_MODE_PLAY) && chip->playback_substream &&
579 (chip->mode & AD1848_MODE_RUNNING))
580 snd_pcm_period_elapsed(chip->playback_substream);
Another CPU could close the substream between the check and the call to
snd_pcm_period_elapsed(). snd_pcm_period_elapsed() could be called with
either NULL or a stale substream pointer depending on how the code compiled.
I'd think creating an irq spinlock would be an easy way to fix this. The irq
handler would grab it, and the same with the open() and close() functions
around the code that sets the substream pointers. Alternatively, one could
disabled the irq handler during open and close.
I think there might also be problem with setting mixers controls during
auto-calibration . While waiting for auto-calibration to finish, the chip
isn't locked. If another thread tries to set the volume via the mixer
interface, it won't be blocked and will try to talk to the chip during AC.
The datasheet just says to wait for auto-calibration to finish, it doesn't say
what happens if you don't. So maybe there isn't any problem here.
ad1848: Fix msleep while atomic
Simplest fix.
Signed-off-by: Trent Piepho <xyzzy@xxxxxxxxxxxxx>
Acked-by: Rene Herman <rene.herman@xxxxxxxxx>
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);
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@xxxxxxxxxxxxx>
Acked-by: Rene Herman <rene.herman@xxxxxxxxx>
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 Tue Sep 18 19:26:42 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 reg;
spin_lock_irqsave(&chip->reg_lock, flags);
for (timeout = 5; timeout > 0; timeout--)
@@ -222,50 +221,36 @@ 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)
+ reg = inb(AD1848P(chip, REGSEL));
+ outb(chip->mce_bit | (reg & 0x1f), AD1848P(chip, REGSEL));
+ if (reg == 0x80)
snd_printk(KERN_WARNING "mce_down [0x%lx]: serious init problem - codec still busy\n", chip->port);
- if ((timeout & AD1848_MCE) == 0) {
+ if ((reg & 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 could 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);
+ reg = snd_ad1848_in(chip, AD1848_TEST_INIT) &
+ AD1848_CALIB_IN_PROGRESS;
+ } while (reg && time_before(jiffies, timeout));
+ spin_unlock_irqrestore(&chip->reg_lock, flags);
+ if (reg)
+ 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)));
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
