At Tue, 18 Sep 2007 22:22:42 -0700 (PDT), Trent Piepho wrote:
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.
OK, I merged them to HG tree now. Thanks.
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.
Yes, it'd be an easy solution.
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.
Hm, this may happen, too.
Anyway, let's do more cleanups after 1.0.15 release.
Takashi