[PATCH 1/2] ALSA: emu10k1: make E-MU dock monitoring interrupt-driven

[PATCH 1/2] ALSA: emu10k1: make E-MU dock monitoring interrupt-driven

[Oswald Buddenhagen]

... instead of using a one-second polling timer.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/sound/emu10k1.h          |  4 +--
 sound/pci/emu10k1/emu10k1.c      |  8 +----
 sound/pci/emu10k1/emu10k1_main.c | 51 ++++++++++++++++++--------------
 sound/pci/emu10k1/io.c           |  2 ++
 sound/pci/emu10k1/irq.c          |  7 +++++
 5 files changed, 40 insertions(+), 32 deletions(-)

diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h
index 386a5f3be3e0..43c097952c3c 100644
--- a/include/sound/emu10k1.h
+++ b/include/sound/emu10k1.h
@@ -1678,8 +1678,7 @@ struct snd_emu1010 {
 	unsigned int clock_fallback;
 	unsigned int optical_in; /* 0:SPDIF, 1:ADAT */
 	unsigned int optical_out; /* 0:SPDIF, 1:ADAT */
-	struct delayed_work firmware_work;
-	u32 last_reg;
+	struct work_struct firmware_work;
 };
 
 struct snd_emu10k1 {
@@ -1761,6 +1760,7 @@ struct snd_emu10k1 {
 	void (*capture_efx_interrupt)(struct snd_emu10k1 *emu, unsigned int status);
 	void (*spdif_interrupt)(struct snd_emu10k1 *emu, unsigned int status);
 	void (*dsp_interrupt)(struct snd_emu10k1 *emu);
+	void (*gpio_interrupt)(struct snd_emu10k1 *emu);
 	void (*p16v_interrupt)(struct snd_emu10k1 *emu);
 
 	struct snd_pcm_substream *pcm_capture_substream;
diff --git a/sound/pci/emu10k1/emu10k1.c b/sound/pci/emu10k1/emu10k1.c
index 23adace1b969..1a13c086e86c 100644
--- a/sound/pci/emu10k1/emu10k1.c
+++ b/sound/pci/emu10k1/emu10k1.c
@@ -176,9 +176,6 @@ static int snd_card_emu10k1_probe(struct pci_dev *pci,
 	if (err < 0)
 		return err;
 
-	if (emu->card_capabilities->emu_model)
-		schedule_delayed_work(&emu->emu1010.firmware_work, 0);
-
 	pci_set_drvdata(pci, card);
 	dev++;
 	return 0;
@@ -194,7 +191,7 @@ static int snd_emu10k1_suspend(struct device *dev)
 
 	emu->suspend = 1;
 
-	cancel_delayed_work_sync(&emu->emu1010.firmware_work);
+	cancel_work_sync(&emu->emu1010.firmware_work);
 
 	snd_ac97_suspend(emu->ac97);
 
@@ -224,9 +221,6 @@ static int snd_emu10k1_resume(struct device *dev)
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
 
-	if (emu->card_capabilities->emu_model)
-		schedule_delayed_work(&emu->emu1010.firmware_work, 0);
-
 	return 0;
 }
 
diff --git a/sound/pci/emu10k1/emu10k1_main.c b/sound/pci/emu10k1/emu10k1_main.c
index 58ed72de6403..661164dbf547 100644
--- a/sound/pci/emu10k1/emu10k1_main.c
+++ b/sound/pci/emu10k1/emu10k1_main.c
@@ -391,7 +391,10 @@ static void snd_emu10k1_audio_enable(struct snd_emu10k1 *emu)
 	}
 #endif
 
-	snd_emu10k1_intr_enable(emu, INTE_PCIERRORENABLE);
+	if (emu->card_capabilities->emu_model)
+		snd_emu10k1_intr_enable(emu, INTE_PCIERRORENABLE | INTE_A_GPIOENABLE);
+	else
+		snd_emu10k1_intr_enable(emu, INTE_PCIERRORENABLE);
 }
 
 int snd_emu10k1_done(struct snd_emu10k1 *emu)
