From 1a462be52f4505a2719631fb5aa7bfdbd37bfd8d Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Thu, 9 Jan 2020 10:01:04 +0100 Subject: 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 Link: https://lore.kernel.org/r/20200109090104.26073-1-tiwai@suse.de Signed-off-by: Takashi Iwai --- sound/hda/hdac_device.c | 1 + sound/hda/hdac_regmap.c | 142 ++++++++++++++++++++++++++++++++++++------------ 2 files changed, 108 insertions(+), 35 deletions(-) (limited to 'sound/hda') 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 @@ -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) - 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); -- cgit v1.2.3-59-g8ed1b