On 09/19/2007 07:22 AM, Trent Piepho wrote:
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.
I'd agree, although I'd suggest to at this point keep anything (in ad1848, cs4231 wants a similar setup in its mce_down) not completely trivial for after 1.0.15.
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.
This would appear to be the case yes.
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.
Also sounds sensible, and the irq spinlock the expected method.
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.
Guess we should be testing this, but generallt, "hand-off" would be the best policy yes.
Rene.