@@ -745,63 +748,61 @@ static void emu1010_firmware_work(struct work_struct *work)
 	int err;
 
 	emu = container_of(work, struct snd_emu10k1,
-			   emu1010.firmware_work.work);
+			   emu1010.firmware_work);
 	if (emu->card->shutdown)
 		return;
 #ifdef CONFIG_PM_SLEEP
 	if (emu->suspend)
 		return;
 #endif
-	snd_emu1010_fpga_read(emu, EMU_HANA_IRQ_STATUS, &tmp); /* IRQ Status */
 	snd_emu1010_fpga_read(emu, EMU_HANA_OPTION_CARDS, &reg); /* OPTIONS: Which cards are attached to the EMU */
 	if (reg & EMU_HANA_OPTION_DOCK_OFFLINE) {
 		/* Audio Dock attached */
 		/* Return to Audio Dock programming mode */
 		dev_info(emu->card->dev,
 			 "emu1010: Loading Audio Dock Firmware\n");
 		snd_emu1010_fpga_write(emu, EMU_HANA_FPGA_CONFIG,
 				       EMU_HANA_FPGA_CONFIG_AUDIODOCK);
 		err = snd_emu1010_load_firmware(emu, 1, &emu->dock_fw);
 		if (err < 0)
-			goto next;
-
+			return;
 		snd_emu1010_fpga_write(emu, EMU_HANA_FPGA_CONFIG, 0);
-		snd_emu1010_fpga_read(emu, EMU_HANA_IRQ_STATUS, &tmp);
-		dev_info(emu->card->dev,
-			 "emu1010: EMU_HANA+DOCK_IRQ_STATUS = 0x%x\n", tmp);
-		/* ID, should read & 0x7f = 0x55 when FPGA programmed. */
 		snd_emu1010_fpga_read(emu, EMU_HANA_ID, &tmp);
 		dev_info(emu->card->dev,
 			 "emu1010: EMU_HANA+DOCK_ID = 0x%x\n", tmp);
 		if ((tmp & 0x1f) != 0x15) {
 			/* FPGA failed to be programmed */
 			dev_info(emu->card->dev,
 				 "emu1010: Loading Audio Dock Firmware file failed, reg = 0x%x\n",
 				 tmp);
-			goto next;
+			return;
 		}
 		dev_info(emu->card->dev,
 			 "emu1010: Audio Dock Firmware loaded\n");
 		snd_emu1010_fpga_read(emu, EMU_DOCK_MAJOR_REV, &tmp);
 		snd_emu1010_fpga_read(emu, EMU_DOCK_MINOR_REV, &tmp2);
 		dev_info(emu->card->dev, "Audio Dock ver: %u.%u\n", tmp, tmp2);
 		/* Sync clocking between 1010 and Dock */
 		/* Allow DLL to settle */
 		msleep(10);
 		/* Unmute all. Default is muted after a firmware load */
 		snd_emu1010_fpga_write(emu, EMU_HANA_UNMUTE, EMU_UNMUTE);
-	} else if (!reg && emu->emu1010.last_reg) {
+	}
+}
+
+static void emu1010_interrupt(struct snd_emu10k1 *emu)
+{
+	u32 sts;
+
+	snd_emu1010_fpga_read(emu, EMU_HANA_IRQ_STATUS, &sts);
+	if (sts & EMU_HANA_IRQ_DOCK_LOST) {
 		/* Audio Dock removed */
 		dev_info(emu->card->dev, "emu1010: Audio Dock detached\n");
 		/* The hardware auto-mutes all, so we unmute again */
 		snd_emu1010_fpga_write(emu, EMU_HANA_UNMUTE, EMU_UNMUTE);
+	} else if (sts & EMU_HANA_IRQ_DOCK) {
+		schedule_work(&emu->emu1010.firmware_work);
 	}
-
- next:
-	emu->emu1010.last_reg = reg;
-	if (!emu->card->shutdown)
-		schedule_delayed_work(&emu->emu1010.firmware_work,
-				      msecs_to_jiffies(1000));
 }
 
 /*
@@ -870,6 +871,8 @@ static int snd_emu10k1_emu1010_init(struct snd_emu10k1 *emu)
 
 	snd_emu1010_fpga_read(emu, EMU_HANA_OPTION_CARDS, &reg);
 	dev_info(emu->card->dev, "emu1010: Card options = 0x%x\n", reg);
+	if (reg & EMU_HANA_OPTION_DOCK_OFFLINE)
+		schedule_work(&emu->emu1010.firmware_work);
 	if (emu->card_capabilities->no_adat) {
 		emu->emu1010.optical_in = 0; /* IN_SPDIF */
 		emu->emu1010.optical_out = 0; /* OUT_SPDIF */
