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.

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.

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.)

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...

I presume that I should leave the existing emu_lock alone and leave it to protect the register twiddling code exclusively.

I'll work on the mutex stuff after receiving feedback on the above.

Thanks for being encouraging about my first non-trivial patch here :-)

Ben Stanley.

On Fri, 2008-08-22 at 17:40 +0200, Takashi Iwai wrote:

At Sat, 23 Aug 2008 00:38:41 +1000, Ben Stanley wrote:

...

Dear list,

This patch provides the ability to play back digital audio via SPDIF at a sampling rate of 44100Hz. It also implements rate constraints to ensure that the hardware restrictions of the card are not violated. Note that if 44100Hz is used, then all used channels must use that frequency. For example, opening hw:0,0 at 48000 and then opening hw:0,1 at 44100 will fail due to constraints. However, you may successfully open both hw:0,0 and hw:0,1 at 44100 (presume no other channels open). Further details of the restriction may be found in the thread http://thread.gmane.org/gmane.linux.alsa.devel/52149/focus=52345

This patch also implements playback format restrictions, such that only one playback format may be in use at any time.

I tested driver playback using the attached script (Test_ca0106_driver.sh). I get good audio output for all combinations of sampling rates and formats except for 192kHz S16 (as noted at the end of the script). Sometimes it works and sometimes it doesn't. The reason for the failure is not apparent at this time. It could have something to do with my receiver. I don't know.

The attached script also checks and validates that the rate and format constraints operate as required.

I periodically have trouble with this driver not producing sound output. This has been investigated and the cause is explained here http://thread.gmane.org/gmane.linux.alsa.devel/55384/focus=55410 I intend to submit a new patch to address this in the future.

This patch has been in use for three months on my MythTV system (Ubuntu 8.04). I find it works reliably for me. I also use it with xine, aplay, mplayer, mame, and childsplay.

I have checked with checkpatch.pl . Patch is against v1.0.18rc1.

Signed-off-by: Ben Stanley Ben.Stanley@exemail.com.au

Thanks for the patch!

Through a quick review, I find it almost fine, but slight concerns below.

...

+static int hw_rule_playback_rate(

• struct snd_pcm_hw_params *params,
• struct snd_pcm_hw_rule *rule)

Put arguments right after the open parenthesis. It's more common style.

...

+{

• struct snd_ca0106 *chip = rule->private;
• int chi, any_44100 = 0, any_non_44100 = 0, mask = 0;

No need to initialize here (at least for mask).

...

• struct snd_ca0106_channel *chp = 0;

Ditto.

...

• struct snd_pcm_runtime *runtime;
• if (snd_BUG_ON(!chip))

• return -EINVAL;
  

• if (chip->spdif_enable) {

• 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.

...

• 	if (!chp->use)
  

• 		continue;
  

• 	if (snd_BUG_ON(!chp->epcm))
  

• 		return -EINVAL;
  

• 	if (!chp->epcm->hw_reserved)
  

• 		continue;
  

• 	if (snd_BUG_ON(!chp->epcm->substream))
  

• 		return -EINVAL;
  

• 	if (snd_BUG_ON(!chp->epcm->substream->runtime))
  

• 		return -EINVAL;
  

• 	runtime = chp->epcm->substream->runtime;
  

• 	snd_printd("snd_hw_rule_playback_rate: ch=%d, "
  

• 		"rate=%d.\n", chi, runtime->rate);
  

Use snd_printdd() instead. CONFIG_SND_DEBUG=y on most cases, and this would be too annoying.

...

• 	any_44100 += runtime->rate == 44100;
  

• 	any_non_44100 += runtime->rate != 44100;
  

• }
  

• if (snd_BUG_ON(any_44100 && any_non_44100))
  

• 	return -EINVAL;
  

• if (any_44100)
  

• 	mask = 0x1;
  

• else if (any_non_44100)
  

• 	mask = 0xE;
  

• else
  

• 	mask = 0xF;
  

This mask value is a bit cryptic, but OK...

...

• } else {

• /* 44100Hz is not supported for DAC (FIXME Why?) */
  

• mask = 0xE;
  

• }
• snd_printd("snd_hw_rule_playback_rate: any_44100=%d, "

• "any_non_44100=%d, mask=0x%X, spdif=%d\n",
  

• any_44100, any_non_44100, mask, chip->spdif_enable);
  

Use snd_printdd().

...

• return snd_interval_list(

• hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE),
  

• ARRAY_SIZE(all_spdif_playback_rates),
  

• all_spdif_playback_rates, mask);
  

Put arguments after the open parenthesis.

...

+}

+static int hw_rule_playback_format(

• struct snd_pcm_hw_params *params,
• struct snd_pcm_hw_rule *rule)

Ditto.

...

+{

• struct snd_ca0106 *chip = rule->private;
• int chi, any_S16 = 0, any_S32 = 0;
• struct snd_ca0106_channel *chp = 0;

No need to initialize.

...

• struct snd_pcm_runtime *runtime;
• struct snd_mask fmt, *f = hw_param_mask(

• 			params, SNDRV_PCM_HW_PARAM_FORMAT);
  

• int result;
• if (snd_BUG_ON(!chip))

• return -EINVAL;
  

• snd_mask_none(&fmt);
• for (chi = 0; chi < 4; ++chi) {

• chp = &(chip->playback_channels[chi]);
  

This also needs a mutex as well.

...

• if (!chp->use)
  

• 	continue;
  

• if (snd_BUG_ON(!chp->epcm))
  

• 	return -EINVAL;
  

• if (!chp->epcm->hw_reserved)
  

• 	continue;
  

• if (snd_BUG_ON(!chp->epcm->substream))
  

• 	return -EINVAL;
  

• if (snd_BUG_ON(!chp->epcm->substream->runtime))
  

• 	return -EINVAL;
  

• runtime = chp->epcm->substream->runtime;
  

• snd_printd("snd_hw_rule_playback_format: ch=%d, format=%d.\n",
  

• 	chi, runtime->format);
  

Use snd_printdd().

...

• any_S16 += runtime->format == SNDRV_PCM_FORMAT_S16_LE;
  

• any_S32 += runtime->format == SNDRV_PCM_FORMAT_S32_LE;
  

• }
• if (snd_BUG_ON(any_S16 && any_S32))

• return -EINVAL;
  

• if (any_S16)

• snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE);
  

• else if (any_S32)

• snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE);
  

• else {

• /* No format yet chosen, so both formats are available. */
  

• snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE);
  

• snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE);
  

• }
• result = snd_mask_refine(f, &fmt);
• snd_printd("snd_hw_rule_playback_format: any_S16=%d, any_S32=%d, "

• "refined_fmt=0x%X, avail_fmt=0x%X, changed=%d\n",
  

• any_S16, any_S32, f->bits[0], fmt.bits[0], result);
  

Use snd_printdd().

...

• return result;

+}

unsigned int snd_ca0106_ptr_read(struct snd_ca0106 * emu, unsigned int reg, unsigned int chn) @@ -508,10 +619,24 @@ static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr //printk("open:channel_id=%d, chip=%p, channel=%p\n",channel_id, chip, channel); //channel->interrupt = snd_ca0106_pcm_channel_interrupt; channel->epcm = epcm;

• if ((err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS)) < 0)

• err = snd_pcm_hw_constraint_integer(

• 	runtime, SNDRV_PCM_HW_PARAM_PERIODS);
  

• if (err < 0) return err;

• if ((err = snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 64)) < 0)

• err = snd_pcm_hw_constraint_step(

• 	runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 64);
  

• if (err < 0) return err;

• err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,

• 	hw_rule_playback_rate, (void *)chip,
  

• 	SNDRV_PCM_HW_PARAM_RATE, -1);
  

• if (err < 0)

• return err;
  

• err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT,

•        hw_rule_playback_format, (void *)chip,
  

• 	SNDRV_PCM_HW_PARAM_FORMAT, -1);
  

• if (err < 0)

