ALSA: hda/cs_dsp_ctl: Fix mutex inversion when creating controls

Redesign the creation of ALSA controls so that the cs_dsp
pwr_lock is not held when calling snd_ctl_add(). Instead of
creating the ALSA control from the cs_dsp control_add callback,
do it after cs_dsp_power_up() has completed. The existing
functions are changed to return void instead of passing errors
back - this duplicates the original behaviour, as cs_dsp does
not abort firmware load if creation of a control fails.

It is safe to walk the control list without taking any mutex
provided that the caller is not trying to load a new firmware
or remove the driver in parallel. There is no other situation
that the list can change. So the caller can trigger creation
of ALSA controls after cs_dsp_power_up() has returned. A cs_dsp
control will have a non-NULL priv pointer if we have created
an ALSA control.

With the previous code the ALSA controls were created from
the cs_dsp control_add callback. But this is called with
pwr_lock held (as it is part of the DSP power-up sequence).
The kernel lock checking will show a mutex inversion between
this and the control creation path:

control_add
  pwr_lock held, takes controls_rwsem (in snd_ctl_add)

get/put
  controls_rwsem held, takes pwr_lock to call cs_dsp.

This is not completely theoretical. Although the time window
is very small, it is possible for these to run in parallel
and deadlock the old implementation.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
Link: https://lore.kernel.org/r/20221011143552.621792-4-sbinding@opensource.cirrus.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index 3952f285370340a6476d1628b8c06ef0fa8346b5..102ac4a94a9d6e94c8b3e311ee787f8310b01fad 100644 (file)
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -91,20 +91,18 @@ static const struct reg_sequence cs35l41_hda_mute[] = {
        { CS35L41_AMP_DIG_VOL_CTRL,     0x0000A678 }, // AMP_VOL_PCM Mute
 };
 
-static int cs35l41_control_add(struct cs_dsp_coeff_ctl *cs_ctl)
+static void cs35l41_add_controls(struct cs35l41_hda *cs35l41)
 {
-       struct cs35l41_hda *cs35l41 = container_of(cs_ctl->dsp, struct cs35l41_hda, cs_dsp);
        struct hda_cs_dsp_ctl_info info;
 
        info.device_name = cs35l41->amp_name;
        info.fw_type = cs35l41->firmware_type;
        info.card = cs35l41->codec->card;
 
-       return hda_cs_dsp_control_add(cs_ctl, &info);
+       hda_cs_dsp_add_controls(&cs35l41->cs_dsp, &info);
 }
 
 static const struct cs_dsp_client_ops client_ops = {
-       .control_add = cs35l41_control_add,
        .control_remove = hda_cs_dsp_control_remove,
 };
 
@@ -435,6 +433,8 @@ static int cs35l41_init_dsp(struct cs35l41_hda *cs35l41)
        if (ret)
                goto err_release;
 
+       cs35l41_add_controls(cs35l41);
+
        ret = cs35l41_save_calibration(cs35l41);
 
 err_release:

diff --git a/sound/pci/hda/hda_cs_dsp_ctl.c b/sound/pci/hda/hda_cs_dsp_ctl.c
index 75fb6918581724da470f26194daf9609b56b9f78..1622a22f96f6a6b2492a00284c8dc80adcf0e835 100644 (file)
--- a/sound/pci/hda/hda_cs_dsp_ctl.c
+++ b/sound/pci/hda/hda_cs_dsp_ctl.c
@@ -97,7 +97,7 @@ static unsigned int wmfw_convert_flags(unsigned int in)
        return out;
 }
 
