| Message ID | 20230712145750.125086-1-oswald.buddenhagen@gmx.de |
|---|---|
| State | Accepted |
| Commit | deb1200f6eb634a6e4d08ada953b72be1e8adcfa |
| Headers | show |
| Series | [1/3] ALSA: emu10k1: fix return value of snd_emu1010_adc_pads_put() | expand |
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
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
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
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
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
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 = {
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(-)