Takashi now "prefers" that the drivers do not rely on the core's locking of card->controls_rwsem, so we undo 06405d8ee8 ("ALSA: emu10k1: remove now superfluous mixer locking") and add more locks that were already missing.
This adds some rather significant critical sections with IRQs disabled, as emu->reg_lock is also accessed by the PCM trigger callbacks, which are called with the hardirq-safe (self-)group lock held.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de
--- the long irq-disabled sections could be avoided by introducing a separate control mutex. i surveyed a few drivers, and the precedents are very mixed, so i'm not sure this would be considered worth it.
--- of the few drivers i checked, pcsp, ak4xxx-adda, pt2258, hal2, sgio2audio, au88x0_eq, and ca0106_mixer appear to be missing locking upon superficial inspection, a percentage well into the double digits.
given that there are ~3700 hits for snd_kcontrol_new (and many more _put() methods to check, due to initializer arrays), the whole endeavor seems just as utterly hopeless to me as i expected.
so i recommend that you reconsider, and consequently reject this patch. --- sound/pci/emu10k1/emufx.c | 5 +++++ sound/pci/emu10k1/emumixer.c | 42 ++++++++++++++++++++++++++++++++++-- sound/pci/emu10k1/emupcm.c | 2 ++ sound/pci/emu10k1/p16v.c | 7 ++++++ 4 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c index 9904bcfee106..cda5311cee46 100644 --- a/sound/pci/emu10k1/emufx.c +++ b/sound/pci/emu10k1/emufx.c @@ -353,12 +353,15 @@ static int snd_emu10k1_gpr_ctl_info(struct snd_kcontrol *kcontrol, struct snd_ct
static int snd_emu10k1_gpr_ctl_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { + struct snd_emu10k1 *emu = snd_kcontrol_chip(kcontrol); struct snd_emu10k1_fx8010_ctl *ctl = (struct snd_emu10k1_fx8010_ctl *) kcontrol->private_value; unsigned int i; + spin_lock_irq(&emu->reg_lock); for (i = 0; i < ctl->vcount; i++) ucontrol->value.integer.value[i] = ctl->value[i]; + spin_unlock_irq(&emu->reg_lock); return 0; }
@@ -371,6 +374,7 @@ static int snd_emu10k1_gpr_ctl_put(struct snd_kcontrol *kcontrol, struct snd_ctl unsigned int i, j; int change = 0; + spin_lock_irq(&emu->reg_lock); for (i = 0; i < ctl->vcount; i++) { nval = ucontrol->value.integer.value[i]; if (nval < ctl->min) @@ -416,6 +420,7 @@ static int snd_emu10k1_gpr_ctl_put(struct snd_kcontrol *kcontrol, struct snd_ctl } } __error: + spin_unlock_irq(&emu->reg_lock); return change; }
diff --git a/sound/pci/emu10k1/emumixer.c b/sound/pci/emu10k1/emumixer.c index 9a94f08f2463..c52ab410b632 100644 --- a/sound/pci/emu10k1/emumixer.c +++ b/sound/pci/emu10k1/emumixer.c @@ -63,10 +63,12 @@ static int snd_emu10k1_spdif_get(struct snd_kcontrol *kcontrol, /* Limit: emu->spdif_bits */ if (idx >= 3) return -EINVAL; + spin_lock_irq(&emu->reg_lock); ucontrol->value.iec958.status[0] = (emu->spdif_bits[idx] >> 0) & 0xff; ucontrol->value.iec958.status[1] = (emu->spdif_bits[idx] >> 8) & 0xff; ucontrol->value.iec958.status[2] = (emu->spdif_bits[idx] >> 16) & 0xff; ucontrol->value.iec958.status[3] = (emu->spdif_bits[idx] >> 24) & 0xff; + spin_unlock_irq(&emu->reg_lock); return 0; }
@@ -664,11 +666,13 @@ static int snd_emu1010_output_source_put(struct snd_kcontrol *kcontrol, return -EINVAL; if (channel >= emu_ri->n_outs) return -EINVAL; + spin_lock_irq(&emu->reg_lock); change = (emu->emu1010.output_source[channel] != val); if (change) { emu->emu1010.output_source[channel] = val; snd_emu1010_output_source_apply(emu, channel, val); } + spin_unlock_irq(&emu->reg_lock); return change; }
@@ -708,11 +712,13 @@ static int snd_emu1010_input_source_put(struct snd_kcontrol *kcontrol, return -EINVAL; if (channel >= emu_ri->n_ins) return -EINVAL; + spin_lock_irq(&emu->reg_lock); change = (emu->emu1010.input_source[channel] != val); if (change) { emu->emu1010.input_source[channel] = val; snd_emu1010_input_source_apply(emu, channel, val); } + spin_unlock_irq(&emu->reg_lock); return change; }
@@ -773,16 +779,18 @@ static int snd_emu1010_adc_pads_put(struct snd_kcontrol *kcontrol, struct snd_ct int change;
val = ucontrol->value.integer.value[0]; + spin_lock_irq(&emu->reg_lock); cache = emu->emu1010.adc_pads; if (val == 1) cache = cache | mask; else cache = cache & ~mask; change = (cache != emu->emu1010.adc_pads); if (change) { snd_emu1010_fpga_write(emu, EMU_HANA_ADC_PADS, cache ); emu->emu1010.adc_pads = cache; } + spin_unlock_irq(&emu->reg_lock);
return change; } @@ -831,16 +839,18 @@ static int snd_emu1010_dac_pads_put(struct snd_kcontrol *kcontrol, struct snd_ct int change;
val = ucontrol->value.integer.value[0]; + spin_lock_irq(&emu->reg_lock); cache = emu->emu1010.dac_pads; if (val == 1) cache = cache | mask; else cache = cache & ~mask; change = (cache != emu->emu1010.dac_pads); if (change) { snd_emu1010_fpga_write(emu, EMU_HANA_DAC_PADS, cache ); emu->emu1010.dac_pads = cache; } + spin_unlock_irq(&emu->reg_lock);
return change; } @@ -986,18 +996,22 @@ static int snd_emu1010_clock_source_put(struct snd_kcontrol *kcontrol, val = ucontrol->value.enumerated.item[0] ; if (val >= emu_ci->num) return -EINVAL; + spin_lock_irq(&emu->reg_lock); change = (emu->emu1010.clock_source != val); if (change) { emu->emu1010.clock_source = val; emu->emu1010.wclock = emu_ci->vals[val];
snd_emu1010_fpga_write(emu, EMU_HANA_UNMUTE, EMU_MUTE); snd_emu1010_fpga_write(emu, EMU_HANA_WCLOCK, emu->emu1010.wclock); + spin_unlock_irq(&emu->reg_lock); msleep(10); // Allow DLL to settle + spin_lock_irq(&emu->reg_lock); snd_emu1010_fpga_write(emu, EMU_HANA_UNMUTE, EMU_UNMUTE);
snd_emu1010_update_clock(emu); } + spin_unlock_irq(&emu->reg_lock); return change; }
@@ -1040,11 +1054,13 @@ static int snd_emu1010_clock_fallback_put(struct snd_kcontrol *kcontrol,
if (val >= 2) return -EINVAL; + spin_lock_irq(&emu->reg_lock); change = (emu->emu1010.clock_fallback != val); if (change) { emu->emu1010.clock_fallback = val; snd_emu1010_fpga_write(emu, EMU_HANA_DEFCLOCK, 1 - val); } + spin_unlock_irq(&emu->reg_lock); return change; }
@@ -1090,13 +1106,15 @@ static int snd_emu1010_optical_out_put(struct snd_kcontrol *kcontrol, /* Limit: uinfo->value.enumerated.items = 2; */ if (val >= 2) return -EINVAL; + spin_lock_irq(&emu->reg_lock); change = (emu->emu1010.optical_out != val); if (change) { emu->emu1010.optical_out = val; tmp = (emu->emu1010.optical_in ? EMU_HANA_OPTICAL_IN_ADAT : EMU_HANA_OPTICAL_IN_SPDIF) | (emu->emu1010.optical_out ? EMU_HANA_OPTICAL_OUT_ADAT : EMU_HANA_OPTICAL_OUT_SPDIF); snd_emu1010_fpga_write(emu, EMU_HANA_OPTICAL_TYPE, tmp); } + spin_unlock_irq(&emu->reg_lock); return change; }
@@ -1141,13 +1159,15 @@ static int snd_emu1010_optical_in_put(struct snd_kcontrol *kcontrol, /* Limit: uinfo->value.enumerated.items = 2; */ if (val >= 2) return -EINVAL; + spin_lock_irq(&emu->reg_lock); change = (emu->emu1010.optical_in != val); if (change) { emu->emu1010.optical_in = val; tmp = (emu->emu1010.optical_in ? EMU_HANA_OPTICAL_IN_ADAT : EMU_HANA_OPTICAL_IN_SPDIF) | (emu->emu1010.optical_out ? EMU_HANA_OPTICAL_OUT_ADAT : EMU_HANA_OPTICAL_OUT_SPDIF); snd_emu1010_fpga_write(emu, EMU_HANA_OPTICAL_TYPE, tmp); } + spin_unlock_irq(&emu->reg_lock); return change; }
@@ -1203,16 +1223,17 @@ static int snd_audigy_i2c_capture_source_put(struct snd_kcontrol *kcontrol, /* emu->i2c_capture_volume */ if (source_id >= 2) return -EINVAL; + spin_lock_irq(&emu->reg_lock); change = (emu->i2c_capture_source != source_id); if (change) { snd_emu10k1_i2c_write(emu, ADC_MUX, 0); /* Mute input */ - spin_lock_irq(&emu->emu_lock); + spin_lock(&emu->emu_lock); gpio = inw(emu->port + A_IOCFG); if (source_id==0) outw(gpio | 0x4, emu->port + A_IOCFG); else outw(gpio & ~0x4, emu->port + A_IOCFG); - spin_unlock_irq(&emu->emu_lock); + spin_unlock(&emu->emu_lock);
ngain = emu->i2c_capture_volume[source_id][0]; /* Left */ ogain = emu->i2c_capture_volume[emu->i2c_capture_source][0]; /* Left */ @@ -1227,6 +1248,7 @@ static int snd_audigy_i2c_capture_source_put(struct snd_kcontrol *kcontrol, snd_emu10k1_i2c_write(emu, ADC_MUX, source); /* Set source */ emu->i2c_capture_source = source_id; } + spin_unlock_irq(&emu->reg_lock); return change; }
@@ -1261,8 +1283,10 @@ static int snd_audigy_i2c_volume_get(struct snd_kcontrol *kcontrol, if (source_id >= 2) return -EINVAL;
+ spin_lock_irq(&emu->reg_lock); ucontrol->value.integer.value[0] = emu->i2c_capture_volume[source_id][0]; ucontrol->value.integer.value[1] = emu->i2c_capture_volume[source_id][1]; + spin_unlock_irq(&emu->reg_lock); return 0; }
@@ -1286,6 +1310,7 @@ static int snd_audigy_i2c_volume_put(struct snd_kcontrol *kcontrol, return -EINVAL; if (ngain1 > 0xff) return -EINVAL; + spin_lock_irq(&emu->reg_lock); ogain = emu->i2c_capture_volume[source_id][0]; /* Left */ if (ogain != ngain0) { if (emu->i2c_capture_source == source_id) @@ -1300,6 +1325,7 @@ static int snd_audigy_i2c_volume_put(struct snd_kcontrol *kcontrol, emu->i2c_capture_volume[source_id][1] = ngain1; change = 1; } + spin_unlock_irq(&emu->reg_lock);
return change; } @@ -1411,11 +1437,13 @@ static int snd_emu10k1_spdif_put(struct snd_kcontrol *kcontrol, (ucontrol->value.iec958.status[1] << 8) | (ucontrol->value.iec958.status[2] << 16) | (ucontrol->value.iec958.status[3] << 24); + spin_lock_irq(&emu->reg_lock); change = val != emu->spdif_bits[idx]; if (change) { snd_emu10k1_ptr_write(emu, SPCS0 + idx, 0, val); emu->spdif_bits[idx] = val; } + spin_unlock_irq(&emu->reg_lock); return change; }
@@ -1487,10 +1515,12 @@ static int snd_emu10k1_send_routing_get(struct snd_kcontrol *kcontrol, int num_efx = emu->audigy ? 8 : 4; int mask = emu->audigy ? 0x3f : 0x0f;
+ spin_lock_irq(&emu->reg_lock); for (voice = 0; voice < 3; voice++) for (idx = 0; idx < num_efx; idx++) ucontrol->value.integer.value[(voice * num_efx) + idx] = mix->send_routing[voice][idx] & mask; + spin_unlock_irq(&emu->reg_lock); return 0; }
@@ -1558,8 +1588,10 @@ static int snd_emu10k1_send_volume_get(struct snd_kcontrol *kcontrol, int idx; int num_efx = emu->audigy ? 8 : 4;
+ spin_lock_irq(&emu->reg_lock); for (idx = 0; idx < 3*num_efx; idx++) ucontrol->value.integer.value[idx] = mix->send_volume[idx/num_efx][idx%num_efx]; + spin_unlock_irq(&emu->reg_lock); return 0; }
@@ -1623,8 +1655,10 @@ static int snd_emu10k1_attn_get(struct snd_kcontrol *kcontrol, &emu->pcm_mixer[snd_ctl_get_ioffidx(kcontrol, &ucontrol->id)]; int idx;
+ spin_lock_irq(&emu->reg_lock); for (idx = 0; idx < 3; idx++) ucontrol->value.integer.value[idx] = mix->attn[idx] * 0xffffU / 0x8000U; + spin_unlock_irq(&emu->reg_lock); return 0; }
@@ -1690,9 +1724,11 @@ static int snd_emu10k1_efx_send_routing_get(struct snd_kcontrol *kcontrol, int num_efx = emu->audigy ? 8 : 4; int mask = emu->audigy ? 0x3f : 0x0f;
+ spin_lock_irq(&emu->reg_lock); for (idx = 0; idx < num_efx; idx++) ucontrol->value.integer.value[idx] = mix->send_routing[0][idx] & mask; + spin_unlock_irq(&emu->reg_lock); return 0; }
@@ -1755,8 +1791,10 @@ static int snd_emu10k1_efx_send_volume_get(struct snd_kcontrol *kcontrol, int idx; int num_efx = emu->audigy ? 8 : 4;
+ spin_lock_irq(&emu->reg_lock); for (idx = 0; idx < num_efx; idx++) ucontrol->value.integer.value[idx] = mix->send_volume[0][idx]; + spin_unlock_irq(&emu->reg_lock); return 0; }
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index 8b3d1b35d6e7..994aa386d074 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -1541,8 +1541,10 @@ static int snd_emu10k1_pcm_efx_voices_mask_get(struct snd_kcontrol *kcontrol, st int nefx = emu->audigy ? 64 : 32; int idx; + spin_lock_irq(&emu->reg_lock); for (idx = 0; idx < nefx; idx++) ucontrol->value.integer.value[idx] = (emu->efx_voices_mask[idx / 32] & (1 << (idx % 32))) ? 1 : 0; + spin_unlock_irq(&emu->reg_lock); return 0; }
diff --git a/sound/pci/emu10k1/p16v.c b/sound/pci/emu10k1/p16v.c index e7f097cae574..1dec937b7d21 100644 --- a/sound/pci/emu10k1/p16v.c +++ b/sound/pci/emu10k1/p16v.c @@ -635,6 +635,7 @@ static int snd_p16v_volume_put(struct snd_kcontrol *kcontrol, int reg = kcontrol->private_value & 0xff; u32 value, oval;
+ spin_lock_irq(&emu->reg_lock); oval = value = snd_emu10k1_ptr20_read(emu, reg, 0); if (high_low == 1) { value &= 0xffff; @@ -647,8 +648,10 @@ static int snd_p16v_volume_put(struct snd_kcontrol *kcontrol, } if (value != oval) { snd_emu10k1_ptr20_write(emu, reg, 0, value); + spin_unlock_irq(&emu->reg_lock); return 1; } + spin_unlock_irq(&emu->reg_lock); return 0; }
@@ -684,13 +687,15 @@ static int snd_p16v_capture_source_put(struct snd_kcontrol *kcontrol, val = ucontrol->value.enumerated.item[0] ; if (val > 7) return -EINVAL; + spin_lock_irq(&emu->reg_lock); change = (emu->p16v_capture_source != val); if (change) { emu->p16v_capture_source = val; source = (val << 28) | (val << 24) | (val << 20) | (val << 16); mask = snd_emu10k1_ptr20_read(emu, BASIC_INTERRUPT, 0) & 0xffff; snd_emu10k1_ptr20_write(emu, BASIC_INTERRUPT, 0, source | mask); } + spin_unlock_irq(&emu->reg_lock); return change; }
@@ -722,12 +727,14 @@ static int snd_p16v_capture_channel_put(struct snd_kcontrol *kcontrol, val = ucontrol->value.enumerated.item[0] ; if (val > 3) return -EINVAL; + spin_lock_irq(&emu->reg_lock); change = (emu->p16v_capture_channel != val); if (change) { emu->p16v_capture_channel = val; tmp = snd_emu10k1_ptr20_read(emu, CAPTURE_P16V_SOURCE, 0) & 0xfffc; snd_emu10k1_ptr20_write(emu, CAPTURE_P16V_SOURCE, 0, tmp | val); } + spin_unlock_irq(&emu->reg_lock); return change; } static const DECLARE_TLV_DB_SCALE(snd_p16v_db_scale1, -5175, 25, 1);