[alsa-devel] [PATCH 1/2] ca0106 playback 44100Hz support to SPDIF and playback format & rate constraints

[Takashi Iwai]

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