diff mbox series

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

Message ID 20230712145750.125086-3-oswald.buddenhagen@gmx.de (mailing list archive)
State New, archived
Headers show
Series [1/3] ALSA: emu10k1: fix return value of snd_emu1010_adc_pads_put() | expand

Commit Message

Oswald Buddenhagen July 12, 2023, 2:57 p.m. UTC 
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(-)

Comments

Takashi Iwai July 13, 2023, 8:33 a.m. UTC | #1 
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
Oswald Buddenhagen July 13, 2023, 9:07 a.m. UTC | #2 
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
Takashi Iwai July 13, 2023, 9:21 a.m. UTC | #3 
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
Oswald Buddenhagen July 13, 2023, 10:01 a.m. UTC | #4 
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
Takashi Iwai July 13, 2023, 10:24 a.m. UTC | #5 
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
Oswald Buddenhagen July 13, 2023, 10:54 a.m. UTC | #6 
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
Takashi Iwai July 13, 2023, 11:56 a.m. UTC | #7 
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
diff mbox series

Patch

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