@@ -895,10 +898,12 @@ static int snd_emu10k1_emu1010_init(struct snd_emu10k1 *emu)
 	/* MIDI routing */
 	snd_emu1010_fpga_write(emu, EMU_HANA_MIDI_IN, EMU_HANA_MIDI_INA_FROM_HAMOA | EMU_HANA_MIDI_INB_FROM_DOCK2);
 	snd_emu1010_fpga_write(emu, EMU_HANA_MIDI_OUT, EMU_HANA_MIDI_OUT_DOCK2 | EMU_HANA_MIDI_OUT_SYNC2);
-	/* IRQ Enable: All on */
-	/* snd_emu1010_fpga_write(emu, EMU_HANA_IRQ_ENABLE, 0x0f); */
-	/* IRQ Enable: All off */
-	snd_emu1010_fpga_write(emu, EMU_HANA_IRQ_ENABLE, 0x00);
+
+	emu->gpio_interrupt = emu1010_interrupt;
+	// Note: The Audigy INTE is set later
+	snd_emu1010_fpga_write(emu, EMU_HANA_IRQ_ENABLE,
+			       EMU_HANA_IRQ_DOCK | EMU_HANA_IRQ_DOCK_LOST);
+	snd_emu1010_fpga_read(emu, EMU_HANA_IRQ_STATUS, &reg);  // Clear pending IRQs
 
 	emu->emu1010.clock_source = 1;  /* 48000 */
 	emu->emu1010.clock_fallback = 1;  /* 48000 */
@@ -938,7 +943,7 @@ static void snd_emu10k1_free(struct snd_card *card)
 		/* Disable 48Volt power to Audio Dock */
 		snd_emu1010_fpga_write(emu, EMU_HANA_DOCK_PWR, 0);
 	}
-	cancel_delayed_work_sync(&emu->emu1010.firmware_work);
+	cancel_work_sync(&emu->emu1010.firmware_work);
 	release_firmware(emu->firmware);
 	release_firmware(emu->dock_fw);
 	snd_util_memhdr_free(emu->memhdr);