• return err;
  

  snd_pcm_set_sync(substream);

  if (chip->details->spi_dac && channel_id != PCM_FRONT_CHANNEL) {

@@ -646,6 +771,9 @@ static int snd_ca0106_pcm_hw_params_playback(struct snd_pcm_substream *substream /* hw_free callback */ static int snd_ca0106_pcm_hw_free_playback(struct snd_pcm_substream *substream) {

• struct snd_pcm_runtime *runtime = substream->runtime;
• struct snd_ca0106_pcm *epcm = runtime->private_data;
• epcm->hw_reserved = 0; return snd_pcm_lib_free_pages(substream);

}

@@ -667,53 +795,103 @@ static int snd_ca0106_pcm_hw_free_capture(struct snd_pcm_substream *substream) static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream) { struct snd_ca0106 *emu = snd_pcm_substream_chip(substream);

• struct snd_pcm_runtime *runtime = substream->runtime;

• struct snd_pcm_runtime *runtime = substream->runtime, *runtimei = 0;

No need to initialize.

...

struct snd_ca0106_pcm *epcm = runtime->private_data;

• int channel = epcm->channel_id;

• struct snd_ca0106_channel *chp = 0;

Ditto.

...

• int channel = epcm->channel_id, chi, any_44100 = 0, any_non_44100 = 0; u32 *table_base = (u32 *)(emu->buffer.area+(8*16*channel)); u32 period_size_bytes = frames_to_bytes(runtime, runtime->period_size); u32 hcfg_mask = HCFG_PLAYBACK_S32_LE; u32 hcfg_set = 0x00000000; u32 hcfg;

• u32 reg40_mask = 0x30000 << (channel<<1);

• u32 reg40_mask = 0xFF0000; u32 reg40_set = 0; u32 reg40;

• /* FIXME: Depending on mixer selection of SPDIF out or not, select the spdif rate or the DAC rate. */
• u32 reg71_mask = 0x03030000 ; /* Global. Set SPDIF rate. We only support 44100 to spdif, not to DAC. */

• u32 reg71_mask;
• u32 reg71_shift; u32 reg71_set = 0; u32 reg71; int i;

•    //snd_printk("prepare:channel_number=%d, rate=%d, format=0x%x, channels=%d, buffer_size=%ld, period_size=%ld, periods=%u, frames_to_bytes=%d\n",channel, runtime->rate, runtime->format, runtime->channels, runtime->buffer_size, runtime->period_size, runtime->periods, frames_to_bytes(runtime, 1));
  

•    //snd_printk("dma_addr=%x, dma_area=%p, table_base=%p\n",runtime->dma_addr, runtime->dma_area, table_base);
  

• //snd_printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",emu->buffer.addr, emu->buffer.area, emu->buffer.bytes);
• /* Rate can be set per channel. */
• /* reg40 control host to fifo */
• /* reg71 controls DAC rate. */
• switch (runtime->rate) {
• case 44100:

• reg40_set = 0x10000 << (channel<<1);
  

• reg71_set = 0x01010000; 
  

• break;
  

•    case 48000:
  

• reg40_set = 0;
  

• reg71_set = 0; 
  

• break;
  

• case 96000:

• reg40_set = 0x20000 << (channel<<1);
  

• reg71_set = 0x02020000; 
  

• break;
  

• case 192000:

• reg40_set = 0x30000 << (channel<<1);
  

• reg71_set = 0x03030000; 
  

• break;
  

• default:

• reg40_set = 0;
  

• reg71_set = 0; 
  

• break;
  

• epcm->hw_reserved = 1;
• /* FIXME CLEAN UP IF spdif_enable IS CHANGED WHILE CHANNELS ARE OPENED

• * OR PREVENT THIS FROM HAPPENING. */
  

• if (emu->spdif_enable)

• reg71_shift = 24; /* SPDIF Output Rate */
  

• else

• reg71_shift = 16; /* I2S Output Rate */
  

• reg71_mask = 0x3 << reg71_shift;
• printk(KERN_DEBUG DRVNAME ": prepare_playback: "

• "channel_number=%d, rate=%d, format=0x%x, channels=%d, "
  

• "buffer_size=%ld,period_size=%ld, periods=%u, "
  

• "frames_to_bytes=%d\n",
  

• channel, runtime->rate, runtime->format, runtime->channels,
  

• runtime->buffer_size, runtime->period_size, runtime->periods,
  

• frames_to_bytes(runtime, 1));
  

Use snd_printdd() (if any).

...

• /*printk("dma_addr=%x, dma_area=%p, table_base=%p\n",

• runtime->dma_addr,runtime->dma_area, table_base);*/
  

• /*printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",

• emu->buffer.addr,emu->buffer.area, emu->buffer.bytes);*/
  

• /* We are forced to build the settings for all the channels. */
• for (chi = 0; chi < 4; ++chi) {

• chp = &(emu->playback_channels[chi]);
  

Use a mutex in this function, too.

...

• if (!chp->use)
  

• 	continue;
  

• if (snd_BUG_ON(!chp->epcm))
  

• 	return -EINVAL;
  

• if (chi != channel && !chp->epcm->hw_reserved)
  

• 	continue;
  

• if (snd_BUG_ON(!chp->epcm->substream))
  

• 	return -EINVAL;
  

• if (snd_BUG_ON(!chp->epcm->substream->runtime))
  

• 	return -EINVAL;
  

• runtimei = chp->epcm->substream->runtime;
  

• any_44100 += runtimei->rate == 44100;
  

• any_non_44100 += runtimei->rate != 44100;
  

• /* Rate can be set per channel. */
  

• /* reg40 control host to fifo */
  

• /* reg71 controls DAC rate. */
  

• switch (runtimei->rate) {
  

• case 44100:
  

• 	/* We only support 44100 to spdif, not to DAC.
  

• 	   (FIXME WHY?)*/
  

• 	if (emu->spdif_enable) {
  

• 		/* When using 44100, *all* channels
  

• 		   must be set to that rate. */
  

• 		reg40_set |= 0x550000;
  

• 		reg71_set |= 0x1 << reg71_shift;
  

• 		break;
  

• 	} else {
  

• 		printk(KERN_ERR DRVNAME
  

• 			"prepare_playback: "
  

• 			"44100Hz is invalid for DAC.\n");
  

• 	}
  

• case 48000:
  

• 	/* reg40_set &= !(0x1 << (chi<<1)); */
  

• 	/* reg71_set &= !(0x1 << reg71_shift); */
  

• 	break;
  

• case 96000:
  

• 	reg40_set |= 0x20000 << (chi<<1);
  

• 	reg71_set |= 0x2 << reg71_shift;
  

• 	break;
  

• case 192000:
  

• 	reg40_set |= 0x30000 << (chi<<1);
  

• 	reg71_set |= 0x3 << reg71_shift;
  

• 	break;
  

• default:
  

• 	printk(KERN_ERR DRVNAME
  

• 		": prepare_playback: "
  

• 		"Bad sampling frequency %d.\n",
  

• 		runtimei->rate);
  

• }
  

  }

• /* Format is a global setting */
• /* FIXME: Only let the first channel accessed set this. */

• printk(KERN_DEBUG DRVNAME ": prepare_playback: any_44100=%d, "

• "any_non_44100=%d, spdif=%d.\n",
  

• any_44100, any_non_44100, emu->spdif_enable);
  

Use snd_printdd().

...

• /* Format is a global setting. */
• /* Only the first channel accessed can set this

•   (enforced by constraints). */
  

  switch (runtime->format) { case SNDRV_PCM_FORMAT_S16_LE: hcfg_set = 0;

@@ -1363,8 +1541,8 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card, pci_read_config_dword(pci, PCI_SUBSYSTEM_VENDOR_ID, &chip->serial); pci_read_config_word(pci, PCI_SUBSYSTEM_ID, &chip->model); #if 1

• printk(KERN_INFO "snd-ca0106: Model %04x Rev %08x Serial %08x\n", chip->model,

•       pci->revision, chip->serial);
  

• printk(KERN_INFO DRVNAME ": Model %04x Rev %08x Serial %08x\n",

• chip->model, pci->revision, chip->serial);
  

#endif strcpy(card->driver, "CA0106"); strcpy(card->shortname, "CA0106"); @@ -1378,7 +1556,9 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card, } chip->details = c; if (subsystem[dev]) {

• printk(KERN_INFO "snd-ca0106: Sound card name=%s, subsystem=0x%x. Forced to subsystem=0x%x\n",
  

• printk(KERN_INFO DRVNAME
  

• 	": Sound card name=%s, subsystem=0x%x. "
  

• 	"Forced to subsystem=0x%x\n",
                  c->name, chip->serial, subsystem[dev]);
  

  }

Could you fix these and repost?

Thanks!

Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Takashi,

Further to your previous comments [1], I have attempted to address the locking issues. I have re-designed the code to simplify the locking problems.

This is the patch against alsa-kmirror.

Signed-off-by: Ben Stanley Ben.Stanley@exemail.com.au

[1] http://thread.gmane.org/gmane.linux.alsa.devel/55527/focus=55539

--- pci/ca0106/ca0106.h | 13 ++- pci/ca0106/ca0106_main.c | 367 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 347 insertions(+), 33 deletions(-)

diff --git a/pci/ca0106/ca0106.h b/pci/ca0106/ca0106.h index 74175fc..bf502b5 100644 --- a/pci/ca0106/ca0106.h +++ b/pci/ca0106/ca0106.h @@ -1,7 +1,7 @@ /* * Copyright (c) 2004 James Courtier-Dutton James@superbug.demon.co.uk * Driver CA0106 chips. e.g. Sound Blaster Audigy LS and Live 24bit - * Version: 0.0.22 + * Version: 0.0.23 * * FEATURES currently supported: * See ca0106_main.c for features. @@ -49,6 +49,8 @@ * Implement support for Line-in capture on SB Live 24bit. * 0.0.22 * Add support for mute control on SB Live 24bit (cards w/ SPI DAC) + * 0.0.23 + * Add support for playback sampling rate and format constraints. * * * This code was initally based on code from ALSA's emu10k1x.c which is: @@ -644,6 +646,8 @@

#include "ca_midi.h"

+#define DRVNAME "snd-ca0106" + struct snd_ca0106;

struct snd_ca0106_channel { @@ -659,6 +663,7 @@ struct snd_ca0106_pcm { struct snd_pcm_substream *substream; int channel_id; unsigned short running; + unsigned short hw_reserved; };

struct snd_ca0106_details { @@ -684,6 +689,7 @@ struct snd_ca0106 { unsigned short model; /* subsystem id */

spinlock_t emu_lock; + spinlock_t pcm_lock;

struct snd_ac97 *ac97; struct snd_pcm *pcm; @@ -703,6 +709,11 @@ struct snd_ca0106 { struct snd_ca_midi midi2;

u16 spi_dac_reg[16]; + + unsigned short count_pb_44100_chan; + unsigned short count_pb_non_44100_chan; + unsigned short count_pb_S16_chan; + unsigned short count_pb_S32_chan; };

int snd_ca0106_mixer(struct snd_ca0106 *emu); diff --git a/pci/ca0106/ca0106_main.c b/pci/ca0106/ca0106_main.c index a7d8966..806f34c 100644 --- a/pci/ca0106/ca0106_main.c +++ b/pci/ca0106/ca0106_main.c @@ -1,7 +1,7 @@ /* * Copyright (c) 2004 James Courtier-Dutton James@superbug.demon.co.uk * Driver CA0106 chips. e.g. Sound Blaster Audigy LS and Live 24bit - * Version: 0.0.25 + * Version: 0.0.26 * * FEATURES currently supported: * Front, Rear and Center/LFE. @@ -83,9 +83,14 @@ * Add support for mute control on SB Live 24bit (cards w/ SPI DAC) * 0.0.25 * Powerdown SPI DAC channels when not in use + * 0.0.26 Ben Stanley + * Added support for output at 44100Hz rate (SPDIF only). + * Implemented constraints system for output rate and format. * * BUGS: * Some stability problems when unloading the snd-ca0106 kernel module. + * Some programs fail to produce sound output (tested on SPDIF). See + * http://thread.gmane.org/gmane.linux.alsa.devel/55384/focus=55410 * -- * * TODO: @@ -145,6 +150,7 @@ #include <sound/core.h> #include <sound/initval.h> #include <sound/pcm.h> +#include <sound/pcm_params.h> #include <sound/ac97_codec.h> #include <sound/info.h>

@@ -285,9 +291,9 @@ static struct snd_pcm_hardware snd_ca0106_playback_hw = { SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_SYNC_START, .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE, - .rates = (SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 | - SNDRV_PCM_RATE_192000), - .rate_min = 48000, + .rates = (SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 | + SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_192000), + .rate_min = 44100, .rate_max = 192000, .channels_min = 2, //1, .channels_max = 2, //6, @@ -319,6 +325,93 @@ static struct snd_pcm_hardware snd_ca0106_capture_hw = { .fifo_size = 0, };

+static unsigned int all_spdif_playback_rates[] = + {44100, 48000, 96000, 192000}; + +static int hw_rule_playback_rate(struct snd_pcm_hw_params *params, + struct snd_pcm_hw_rule *rule) +{ + struct snd_ca0106 *chip = rule->private; + int mask; + if (snd_BUG_ON(!chip)) + return -EINVAL; + + spin_lock(&chip->pcm_lock); + if (chip->spdif_enable) { + if (snd_BUG_ON(chip->count_pb_44100_chan && + chip->count_pb_non_44100_chan)) { + spin_unlock(&chip->pcm_lock); + return -EINVAL; + } + if (snd_BUG_ON(chip->count_pb_44100_chan + + chip->count_pb_non_44100_chan > 4)) { + spin_unlock(&chip->pcm_lock); + return -EINVAL; + } + /* Compute the mask applied to all_spdif_playback_rates */ + if (chip->count_pb_44100_chan) + mask = 0x1; + else if (chip->count_pb_non_44100_chan) + mask = 0xE; + else + mask = 0xF; + } else { + /* 44100Hz is not supported for DAC (FIXME Why?) */ + mask = 0xE; + } + snd_printdd("snd_hw_rule_playback_rate: any_44100=%d, " + "any_non_44100=%d, mask=0x%X, spdif=%d\n", + chip->count_pb_44100_chan, + chip->count_pb_non_44100_chan, + mask, chip->spdif_enable); + spin_unlock(&chip->pcm_lock); + return snd_interval_list(hw_param_interval(params, + SNDRV_PCM_HW_PARAM_RATE), + ARRAY_SIZE(all_spdif_playback_rates), + all_spdif_playback_rates, mask); +} + +static int hw_rule_playback_format(struct snd_pcm_hw_params *params, + struct snd_pcm_hw_rule *rule) +{ + struct snd_ca0106 *chip = rule->private; + struct snd_mask fmt, *f = hw_param_mask( + params, SNDRV_PCM_HW_PARAM_FORMAT); + int result; + if (snd_BUG_ON(!chip)) + return -EINVAL; + snd_mask_none(&fmt); + + spin_lock(&chip->pcm_lock); + if (snd_BUG_ON(chip->count_pb_S16_chan && + chip->count_pb_S32_chan)) { + spin_unlock(&chip->pcm_lock); + return -EINVAL; + } + if (snd_BUG_ON(chip->count_pb_S16_chan + + chip->count_pb_S32_chan > 4)) { + spin_unlock(&chip->pcm_lock); + return -EINVAL; + } + if (chip->count_pb_S16_chan) + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE); + else if (chip->count_pb_S32_chan) + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE); + else { + /* No format yet chosen, so both formats are available. */ + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE); + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE); + } + result = snd_mask_refine(f, &fmt); + snd_printdd("snd_hw_rule_playback_format: any_S16=%d, any_S32=%d, " + "refined_fmt=0x%X, avail_fmt=0x%X, changed=%d\n", + chip->count_pb_S16_chan, + chip->count_pb_S32_chan, f->bits[0], fmt.bits[0], + result); + spin_unlock(&chip->pcm_lock); + return result; +} + unsigned int snd_ca0106_ptr_read(struct snd_ca0106 * emu, unsigned int reg, unsigned int chn) @@ -509,10 +602,24 @@ static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr //printk("open:channel_id=%d, chip=%p, channel=%p\n",channel_id, chip, channel); //channel->interrupt = snd_ca0106_pcm_channel_interrupt; channel->epcm = epcm; - if ((err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS)) < 0) + err = snd_pcm_hw_constraint_integer( + runtime, SNDRV_PCM_HW_PARAM_PERIODS); + if (err < 0) return err; - if ((err = snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 64)) < 0) + err = snd_pcm_hw_constraint_step( + runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 64); + if (err < 0) return err; + err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, + hw_rule_playback_rate, (void *)chip, + SNDRV_PCM_HW_PARAM_RATE, -1); + if (err < 0) + return err; + err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT, + hw_rule_playback_format, (void *)chip, + SNDRV_PCM_HW_PARAM_FORMAT, -1); + if (err < 0) + return err; snd_pcm_set_sync(substream);