-static int hda_cs_dsp_add_kcontrol(struct hda_cs_dsp_coeff_ctl *ctl, const char *name)
+static void hda_cs_dsp_add_kcontrol(struct hda_cs_dsp_coeff_ctl *ctl, const char *name)
 {
        struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl;
        struct snd_kcontrol_new kcontrol = {0};
@@ -107,7 +107,7 @@ static int hda_cs_dsp_add_kcontrol(struct hda_cs_dsp_coeff_ctl *ctl, const char
        if (cs_ctl->len > ADSP_MAX_STD_CTRL_SIZE) {
                dev_err(cs_ctl->dsp->dev, "KControl %s: length %zu exceeds maximum %d\n", name,
                        cs_ctl->len, ADSP_MAX_STD_CTRL_SIZE);
-               return -EINVAL;
+               return;
        }
 
        kcontrol.name = name;
@@ -120,24 +120,21 @@ static int hda_cs_dsp_add_kcontrol(struct hda_cs_dsp_coeff_ctl *ctl, const char
        /* Save ctl inside private_data, ctl is owned by cs_dsp,
         * and will be freed when cs_dsp removes the control */
        kctl = snd_ctl_new1(&kcontrol, (void *)ctl);
-       if (!kctl) {
-               ret = -ENOMEM;
-               return ret;
-       }
+       if (!kctl)
+               return;
 
        ret = snd_ctl_add(ctl->card, kctl);
        if (ret) {
                dev_err(cs_ctl->dsp->dev, "Failed to add KControl %s = %d\n", kcontrol.name, ret);
-               return ret;
+               return;
        }
 
        dev_dbg(cs_ctl->dsp->dev, "Added KControl: %s\n", kcontrol.name);
        ctl->kctl = kctl;
-
-       return 0;
 }
 
-int hda_cs_dsp_control_add(struct cs_dsp_coeff_ctl *cs_ctl, struct hda_cs_dsp_ctl_info *info)
+static void hda_cs_dsp_control_add(struct cs_dsp_coeff_ctl *cs_ctl,
+                                  const struct hda_cs_dsp_ctl_info *info)
 {
        struct cs_dsp *cs_dsp = cs_ctl->dsp;
        char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
@@ -145,13 +142,10 @@ int hda_cs_dsp_control_add(struct cs_dsp_coeff_ctl *cs_ctl, struct hda_cs_dsp_ct
        const char *region_name;
        int ret;
 
-       if (cs_ctl->flags & WMFW_CTL_FLAG_SYS)
-               return 0;
-
        region_name = cs_dsp_mem_region_name(cs_ctl->alg_region.type);
        if (!region_name) {
-               dev_err(cs_dsp->dev, "Unknown region type: %d\n", cs_ctl->alg_region.type);
-               return -EINVAL;
+               dev_warn(cs_dsp->dev, "Unknown region type: %d\n", cs_ctl->alg_region.type);
+               return;
        }
 
        ret = scnprintf(name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, "%s %s %.12s %x", info->device_name,
@@ -171,22 +165,39 @@ int hda_cs_dsp_control_add(struct cs_dsp_coeff_ctl *cs_ctl, struct hda_cs_dsp_ct
 
        ctl = kzalloc(sizeof(*ctl), GFP_KERNEL);
        if (!ctl)
-               return -ENOMEM;
+               return;
 
        ctl->cs_ctl = cs_ctl;
        ctl->card = info->card;
        cs_ctl->priv = ctl;
 
-       ret = hda_cs_dsp_add_kcontrol(ctl, name);
-       if (ret) {
-               dev_err(cs_dsp->dev, "Error (%d) adding control %s\n", ret, name);
-               kfree(ctl);
-               return ret;
-       }
+       hda_cs_dsp_add_kcontrol(ctl, name);
+}
 
-       return 0;
+void hda_cs_dsp_add_controls(struct cs_dsp *dsp, const struct hda_cs_dsp_ctl_info *info)
+{
+       struct cs_dsp_coeff_ctl *cs_ctl;
+
+       /*
+        * pwr_lock would cause mutex inversion with ALSA control lock compared
+        * to the get/put functions.
+        * It is safe to walk the list without holding a mutex because entries
+        * are persistent and only cs_dsp_power_up() or cs_dsp_remove() can
+        * change the list.
+        */
+       lockdep_assert_not_held(&dsp->pwr_lock);
+
+       list_for_each_entry(cs_ctl, &dsp->ctl_list, list) {
+               if (cs_ctl->flags & WMFW_CTL_FLAG_SYS)
+                       continue;
+
+               if (cs_ctl->priv)
+                       continue;
+
+               hda_cs_dsp_control_add(cs_ctl, info);
+       }
 }
-EXPORT_SYMBOL_NS_GPL(hda_cs_dsp_control_add, SND_HDA_CS_DSP_CONTROLS);
+EXPORT_SYMBOL_NS_GPL(hda_cs_dsp_add_controls, SND_HDA_CS_DSP_CONTROLS);
 
 void hda_cs_dsp_control_remove(struct cs_dsp_coeff_ctl *cs_ctl)
 {

diff --git a/sound/pci/hda/hda_cs_dsp_ctl.h b/sound/pci/hda/hda_cs_dsp_ctl.h
index 4babc69cf2f0c86a00c30ddd56188b4a04823dca..2cf93359c4f230903028c986ff31c92378df68d3 100644 (file)
--- a/sound/pci/hda/hda_cs_dsp_ctl.h
+++ b/sound/pci/hda/hda_cs_dsp_ctl.h
@@ -29,7 +29,7 @@ struct hda_cs_dsp_ctl_info {
 
 extern const char * const hda_cs_dsp_fw_ids[HDA_CS_DSP_NUM_FW];
 
-int hda_cs_dsp_control_add(struct cs_dsp_coeff_ctl *cs_ctl, struct hda_cs_dsp_ctl_info *info);
+void hda_cs_dsp_add_controls(struct cs_dsp *dsp, const struct hda_cs_dsp_ctl_info *info);
 void hda_cs_dsp_control_remove(struct cs_dsp_coeff_ctl *cs_ctl);
 int hda_cs_dsp_write_ctl(struct cs_dsp *dsp, const char *name, int type,
                         unsigned int alg, const void *buf, size_t len);