Commit 2176c6b5 authored by Richard Fitzgerald's avatar Richard Fitzgerald Committed by Takashi Iwai
Browse files

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: default avatarRichard Fitzgerald <rf@opensource.cirrus.com>
Signed-off-by: default avatarStefan Binding <sbinding@opensource.cirrus.com>
Link: https://lore.kernel.org/r/20221011143552.621792-4-sbinding@opensource.cirrus.com


Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
parent 06f3a0a7

sound/pci/hda/cs35l41_hda.c

+4 −4
Original line number Diff line number Diff line
@@ -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:

sound/pci/hda/hda_cs_dsp_ctl.c

+35 −24
Original line number Diff line number Diff line
@@ -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)

{

sound/pci/hda/hda_cs_dsp_ctl.h

+1 −1
Original line number Diff line number Diff line
@@ -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);