At Sun, 24 Aug 2008 00:37:04 +1000, Ben Stanley wrote:
Takashi,
I have tried to address all of your concerns regarding formatting, initialisers and printk/snd_printd/snd_printdd. I have a new patch attached addressing these points.
Thanks!
However, you also raised three mutex issues, which I have not yet addressed. I would like some more comments from you before I proceed.
hw_rule_playback_rate:
for (chi = 0; chi < 4; ++chi) {
chp = &(chip->playback_channels[chi]);I'm afraid it's racy. The chip->playback_channels[] can be changed at any time. Try to put a mutex to protect this check.
hw_rule_playback_format:
- for (chi = 0; chi < 4; ++chi) {
chp = &(chip->playback_channels[chi]);This also needs a mutex as well.
snd_ca0106_pcm_prepare_playback:
- for (chi = 0; chi < 4; ++chi) {
chp = &(emu->playback_channels[chi]);Use a mutex in this function, too.
It seems you want to guard against interference to the members of snd_ca0106_channel (and also the associated snd_ca0106_pcm). So far I have detected that the following routines make use of these data structures: hw_rule_playback_rate (new) hw_rule_playback_format (new) snd_ca0106_pcm_open_playback_channel snd_ca0106_pcm_close_playback snd_ca0106_pcm_prepare_playback snd_ca0106_interrupt snd_ca0106_pcm_hw_free_playback I don't yet claim that this list is exhaustive. I'm not sure yet how I should do the mutex. Here is what I'm thinking about:
- introduce a new spinlock_t pcm_lock within snd_ca0106 to cover access
to *all* the pcm data (snd_ca0106_channel *4 + snd_ca0106_pcm * 4) 2) introduce a new spinlock_t pcm_lock within snd_ca0106_channel to cover access to the snd_ca0106_channel *1 + snd_ca0106_pcm *1
Option 2 makes it difficult to make sure that hw_rule_playback_rate and hw_rule_playback_format will work correctly when a channel is opened or closed while the hw_rule_* call is in progress, so that suggests option
- might be better.
Right. The 1 is the right choice in this case.
Introducing the pcm_lock means that all the abovenamed functions will have to hold the lock while doing their work.
(I'm not familiar with mutexes vs spinlocks in the kernel - point me to a documentation url if you want me to switch.)
I forgot about that it's referred in the interrupt handler, too. So it has to be a spinlock, indeed.
But, it seems to me that a race condition remains after all this. The client code can do the hw_rule check on a channel, but another program can prepare another channel using (using parameters which will change the hw_rule result) before the first client gets back to prepare and open its channel. In fact, I should change snd_ca0106_pcm_prepare_playback so that it repeats the hardware check (within a mutex) and returns -EINVAL if it fails. Unless there is another mutex around that whole system to prevent this mess from happening...
It's a known issue that this constraint can't be race-free. The only problem we need to fix right now is not to Oopsen -- to protect the playback_channels[].
thanks,
Takashi