@@ -1517,7 +1522,7 @@ int snd_emu10k1_create(struct snd_card *card,
 	emu->irq = -1;
 	emu->synth = NULL;
 	emu->get_synth_voice = NULL;
-	INIT_DELAYED_WORK(&emu->emu1010.firmware_work, emu1010_firmware_work);
+	INIT_WORK(&emu->emu1010.firmware_work, emu1010_firmware_work);
 	/* read revision & serial */
 	emu->revision = pci->revision;
 	pci_read_config_dword(pci, PCI_SUBSYSTEM_VENDOR_ID, &emu->serial);
diff --git a/sound/pci/emu10k1/io.c b/sound/pci/emu10k1/io.c
index a0d66ce3ee83..c695cb863e5e 100644
--- a/sound/pci/emu10k1/io.c
+++ b/sound/pci/emu10k1/io.c
@@ -302,6 +302,8 @@ static void snd_emu1010_fpga_read_locked(struct snd_emu10k1 *emu, u32 reg, u32 *
 {
 	// The higest input pin is used as the designated interrupt trigger,
 	// so it needs to be masked out.
+	// But note that any other input pin change will also cause an IRQ,
+	// so using this function often causes an IRQ as a side effect.
 	u32 mask = emu->card_capabilities->ca0108_chip ? 0x1f : 0x7f;
 	if (snd_BUG_ON(reg > 0x3f))
 		return;
diff --git a/sound/pci/emu10k1/irq.c b/sound/pci/emu10k1/irq.c
index a813ef8c2f8d..8573248dd799 100644
--- a/sound/pci/emu10k1/irq.c
+++ b/sound/pci/emu10k1/irq.c
@@ -149,6 +149,13 @@ irqreturn_t snd_emu10k1_interrupt(int irq, void *dev_id)
 				outl(0, emu->port + INTE2);
 			status &= ~IPR_P16V;
 		}
+		if (status & IPR_A_GPIO) {
+			if (emu->gpio_interrupt)
+				emu->gpio_interrupt(emu);
+			else
+				snd_emu10k1_intr_disable(emu, INTE_A_GPIOENABLE);
+			status &= ~IPR_A_GPIO;
+		}
 
 		if (status) {
 			dev_err(emu->card->dev,
-- 
2.40.0.152.g15d061e6df

[PATCH 2/2] ALSA: emu10k1: track loss of external clock on E-MU cards

[Oswald Buddenhagen]

This uses IRQs to track spontaneous changes to the word clock source
register.

FWIW, that this can happen in the first place is the reason why it is
futile to lock the clock source mixer setting while the device is open -
we can't consistently control the rate anyway. Though arguably, we
should reset any open streams when that happens, as they become
corrupted anyway.

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

---

FIXME? while i'm not sure, i think this won't notice seamless switches
between 44.1 and 48 kHz. assuming that's the case, this seems like a
minor issue: firstly, just about nothing actually produces such a
seamless switch - my only device that can even do that is the e-mu card
itself, but the driver disrupts that by temporarily muting the output.
secondly, the user is unlikely to select an external source before
setting it up properly. and the easy workaround is actually never doing
that.
regardless, to actually test that i'd need a second e-mu card.
---
 include/sound/emu10k1.h          |  5 +++++
 sound/pci/emu10k1/emu10k1.c      |  1 +
 sound/pci/emu10k1/emu10k1_main.c | 32 +++++++++++++++++++++++++++++++-
 sound/pci/emu10k1/emumixer.c     |  4 ++--
 4 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h
index 43c097952c3c..7c55a8244747 100644
--- a/include/sound/emu10k1.h
+++ b/include/sound/emu10k1.h
@@ -992,6 +992,9 @@ SUB_REG_NC(A_EHC, A_I2S_CAPTURE_RATE, 0x00000e00)  /* This sets the capture PCM
 #define EMU_HANA_WCLOCK_4X		0x10
 #define EMU_HANA_WCLOCK_MULT_RESERVED	0x18
 
+// If the selected external clock source is/becomes invalid or incompatible
+// with the clock multiplier, the clock source is reset to this value, and
+// a WCLK_CHANGED interrupt is raised.
 #define EMU_HANA_DEFCLOCK	0x06	/* 000000x  1 bits Default Word Clock  */
 #define EMU_HANA_DEFCLOCK_48K		0x00
 #define EMU_HANA_DEFCLOCK_44_1K		0x01
@@ -1679,6 +1682,7 @@ struct snd_emu1010 {
 	unsigned int optical_in; /* 0:SPDIF, 1:ADAT */
 	unsigned int optical_out; /* 0:SPDIF, 1:ADAT */
 	struct work_struct firmware_work;
+	struct work_struct clock_work;
 };
 
 struct snd_emu10k1 {
@@ -1753,6 +1757,7 @@ struct snd_emu10k1 {
 	struct snd_kcontrol *ctl_efx_send_routing;
 	struct snd_kcontrol *ctl_efx_send_volume;
 	struct snd_kcontrol *ctl_efx_attn;
+	struct snd_kcontrol *ctl_clock_source;
 
 	void (*hwvol_interrupt)(struct snd_emu10k1 *emu, unsigned int status);
 	void (*capture_interrupt)(struct snd_emu10k1 *emu, unsigned int status);
diff --git a/sound/pci/emu10k1/emu10k1.c b/sound/pci/emu10k1/emu10k1.c
index 1a13c086e86c..421053569aa0 100644
--- a/sound/pci/emu10k1/emu10k1.c
+++ b/sound/pci/emu10k1/emu10k1.c
@@ -192,6 +192,7 @@ static int snd_emu10k1_suspend(struct device *dev)
 	emu->suspend = 1;
 
 	cancel_work_sync(&emu->emu1010.firmware_work);
+	cancel_work_sync(&emu->emu1010.clock_work);
 
 	snd_ac97_suspend(emu->ac97);
 
diff --git a/sound/pci/emu10k1/emu10k1_main.c b/sound/pci/emu10k1/emu10k1_main.c
index 661164dbf547..7c114fe31831 100644
--- a/sound/pci/emu10k1/emu10k1_main.c
+++ b/sound/pci/emu10k1/emu10k1_main.c
@@ -790,6 +790,32 @@ static void emu1010_firmware_work(struct work_struct *work)
 	}
 }
 
+static void emu1010_clock_work(struct work_struct *work)
+{
+	struct snd_emu10k1 *emu;
+	struct snd_ctl_elem_id id;
+
+	emu = container_of(work, struct snd_emu10k1,
+			   emu1010.clock_work);
+	if (emu->card->shutdown)
+		return;
+#ifdef CONFIG_PM_SLEEP
+	if (emu->suspend)
+		return;
+#endif
+
+	// We consider this a mixer setting, so use the mixer's lock
+	down_write(&emu->card->controls_rwsem);
+	// This is the only thing that can actually happen.
+	emu->emu1010.clock_source = emu->emu1010.clock_fallback;
+	emu->emu1010.wclock = 1 - emu->emu1010.clock_source;
+	snd_emu1010_update_clock(emu);
+	downgrade_write(&emu->card->controls_rwsem);
+	snd_ctl_build_ioff(&id, emu->ctl_clock_source, 0);
+	snd_ctl_notify(emu->card, SNDRV_CTL_EVENT_MASK_VALUE, &id);
+	up_read(&emu->card->controls_rwsem);
+}
+
 static void emu1010_interrupt(struct snd_emu10k1 *emu)
 {
 	u32 sts;
@@ -803,6 +829,8 @@ static void emu1010_interrupt(struct snd_emu10k1 *emu)
 	} else if (sts & EMU_HANA_IRQ_DOCK) {
 		schedule_work(&emu->emu1010.firmware_work);
 	}
+	if (sts & EMU_HANA_IRQ_WCLK_CHANGED)
+		schedule_work(&emu->emu1010.clock_work);
 }
 
 /*
@@ -902,7 +930,7 @@ static int snd_emu10k1_emu1010_init(struct snd_emu10k1 *emu)
 	emu->gpio_interrupt = emu1010_interrupt;
 	// Note: The Audigy INTE is set later
 	snd_emu1010_fpga_write(emu, EMU_HANA_IRQ_ENABLE,
-			       EMU_HANA_IRQ_DOCK | EMU_HANA_IRQ_DOCK_LOST);
+			       EMU_HANA_IRQ_DOCK | EMU_HANA_IRQ_DOCK_LOST | EMU_HANA_IRQ_WCLK_CHANGED);
 	snd_emu1010_fpga_read(emu, EMU_HANA_IRQ_STATUS, &reg);  // Clear pending IRQs
 
 	emu->emu1010.clock_source = 1;  /* 48000 */
@@ -944,6 +972,7 @@ static void snd_emu10k1_free(struct snd_card *card)
 		snd_emu1010_fpga_write(emu, EMU_HANA_DOCK_PWR, 0);
 	}
 	cancel_work_sync(&emu->emu1010.firmware_work);
+	cancel_work_sync(&emu->emu1010.clock_work);
 	release_firmware(emu->firmware);
 	release_firmware(emu->dock_fw);
 	snd_util_memhdr_free(emu->memhdr);
@@ -1523,6 +1552,7 @@ int snd_emu10k1_create(struct snd_card *card,
 	emu->synth = NULL;
 	emu->get_synth_voice = NULL;
 	INIT_WORK(&emu->emu1010.firmware_work, emu1010_firmware_work);
+	INIT_WORK(&emu->emu1010.clock_work, emu1010_clock_work);
 	/* read revision & serial */
 	emu->revision = pci->revision;
 	pci_read_config_dword(pci, PCI_SUBSYSTEM_VENDOR_ID, &emu->serial);
diff --git a/sound/pci/emu10k1/emumixer.c b/sound/pci/emu10k1/emumixer.c
index f9500cd50a4b..a9302db535d9 100644
--- a/sound/pci/emu10k1/emumixer.c
+++ b/sound/pci/emu10k1/emumixer.c
@@ -2342,8 +2342,8 @@ int snd_emu10k1_mixer(struct snd_emu10k1 *emu,
 				emu1010_map_source(emu_ri, emu_ri->out_dflts[i]);
 		snd_emu1010_apply_sources(emu);
 
-		err = snd_ctl_add(card,
-			snd_ctl_new1(&snd_emu1010_clock_source, emu));
+		kctl = emu->ctl_clock_source = snd_ctl_new1(&snd_emu1010_clock_source, emu);
+		err = snd_ctl_add(card, kctl);
 		if (err < 0)
 			return err;
 		err = snd_ctl_add(card,
-- 
2.40.0.152.g15d061e6df

Re: [PATCH 2/2] ALSA: emu10k1: track loss of external clock on E-MU cards

[Takashi Iwai]

On Mon, 10 Jul 2023 08:59:56 +0200,
Oswald Buddenhagen wrote:
> 
> +static void emu1010_clock_work(struct work_struct *work)
> +{
> +	struct snd_emu10k1 *emu;
> +	struct snd_ctl_elem_id id;
> +
> +	emu = container_of(work, struct snd_emu10k1,
> +			   emu1010.clock_work);
> +	if (emu->card->shutdown)
> +		return;
> +#ifdef CONFIG_PM_SLEEP
> +	if (emu->suspend)
> +		return;
> +#endif
> +
> +	// We consider this a mixer setting, so use the mixer's lock
> +	down_write(&emu->card->controls_rwsem);

I really don't want to see the driver touching this lock.  It's
basically an internal stuff of ALSA core code.  And, as already
discussed, the controls_rwsem wasn't intended as a lock for the mixer
content protection originally.  It took over the role partially, but
the drivers shouldn't rely on it.

> +	snd_emu1010_update_clock(emu);
> +	downgrade_write(&emu->card->controls_rwsem);
> +	snd_ctl_build_ioff(&id, emu->ctl_clock_source, 0);
> +	snd_ctl_notify(emu->card, SNDRV_CTL_EVENT_MASK_VALUE, &id);
> +	up_read(&emu->card->controls_rwsem);

Err, too ugly and unnecessary change.  snd_ctl_notify() doesn't take
the rwsem, and it can be called at any place without fiddling the
rwsem.  It's snd_ctl_notify_one() that needs the downgrade to read
(and maybe we should rename the function to be clearer).


thanks,

Takashi

Re: [PATCH 2/2] ALSA: emu10k1: track loss of external clock on E-MU cards

[Oswald Buddenhagen]

On Mon, Jul 10, 2023 at 05:05:06PM +0200, Takashi Iwai wrote:
>On Mon, 10 Jul 2023 08:59:56 +0200,
>Oswald Buddenhagen wrote:
>> +	// We consider this a mixer setting, so use the mixer's lock
>> +	down_write(&emu->card->controls_rwsem);
>
>I really don't want to see the driver touching this lock.  It's
>basically an internal stuff of ALSA core code.  And, as already
>discussed, the controls_rwsem wasn't intended as a lock for the mixer
>content protection originally.  It took over the role partially, but
>the drivers shouldn't rely on it.
>
i know that this is technically true, but i think that from a pragmatic 
point of view it just makes no sense.

if we are pedantic about it, we need to revert my 06405d8ee8c (emu10k1: 
remove now superfluous mixer locking) and do the opposite change, to add 
the technically missing locks. that's several tens of irq-safe spinlock 
operations in this driver alone. which are hundreds of bytes spent 
entirely pointlessly, because we _know_ that the operation is already 
locked, because it must be.

so i think the most sensible approach is to just make it official, which 
is what my 37bb927d5bb (core: update comment on snd_card.controls_rwsem) 
actually works towards. at least i can't think of a reason not to do 
that, except for some puristic ideals.

if somebody actually finds a _good_ reason to change this and wants to 
embark on the mammoth task of proving that everything is still safe 
afterwards, they can just do so. adjusting this commit for the new 
reality wouldn't be hard or laborious.

> > +     snd_emu1010_update_clock(emu);
> > +     downgrade_write(&emu->card->controls_rwsem);
> > +     snd_ctl_build_ioff(&id, emu->ctl_clock_source, 0);
> > +     snd_ctl_notify(emu->card, SNDRV_CTL_EVENT_MASK_VALUE, &id);
> > +     up_read(&emu->card->controls_rwsem);
> 
> Err, too ugly and unnecessary change.  snd_ctl_notify() doesn't take
> the rwsem, and it can be called at any place without fiddling the
> rwsem.  It's snd_ctl_notify_one() that needs the downgrade to read
> (and maybe we should rename the function to be clearer).
>
well, that lock is necessary exactly because we (ab-)use the rwsem as 
the general mixer lock, so we basically need to emulate the ioctl entry 
path, so a concurrent actual put ioctl doesn't blow up in our face. i 
actually agree that it's kinda ugly, but the argument remains the same - 
it just isn't worth doing it differently (we'd have to sprinkle around 
quite some reg_lock operations instead).

regards

Re: [PATCH 2/2] ALSA: emu10k1: track loss of external clock on E-MU cards

[Takashi Iwai]

On Mon, 10 Jul 2023 19:34:29 +0200,
Oswald Buddenhagen wrote:
> 
> On Mon, Jul 10, 2023 at 05:05:06PM +0200, Takashi Iwai wrote:
> > On Mon, 10 Jul 2023 08:59:56 +0200,
> > Oswald Buddenhagen wrote:
> >> +	// We consider this a mixer setting, so use the mixer's lock
> >> +	down_write(&emu->card->controls_rwsem);
> > 
> > I really don't want to see the driver touching this lock.  It's
> > basically an internal stuff of ALSA core code.  And, as already
> > discussed, the controls_rwsem wasn't intended as a lock for the mixer
> > content protection originally.  It took over the role partially, but
> > the drivers shouldn't rely on it.
> > 
> i know that this is technically true, but i think that from a
> pragmatic point of view it just makes no sense.
> 
> if we are pedantic about it, we need to revert my 06405d8ee8c
> (emu10k1: remove now superfluous mixer locking) and do the opposite
> change, to add the technically missing locks. that's several tens of
> irq-safe spinlock operations in this driver alone. which are hundreds
> of bytes spent entirely pointlessly, because we _know_ that the
> operation is already locked, because it must be.

Yes, I'm for reintroducing the driver sepcific lock (it can be mutex,
too).

> so i think the most sensible approach is to just make it official,
> which is what my 37bb927d5bb (core: update comment on
> snd_card.controls_rwsem) actually works towards. at least i can't
> think of a reason not to do that, except for some puristic ideals.
> 
> if somebody actually finds a _good_ reason to change this and wants to
> embark on the mammoth task of proving that everything is still safe
> afterwards, they can just do so. adjusting this commit for the new
> reality wouldn't be hard or laborious.
> 
> > > +     snd_emu1010_update_clock(emu);
> > > +     downgrade_write(&emu->card->controls_rwsem);
> > > +     snd_ctl_build_ioff(&id, emu->ctl_clock_source, 0);
> > > +     snd_ctl_notify(emu->card, SNDRV_CTL_EVENT_MASK_VALUE, &id);
> > > +     up_read(&emu->card->controls_rwsem);
> > 
> > Err, too ugly and unnecessary change.  snd_ctl_notify() doesn't take
> > the rwsem, and it can be called at any place without fiddling the
> > rwsem.  It's snd_ctl_notify_one() that needs the downgrade to read
> > (and maybe we should rename the function to be clearer).
> > 
> well, that lock is necessary exactly because we (ab-)use the rwsem as
> the general mixer lock, so we basically need to emulate the ioctl
> entry path, so a concurrent actual put ioctl doesn't blow up in our
> face. i actually agree that it's kinda ugly, but the argument remains
> the same - it just isn't worth doing it differently (we'd have to
> sprinkle around quite some reg_lock operations instead).

Again, snd_ctl_notify() itself doesn't need the rwsem lock at all.
It's snd_ctl_notify_one() that needs a more careful call pattern.

And, that ugly implementation is a thing to be improved in future in
ALSA core side.  If we have this in each driver, it'll be a
nightmare.  So, please don't follow it.


thanks,

Takashi

Re: [PATCH 2/2] ALSA: emu10k1: track loss of external clock on E-MU cards

[Oswald Buddenhagen]

On Tue, Jul 11, 2023 at 07:28:22AM +0200, Takashi Iwai wrote:
>Again, snd_ctl_notify() itself doesn't need the rwsem lock at all.
>
ah, you mean i could fully release it before the notification.

>It's snd_ctl_notify_one() that needs a more careful call pattern.
>
i suppose that's because the snd_ctl_layer callbacks might require it.
i would recommend actually documenting that.

>And, that ugly implementation is a thing to be improved in future in
>ALSA core side.
>
it is? like, really? or is it just a far-off idea with no concrete plan 
whatsoever? is there an actual problem to solve, or is it just a sense 
of "yeah, this could be nicer ... somehow"? i mean, this is the mixer -  
one would be hard-pressed to find an actual bottleneck in there.

regards

Re: [PATCH 2/2] ALSA: emu10k1: track loss of external clock on E-MU cards

[Takashi Iwai]

On Tue, 11 Jul 2023 12:11:30 +0200,
Oswald Buddenhagen wrote:
> 
> On Tue, Jul 11, 2023 at 07:28:22AM +0200, Takashi Iwai wrote:
> > Again, snd_ctl_notify() itself doesn't need the rwsem lock at all.
> > 
> ah, you mean i could fully release it before the notification.
> 
> > It's snd_ctl_notify_one() that needs a more careful call pattern.
> > 
> i suppose that's because the snd_ctl_layer callbacks might require it.
> i would recommend actually documenting that.

Yes, but this helper itself needs more change at first, I'm afraid.
The current implementation with the nested rwsem is fragile.  It's a
new stuff (or new restriction), and it's to be revisited.

> > And, that ugly implementation is a thing to be improved in future in
> > ALSA core side.
> > 
> it is? like, really?

Yes.  See my earlier RFC patch for reducing the nested rwlock, for
example.  Jaroslav didn't like the implementation, so it needs more
respin, though.

Another idea could to be make the controls_rwsem back to read-only for
both get and put, but introduce another lock just wrapping around
get/put call (but conditionally - there are drivers that don't need
it).  This will avoid the rwsem deadlock problem.


Takashi

Re: [PATCH 1/2] ALSA: emu10k1: make E-MU dock monitoring interrupt-driven

[Takashi Iwai]

On Mon, 10 Jul 2023 08:59:55 +0200,
Oswald Buddenhagen wrote:
> 
> ... instead of using a one-second polling timer.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

Applied this one.  Thanks.


Takashi