summaryrefslogtreecommitdiff
path: root/sound/hda
diff options
context:
space:
mode:
authorTakashi Iwai <tiwai@suse.de>2020-01-09 10:01:04 +0100
committerTakashi Iwai <tiwai@suse.de>2020-01-13 13:40:41 +0100
commit1a462be52f4505a2719631fb5aa7bfdbd37bfd8d (patch)
treed93981fbfa1a8514aedf01a4c4a85f2ed99bae64 /sound/hda
parent73ac9f5e5b43a5dbadb61f27dae7a971f7ec0d22 (diff)
ALSA: hda: Manage concurrent reg access more properly
In the commit 8e85def5723e ("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: Kai Vehmanen <kai.vehmanen@linux.intel.com> Link: https://lore.kernel.org/r/20200109090104.26073-1-tiwai@suse.de Signed-off-by: Takashi Iwai <tiwai@suse.de>
Diffstat (limited to 'sound/hda')
-rw-r--r--sound/hda/hdac_device.c1
-rw-r--r--sound/hda/hdac_regmap.c142
2 files changed, 108 insertions, 35 deletions
diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index b4f8725f5ddf..7bcdb0e454f2 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -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);
diff --git a/sound/hda/hdac_regmap.c b/sound/hda/hdac_regmap.c
index 0c8188a48a00..d75f31eb9d78 100644
--- a/sound/hda/hdac_regmap.c
+++ b/sound/hda/hdac_regmap.c
@@ -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
@@ -516,19 +550,57 @@ 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)
- return err;
- return 1;
+ err = regmap_update_bits(codec->regmap, reg, mask, val);
+ mutex_unlock(&codec->regmap_lock);
+ return err;
}
-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);