if (chip->details->spi_dac && channel_id != PCM_FRONT_CHANNEL) { @@ -647,6 +754,55 @@ static int snd_ca0106_pcm_hw_params_playback(struct snd_pcm_substream *substream /* hw_free callback */ static int snd_ca0106_pcm_hw_free_playback(struct snd_pcm_substream *substream) { + struct snd_ca0106 *chip = snd_pcm_substream_chip(substream); + struct snd_pcm_runtime *runtime = substream->runtime, *runtimei; + struct snd_ca0106_pcm *epcm = runtime->private_data; + struct snd_ca0106_channel *pchannel; + int channel = epcm->channel_id, chi; + + spin_lock(&chip->pcm_lock); + epcm->hw_reserved = 0; + chip->count_pb_44100_chan = chip->count_pb_non_44100_chan = 0; + chip->count_pb_S16_chan = chip->count_pb_S32_chan = 0; + for (chi = 0; chi < 4; ++chi) { + if (chi == channel) + continue; + pchannel = &(chip->playback_channels[chi]); + if (!pchannel->use) + continue; + if (snd_BUG_ON(!pchannel->epcm)) { + spin_unlock(&chip->pcm_lock); + return -EINVAL; + } + if (!pchannel->epcm->hw_reserved) + continue; + if (snd_BUG_ON(!pchannel->epcm->substream)) { + spin_unlock(&chip->pcm_lock); + return -EINVAL; + } + if (snd_BUG_ON(!pchannel->epcm->substream->runtime)) { + spin_unlock(&chip->pcm_lock); + return -EINVAL; + } + runtimei = pchannel->epcm->substream->runtime; + chip->count_pb_44100_chan += runtimei->rate == 44100; + chip->count_pb_non_44100_chan += runtimei->rate != 44100; + chip->count_pb_S16_chan += + runtimei->format == SNDRV_PCM_FORMAT_S16_LE; + chip->count_pb_S32_chan += + runtimei->format == SNDRV_PCM_FORMAT_S32_LE; + } + snd_BUG_ON(chip->count_pb_44100_chan && chip->count_pb_non_44100_chan); + snd_BUG_ON(chip->count_pb_44100_chan + + chip->count_pb_non_44100_chan > 4); + snd_BUG_ON(chip->count_pb_S16_chan && chip->count_pb_S32_chan); + snd_BUG_ON(chip->count_pb_S16_chan + chip->count_pb_S32_chan > 4); + snd_printdd(KERN_DEBUG DRVNAME ": free_playback: any_44100=%d, " + "any_non_44100=%d, spdif=%d.\n", + chip->count_pb_44100_chan, chip->count_pb_non_44100_chan, + emu->spdif_enable); + spin_unlock(&chip->pcm_lock); + return snd_pcm_lib_free_pages(substream); }

@@ -668,53 +824,196 @@ static int snd_ca0106_pcm_hw_free_capture(struct snd_pcm_substream *substream) static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream) { struct snd_ca0106 *emu = snd_pcm_substream_chip(substream); - struct snd_pcm_runtime *runtime = substream->runtime; + struct snd_pcm_runtime *runtime = substream->runtime, *runtimei; struct snd_ca0106_pcm *epcm = runtime->private_data; - int channel = epcm->channel_id; + struct snd_ca0106_channel *pchannel; + int channel = epcm->channel_id, chi; + unsigned short count_pb_44100_chan, count_pb_non_44100_chan; + unsigned short count_pb_S16_chan, count_pb_S32_chan; u32 *table_base = (u32 *)(emu->buffer.area+(8*16*channel)); u32 period_size_bytes = frames_to_bytes(runtime, runtime->period_size); u32 hcfg_mask = HCFG_PLAYBACK_S32_LE; u32 hcfg_set = 0x00000000; u32 hcfg; - u32 reg40_mask = 0x30000 << (channel<<1); + u32 reg40_mask = 0xFF0000; u32 reg40_set = 0; u32 reg40; - /* FIXME: Depending on mixer selection of SPDIF out or not, select the spdif rate or the DAC rate. */ - u32 reg71_mask = 0x03030000 ; /* Global. Set SPDIF rate. We only support 44100 to spdif, not to DAC. */ + u32 reg71_mask; + u32 reg71_shift; u32 reg71_set = 0; u32 reg71; int i; - //snd_printk("prepare:channel_number=%d, rate=%d, format=0x%x, channels=%d, buffer_size=%ld, period_size=%ld, periods=%u, frames_to_bytes=%d\n",channel, runtime->rate, runtime->format, runtime->channels, runtime->buffer_size, runtime->period_size, runtime->periods, frames_to_bytes(runtime, 1)); - //snd_printk("dma_addr=%x, dma_area=%p, table_base=%p\n",runtime->dma_addr, runtime->dma_area, table_base); - //snd_printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",emu->buffer.addr, emu->buffer.area, emu->buffer.bytes); + /* FIXME CLEAN UP IF spdif_enable IS CHANGED WHILE CHANNELS ARE OPENED + * OR PREVENT THIS FROM HAPPENING. */ + if (emu->spdif_enable) + reg71_shift = 24; /* SPDIF Output Rate */ + else + reg71_shift = 16; /* I2S Output Rate */ + reg71_mask = 0x3 << reg71_shift; + + /*printk(KERN_DEBUG DRVNAME ": prepare_playback: " + "channel_number=%d, rate=%d, format=0x%x, channels=%d, " + "buffer_size=%ld,period_size=%ld, periods=%u, " + "frames_to_bytes=%d\n", + channel, runtime->rate, runtime->format, runtime->channels, + runtime->buffer_size, runtime->period_size, runtime->periods, + frames_to_bytes(runtime, 1));*/ + /*printk("dma_addr=%x, dma_area=%p, table_base=%p\n", + runtime->dma_addr,runtime->dma_area, table_base);*/ + /*printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n", + emu->buffer.addr,emu->buffer.area, emu->buffer.bytes);*/ + /* We are forced to build the settings for all the channels. */ + spin_lock(&emu->pcm_lock); + emu->count_pb_44100_chan = emu->count_pb_non_44100_chan = 0; + emu->count_pb_S16_chan = emu->count_pb_S32_chan = 0; + for (chi = 0; chi < 4; ++chi) { + if (chi == channel) + continue; + pchannel = &(emu->playback_channels[chi]); + if (!pchannel->use) + continue; + if (snd_BUG_ON(!pchannel->epcm)) { + spin_unlock(&emu->pcm_lock); + return -EINVAL; + } + if (!pchannel->epcm->hw_reserved) + continue; + if (snd_BUG_ON(!pchannel->epcm->substream)) { + spin_unlock(&emu->pcm_lock); + return -EINVAL; + } + if (snd_BUG_ON(!pchannel->epcm->substream->runtime)) { + spin_unlock(&emu->pcm_lock); + return -EINVAL; + } + runtimei = pchannel->epcm->substream->runtime; + emu->count_pb_44100_chan += runtimei->rate == 44100; + emu->count_pb_non_44100_chan += runtimei->rate != 44100; + emu->count_pb_S16_chan += + runtimei->format == SNDRV_PCM_FORMAT_S16_LE; + emu->count_pb_S32_chan += + runtimei->format == SNDRV_PCM_FORMAT_S32_LE; + /* Rate can be set per channel. */ + /* reg40 control host to fifo */ + /* reg71 controls DAC rate. */ + switch (runtimei->rate) { + case 44100: + /* We only support 44100 to spdif, not to DAC. + (FIXME WHY?)*/ + if (emu->spdif_enable) { + /* When using 44100, *all* channels + must be set to that rate. */ + reg40_set |= 0x550000; + reg71_set |= 0x1 << reg71_shift; + break; + } else { + printk(KERN_ERR DRVNAME + "prepare_playback: " + "44100Hz is invalid for DAC.\n"); + } + case 48000: + /* reg40_set &= !(0x1 << (chi<<1)); */ + /* reg71_set &= !(0x1 << reg71_shift); */ + break; + case 96000: + reg40_set |= 0x20000 << (chi<<1); + reg71_set |= 0x2 << reg71_shift; + break; + case 192000: + reg40_set |= 0x30000 << (chi<<1); + reg71_set |= 0x3 << reg71_shift; + break; + default: + printk(KERN_ERR DRVNAME + ": prepare_playback: " + "Bad sampling frequency %d.\n", + runtimei->rate); + } + } + snd_BUG_ON(emu->count_pb_44100_chan && emu->count_pb_non_44100_chan); + snd_BUG_ON(emu->count_pb_44100_chan + + emu->count_pb_non_44100_chan > 4); + snd_BUG_ON(emu->count_pb_S16_chan && emu->count_pb_S32_chan); + snd_BUG_ON(emu->count_pb_S16_chan + emu->count_pb_S32_chan > 4); + + count_pb_44100_chan = + emu->count_pb_44100_chan + runtime->rate == 44100; + count_pb_non_44100_chan = + emu->count_pb_non_44100_chan + runtime->rate != 44100; + count_pb_S16_chan = emu->count_pb_S16_chan + + runtime->format == SNDRV_PCM_FORMAT_S16_LE; + count_pb_S32_chan = emu->count_pb_S32_chan + + runtime->format == SNDRV_PCM_FORMAT_S32_LE; + snd_BUG_ON(count_pb_44100_chan + count_pb_non_44100_chan > 4); + snd_BUG_ON(count_pb_S16_chan + count_pb_S32_chan > 4); + /* Rate can be set per channel. */ /* reg40 control host to fifo */ /* reg71 controls DAC rate. */ switch (runtime->rate) { case 44100: - reg40_set = 0x10000 << (channel<<1); - reg71_set = 0x01010000; - break; - case 48000: - reg40_set = 0; - reg71_set = 0; + /* We only support 44100 to spdif, not to DAC. + (FIXME WHY?)*/ + if (emu->spdif_enable) { + /* When using 44100, *all* channels + must be set to that rate. */ + reg40_set |= 0x550000; + reg71_set |= 0x1 << reg71_shift; + break; + } else { + printk(KERN_ERR DRVNAME + "prepare_playback: " + "44100Hz is invalid for DAC.\n"); + } + case 48000: + /* reg40_set &= !(0x1 << (channel<<1)); */ + /* reg71_set &= !(0x1 << reg71_shift); */ break; case 96000: - reg40_set = 0x20000 << (channel<<1); - reg71_set = 0x02020000; + reg40_set |= 0x20000 << (channel<<1); + reg71_set |= 0x2 << reg71_shift; break; case 192000: - reg40_set = 0x30000 << (channel<<1); - reg71_set = 0x03030000; + reg40_set |= 0x30000 << (channel<<1); + reg71_set |= 0x3 << reg71_shift; break; default: - reg40_set = 0; - reg71_set = 0; - break; + spin_unlock(&emu->pcm_lock); + printk(KERN_ERR DRVNAME + ": prepare_playback: Bad sampling frequency %dHz.\n", + runtime->rate); + return -EINVAL; } - /* Format is a global setting */ - /* FIXME: Only let the first channel accessed set this. */ + if (count_pb_44100_chan && count_pb_non_44100_chan) { + spin_unlock(&emu->pcm_lock); + printk(KERN_ERR DRVNAME + "prepare_playback: requested sampling rate %dHz" + " conflicts with other selected sampling rates.\n", + runtime->rate); + return -EINVAL; + } + if (count_pb_S16_chan && count_pb_S32_chan) { + spin_unlock(&emu->pcm_lock); + printk(KERN_ERR DRVNAME + "prepare_playback: requested sample format %d" + " conflicts with other selected sample formats.\n", + runtime->format); + return -EINVAL; + } + emu->count_pb_44100_chan = count_pb_44100_chan; + emu->count_pb_non_44100_chan = count_pb_non_44100_chan; + emu->count_pb_S16_chan = count_pb_S16_chan; + emu->count_pb_S32_chan = count_pb_S32_chan; + epcm->hw_reserved = 1; + + snd_printdd(KERN_DEBUG DRVNAME ": prepare_playback: any_44100=%d, " + "any_non_44100=%d, spdif=%d.\n", + emu->count_pb_44100_chan, emu->count_pb_non_44100_chan, + emu->spdif_enable); + /* Format is a global setting. */ + /* Only the first channel accessed can set this + (enforced by constraints). */ switch (runtime->format) { case SNDRV_PCM_FORMAT_S16_LE: hcfg_set = 0; @@ -726,6 +1025,7 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream) hcfg_set = 0; break; } + spin_unlock(&emu->pcm_lock); hcfg = inl(emu->port + HCFG) ; hcfg = (hcfg & ~hcfg_mask) | hcfg_set; outl(hcfg, emu->port + HCFG); @@ -1336,6 +1636,7 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card, chip->irq = -1;

spin_lock_init(&chip->emu_lock); + spin_lock_init(&chip->pcm_lock);

chip->port = pci_resource_start(pci, 0); if ((chip->res_port = request_region(chip->port, 0x20, @@ -1364,8 +1665,8 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card, pci_read_config_dword(pci, PCI_SUBSYSTEM_VENDOR_ID, &chip->serial); pci_read_config_word(pci, PCI_SUBSYSTEM_ID, &chip->model); #if 1 - printk(KERN_INFO "snd-ca0106: Model %04x Rev %08x Serial %08x\n", chip->model, - pci->revision, chip->serial); + printk(KERN_INFO DRVNAME ": Model %04x Rev %08x Serial %08x\n", + chip->model, pci->revision, chip->serial); #endif strcpy(card->driver, "CA0106"); strcpy(card->shortname, "CA0106"); @@ -1379,7 +1680,9 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card, } chip->details = c; if (subsystem[dev]) { - printk(KERN_INFO "snd-ca0106: Sound card name=%s, subsystem=0x%x. Forced to subsystem=0x%x\n", + printk(KERN_INFO DRVNAME + ": Sound card name=%s, subsystem=0x%x. " + "Forced to subsystem=0x%x\n", c->name, chip->serial, subsystem[dev]); }

Takashi,

Thankyou for your comments.

My modifications would be much simpler if I didn't have to re-build the counts of how many channels are in each mode every time the prepare and hw_free functions are called.

In the case of the prepare function, I could modify the existing counts if I knew what the previous state (rate, format) was before the most recent change.

In the case of the hw_free function, I think that the rate and format information is invalid for the current channel by the time the hw_free function is called.

If I knew these pieces of information, I could completely avoid re-building the counts.

Regarding the locking, initially I only tried to protect my count fields. I have now tried to work on the locking some more (see incremental untested patch at end). Each time I learn some more... thanks for your patience. Perhaps there is another driver that already has to do this level of locking that you could point me to look at. The other drivers I have looked at so far don't have to do this much locking. (I gather that means they are designed to avoid the need to lock?)

Regarding atomicity, the documentation states that snd_ca0106_pcm_trigger_capture, snd_ca0106_pcm_pointer_playback, snd_ca0106_pcm_pointer_capture are called with a spinlock held by the middle layer. What lock is that? Do I need to hold my spinlock in addition? Will that possibly cause deadlock if the spinlocks are grabbed in different orders? So many questions...

Regarding snd_BUG_ON, I have used it like an assert, intending that it should be turned off in normal production code. Is that not the case?

Further comments interleaved below.

On Wed, 2008-09-03 at 11:49 +0200, Takashi Iwai wrote:

At Wed, 03 Sep 2008 03:11:07 +1000, Ben Stanley wrote:

...

Takashi,

Further to your previous comments [1], I have attempted to address the locking issues. I have re-designed the code to simplify the locking problems.

This is the patch against alsa-kmirror.

Signed-off-by: Ben Stanley Ben.Stanley@exemail.com.au

Thanks for the patch!

...

[1] http://thread.gmane.org/gmane.linux.alsa.devel/55527/focus=55539

---

pci/ca0106/ca0106.h | 13 ++- pci/ca0106/ca0106_main.c | 367 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 347 insertions(+), 33 deletions(-)

diff --git a/pci/ca0106/ca0106.h b/pci/ca0106/ca0106.h index 74175fc..bf502b5 100644 --- a/pci/ca0106/ca0106.h +++ b/pci/ca0106/ca0106.h @@ -1,7 +1,7 @@ /*

• Copyright (c) 2004 James Courtier-Dutton James@superbug.demon.co.uk
• Driver CA0106 chips. e.g. Sound Blaster Audigy LS and Live 24bit

• • Version: 0.0.22

• • Version: 0.0.23
  • FEATURES currently supported:
  • See ca0106_main.c for features.

@@ -49,6 +49,8 @@

• Implement support for Line-in capture on SB Live 24bit.
• 0.0.22
• Add support for mute control on SB Live 24bit (cards w/ SPI DAC)

• • 0.0.23
• • Add support for playback sampling rate and format constraints.
  • This code was initally based on code from ALSA's emu10k1x.c which is:

@@ -644,6 +646,8 @@

#include "ca_midi.h"

+#define DRVNAME "snd-ca0106"

struct snd_ca0106;

struct snd_ca0106_channel { @@ -659,6 +663,7 @@ struct snd_ca0106_pcm { struct snd_pcm_substream *substream; int channel_id; unsigned short running;

• unsigned short hw_reserved;

};

struct snd_ca0106_details { @@ -684,6 +689,7 @@ struct snd_ca0106 { unsigned short model; /* subsystem id */

spinlock_t emu_lock;

• spinlock_t pcm_lock;

The spinlock is needed if the lock is accessed in the IRQ handler. For your patch, use mutex instead.

However, the problem is that the lock is missing in some important places, namely, *_open(), *_close() and snd_ca0106_interrupt(). If it's used in snd_ca0106_interrupt(), it has to be spinlock indeed.

I tried to address this some more in the incremental patch at the end of this email.

...

struct snd_ac97 *ac97; struct snd_pcm *pcm; @@ -703,6 +709,11 @@ struct snd_ca0106 { struct snd_ca_midi midi2;

u16 spi_dac_reg[16];

• unsigned short count_pb_44100_chan;
• unsigned short count_pb_non_44100_chan;
• unsigned short count_pb_S16_chan;
• unsigned short count_pb_S32_chan;

Any reason to use short instead of char?

I could change that.

...

+static unsigned int all_spdif_playback_rates[] =

• {44100, 48000, 96000, 192000};

+static int hw_rule_playback_rate(struct snd_pcm_hw_params *params,

• 		 struct snd_pcm_hw_rule   *rule)
  

+{

• struct snd_ca0106 *chip = rule->private;
• int mask;
• if (snd_BUG_ON(!chip))

• return -EINVAL;
  

• spin_lock(&chip->pcm_lock);
• if (chip->spdif_enable) {

• if (snd_BUG_ON(chip->count_pb_44100_chan &&
  

• 	 chip->count_pb_non_44100_chan)) {
  

• 	spin_unlock(&chip->pcm_lock);
  

• 	return -EINVAL;
  

• }
  

Don't use too much snd_BUG_ON(). This would be better to be a normal check since we have anyway races in parameter constraints, and this may happen indeed.

This is just asserting the invariants, and it did find some problems during development. This condition should not be triggered by the races in parameter constraints, as I took special care to deal with that problem in the prepare function. I would consider removal before promotion to an 'error' check.

...

• if (snd_BUG_ON(chip->count_pb_44100_chan +
  

• 	 chip->count_pb_non_44100_chan > 4)) {
  

• 	spin_unlock(&chip->pcm_lock);
  

• 	return -EINVAL;
  

• }
  

This check can be dropped. Whether it's more than 4 doesn't affect the behavior of this function.

Asserting invariants, same comments as above. If the total count of channels in the 44100 and non-44100 sets is above the number of channels on the card, there is a problem. However, this cannot happen with the current design of the patch, so I am happy to remove it.

...

• /* Compute the mask applied to all_spdif_playback_rates */
  

• if (chip->count_pb_44100_chan)
  

• 	mask = 0x1;
  

• else if (chip->count_pb_non_44100_chan)
  

• 	mask = 0xE;
  

• else
  

• 	mask = 0xF;
  

• } else {

• /* 44100Hz is not supported for DAC (FIXME Why?) */
  

Remove FIXME Why, as James suggested.

Good, I'm glad James responded to clear that up :-)

...

+static int hw_rule_playback_format(struct snd_pcm_hw_params *params,

• 		   struct snd_pcm_hw_rule *rule)
  

+{

• struct snd_ca0106 *chip = rule->private;
• struct snd_mask fmt, *f = hw_param_mask(

• 			params, SNDRV_PCM_HW_PARAM_FORMAT);
  

Better to put f = hw_params_mask() outside so that you don't need ugly line break here.

Can do.

...

• int result;
• if (snd_BUG_ON(!chip))

• return -EINVAL;
  

• snd_mask_none(&fmt);
• spin_lock(&chip->pcm_lock);
• if (snd_BUG_ON(chip->count_pb_S16_chan &&

•  chip->count_pb_S32_chan)) {
  

• spin_unlock(&chip->pcm_lock);
  

• return -EINVAL;
  

• }

Remove snd_BUG_ON().

...

• if (snd_BUG_ON(chip->count_pb_S16_chan +

•  chip->count_pb_S32_chan > 4)) {
  

• spin_unlock(&chip->pcm_lock);
  

• return -EINVAL;
  

• }

This check is needless.

During development these checks were useful. Now they can be removed.

...

@@ -509,10 +602,24 @@ static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr //printk("open:channel_id=%d, chip=%p, channel=%p\n",channel_id, chip, channel); //channel->interrupt = snd_ca0106_pcm_channel_interrupt; channel->epcm = epcm;

• if ((err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS)) < 0)

• err = snd_pcm_hw_constraint_integer(

• 	runtime, SNDRV_PCM_HW_PARAM_PERIODS);
  

Usually runtime is in the same line of 'snd_pcm_hw_constraint_integer('

Can change.

...

• if (err < 0) return err;

• if ((err = snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 64)) < 0)

• err = snd_pcm_hw_constraint_step(

• 	runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 64);
  

Ditto.

Can change.

...

@@ -647,6 +754,55 @@ static int snd_ca0106_pcm_hw_params_playback(struct snd_pcm_substream *substream /* hw_free callback */ static int snd_ca0106_pcm_hw_free_playback(struct snd_pcm_substream *substream) {

• struct snd_ca0106 *chip = snd_pcm_substream_chip(substream);
• struct snd_pcm_runtime *runtime = substream->runtime, *runtimei;
• struct snd_ca0106_pcm *epcm = runtime->private_data;
• struct snd_ca0106_channel *pchannel;
• int channel = epcm->channel_id, chi;
• spin_lock(&chip->pcm_lock);
• epcm->hw_reserved = 0;
• chip->count_pb_44100_chan = chip->count_pb_non_44100_chan = 0;
• chip->count_pb_S16_chan = chip->count_pb_S32_chan = 0;
• for (chi = 0; chi < 4; ++chi) {

• if (chi == channel)
  

• 	continue;
  

• pchannel = &(chip->playback_channels[chi]);
  

• if (!pchannel->use)
  

• 	continue;
  

• if (snd_BUG_ON(!pchannel->epcm)) {
  

• 	spin_unlock(&chip->pcm_lock);
  

• 	return -EINVAL;
  

• }
  

snd_BUG_ON() is wrong here. Note that pchannel->epcm isn't protected at all by chip->pcm_lock in your patch because it's not used in *_open() and *_close() callbacks. Use the lock to protect the assignment of these.

Hmmm... The open callback allocates a new snd_ca0106_pcm by kmalloc, but the close callback does not kfree it. Is that a leak? Oh, I see that it is kfreed by snd_ca0106_pcm_free_substream.

I can't find any other part of the driver that writes to the epcm field. Furthermore, I observe that the use field is set to 1 before epcm field is assigned. So perhaps the lock should cover this case.

...

• if (!pchannel->epcm->hw_reserved)
  

• 	continue;
  

• if (snd_BUG_ON(!pchannel->epcm->substream)) {
  

• 	spin_unlock(&chip->pcm_lock);
  

• 	return -EINVAL;
  

• }
  

• if (snd_BUG_ON(!pchannel->epcm->substream->runtime)) {
  

• 	spin_unlock(&chip->pcm_lock);
  

• 	return -EINVAL;
  

• }
  

Ditto.

Yeah, I see you don't like my defensive programming. I'll work on understanding these better so that I can be confident of removing the checks.

...

• runtimei = pchannel->epcm->substream->runtime;
  

• chip->count_pb_44100_chan += runtimei->rate == 44100;
  

• chip->count_pb_non_44100_chan += runtimei->rate != 44100;
  

• chip->count_pb_S16_chan +=
  

• 	runtimei->format == SNDRV_PCM_FORMAT_S16_LE;
  

• chip->count_pb_S32_chan +=
  

• 	runtimei->format == SNDRV_PCM_FORMAT_S32_LE;
  

• }
• snd_BUG_ON(chip->count_pb_44100_chan && chip->count_pb_non_44100_chan);
• snd_BUG_ON(chip->count_pb_44100_chan +

• chip->count_pb_non_44100_chan > 4);
  

• snd_BUG_ON(chip->count_pb_S16_chan && chip->count_pb_S32_chan);
• snd_BUG_ON(chip->count_pb_S16_chan + chip->count_pb_S32_chan > 4);

snd_BUG_ON() here is rather useless.

Can remove. Was useful during development.

...

@@ -668,53 +824,196 @@ static int snd_ca0106_pcm_hw_free_capture(struct snd_pcm_substream *substream) static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream)

(snip)

...

• /* We are forced to build the settings for all the channels. */
• spin_lock(&emu->pcm_lock);
• emu->count_pb_44100_chan = emu->count_pb_non_44100_chan = 0;
• emu->count_pb_S16_chan = emu->count_pb_S32_chan = 0;
• for (chi = 0; chi < 4; ++chi) {

• if (chi == channel)
  

• 	continue;
  

• pchannel = &(emu->playback_channels[chi]);
  

• if (!pchannel->use)
  

• 	continue;
  

• if (snd_BUG_ON(!pchannel->epcm)) {
  

• 	spin_unlock(&emu->pcm_lock);
  

• 	return -EINVAL;
  

• }
  

• if (!pchannel->epcm->hw_reserved)
  

• 	continue;
  

• if (snd_BUG_ON(!pchannel->epcm->substream)) {
  

• 	spin_unlock(&emu->pcm_lock);
  

• 	return -EINVAL;
  

• }
  

• if (snd_BUG_ON(!pchannel->epcm->substream->runtime)) {
  

• 	spin_unlock(&emu->pcm_lock);
  

• 	return -EINVAL;
  

• }
  

• runtimei = pchannel->epcm->substream->runtime;
  

• emu->count_pb_44100_chan += runtimei->rate == 44100;
  

• emu->count_pb_non_44100_chan += runtimei->rate != 44100;
  

• emu->count_pb_S16_chan +=
  

• 	runtimei->format == SNDRV_PCM_FORMAT_S16_LE;
  

• emu->count_pb_S32_chan +=
  

• 	runtimei->format == SNDRV_PCM_FORMAT_S32_LE;
  

This is as same as in *_hw_free(). Better to split this as another function and call it from both appropriately.

Yes, I thought about that after my late night of hacking...

Even better would be to figure out how to avoid re-building the counts, as discussed at the top. But, this works and seems to be robust against my nasty test script.

It is not quite the same between hw_free and prepare, as the prepare function takes special care to check whether what is requested fulfils the constraints. If it satisfies the constraints, it is configured. The check and the configuration are performed atomically.

So it looks very similar between the two, but is subtly different. Perhaps I can code both versions in one function.

Incremental untested patch trying to address the locking is shown below. I'm sure you'll find things you don't like :-). Please ignore the other issues outlined above for the moment - I will work on addressing your other comments another night. Please review the locking.

Ben Stanley.

From c1c72cf2584c493080ddc04f9b9c36c38bb1b2bd Mon Sep 17 00:00:00 2001

From: Ben Stanley Ben.Stanley@exemail.com.au Date: Thu, 4 Sep 2008 01:00:26 +1000 Subject: [PATCH] Change locking to protect more stuff in accordance with Takashi's comments.

--- pci/ca0106/ca0106_main.c | 91 +++++++++++++++++++++++++++++++-------------- pci/ca0106/ca0106_mixer.c | 3 + 2 files changed, 66 insertions(+), 28 deletions(-)

diff --git a/pci/ca0106/ca0106_main.c b/pci/ca0106/ca0106_main.c index 806f34c..794de68 100644 --- a/pci/ca0106/ca0106_main.c +++ b/pci/ca0106/ca0106_main.c @@ -333,19 +333,20 @@ static int hw_rule_playback_rate(struct snd_pcm_hw_params *params, { struct snd_ca0106 *chip = rule->private; int mask; + unsigned long flags; if (snd_BUG_ON(!chip)) return -EINVAL;

- spin_lock(&chip->pcm_lock); + spin_lock_irqsave(&chip->pcm_lock, flags); if (chip->spdif_enable) { if (snd_BUG_ON(chip->count_pb_44100_chan && chip->count_pb_non_44100_chan)) { - spin_unlock(&chip->pcm_lock); + spin_unlock_irqrestore(&chip->pcm_lock, flags); return -EINVAL; } if (snd_BUG_ON(chip->count_pb_44100_chan + chip->count_pb_non_44100_chan > 4)) { - spin_unlock(&chip->pcm_lock); + spin_unlock_irqrestore(&chip->pcm_lock, flags); return -EINVAL; } /* Compute the mask applied to all_spdif_playback_rates */ @@ -364,7 +365,7 @@ static int hw_rule_playback_rate(struct snd_pcm_hw_params *params, chip->count_pb_44100_chan, chip->count_pb_non_44100_chan, mask, chip->spdif_enable); - spin_unlock(&chip->pcm_lock); + spin_unlock_irqrestore(&chip->pcm_lock, flags); return snd_interval_list(hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE), ARRAY_SIZE(all_spdif_playback_rates), @@ -377,20 +378,21 @@ static int hw_rule_playback_format(struct snd_pcm_hw_params *params, struct snd_ca0106 *chip = rule->private; struct snd_mask fmt, *f = hw_param_mask( params, SNDRV_PCM_HW_PARAM_FORMAT); + unsigned long flags; int result; if (snd_BUG_ON(!chip)) return -EINVAL; snd_mask_none(&fmt);

- spin_lock(&chip->pcm_lock); + spin_lock_irqsave(&chip->pcm_lock, flags); if (snd_BUG_ON(chip->count_pb_S16_chan && chip->count_pb_S32_chan)) { - spin_unlock(&chip->pcm_lock); + spin_unlock_irqrestore(&chip->pcm_lock, flags); return -EINVAL; } if (snd_BUG_ON(chip->count_pb_S16_chan + chip->count_pb_S32_chan > 4)) { - spin_unlock(&chip->pcm_lock); + spin_unlock_irqrestore(&chip->pcm_lock, flags); return -EINVAL; } if (chip->count_pb_S16_chan) @@ -408,7 +410,7 @@ static int hw_rule_playback_format(struct snd_pcm_hw_params *params, chip->count_pb_S16_chan, chip->count_pb_S32_chan, f->bits[0], fmt.bits[0], result); - spin_unlock(&chip->pcm_lock); + spin_unlock_irqrestore(&chip->pcm_lock, flags); return result; }

@@ -581,6 +583,7 @@ static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr struct snd_ca0106_pcm *epcm; struct snd_pcm_runtime *runtime = substream->runtime; int err; + unsigned long flags;

epcm = kzalloc(sizeof(*epcm), GFP_KERNEL);

@@ -589,7 +592,8 @@ static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr epcm->emu = chip; epcm->substream = substream; epcm->channel_id=channel_id; - + + spin_lock_irqsave(&chip->pcm_lock, flags); runtime->private_data = epcm; runtime->private_free = snd_ca0106_pcm_free_substream;

@@ -605,22 +609,23 @@ static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr err = snd_pcm_hw_constraint_integer( runtime, SNDRV_PCM_HW_PARAM_PERIODS); if (err < 0) - return err; + goto error; err = snd_pcm_hw_constraint_step( runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 64); if (err < 0) - return err; + goto error; err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, hw_rule_playback_rate, (void *)chip, SNDRV_PCM_HW_PARAM_RATE, -1); if (err < 0) - return err; + goto error; err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT, hw_rule_playback_format, (void *)chip, SNDRV_PCM_HW_PARAM_FORMAT, -1); if (err < 0) - return err; + goto error; snd_pcm_set_sync(substream); + spin_unlock_irqrestore(&chip->pcm_lock, flags);

if (chip->details->spi_dac && channel_id != PCM_FRONT_CHANNEL) { const int reg = spi_dacd_reg[channel_id]; @@ -632,6 +637,9 @@ static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr return err; } return 0; +error: + spin_unlock_irqrestore(&chip->pcm_lock, flags); + return err; }

/* close callback */ @@ -639,7 +647,11 @@ static int snd_ca0106_pcm_close_playback(struct snd_pcm_substream *substream) { struct snd_ca0106 *chip = snd_pcm_substream_chip(substream); struct snd_pcm_runtime *runtime = substream->runtime; - struct snd_ca0106_pcm *epcm = runtime->private_data; + struct snd_ca0106_pcm *epcm; + unsigned long flags; + + spin_lock_irqsave(&chip->pcm_lock, flags); + epcm = runtime->private_data; chip->playback_channels[epcm->channel_id].use = 0;

if (chip->details->spi_dac && epcm->channel_id != PCM_FRONT_CHANNEL) { @@ -650,6 +662,7 @@ static int snd_ca0106_pcm_close_playback(struct snd_pcm_substream *substream) snd_ca0106_spi_write(chip, chip->spi_dac_reg[reg]); } /* FIXME: maybe zero others */ + spin_unlock_irqrestore(&chip->pcm_lock, flags); return 0; }

@@ -681,6 +694,7 @@ static int snd_ca0106_pcm_open_capture_channel(struct snd_pcm_substream *substre struct snd_ca0106_channel *channel = &(chip->capture_channels[channel_id]); struct snd_ca0106_pcm *epcm; struct snd_pcm_runtime *runtime = substream->runtime; + unsigned long flags; int err;

epcm = kzalloc(sizeof(*epcm), GFP_KERNEL); @@ -692,6 +706,7 @@ static int snd_ca0106_pcm_open_capture_channel(struct snd_pcm_substream *substre epcm->substream = substream; epcm->channel_id=channel_id;

+ spin_lock_irqsave(&chip->pcm_lock, flags); runtime->private_data = epcm; runtime->private_free = snd_ca0106_pcm_free_substream;

@@ -704,6 +719,7 @@ static int snd_ca0106_pcm_open_capture_channel(struct snd_pcm_substream *substre //printk("open:channel_id=%d, chip=%p, channel=%p\n",channel_id, chip, channel); //channel->interrupt = snd_ca0106_pcm_channel_interrupt; channel->epcm = epcm; + spin_unlock_irqsave(&chip->pcm_lock, flags); if ((err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS)) < 0) return err; //snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, &hw_constraints_capture_period_sizes); @@ -717,9 +733,14 @@ static int snd_ca0106_pcm_close_capture(struct snd_pcm_substream *substream) { struct snd_ca0106 *chip = snd_pcm_substream_chip(substream); struct snd_pcm_runtime *runtime = substream->runtime; - struct snd_ca0106_pcm *epcm = runtime->private_data; + struct snd_ca0106_pcm *epcm; + unsigned long flags; + spin_lock_irqsave(&chip->pcm_lock, flags); + epcm = runtime->private_data; + chip->capture_channels[epcm->channel_id].use = 0; /* FIXME: maybe zero others */ + spin_unlock_irqrestore(&chip->pcm_lock, flags); return 0; }

@@ -759,8 +780,9 @@ static int snd_ca0106_pcm_hw_free_playback(struct snd_pcm_substream *substream) struct snd_ca0106_pcm *epcm = runtime->private_data; struct snd_ca0106_channel *pchannel; int channel = epcm->channel_id, chi; + unsigned long flags;

- spin_lock(&chip->pcm_lock); + spin_lock_irqsave(&chip->pcm_lock, flags); epcm->hw_reserved = 0; chip->count_pb_44100_chan = chip->count_pb_non_44100_chan = 0; chip->count_pb_S16_chan = chip->count_pb_S32_chan = 0; @@ -771,17 +793,17 @@ static int snd_ca0106_pcm_hw_free_playback(struct snd_pcm_substream *substream) if (!pchannel->use) continue; if (snd_BUG_ON(!pchannel->epcm)) { - spin_unlock(&chip->pcm_lock); + spin_unlock_irqrestore(&chip->pcm_lock, flags); return -EINVAL; } if (!pchannel->epcm->hw_reserved) continue; if (snd_BUG_ON(!pchannel->epcm->substream)) { - spin_unlock(&chip->pcm_lock); + spin_unlock_irqrestore(&chip->pcm_lock, flags); return -EINVAL; } if (snd_BUG_ON(!pchannel->epcm->substream->runtime)) { - spin_unlock(&chip->pcm_lock); + spin_unlock_irqrestore(&chip->pcm_lock, flags); return -EINVAL; } runtimei = pchannel->epcm->substream->runtime; @@ -801,7 +823,7 @@ static int snd_ca0106_pcm_hw_free_playback(struct snd_pcm_substream *substream) "any_non_44100=%d, spdif=%d.\n", chip->count_pb_44100_chan, chip->count_pb_non_44100_chan, emu->spdif_enable); - spin_unlock(&chip->pcm_lock); + spin_unlock_irqrestore(&chip->pcm_lock, flags);

return snd_pcm_lib_free_pages(substream); } @@ -843,7 +865,9 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream) u32 reg71_set = 0; u32 reg71; int i; + unsigned long flags; + spin_lock_irqsave(&emu->pcm_lock, flags); /* FIXME CLEAN UP IF spdif_enable IS CHANGED WHILE CHANNELS ARE OPENED * OR PREVENT THIS FROM HAPPENING. */ if (emu->spdif_enable) @@ -864,7 +888,6 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream) /*printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n", emu->buffer.addr,emu->buffer.area, emu->buffer.bytes);*/ /* We are forced to build the settings for all the channels. */ - spin_lock(&emu->pcm_lock); emu->count_pb_44100_chan = emu->count_pb_non_44100_chan = 0; emu->count_pb_S16_chan = emu->count_pb_S32_chan = 0; for (chi = 0; chi < 4; ++chi) { @@ -874,17 +897,17 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream) if (!pchannel->use) continue; if (snd_BUG_ON(!pchannel->epcm)) { - spin_unlock(&emu->pcm_lock); + spin_unlock_irqrestore(&emu->pcm_lock, flags); return -EINVAL; } if (!pchannel->epcm->hw_reserved) continue; if (snd_BUG_ON(!pchannel->epcm->substream)) { - spin_unlock(&emu->pcm_lock); + spin_unlock_irqrestore(&emu->pcm_lock, flags); return -EINVAL; } if (snd_BUG_ON(!pchannel->epcm->substream->runtime)) { - spin_unlock(&emu->pcm_lock); + spin_unlock_irqrestore(&emu->pcm_lock, flags); return -EINVAL; } runtimei = pchannel->epcm->substream->runtime; @@ -979,14 +1002,14 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream) reg71_set |= 0x3 << reg71_shift; break; default: - spin_unlock(&emu->pcm_lock); + spin_unlock_irqrestore(&emu->pcm_lock, flags); printk(KERN_ERR DRVNAME ": prepare_playback: Bad sampling frequency %dHz.\n", runtime->rate); return -EINVAL; } if (count_pb_44100_chan && count_pb_non_44100_chan) { - spin_unlock(&emu->pcm_lock); + spin_unlock_irqrestore(&emu->pcm_lock, flags); printk(KERN_ERR DRVNAME "prepare_playback: requested sampling rate %dHz" " conflicts with other selected sampling rates.\n", @@ -994,7 +1017,7 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream) return -EINVAL; } if (count_pb_S16_chan && count_pb_S32_chan) { - spin_unlock(&emu->pcm_lock); + spin_unlock_irqrestore(&emu->pcm_lock, flags); printk(KERN_ERR DRVNAME "prepare_playback: requested sample format %d" " conflicts with other selected sample formats.\n", @@ -1025,7 +1048,7 @@ static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream) hcfg_set = 0; break; } - spin_unlock(&emu->pcm_lock); + spin_unlock_irqrestore(&emu->pcm_lock, flags); hcfg = inl(emu->port + HCFG) ; hcfg = (hcfg & ~hcfg_mask) | hcfg_set; outl(hcfg, emu->port + HCFG); @@ -1079,10 +1102,12 @@ static int snd_ca0106_pcm_prepare_capture(struct snd_pcm_substream *substream) u32 reg71_mask = 0x0000c000 ; /* Global. Set ADC rate. */ u32 reg71_set = 0; u32 reg71; + unsigned long flags; //snd_printk("prepare:channel_number=%d, rate=%d, format=0x%x, channels=%d, buffer_size=%ld, period_size=%ld, periods=%u, frames_to_bytes=%d\n",channel, runtime->rate, runtime->format, runtime->channels, runtime->buffer_size, runtime->period_size, runtime->periods, frames_to_bytes(runtime, 1)); //snd_printk("dma_addr=%x, dma_area=%p, table_base=%p\n",runtime->dma_addr, runtime->dma_area, table_base); //snd_printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",emu->buffer.addr, emu->buffer.area, emu->buffer.bytes); + spin_lock_irqsave(&emu->pcm_lock, flags); /* reg71 controls ADC rate. */ switch (runtime->rate) { case 44100: @@ -1116,6 +1141,7 @@ static int snd_ca0106_pcm_prepare_capture(struct snd_pcm_substream *substream) hcfg_set = 0; break; } + spin_unlock_irqrestore(&emu->pcm_lock, flags); hcfg = inl(emu->port + HCFG) ; hcfg = (hcfg & ~hcfg_mask) | hcfg_set; outl(hcfg, emu->port + HCFG); @@ -1149,6 +1175,7 @@ static int snd_ca0106_pcm_trigger_playback(struct snd_pcm_substream *substream, u32 basic = 0; u32 extended = 0; int running=0; + unsigned long flags;

switch (cmd) { case SNDRV_PCM_TRIGGER_START: @@ -1159,6 +1186,7 @@ static int snd_ca0106_pcm_trigger_playback(struct snd_pcm_substream *substream, running=0; break; } + spin_lock_irqsave(&emu->pcm_lock, flags); snd_pcm_group_for_each_entry(s, substream) { if (snd_pcm_substream_chip(s) != emu || s->stream != SNDRV_PCM_STREAM_PLAYBACK) @@ -1187,6 +1215,7 @@ static int snd_ca0106_pcm_trigger_playback(struct snd_pcm_substream *substream, result = -EINVAL; break; } + spin_unlock_irqrestore(&emu->pcm_lock, flags); return result; }

@@ -1199,7 +1228,9 @@ static int snd_ca0106_pcm_trigger_capture(struct snd_pcm_substream *substream, struct snd_ca0106_pcm *epcm = runtime->private_data; int channel = epcm->channel_id; int result = 0; + unsigned long flags;

+ spin_lock_irqsave(&emu->pcm_lock, flags); switch (cmd) { case SNDRV_PCM_TRIGGER_START: snd_ca0106_ptr_write(emu, EXTENDED_INT_MASK, 0, snd_ca0106_ptr_read(emu, EXTENDED_INT_MASK, 0) | (0x110000<<channel)); @@ -1215,6 +1246,7 @@ static int snd_ca0106_pcm_trigger_capture(struct snd_pcm_substream *substream, result = -EINVAL; break; } + spin_unlock_irqrestore(&emu->pcm_lock, flags); return result; }

@@ -1451,6 +1483,7 @@ static irqreturn_t snd_ca0106_interrupt(int irq, void *dev_id) int mask; unsigned int stat76; struct snd_ca0106_channel *pchannel; + unsigned long flags;

status = inl(chip->port + IPR); if (! status) @@ -1460,6 +1493,7 @@ static irqreturn_t snd_ca0106_interrupt(int irq, void *dev_id) //snd_printk("interrupt status = 0x%08x, stat76=0x%08x\n", status, stat76); //snd_printk("ptr=0x%08x\n",snd_ca0106_ptr_read(chip, PLAYBACK_POINTER, 0)); mask = 0x11; /* 0x1 for one half, 0x10 for the other half period. */ + spin_lock_irqsave(&chip->pcm_lock, flags); for(i = 0; i < 4; i++) { pchannel = &(chip->playback_channels[i]); if (stat76 & mask) { @@ -1487,6 +1521,7 @@ static irqreturn_t snd_ca0106_interrupt(int irq, void *dev_id) //printk(KERN_INFO "interrupt stat76[%d] = %08x, use=%d, channel=%d\n", i, stat76, pchannel->use, pchannel->number); mask <<= 1; } + spin_unlock_irqrestore(&chip->pcm_lock, flags);

snd_ca0106_ptr_write(chip, EXTENDED_INT, 0, stat76);

diff --git a/pci/ca0106/ca0106_mixer.c b/pci/ca0106/ca0106_mixer.c index 3025ed1..6752a05 100644 --- a/pci/ca0106/ca0106_mixer.c +++ b/pci/ca0106/ca0106_mixer.c @@ -96,7 +96,9 @@ static int snd_ca0106_shared_spdif_put(struct snd_kcontrol *kcontrol, unsigned int val; int change = 0; u32 mask; + unsigned long flags;

+ spin_lock_irqsave(&emu->pcm_lock, flags); val = !!ucontrol->value.integer.value[0]; change = (emu->spdif_enable != val); if (change) { @@ -120,6 +122,7 @@ static int snd_ca0106_shared_spdif_put(struct snd_kcontrol *kcontrol, outl(mask, emu->port + GPIO); } } + spin_unlock_irqrestore(&emu->pcm_lock, flags ); return change; }

Takashi,

Just a quick note:

The current version of my patch causes kernel lockups.

I have not yet figured out how to debug this.

I will do some research, possibly compile a debug kernel with the magic sysreq key enabled or something. I need to figure out where it is locking up. Any hints appreciated.

For now I have reverted to an earlier version of the patch before I included the spinlocks, and I now get no lockups. The machine is in daily use running MythTV. However, only one of the four outputs is in regular use. (My card only has one connection on the back, anyway.)

But perhaps you could tell by examining my locking code whether I have broken some cardinal rule.

I already found a bug in an error path in prepare where I forgot to unlock.

Thanks, Ben Stanley.

On Mon, 2008-09-08 at 16:58 +0200, Takashi Iwai wrote:

At Thu, 04 Sep 2008 01:15:47 +1000, Ben Stanley wrote:

...

Takashi,

Thankyou for your comments.

My modifications would be much simpler if I didn't have to re-build the counts of how many channels are in each mode every time the prepare and hw_free functions are called.

In the case of the prepare function, I could modify the existing counts if I knew what the previous state (rate, format) was before the most recent change.

In the case of the hw_free function, I think that the rate and format information is invalid for the current channel by the time the hw_free function is called.

If I knew these pieces of information, I could completely avoid re-building the counts.

Regarding the locking, initially I only tried to protect my count fields. I have now tried to work on the locking some more (see incremental untested patch at end). Each time I learn some more... thanks for your patience. Perhaps there is another driver that already has to do this level of locking that you could point me to look at. The other drivers I have looked at so far don't have to do this much locking. (I gather that means they are designed to avoid the need to lock?)

There must be some, but each driver has different things :)

...

Regarding atomicity, the documentation states that snd_ca0106_pcm_trigger_capture, snd_ca0106_pcm_pointer_playback, snd_ca0106_pcm_pointer_capture are called with a spinlock held by the middle layer. What lock is that?

It's PCM substream lock.

...

Do I need to hold my spinlock in addition? Will that possibly cause deadlock if the spinlocks are grabbed in different orders? So many questions...

Yes, you need another type of lock because PCM substream lock is specific to each substream, and you need a global lock over all substreams.

...

Regarding snd_BUG_ON, I have used it like an assert, intending that it should be turned off in normal production code. Is that not the case?

The problem is that some checks should be done even for the production system, not only for debugging. At least, exclusivity of 44.1kHz/48kHZ rates should be checked always, because the hardware constraint can be racy.

...

...

...

• spin_lock(&chip->pcm_lock);
• if (chip->spdif_enable) {

• if (snd_BUG_ON(chip->count_pb_44100_chan &&
  

• 	 chip->count_pb_non_44100_chan)) {
  

• 	spin_unlock(&chip->pcm_lock);
  

• 	return -EINVAL;
  

• }
  

Don't use too much snd_BUG_ON(). This would be better to be a normal check since we have anyway races in parameter constraints, and this may happen indeed.

This is just asserting the invariants, and it did find some problems during development. This condition should not be triggered by the races in parameter constraints,

... but this could happen because of races, I'm afraid.

...

...

...

@@ -647,6 +754,55 @@ static int snd_ca0106_pcm_hw_params_playback(struct snd_pcm_substream *substream /* hw_free callback */ static int snd_ca0106_pcm_hw_free_playback(struct snd_pcm_substream *substream) {

• struct snd_ca0106 *chip = snd_pcm_substream_chip(substream);
• struct snd_pcm_runtime *runtime = substream->runtime, *runtimei;
• struct snd_ca0106_pcm *epcm = runtime->private_data;
• struct snd_ca0106_channel *pchannel;
• int channel = epcm->channel_id, chi;
• spin_lock(&chip->pcm_lock);
• epcm->hw_reserved = 0;
• chip->count_pb_44100_chan = chip->count_pb_non_44100_chan = 0;
• chip->count_pb_S16_chan = chip->count_pb_S32_chan = 0;
• for (chi = 0; chi < 4; ++chi) {

• if (chi == channel)
  

• 	continue;
  

• pchannel = &(chip->playback_channels[chi]);
  

• if (!pchannel->use)
  

• 	continue;
  

• if (snd_BUG_ON(!pchannel->epcm)) {
  

• 	spin_unlock(&chip->pcm_lock);
  

• 	return -EINVAL;
  

• }
  

snd_BUG_ON() is wrong here. Note that pchannel->epcm isn't protected at all by chip->pcm_lock in your patch because it's not used in *_open() and *_close() callbacks. Use the lock to protect the assignment of these.

Hmmm... The open callback allocates a new snd_ca0106_pcm by kmalloc, but the close callback does not kfree it. Is that a leak? Oh, I see that it is kfreed by snd_ca0106_pcm_free_substream.

I can't find any other part of the driver that writes to the epcm field. Furthermore, I observe that the use field is set to 1 before epcm field is assigned. So perhaps the lock should cover this case.

That's a part of the problem. It can run between use = 1 and epcm set.

...

It is not quite the same between hw_free and prepare, as the prepare function takes special care to check whether what is requested fulfils the constraints. If it satisfies the constraints, it is configured. The check and the configuration are performed atomically.

So it looks very similar between the two, but is subtly different. Perhaps I can code both versions in one function.

Incremental untested patch trying to address the locking is shown below. I'm sure you'll find things you don't like :-). Please ignore the other issues outlined above for the moment - I will work on addressing your other comments another night. Please review the locking.

I'll check the patch later (now busy during attending conference...)

Thanks!

Takashi