Commit 1a462be5 authored by Takashi Iwai's avatar Takashi Iwai
Browse files

ALSA: hda: Manage concurrent reg access more properly 



In the commit 8e85def5 ("ALSA: hda: enable regmap internal
locking"), we re-enabled the regmap lock due to the reported
regression that showed the possible concurrent accesses.  It was a
temporary workaround, and there are still a few opened races even
after the revert.  In this patch, we cover those still opened windows
with a proper mutex lock and disable the regmap internal lock again.

First off, the patch introduces a new snd_hdac_device.regmap_lock
mutex that is applied for each snd_hdac_regmap_*() call, including
read, write and update helpers.  The mutex is applied carefully so
that it won't block the self-power-up procedure in the helper
function.  Also, this assures the protection for the accesses without
regmap, too.

The snd_hdac_regmap_update_raw() is refactored to use the standard
regmap_update_bits_check() function instead of the open-code.  The
non-regmap case is still open-coded but it's an easy part.  The all
read and write operations are in the single mutex protection, so it's
now race-free.

In addition, a couple of new helper functions are added:
snd_hdac_regmap_update_raw_once() and snd_hdac_regmap_sync().  Both
are called from HD-audio legacy driver.  The former is to initialize
the given verb bits but only once when it's not initialized yet.  Due
to this condition, the function invokes regcache_cache_only(), and
it's now performed inside the regmap_lock (formerly it was racy) too.
The latter function is for simply invoking regcache_sync() inside the
regmap_lock, which is called from the codec resume call path.
Along with that, the HD-audio codec driver code is slightly modified /
simplified to adapt those new functions.

And finally, snd_hdac_regmap_read_raw(), *_write_raw(), etc are
rewritten with the helper macro.  It's just for simplification because
the code logic is identical among all those functions.

Tested-by: default avatarKai Vehmanen <kai.vehmanen@linux.intel.com>
Link: https://lore.kernel.org/r/20200109090104.26073-1-tiwai@suse.de


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

include/sound/hda_regmap.h

+3 −0
Original line number Diff line number Diff line
@@ -24,6 +24,9 @@ int snd_hdac_regmap_write_raw(struct hdac_device *codec, unsigned int reg, 
			      unsigned int val);

int snd_hdac_regmap_update_raw(struct hdac_device *codec, unsigned int reg,

			       unsigned int mask, unsigned int val);

int snd_hdac_regmap_update_raw_once(struct hdac_device *codec, unsigned int reg,

				    unsigned int mask, unsigned int val);

void snd_hdac_regmap_sync(struct hdac_device *codec);



/**

 * snd_hdac_regmap_encode_verb - encode the verb to a pseudo register

include/sound/hdaudio.h

+1 −0
Original line number Diff line number Diff line
@@ -86,6 +86,7 @@ struct hdac_device { 


	/* regmap */

	struct regmap *regmap;

	struct mutex regmap_lock;

	struct snd_array vendor_verbs;

	bool lazy_cache:1;	/* don't wake up for writes */

	bool caps_overwriting:1; /* caps overwrite being in process */

sound/hda/hdac_device.c

+1 −0
Original line number Diff line number Diff line
@@ -57,6 +57,7 @@ int snd_hdac_device_init(struct hdac_device *codec, struct hdac_bus *bus, 
	codec->addr = addr;

	codec->type = HDA_DEV_CORE;

	mutex_init(&codec->widget_lock);

	mutex_init(&codec->regmap_lock);

	pm_runtime_set_active(&codec->dev);

	pm_runtime_get_noresume(&codec->dev);

	atomic_set(&codec->in_pm, 0);

sound/hda/hdac_regmap.c

+107 −35
Original line number Diff line number Diff line
@@ -363,6 +363,7 @@ static const struct regmap_config hda_regmap_cfg = { 
	.reg_write = hda_reg_write,

	.use_single_read = true,

	.use_single_write = true,

	.disable_locking = true,

};



/**
@@ -425,12 +426,29 @@ EXPORT_SYMBOL_GPL(snd_hdac_regmap_add_vendor_verb); 
static int reg_raw_write(struct hdac_device *codec, unsigned int reg,

			 unsigned int val)

{

	int err;



	mutex_lock(&codec->regmap_lock);

	if (!codec->regmap)

		return hda_reg_write(codec, reg, val);

		err = hda_reg_write(codec, reg, val);

	else

		return regmap_write(codec->regmap, reg, val);

		err = regmap_write(codec->regmap, reg, val);

	mutex_unlock(&codec->regmap_lock);

	return err;

}



/* a helper macro to call @func_call; retry with power-up if failed */

#define CALL_RAW_FUNC(codec, func_call)				\

	({							\

		int _err = func_call;				\

		if (_err == -EAGAIN) {				\

			_err = snd_hdac_power_up_pm(codec);	\

			if (_err >= 0)				\

				_err = func_call;		\

			snd_hdac_power_down_pm(codec);		\

		}						\

		_err;})



/**

 * snd_hdac_regmap_write_raw - write a pseudo register with power mgmt

 * @codec: the codec object
@@ -442,42 +460,29 @@ static int reg_raw_write(struct hdac_device *codec, unsigned int reg, 
int snd_hdac_regmap_write_raw(struct hdac_device *codec, unsigned int reg,

			      unsigned int val)

{

	int err;



	err = reg_raw_write(codec, reg, val);

	if (err == -EAGAIN) {

		err = snd_hdac_power_up_pm(codec);

		if (err >= 0)

			err = reg_raw_write(codec, reg, val);

		snd_hdac_power_down_pm(codec);

	}

	return err;

	return CALL_RAW_FUNC(codec, reg_raw_write(codec, reg, val));

}

EXPORT_SYMBOL_GPL(snd_hdac_regmap_write_raw);



static int reg_raw_read(struct hdac_device *codec, unsigned int reg,

			unsigned int *val, bool uncached)

{

	int err;



	mutex_lock(&codec->regmap_lock);

	if (uncached || !codec->regmap)

		return hda_reg_read(codec, reg, val);

		err = hda_reg_read(codec, reg, val);

	else

		return regmap_read(codec->regmap, reg, val);

		err = regmap_read(codec->regmap, reg, val);

	mutex_unlock(&codec->regmap_lock);

	return err;

}



static int __snd_hdac_regmap_read_raw(struct hdac_device *codec,

				      unsigned int reg, unsigned int *val,

				      bool uncached)

{

	int err;



	err = reg_raw_read(codec, reg, val, uncached);

	if (err == -EAGAIN) {

		err = snd_hdac_power_up_pm(codec);

		if (err >= 0)

			err = reg_raw_read(codec, reg, val, uncached);

		snd_hdac_power_down_pm(codec);

	}

	return err;

	return CALL_RAW_FUNC(codec, reg_raw_read(codec, reg, val, uncached));

}



/**
@@ -504,6 +509,35 @@ int snd_hdac_regmap_read_raw_uncached(struct hdac_device *codec, 
	return __snd_hdac_regmap_read_raw(codec, reg, val, true);

}



static int reg_raw_update(struct hdac_device *codec, unsigned int reg,

			  unsigned int mask, unsigned int val)

{

	unsigned int orig;

	bool change;

	int err;



	mutex_lock(&codec->regmap_lock);

	if (codec->regmap) {

		err = regmap_update_bits_check(codec->regmap, reg, mask, val,

					       &change);

		if (!err)

			err = change ? 1 : 0;

	} else {

		err = hda_reg_read(codec, reg, &orig);

		if (!err) {

			val &= mask;

			val |= orig & ~mask;

			if (val != orig) {

				err = hda_reg_write(codec, reg, val);

				if (!err)

					err = 1;

			}

		}

	}

	mutex_unlock(&codec->regmap_lock);

	return err;

}



/**

 * snd_hdac_regmap_update_raw - update a pseudo register with power mgmt

 * @codec: the codec object
@@ -515,20 +549,58 @@ int snd_hdac_regmap_read_raw_uncached(struct hdac_device *codec, 
 */

int snd_hdac_regmap_update_raw(struct hdac_device *codec, unsigned int reg,

			       unsigned int mask, unsigned int val)

{

	return CALL_RAW_FUNC(codec, reg_raw_update(codec, reg, mask, val));

}

EXPORT_SYMBOL_GPL(snd_hdac_regmap_update_raw);



static int reg_raw_update_once(struct hdac_device *codec, unsigned int reg,

			       unsigned int mask, unsigned int val)

{

	unsigned int orig;

	int err;



	val &= mask;

	err = snd_hdac_regmap_read_raw(codec, reg, &orig);

	if (err < 0)

		return err;

	val |= orig & ~mask;

	if (val == orig)

		return 0;

	err = snd_hdac_regmap_write_raw(codec, reg, val);

	if (!codec->regmap)

		return reg_raw_update(codec, reg, mask, val);



	mutex_lock(&codec->regmap_lock);

	regcache_cache_only(codec->regmap, true);

	err = regmap_read(codec->regmap, reg, &orig);

	regcache_cache_only(codec->regmap, false);

	if (err < 0)

		err = regmap_update_bits(codec->regmap, reg, mask, val);

	mutex_unlock(&codec->regmap_lock);

	return err;

	return 1;

}

EXPORT_SYMBOL_GPL(snd_hdac_regmap_update_raw);
 


/**

 * snd_hdac_regmap_update_raw_once - initialize the register value only once

 * @codec: the codec object

 * @reg: pseudo register

 * @mask: bit mask to update

 * @val: value to update

 *

 * Performs the update of the register bits only once when the register

 * hasn't been initialized yet.  Used in HD-audio legacy driver.

 * Returns zero if successful or a negative error code

 */

int snd_hdac_regmap_update_raw_once(struct hdac_device *codec, unsigned int reg,

				    unsigned int mask, unsigned int val)

{

	return CALL_RAW_FUNC(codec, reg_raw_update_once(codec, reg, mask, val));

}

EXPORT_SYMBOL_GPL(snd_hdac_regmap_update_raw_once);



/**

 * snd_hdac_regmap_sync - sync out the cached values for PM resume

 * @codec: the codec object

 */

void snd_hdac_regmap_sync(struct hdac_device *codec)

{

	if (codec->regmap) {

		mutex_lock(&codec->regmap_lock);

		regcache_sync(codec->regmap);

		mutex_unlock(&codec->regmap_lock);

	}

}

EXPORT_SYMBOL_GPL(snd_hdac_regmap_sync);

sound/pci/hda/hda_codec.c

+16 −14
Original line number Diff line number Diff line
@@ -1267,6 +1267,18 @@ int snd_hda_override_amp_caps(struct hda_codec *codec, hda_nid_t nid, int dir, 
}

EXPORT_SYMBOL_GPL(snd_hda_override_amp_caps);



static unsigned int encode_amp(struct hda_codec *codec, hda_nid_t nid,

			       int ch, int dir, int idx)

{

	unsigned int cmd = snd_hdac_regmap_encode_amp(nid, ch, dir, idx);



	/* enable fake mute if no h/w mute but min=mute */

	if ((query_amp_caps(codec, nid, dir) &

	     (AC_AMPCAP_MUTE | AC_AMPCAP_MIN_MUTE)) == AC_AMPCAP_MIN_MUTE)

		cmd |= AC_AMP_FAKE_MUTE;

	return cmd;

}



/**

 * snd_hda_codec_amp_update - update the AMP mono value

 * @codec: HD-audio codec
@@ -1282,12 +1294,8 @@ EXPORT_SYMBOL_GPL(snd_hda_override_amp_caps); 
int snd_hda_codec_amp_update(struct hda_codec *codec, hda_nid_t nid,

			     int ch, int dir, int idx, int mask, int val)

{

	unsigned int cmd = snd_hdac_regmap_encode_amp(nid, ch, dir, idx);

	unsigned int cmd = encode_amp(codec, nid, ch, dir, idx);



	/* enable fake mute if no h/w mute but min=mute */

	if ((query_amp_caps(codec, nid, dir) &

	     (AC_AMPCAP_MUTE | AC_AMPCAP_MIN_MUTE)) == AC_AMPCAP_MIN_MUTE)

		cmd |= AC_AMP_FAKE_MUTE;

	return snd_hdac_regmap_update_raw(&codec->core, cmd, mask, val);

}

EXPORT_SYMBOL_GPL(snd_hda_codec_amp_update);
@@ -1335,16 +1343,11 @@ EXPORT_SYMBOL_GPL(snd_hda_codec_amp_stereo); 
int snd_hda_codec_amp_init(struct hda_codec *codec, hda_nid_t nid, int ch,

			   int dir, int idx, int mask, int val)

{

	int orig;

	unsigned int cmd = encode_amp(codec, nid, ch, dir, idx);



	if (!codec->core.regmap)

		return -EINVAL;

	regcache_cache_only(codec->core.regmap, true);

	orig = snd_hda_codec_amp_read(codec, nid, ch, dir, idx);

	regcache_cache_only(codec->core.regmap, false);

	if (orig >= 0)

		return 0;

	return snd_hda_codec_amp_update(codec, nid, ch, dir, idx, mask, val);

	return snd_hdac_regmap_update_raw_once(&codec->core, cmd, mask, val);

}

EXPORT_SYMBOL_GPL(snd_hda_codec_amp_init);
@@ -2905,8 +2908,7 @@ static void hda_call_codec_resume(struct hda_codec *codec) 
	else {

		if (codec->patch_ops.init)

			codec->patch_ops.init(codec);

		if (codec->core.regmap)

			regcache_sync(codec->core.regmap);

		snd_hda_regmap_sync(codec);

	}



	if (codec->jackpoll_interval)

CRA Git | Maintained and supported by SUSTech CRA and CCSE