[PATCH 1/3] ALSA: emu10k1: fix return value of snd_emu1010_adc_pads_put()

[PATCH 1/3] ALSA: emu10k1: fix return value of snd_emu1010_adc_pads_put()

[Oswald Buddenhagen]

It returned zero even if the value had changed.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---
this should have been in cc766807a2 (fix return value of
snd_emu1010_dac_pads_put()), but, well.
---
 sound/pci/emu10k1/emumixer.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/pci/emu10k1/emumixer.c b/sound/pci/emu10k1/emumixer.c
index f9500cd50a4b..573e1c7c5e50 100644
--- a/sound/pci/emu10k1/emumixer.c
+++ b/sound/pci/emu10k1/emumixer.c
@@ -770,18 +770,21 @@ static int snd_emu1010_adc_pads_put(struct snd_kcontrol *kcontrol, struct snd_ct
 	struct snd_emu10k1 *emu = snd_kcontrol_chip(kcontrol);
 	unsigned int mask = snd_emu1010_adc_pad_regs[kcontrol->private_value];
 	unsigned int val, cache;
+	int change;
+
 	val = ucontrol->value.integer.value[0];
 	cache = emu->emu1010.adc_pads;
 	if (val == 1) 
 		cache = cache | mask;
 	else
 		cache = cache & ~mask;
-	if (cache != emu->emu1010.adc_pads) {
+	change = (cache != emu->emu1010.adc_pads);
+	if (change) {
 		snd_emu1010_fpga_write(emu, EMU_HANA_ADC_PADS, cache );
 	        emu->emu1010.adc_pads = cache;
 	}
 
-	return 0;
+	return change;
 }
 
 static const struct snd_kcontrol_new emu1010_adc_pads_ctl = {
-- 
2.40.0.152.g15d061e6df

[PATCH 2/3] ALSA: emu10k1: remove superfluous IRQ enable state saving

[Oswald Buddenhagen]

The mixer, PCM prepare, MIDI, synth driver, and procfs callbacks are all
always invoked with IRQs enabled, so there is no point in saving the
state.

snd_emu1010_load_firmware_entry() is called from emu1010_firmware_work()
and snd_emu10k1_emu1010_init(); the latter from snd_emu10k1_create() and
snd_emu10k1_resume(), all of which have IRQs enabled.

The voice and memory functions are called from mixed contexts, so they
keep the state saving.

The low-level functions all keep the state saving, because it's not
feasible to keep track of what is called where.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_main.c  |  5 ++--
 sound/pci/emu10k1/emu10k1_synth.c | 10 +++----
 sound/pci/emu10k1/emumixer.c      | 45 +++++++++++++------------------
 sound/pci/emu10k1/emumpu401.c     | 40 ++++++++++++---------------
 sound/pci/emu10k1/emupcm.c        |  6 ++---
 sound/pci/emu10k1/emuproc.c       | 10 +++----
 6 files changed, 47 insertions(+), 69 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_main.c b/sound/pci/emu10k1/emu10k1_main.c
index 58ed72de6403..f9418c4a9a6f 100644
--- a/sound/pci/emu10k1/emu10k1_main.c
+++ b/sound/pci/emu10k1/emu10k1_main.c
@@ -664,19 +664,18 @@ static int snd_emu1010_load_firmware_entry(struct snd_emu10k1 *emu,
 	u16 reg;
 	u8 value;
 	__always_unused u16 write_post;
-	unsigned long flags;
 
 	if (!fw_entry)
 		return -EIO;
 
 	/* The FPGA is a Xilinx Spartan IIE XC2S50E */
 	/* On E-MU 0404b it is a Xilinx Spartan III XC3S50 */
 	/* GPIO7 -> FPGA PGMN
 	 * GPIO6 -> FPGA CCLK
 	 * GPIO5 -> FPGA DIN
 	 * FPGA CONFIG OFF -> FPGA PGMN
 	 */
-	spin_lock_irqsave(&emu->emu_lock, flags);
+	spin_lock_irq(&emu->emu_lock);
 	outw(0x00, emu->port + A_GPIO); /* Set PGMN low for 100uS. */
 	write_post = inw(emu->port + A_GPIO);
 	udelay(100);
@@ -699,7 +698,7 @@ static int snd_emu1010_load_firmware_entry(struct snd_emu10k1 *emu,
 	/* After programming, set GPIO bit 4 high again. */
 	outw(0x10, emu->port + A_GPIO);
 	write_post = inw(emu->port + A_GPIO);
-	spin_unlock_irqrestore(&emu->emu_lock, flags);
+	spin_unlock_irq(&emu->emu_lock);
 
 	return 0;
 }
diff --git a/sound/pci/emu10k1/emu10k1_synth.c b/sound/pci/emu10k1/emu10k1_synth.c
index 759e66e1105a..68dfcb24b889 100644
--- a/sound/pci/emu10k1/emu10k1_synth.c
+++ b/sound/pci/emu10k1/emu10k1_synth.c
@@ -22,7 +22,6 @@ static int snd_emu10k1_synth_probe(struct device *_dev)
 	struct snd_emux *emux;
 	struct snd_emu10k1 *hw;
 	struct snd_emu10k1_synth_arg *arg;
-	unsigned long flags;
 
 	arg = SNDRV_SEQ_DEVICE_ARGPTR(dev);
 	if (arg == NULL)
@@ -56,33 +55,32 @@ static int snd_emu10k1_synth_probe(struct device *_dev)
 		return -ENOMEM;
 	}
 
-	spin_lock_irqsave(&hw->voice_lock, flags);
+	spin_lock_irq(&hw->voice_lock);
 	hw->synth = emux;
 	hw->get_synth_voice = snd_emu10k1_synth_get_voice;
-	spin_unlock_irqrestore(&hw->voice_lock, flags);
+	spin_unlock_irq(&hw->voice_lock);
 
 	dev->driver_data = emux;
 
 	return 0;
 }
 
 static int snd_emu10k1_synth_remove(struct device *_dev)
 {
 	struct snd_seq_device *dev = to_seq_dev(_dev);
 	struct snd_emux *emux;
 	struct snd_emu10k1 *hw;
-	unsigned long flags;
 
 	if (dev->driver_data == NULL)
 		return 0; /* not registered actually */
 
 	emux = dev->driver_data;
 
 	hw = emux->hw;
-	spin_lock_irqsave(&hw->voice_lock, flags);
+	spin_lock_irq(&hw->voice_lock);
 	hw->synth = NULL;
 	hw->get_synth_voice = NULL;
-	spin_unlock_irqrestore(&hw->voice_lock, flags);
+	spin_unlock_irq(&hw->voice_lock);
 
 	snd_emux_free(emux);
 	return 0;
diff --git a/sound/pci/emu10k1/emumixer.c b/sound/pci/emu10k1/emumixer.c
index 573e1c7c5e50..9a94f08f2463 100644
--- a/sound/pci/emu10k1/emumixer.c
+++ b/sound/pci/emu10k1/emumixer.c
@@ -1193,7 +1193,6 @@ static int snd_audigy_i2c_capture_source_put(struct snd_kcontrol *kcontrol,
 	unsigned int ngain, ogain;
 	u16 gpio;
 	int change = 0;
-	unsigned long flags;
 	u32 source;
 	/* If the capture source has changed,
 	 * update the capture volume from the cached value
@@ -1207,13 +1206,13 @@ static int snd_audigy_i2c_capture_source_put(struct snd_kcontrol *kcontrol,
 	change = (emu->i2c_capture_source != source_id);
 	if (change) {
 		snd_emu10k1_i2c_write(emu, ADC_MUX, 0); /* Mute input */
-		spin_lock_irqsave(&emu->emu_lock, flags);
+		spin_lock_irq(&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_irqrestore(&emu->emu_lock, flags);
+		spin_unlock_irq(&emu->emu_lock);
 
 		ngain = emu->i2c_capture_volume[source_id][0]; /* Left */
 		ogain = emu->i2c_capture_volume[emu->i2c_capture_source][0]; /* Left */
@@ -1357,7 +1356,6 @@ static int snd_audigy_spdif_output_rate_put(struct snd_kcontrol *kcontrol,
 	struct snd_emu10k1 *emu = snd_kcontrol_chip(kcontrol);
 	int change;
 	unsigned int reg, val, tmp;
-	unsigned long flags;
 
 	switch(ucontrol->value.enumerated.item[0]) {
 	case 0:
@@ -1375,14 +1373,14 @@ static int snd_audigy_spdif_output_rate_put(struct snd_kcontrol *kcontrol,
 	}
 
 	
-	spin_lock_irqsave(&emu->reg_lock, flags);
+	spin_lock_irq(&emu->reg_lock);
 	reg = snd_emu10k1_ptr_read(emu, A_SPDIF_SAMPLERATE, 0);
 	tmp = reg & ~A_SPDIF_RATE_MASK;
 	tmp |= val;
 	change = (tmp != reg);
 	if (change)
 		snd_emu10k1_ptr_write(emu, A_SPDIF_SAMPLERATE, 0, tmp);
-	spin_unlock_irqrestore(&emu->reg_lock, flags);
+	spin_unlock_irq(&emu->reg_lock);
 	return change;
 }
 
@@ -1499,15 +1497,14 @@ static int snd_emu10k1_send_routing_get(struct snd_kcontrol *kcontrol,
 static int snd_emu10k1_send_routing_put(struct snd_kcontrol *kcontrol,
                                         struct snd_ctl_elem_value *ucontrol)
 {
-	unsigned long flags;
 	struct snd_emu10k1 *emu = snd_kcontrol_chip(kcontrol);
 	struct snd_emu10k1_pcm_mixer *mix =
 		&emu->pcm_mixer[snd_ctl_get_ioffidx(kcontrol, &ucontrol->id)];
 	int change = 0, voice, idx, val;
 	int num_efx = emu->audigy ? 8 : 4;
 	int mask = emu->audigy ? 0x3f : 0x0f;
 
-	spin_lock_irqsave(&emu->reg_lock, flags);
+	spin_lock_irq(&emu->reg_lock);
 	for (voice = 0; voice < 3; voice++)
 		for (idx = 0; idx < num_efx; idx++) {
 			val = ucontrol->value.integer.value[(voice * num_efx) + idx] & mask;
@@ -1527,7 +1524,7 @@ static int snd_emu10k1_send_routing_put(struct snd_kcontrol *kcontrol,
 					    &mix->send_routing[0][0]);
 		}
 	}
-	spin_unlock_irqrestore(&emu->reg_lock, flags);
+	spin_unlock_irq(&emu->reg_lock);
 	return change;
 }
 
@@ -1569,14 +1566,13 @@ static int snd_emu10k1_send_volume_get(struct snd_kcontrol *kcontrol,
 static int snd_emu10k1_send_volume_put(struct snd_kcontrol *kcontrol,
                                        struct snd_ctl_elem_value *ucontrol)
 {
-	unsigned long flags;
 	struct snd_emu10k1 *emu = snd_kcontrol_chip(kcontrol);
 	struct snd_emu10k1_pcm_mixer *mix =
 		&emu->pcm_mixer[snd_ctl_get_ioffidx(kcontrol, &ucontrol->id)];
 	int change = 0, idx, val;
 	int num_efx = emu->audigy ? 8 : 4;
 
-	spin_lock_irqsave(&emu->reg_lock, flags);
+	spin_lock_irq(&emu->reg_lock);
 	for (idx = 0; idx < 3*num_efx; idx++) {
 		val = ucontrol->value.integer.value[idx] & 255;
 		if (mix->send_volume[idx/num_efx][idx%num_efx] != val) {
@@ -1595,7 +1591,7 @@ static int snd_emu10k1_send_volume_put(struct snd_kcontrol *kcontrol,
 						   &mix->send_volume[0][0]);
 		}
 	}
-	spin_unlock_irqrestore(&emu->reg_lock, flags);
+	spin_unlock_irq(&emu->reg_lock);
 	return change;
 }
 
@@ -1635,13 +1631,12 @@ static int snd_emu10k1_attn_get(struct snd_kcontrol *kcontrol,
 static int snd_emu10k1_attn_put(struct snd_kcontrol *kcontrol,
 				struct snd_ctl_elem_value *ucontrol)
 {
-	unsigned long flags;
 	struct snd_emu10k1 *emu = snd_kcontrol_chip(kcontrol);
 	struct snd_emu10k1_pcm_mixer *mix =
 		&emu->pcm_mixer[snd_ctl_get_ioffidx(kcontrol, &ucontrol->id)];
 	int change = 0, idx, val;
 
-	spin_lock_irqsave(&emu->reg_lock, flags);
+	spin_lock_irq(&emu->reg_lock);
 	for (idx = 0; idx < 3; idx++) {
 		unsigned uval = ucontrol->value.integer.value[idx] & 0x1ffff;
 		val = uval * 0x8000U / 0xffffU;
@@ -1658,7 +1653,7 @@ static int snd_emu10k1_attn_put(struct snd_kcontrol *kcontrol,
 			snd_emu10k1_ptr_write(emu, VTFT_VOLUMETARGET, mix->epcm->voices[0]->number, mix->attn[0]);
 		}
 	}
-	spin_unlock_irqrestore(&emu->reg_lock, flags);
+	spin_unlock_irq(&emu->reg_lock);
 	return change;
 }
 
@@ -1704,15 +1699,14 @@ static int snd_emu10k1_efx_send_routing_get(struct snd_kcontrol *kcontrol,
 static int snd_emu10k1_efx_send_routing_put(struct snd_kcontrol *kcontrol,
                                         struct snd_ctl_elem_value *ucontrol)
 {
-	unsigned long flags;
 	struct snd_emu10k1 *emu = snd_kcontrol_chip(kcontrol);
 	int ch = snd_ctl_get_ioffidx(kcontrol, &ucontrol->id);
 	struct snd_emu10k1_pcm_mixer *mix = &emu->efx_pcm_mixer[ch];
 	int change = 0, idx, val;
 	int num_efx = emu->audigy ? 8 : 4;
 	int mask = emu->audigy ? 0x3f : 0x0f;
 
-	spin_lock_irqsave(&emu->reg_lock, flags);
+	spin_lock_irq(&emu->reg_lock);
 	for (idx = 0; idx < num_efx; idx++) {
 		val = ucontrol->value.integer.value[idx] & mask;
 		if (mix->send_routing[0][idx] != val) {
@@ -1727,7 +1721,7 @@ static int snd_emu10k1_efx_send_routing_put(struct snd_kcontrol *kcontrol,
 					&mix->send_routing[0][0]);
 		}
 	}
-	spin_unlock_irqrestore(&emu->reg_lock, flags);
+	spin_unlock_irq(&emu->reg_lock);
 	return change;
 }
 
@@ -1769,14 +1763,13 @@ static int snd_emu10k1_efx_send_volume_get(struct snd_kcontrol *kcontrol,
 static int snd_emu10k1_efx_send_volume_put(struct snd_kcontrol *kcontrol,
                                        struct snd_ctl_elem_value *ucontrol)
 {
-	unsigned long flags;
 	struct snd_emu10k1 *emu = snd_kcontrol_chip(kcontrol);
 	int ch = snd_ctl_get_ioffidx(kcontrol, &ucontrol->id);
 	struct snd_emu10k1_pcm_mixer *mix = &emu->efx_pcm_mixer[ch];
 	int change = 0, idx, val;
 	int num_efx = emu->audigy ? 8 : 4;
 
-	spin_lock_irqsave(&emu->reg_lock, flags);
+	spin_lock_irq(&emu->reg_lock);
 	for (idx = 0; idx < num_efx; idx++) {
 		val = ucontrol->value.integer.value[idx] & 255;
 		if (mix->send_volume[0][idx] != val) {
@@ -1790,7 +1783,7 @@ static int snd_emu10k1_efx_send_volume_put(struct snd_kcontrol *kcontrol,
 						   &mix->send_volume[0][0]);
 		}
 	}
-	spin_unlock_irqrestore(&emu->reg_lock, flags);
+	spin_unlock_irq(&emu->reg_lock);
 	return change;
 }
 
@@ -1829,26 +1822,25 @@ static int snd_emu10k1_efx_attn_get(struct snd_kcontrol *kcontrol,
 static int snd_emu10k1_efx_attn_put(struct snd_kcontrol *kcontrol,
 				struct snd_ctl_elem_value *ucontrol)
 {
-	unsigned long flags;
 	struct snd_emu10k1 *emu = snd_kcontrol_chip(kcontrol);
 	int ch = snd_ctl_get_ioffidx(kcontrol, &ucontrol->id);
 	struct snd_emu10k1_pcm_mixer *mix = &emu->efx_pcm_mixer[ch];
 	int change = 0, val;
 	unsigned uval;
 
-	spin_lock_irqsave(&emu->reg_lock, flags);
+	spin_lock_irq(&emu->reg_lock);
 	uval = ucontrol->value.integer.value[0] & 0x1ffff;
 	val = uval * 0x8000U / 0xffffU;
 	if (mix->attn[0] != val) {
 		mix->attn[0] = val;
 		change = 1;
 	}
 	if (change && mix->epcm) {
 		if (mix->epcm->voices[ch]) {
 			snd_emu10k1_ptr_write(emu, VTFT_VOLUMETARGET, mix->epcm->voices[ch]->number, mix->attn[0]);
 		}
 	}
-	spin_unlock_irqrestore(&emu->reg_lock, flags);
+	spin_unlock_irq(&emu->reg_lock);
 	return change;
 }
 
@@ -1884,15 +1876,14 @@ static int snd_emu10k1_shared_spdif_get(struct snd_kcontrol *kcontrol,
 static int snd_emu10k1_shared_spdif_put(struct snd_kcontrol *kcontrol,
 					struct snd_ctl_elem_value *ucontrol)
 {
-	unsigned long flags;
 	struct snd_emu10k1 *emu = snd_kcontrol_chip(kcontrol);
 	unsigned int reg, val, sw;
 	int change = 0;
 
 	sw = ucontrol->value.integer.value[0];
 	if (emu->card_capabilities->invert_shared_spdif)
 		sw = !sw;
-	spin_lock_irqsave(&emu->emu_lock, flags);
+	spin_lock_irq(&emu->emu_lock);
 	if ( emu->card_capabilities->i2c_adc) {
 		/* Do nothing for Audigy 2 ZS Notebook */
 	} else if (emu->audigy) {
@@ -1913,7 +1904,7 @@ static int snd_emu10k1_shared_spdif_put(struct snd_kcontrol *kcontrol,
 		reg |= val;
 		outl(reg | val, emu->port + HCFG);
 	}
-	spin_unlock_irqrestore(&emu->emu_lock, flags);
+	spin_unlock_irq(&emu->emu_lock);
 	return change;
 }
 
diff --git a/sound/pci/emu10k1/emumpu401.c b/sound/pci/emu10k1/emumpu401.c
index 3ce9b2129ce6..747c34b3d566 100644
--- a/sound/pci/emu10k1/emumpu401.c
+++ b/sound/pci/emu10k1/emumpu401.c
@@ -104,10 +104,9 @@ static void snd_emu10k1_midi_interrupt2(struct snd_emu10k1 *emu, unsigned int st
 
 static int snd_emu10k1_midi_cmd(struct snd_emu10k1 * emu, struct snd_emu10k1_midi *midi, unsigned char cmd, int ack)
 {
-	unsigned long flags;
 	int timeout, ok;
 
-	spin_lock_irqsave(&midi->input_lock, flags);
+	spin_lock_irq(&midi->input_lock);
 	mpu401_write_data(emu, midi, 0x00);
 	/* mpu401_clear_rx(emu, midi); */
 
@@ -126,7 +125,7 @@ static int snd_emu10k1_midi_cmd(struct snd_emu10k1 * emu, struct snd_emu10k1_mid
 	} else {
 		ok = 1;
 	}
-	spin_unlock_irqrestore(&midi->input_lock, flags);
+	spin_unlock_irq(&midi->input_lock);
 	if (!ok) {
 		dev_err(emu->card->dev,
 			"midi_cmd: 0x%x failed at 0x%lx (status = 0x%x, data = 0x%x)!!!\n",
@@ -142,98 +141,94 @@ static int snd_emu10k1_midi_input_open(struct snd_rawmidi_substream *substream)
 {
 	struct snd_emu10k1 *emu;
 	struct snd_emu10k1_midi *midi = (struct snd_emu10k1_midi *)substream->rmidi->private_data;
-	unsigned long flags;
 
 	emu = midi->emu;
 	if (snd_BUG_ON(!emu))
 		return -ENXIO;
-	spin_lock_irqsave(&midi->open_lock, flags);
+	spin_lock_irq(&midi->open_lock);
 	midi->midi_mode |= EMU10K1_MIDI_MODE_INPUT;
 	midi->substream_input = substream;
 	if (!(midi->midi_mode & EMU10K1_MIDI_MODE_OUTPUT)) {
-		spin_unlock_irqrestore(&midi->open_lock, flags);
+		spin_unlock_irq(&midi->open_lock);
 		if (snd_emu10k1_midi_cmd(emu, midi, MPU401_RESET, 1))
 			goto error_out;
 		if (snd_emu10k1_midi_cmd(emu, midi, MPU401_ENTER_UART, 1))
 			goto error_out;
 	} else {
-		spin_unlock_irqrestore(&midi->open_lock, flags);
+		spin_unlock_irq(&midi->open_lock);
 	}
 	return 0;
 
 error_out:
 	return -EIO;
 }
 
 static int snd_emu10k1_midi_output_open(struct snd_rawmidi_substream *substream)
 {
 	struct snd_emu10k1 *emu;
 	struct snd_emu10k1_midi *midi = (struct snd_emu10k1_midi *)substream->rmidi->private_data;
-	unsigned long flags;
 
 	emu = midi->emu;
 	if (snd_BUG_ON(!emu))
 		return -ENXIO;
-	spin_lock_irqsave(&midi->open_lock, flags);
+	spin_lock_irq(&midi->open_lock);
 	midi->midi_mode |= EMU10K1_MIDI_MODE_OUTPUT;
 	midi->substream_output = substream;
 	if (!(midi->midi_mode & EMU10K1_MIDI_MODE_INPUT)) {
-		spin_unlock_irqrestore(&midi->open_lock, flags);
+		spin_unlock_irq(&midi->open_lock);
 		if (snd_emu10k1_midi_cmd(emu, midi, MPU401_RESET, 1))
 			goto error_out;
 		if (snd_emu10k1_midi_cmd(emu, midi, MPU401_ENTER_UART, 1))
 			goto error_out;
 	} else {
-		spin_unlock_irqrestore(&midi->open_lock, flags);
+		spin_unlock_irq(&midi->open_lock);
 	}
 	return 0;
 
 error_out:
 	return -EIO;
 }
 
 static int snd_emu10k1_midi_input_close(struct snd_rawmidi_substream *substream)
 {
 	struct snd_emu10k1 *emu;
 	struct snd_emu10k1_midi *midi = (struct snd_emu10k1_midi *)substream->rmidi->private_data;
-	unsigned long flags;
 	int err = 0;
 
 	emu = midi->emu;
 	if (snd_BUG_ON(!emu))
 		return -ENXIO;
-	spin_lock_irqsave(&midi->open_lock, flags);
+	spin_lock_irq(&midi->open_lock);
 	snd_emu10k1_intr_disable(emu, midi->rx_enable);
 	midi->midi_mode &= ~EMU10K1_MIDI_MODE_INPUT;
 	midi->substream_input = NULL;
 	if (!(midi->midi_mode & EMU10K1_MIDI_MODE_OUTPUT)) {
-		spin_unlock_irqrestore(&midi->open_lock, flags);
+		spin_unlock_irq(&midi->open_lock);
 		err = snd_emu10k1_midi_cmd(emu, midi, MPU401_RESET, 0);
 	} else {
-		spin_unlock_irqrestore(&midi->open_lock, flags);
+		spin_unlock_irq(&midi->open_lock);
 	}
 	return err;
 }
 
 static int snd_emu10k1_midi_output_close(struct snd_rawmidi_substream *substream)
 {
 	struct snd_emu10k1 *emu;
 	struct snd_emu10k1_midi *midi = (struct snd_emu10k1_midi *)substream->rmidi->private_data;
-	unsigned long flags;
 	int err = 0;
 
 	emu = midi->emu;
 	if (snd_BUG_ON(!emu))
 		return -ENXIO;
-	spin_lock_irqsave(&midi->open_lock, flags);
+	spin_lock_irq(&midi->open_lock);
 	snd_emu10k1_intr_disable(emu, midi->tx_enable);
 	midi->midi_mode &= ~EMU10K1_MIDI_MODE_OUTPUT;
 	midi->substream_output = NULL;
 	if (!(midi->midi_mode & EMU10K1_MIDI_MODE_INPUT)) {
-		spin_unlock_irqrestore(&midi->open_lock, flags);
+		spin_unlock_irq(&midi->open_lock);
 		err = snd_emu10k1_midi_cmd(emu, midi, MPU401_RESET, 0);
 	} else {
-		spin_unlock_irqrestore(&midi->open_lock, flags);
+		spin_unlock_irq(&midi->open_lock);
 	}
 	return err;
 }
@@ -256,33 +251,32 @@ static void snd_emu10k1_midi_output_trigger(struct snd_rawmidi_substream *substr
 {
 	struct snd_emu10k1 *emu;
 	struct snd_emu10k1_midi *midi = (struct snd_emu10k1_midi *)substream->rmidi->private_data;
-	unsigned long flags;
 
 	emu = midi->emu;
 	if (snd_BUG_ON(!emu))
 		return;
 
 	if (up) {
 		int max = 4;
 		unsigned char byte;
 	
 		/* try to send some amount of bytes here before interrupts */
-		spin_lock_irqsave(&midi->output_lock, flags);
+		spin_lock_irq(&midi->output_lock);
 		while (max > 0) {
 			if (mpu401_output_ready(emu, midi)) {
 				if (!(midi->midi_mode & EMU10K1_MIDI_MODE_OUTPUT) ||
 				    snd_rawmidi_transmit(substream, &byte, 1) != 1) {
 					/* no more data */
-					spin_unlock_irqrestore(&midi->output_lock, flags);
+					spin_unlock_irq(&midi->output_lock);
 					return;
 				}
 				mpu401_write_data(emu, midi, byte);
 				max--;
 			} else {
 				break;
 			}
 		}
-		spin_unlock_irqrestore(&midi->output_lock, flags);
+		spin_unlock_irq(&midi->output_lock);
 		snd_emu10k1_intr_enable(emu, midi->tx_enable);
 	} else {
 		snd_emu10k1_intr_disable(emu, midi->tx_enable);
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
index 387288d623d7..8b3d1b35d6e7 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -343,19 +343,17 @@ static void snd_emu10k1_pcm_init_voices(struct snd_emu10k1 *emu,
 					unsigned int end_addr,
 					struct snd_emu10k1_pcm_mixer *mix)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&emu->reg_lock, flags);
+	spin_lock_irq(&emu->reg_lock);
 	snd_emu10k1_pcm_init_voice(emu, evoice, w_16, stereo,
 				   start_addr, end_addr,
 				   &mix->send_routing[stereo][0],
 				   &mix->send_volume[stereo][0]);
 	if (stereo)
 		snd_emu10k1_pcm_init_voice(emu, evoice + 1, w_16, true,
 					   start_addr, end_addr,
 					   &mix->send_routing[2][0],
 					   &mix->send_volume[2][0]);
-	spin_unlock_irqrestore(&emu->reg_lock, flags);
+	spin_unlock_irq(&emu->reg_lock);
 }
 
 static void snd_emu10k1_pcm_init_extra_voice(struct snd_emu10k1 *emu,
diff --git a/sound/pci/emu10k1/emuproc.c b/sound/pci/emu10k1/emuproc.c
index 7e2cc532471f..5533277e4d47 100644
--- a/sound/pci/emu10k1/emuproc.c
+++ b/sound/pci/emu10k1/emuproc.c
@@ -536,33 +536,31 @@ static unsigned int snd_ptr_read(struct snd_emu10k1 * emu,
 				 unsigned int reg,
 				 unsigned int chn)
 {
-	unsigned long flags;
 	unsigned int regptr, val;
 
 	regptr = (reg << 16) | chn;
 
-	spin_lock_irqsave(&emu->emu_lock, flags);
+	spin_lock_irq(&emu->emu_lock);
 	outl(regptr, emu->port + iobase + PTR);
 	val = inl(emu->port + iobase + DATA);
-	spin_unlock_irqrestore(&emu->emu_lock, flags);
+	spin_unlock_irq(&emu->emu_lock);
 	return val;
 }
 
 static void snd_ptr_write(struct snd_emu10k1 *emu,
 			  unsigned int iobase,
 			  unsigned int reg,
 			  unsigned int chn,
 			  unsigned int data)
 {
 	unsigned int regptr;
-	unsigned long flags;
 
 	regptr = (reg << 16) | chn;
 
-	spin_lock_irqsave(&emu->emu_lock, flags);
+	spin_lock_irq(&emu->emu_lock);
 	outl(regptr, emu->port + iobase + PTR);
 	outl(data, emu->port + iobase + DATA);
-	spin_unlock_irqrestore(&emu->emu_lock, flags);
+	spin_unlock_irq(&emu->emu_lock);
 }
 
 
-- 
2.40.0.152.g15d061e6df

Re: [PATCH 2/3] ALSA: emu10k1: remove superfluous IRQ enable state saving

[Takashi Iwai]

On Wed, 12 Jul 2023 16:57:49 +0200,
Oswald Buddenhagen wrote:
> 
> The mixer, PCM prepare, MIDI, synth driver, and procfs callbacks are all
> always invoked with IRQs enabled, so there is no point in saving the
> state.
> 
> snd_emu1010_load_firmware_entry() is called from emu1010_firmware_work()
> and snd_emu10k1_emu1010_init(); the latter from snd_emu10k1_create() and
> snd_emu10k1_resume(), all of which have IRQs enabled.
> 
> The voice and memory functions are called from mixed contexts, so they
> keep the state saving.
> 
> The low-level functions all keep the state saving, because it's not
> feasible to keep track of what is called where.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

Wouldn't it make more sense if you replace it with a mutex?
It'll become more obvious that it's only for non-IRQ context, too.


thanks,

Takashi

Re: [PATCH 2/3] ALSA: emu10k1: remove superfluous IRQ enable state saving

[Oswald Buddenhagen]

On Thu, Jul 13, 2023 at 07:55:31AM +0200, Takashi Iwai wrote:
>On Wed, 12 Jul 2023 16:57:49 +0200,
>Oswald Buddenhagen wrote:
>> 
>> The mixer, PCM prepare, MIDI, synth driver, and procfs callbacks are all
>> always invoked with IRQs enabled, so there is no point in saving the
>> state.
>> 
>> snd_emu1010_load_firmware_entry() is called from emu1010_firmware_work()
>> and snd_emu10k1_emu1010_init(); the latter from snd_emu10k1_create() and
>> snd_emu10k1_resume(), all of which have IRQs enabled.
>> 
>> The voice and memory functions are called from mixed contexts, so they
>> keep the state saving.
>> 
>> The low-level functions all keep the state saving, because it's not
>> feasible to keep track of what is called where.
>> 
>Wouldn't it make more sense if you replace it with a mutex?
>It'll become more obvious that it's only for non-IRQ context, too.
>
huh?
at least some of the ~six different locks touched by this patch 
absolutely _are_ used in irq context. this patch is concerned only about 
the specific call sites, where we know that local irqs are enabled, so 
we can unconditionally re-enable them rather than restoring the old 
state (the latter being a much more expensive operation). the code 
already contains precedents for this, and the complementary optimization 
of not disabling/restoring irqs where we know that they are already 
disabled.

the reg_lock would be convertible to a mixer_mutex in most mixer 
callbacks, but that is an orthogonal question, which is raised in the 
next commit.

regards

Re: [PATCH 2/3] ALSA: emu10k1: remove superfluous IRQ enable state saving

[Takashi Iwai]

On Thu, 13 Jul 2023 10:15:21 +0200,
Oswald Buddenhagen wrote:
> 
> On Thu, Jul 13, 2023 at 07:55:31AM +0200, Takashi Iwai wrote:
> > On Wed, 12 Jul 2023 16:57:49 +0200,
> > Oswald Buddenhagen wrote:
> >> 
> >> The mixer, PCM prepare, MIDI, synth driver, and procfs callbacks are all
> >> always invoked with IRQs enabled, so there is no point in saving the
> >> state.
> >> 
> >> snd_emu1010_load_firmware_entry() is called from emu1010_firmware_work()
> >> and snd_emu10k1_emu1010_init(); the latter from snd_emu10k1_create() and
> >> snd_emu10k1_resume(), all of which have IRQs enabled.
> >> 
> >> The voice and memory functions are called from mixed contexts, so they
> >> keep the state saving.
> >> 
> >> The low-level functions all keep the state saving, because it's not
> >> feasible to keep track of what is called where.
> >> 
> > Wouldn't it make more sense if you replace it with a mutex?
> > It'll become more obvious that it's only for non-IRQ context, too.
> > 
> huh?
> at least some of the ~six different locks touched by this patch
> absolutely _are_ used in irq context. this patch is concerned only
> about the specific call sites, where we know that local irqs are
> enabled, so we can unconditionally re-enable them rather than
> restoring the old state (the latter being a much more expensive
> operation). the code already contains precedents for this, and the
> complementary optimization of not disabling/restoring irqs where we
> know that they are already disabled.
> 
> the reg_lock would be convertible to a mixer_mutex in most mixer
> callbacks, but that is an orthogonal question, which is raised in the
> next commit.

Ah, sorry, I misread as if it were dropping the whole *_irq.
Then the patch should be fine.


Takashi

Re: [PATCH 2/3] ALSA: emu10k1: remove superfluous IRQ enable state saving

[Takashi Iwai]

On Wed, 12 Jul 2023 16:57:49 +0200,
Oswald Buddenhagen wrote:
> 
> The mixer, PCM prepare, MIDI, synth driver, and procfs callbacks are all
> always invoked with IRQs enabled, so there is no point in saving the
> state.
> 
> snd_emu1010_load_firmware_entry() is called from emu1010_firmware_work()
> and snd_emu10k1_emu1010_init(); the latter from snd_emu10k1_create() and
> snd_emu10k1_resume(), all of which have IRQs enabled.
> 
> The voice and memory functions are called from mixed contexts, so they
> keep the state saving.
> 
> The low-level functions all keep the state saving, because it's not
> feasible to keep track of what is called where.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

Applied now.  Thanks.


Takashi

[PATCH 3/3] ALSA: emu10k1: (re-)add mixer locking

[Oswald Buddenhagen]

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);
-- 
2.40.0.152.g15d061e6df

Re: [PATCH 3/3] ALSA: emu10k1: (re-)add mixer locking

[Takashi Iwai]

On Wed, 12 Jul 2023 16:57:50 +0200,
Oswald Buddenhagen wrote:
> 
> 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.

I applied this patch now.  We may optimize out the whole locking
conditionally for the known-good drivers, instead of relying on a
(hackish) big iron lock that wasn't considered to be used originally
at all.


thanks,

Takashi

Re: [PATCH 3/3] ALSA: emu10k1: (re-)add mixer locking

[Oswald Buddenhagen]

On Thu, Jul 13, 2023 at 10:33:26AM +0200, Takashi Iwai wrote:
>We may optimize out the whole locking conditionally for the known-good 
>drivers,
>
well, that would work, though obviously someone would still have to 
check the drivers and set the flag. also, this kind of technically 
unnecessary fragmentation doesn't really help.

>instead of relying on a (hackish) big iron lock that wasn't considered 
>to be used originally at all.
>
i think you're focusing on the wrong thing here.
the fact that the lock was originally meant to do something else is 
meaningless. you could just as well create a dedicated lock specifically 
for that task - the important thing is that the core would provide a 
guarantee to the drivers that mixer callbacks are locked, just like it 
does for some pcm callbacks unless the driver opts out. given that mixer 
operations are rare in the big picture, fine-grained locking in the 
drivers is unnecessary (except where not mixer-only data is accessed).  
given the amount of code this saves, this seems like a rather worthwhile 
trade-off with the formal cleanness of drivers having self-contained 
locking.

regards

Re: [PATCH 3/3] ALSA: emu10k1: (re-)add mixer locking

[Takashi Iwai]

On Thu, 13 Jul 2023 11:07:12 +0200,
Oswald Buddenhagen wrote:
> 
> On Thu, Jul 13, 2023 at 10:33:26AM +0200, Takashi Iwai wrote:
> > We may optimize out the whole locking conditionally for the
> > known-good drivers,
> > 
> well, that would work, though obviously someone would still have to
> check the drivers and set the flag. also, this kind of technically
> unnecessary fragmentation doesn't really help.
> 
> > instead of relying on a (hackish) big iron lock that wasn't
> > considered to be used originally at all.
> > 
> i think you're focusing on the wrong thing here.
> the fact that the lock was originally meant to do something else is
> meaningless. you could just as well create a dedicated lock
> specifically for that task - the important thing is that the core
> would provide a guarantee to the drivers that mixer callbacks are
> locked, just like it does for some pcm callbacks unless the driver
> opts out. given that mixer operations are rare in the big picture,
> fine-grained locking in the drivers is unnecessary (except where not
> mixer-only data is accessed).  given the amount of code this saves,
> this seems like a rather worthwhile trade-off with the formal
> cleanness of drivers having self-contained locking.

My whole point is that no driver should touch card->controls_rwsem
from outside (unless the driver needs to traverse the card's linked
list by some special reasons).  It's not for protecting the driver's
own content.  It's used casually now for get/put, but it should be
seen only for protecting the list.

Unlike PCM, the control get/put has never been considered to be fully
protected, and it was always driver's responsibility.


Takashi

Re: [PATCH 3/3] ALSA: emu10k1: (re-)add mixer locking

[Oswald Buddenhagen]

On Thu, Jul 13, 2023 at 11:21:38AM +0200, Takashi Iwai wrote:
>On Thu, 13 Jul 2023 11:07:12 +0200,
>Oswald Buddenhagen wrote:
>> 
>> On Thu, Jul 13, 2023 at 10:33:26AM +0200, Takashi Iwai wrote:
>> > instead of relying on a (hackish) big iron lock that wasn't
>> > considered to be used originally at all.
>> > 
>> i think you're focusing on the wrong thing here.
>> the fact that the lock was originally meant to do something else is
>> meaningless. you could just as well create a dedicated lock
>> specifically for that task - the important thing is that the core
>> would provide a guarantee to the drivers that mixer callbacks are
>> locked, just like it does for some pcm callbacks unless the driver
>> opts out. given that mixer operations are rare in the big picture,
>> fine-grained locking in the drivers is unnecessary (except where not
>> mixer-only data is accessed).  given the amount of code this saves,
>> this seems like a rather worthwhile trade-off with the formal
>> cleanness of drivers having self-contained locking.
>
>My whole point is that no driver should touch card->controls_rwsem
>from outside (unless the driver needs to traverse the card's linked
>list by some special reasons).
>
nothing in what i wrote even suggests that it _should_. how a driver 
would explicitly interact with _a_ mixer callback lock is entirely open 
so far.

>Unlike PCM, the control get/put has never been considered to be fully
>protected,
>
the whole argument is that it _should_.

>and it was always driver's responsibility.
>
clearly a responsibility that has been widely shirked, even before it 
was actually safe to do so. the pragmatic thing to do would be accepting 
this reality and ensuring locking by the core, in whichever way.

regards

Re: [PATCH 3/3] ALSA: emu10k1: (re-)add mixer locking

[Takashi Iwai]

On Thu, 13 Jul 2023 12:01:43 +0200,
Oswald Buddenhagen wrote:
> 
> On Thu, Jul 13, 2023 at 11:21:38AM +0200, Takashi Iwai wrote:
> > On Thu, 13 Jul 2023 11:07:12 +0200,
> > Oswald Buddenhagen wrote:
> >> 
> >> On Thu, Jul 13, 2023 at 10:33:26AM +0200, Takashi Iwai wrote:
> >> > instead of relying on a (hackish) big iron lock that wasn't
> >> > considered to be used originally at all.
> >> > i think you're focusing on the wrong thing here.
> >> the fact that the lock was originally meant to do something else is
> >> meaningless. you could just as well create a dedicated lock
> >> specifically for that task - the important thing is that the core
> >> would provide a guarantee to the drivers that mixer callbacks are
> >> locked, just like it does for some pcm callbacks unless the driver
> >> opts out. given that mixer operations are rare in the big picture,
> >> fine-grained locking in the drivers is unnecessary (except where not
> >> mixer-only data is accessed).  given the amount of code this saves,
> >> this seems like a rather worthwhile trade-off with the formal
> >> cleanness of drivers having self-contained locking.
> > 
> > My whole point is that no driver should touch card->controls_rwsem
> > from outside (unless the driver needs to traverse the card's linked
> > list by some special reasons).
> > 
> nothing in what i wrote even suggests that it _should_. how a driver
> would explicitly interact with _a_ mixer callback lock is entirely
> open so far.
> 
> > Unlike PCM, the control get/put has never been considered to be fully
> > protected,
> > 
> the whole argument is that it _should_.
> 
> > and it was always driver's responsibility.
> > 
> clearly a responsibility that has been widely shirked, even before it
> was actually safe to do so. the pragmatic thing to do would be
> accepting this reality and ensuring locking by the core, in whichever
> way.

Well, I took your patch 3 just because you wanted to have a protection
of your data from both get/put callback and from another code path in
another patch.  It was an (ab)use of controls->rwsem that couldn't be
accepted, so the patch 3 was taken as an alternative.

If this isn't the scenario, let me know: I'd rather drop the patch
again, as it's superfluous.

Again, my point is that you shouldn't use controls_rwsem for the
driver's data protection purpose.

There's many rooms for improvements in ALSA core, and things may
change.  So, if the driver needs to protect its own data from both
mixer code path and from another, use the own lock, instead of
touching controls_rwsem (which is basically an internal stuff for
ALSA control core).


Takashi

Re: [PATCH 3/3] ALSA: emu10k1: (re-)add mixer locking

[Oswald Buddenhagen]

On Thu, Jul 13, 2023 at 12:24:28PM +0200, Takashi Iwai wrote:
>Well, I took your patch 3 just because you wanted to have a protection
>of your data from both get/put callback and from another code path in
>another patch.  It was an (ab)use of controls->rwsem that couldn't be
>accepted, so the patch 3 was taken as an alternative.
>
>If this isn't the scenario, let me know: I'd rather drop the patch
>again, as it's superfluous.
>
ok, then please drop it.

you're quite right that this is the scenario, but i can do that 
selectively just as well, like it already is the case in some other 
callbacks. i just wanted to have this patch first if it was to be 
applied at all, because it seemed cleaner.

regards

Re: [PATCH 3/3] ALSA: emu10k1: (re-)add mixer locking

[Takashi Iwai]

On Thu, 13 Jul 2023 12:54:11 +0200,
Oswald Buddenhagen wrote:
> 
> On Thu, Jul 13, 2023 at 12:24:28PM +0200, Takashi Iwai wrote:
> > Well, I took your patch 3 just because you wanted to have a protection
> > of your data from both get/put callback and from another code path in
> > another patch.  It was an (ab)use of controls->rwsem that couldn't be
> > accepted, so the patch 3 was taken as an alternative.
> > 
> > If this isn't the scenario, let me know: I'd rather drop the patch
> > again, as it's superfluous.
> > 
> ok, then please drop it.
> 
> you're quite right that this is the scenario, but i can do that
> selectively just as well, like it already is the case in some other
> callbacks. i just wanted to have this patch first if it was to be
> applied at all, because it seemed cleaner.

OK, dropped now.


thanks,

Takashi

Re: [PATCH 1/3] ALSA: emu10k1: fix return value of snd_emu1010_adc_pads_put()

[Takashi Iwai]

On Wed, 12 Jul 2023 16:57:48 +0200,
Oswald Buddenhagen wrote:
> 
> It returned zero even if the value had changed.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

Applied this one.  Thanks.


Takashi