[RFC][stable 6.6] ASoC: tas2781: ABBA deadlock in tasdevice_fw_ready()

[Sergey Senozhatsky]

Not a formal patch, just RFC.

I'm seeing the following deadlock in tasdevice_fw_ready():

TaskA first takes ->codec_lock in tasdevice_fw_ready() and then
->controls_rwsem in snd_ctl_add_replace():

 down_write+0x88/0xb8
 snd_ctl_add+0x38/0xb0
 snd_soc_add_component_controls+0xe8/0x160
 tasdevice_fw_ready+0x284/0x620
 request_firmware_work_func+0x54/0xa8
 worker_thread+0x368/0x9d8
 kthread+0x108/0x1c8
 ret_from_fork+0x10/0x20

TaskB first takes ->controls_rwsem in snd_ctl_elem_read() and then
->codec_lock in tas2563_digital_gain_get():

 mutex_lock+0x50/0x80
 tas2563_digital_gain_get+0x4c/0x138
 snd_ctl_elem_read+0xf4/0x178
 snd_ctl_ioctl+0xa04/0x1430
 __arm64_sys_ioctl+0x2e4/0x548
 invoke_syscall+0x6c/0xe0
 do_el0_svc+0x68/0xe8
 el0_svc+0x34/0x88
 el0t_64_sync_handler+0x1c/0xf8
 el0t_64_sync+0x180/0x188

Which results in ABBA deadlock.

This is seen on stable 6.6, but looking at linux-next the ABBA
pattern still seem to be there (tas2563_digital_gain_get() was
renamed into tasdevice_digital_gain_get()).

So I have a patch that drops ->codec_lock before calling functions
that add ALSA controls (which internally acquire ->controls_rwsem),
and re-acquire it afterwards.  snd_ctl_add() doesn't modify tas_priv
members, at the same we are still in init (loading firmware) so the
controls that are already visible to user space don't modify tas_priv
either.  (at least it seems so).

Does it make sense? Or what should be the right way to fix this?

---

diff --git a/sound/soc/codecs/tas2781-i2c.c b/sound/soc/codecs/tas2781-i2c.c
index 41b89fcc69c3..18c8c7471301 100644
--- a/sound/soc/codecs/tas2781-i2c.c
+++ b/sound/soc/codecs/tas2781-i2c.c
@@ -1638,7 +1638,19 @@ static void tasdevice_fw_ready(const struct firmware *fmw,
 		tasdevice_config_info_remove(tas_priv);
 		goto out;
 	}
+
+	/*
+	 * Release codec_lock before adding ALSA controls to avoid ABBA
+	 * deadlock: the ALSA core acquires controls_rwsem (read) before
+	 * calling kcontrol .get/.put callbacks which then take codec_lock,
+	 * establishing the lock order controls_rwsem -> codec_lock.
+	 * Calling snd_ctl_add() (via snd_soc_add_component_controls())
+	 * under codec_lock would acquire controls_rwsem (write) under
+	 * codec_lock, inverting the lock order.
+	 */
+	mutex_unlock(&tas_priv->codec_lock);
 	tasdevice_create_control(tas_priv);
+	mutex_lock(&tas_priv->codec_lock);
 
 	tasdevice_dsp_remove(tas_priv);
 	tasdevice_calbin_remove(tas_priv);
@@ -1675,8 +1687,11 @@ static void tasdevice_fw_ready(const struct firmware *fmw,
 
 	/*
 	 * If no dsp-related kcontrol created, the dsp resource will be freed.
+	 * Release codec_lock before adding controls (see comment above).
 	 */
+	mutex_unlock(&tas_priv->codec_lock);
 	ret = tasdevice_dsp_create_ctrls(tas_priv);
+	mutex_lock(&tas_priv->codec_lock);
 	if (ret) {
 		dev_err(tas_priv->dev, "dsp controls error\n");
 		goto out;
@@ -1685,7 +1700,9 @@ static void tasdevice_fw_ready(const struct firmware *fmw,
 
 	/* There is no calibration required for TAS58XX. */
 	if (tas_priv->chip_id < TAS5802) {
+		mutex_unlock(&tas_priv->codec_lock);
 		ret = tasdevice_create_cali_ctrls(tas_priv);
+		mutex_lock(&tas_priv->codec_lock);
 		if (ret) {
 			dev_err(tas_priv->dev, "cali controls error\n");
 			goto out;