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:
> 1) 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
> 1) 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
