On Mon, Jun 01, 2015 at 02:04:48PM +0100, Richard Fitzgerald wrote:
> +int arizona_dvfs_down(struct snd_soc_codec *codec, unsigned int flags)
> +{
> + struct arizona_priv *priv = snd_soc_codec_get_drvdata(codec);
> + unsigned int old_reqs = priv->dvfs_reqs;
> + int ret = 0;
> +
> + mutex_lock(&priv->dvfs_lock);
> +
> + priv->dvfs_reqs &= ~flags;
> +
> + if (!priv->dvfs_cached && old_reqs && !priv->dvfs_reqs)
> + ret = arizona_dvfs_disable(codec);What is the lock intended to protect here? We read old_reqs outside the lock so it's possible that dvfs_reqs could change between us reading old_reqs and the locked section - I would have expected to see all the reads and updates to be in the locked section but perhaps it doesn't protect what I think it protects (all the DVFS-related variables). > + case SND_SOC_DAPM_PRE_PMD: > + /* We must ensure DVFS is disabled before the codec goes into > + * suspend so that we are never in an illegal state of DVFS > + * enabled without enough DCVDD > + */ > + priv->dvfs_cached = true; > + > + if (priv->dvfs_reqs) > + ret = arizona_dvfs_disable(codec); Are you sure that the function shouldn't check for requests? It seems like every caller is repeating the same check.
signature.asc
Description: Digital signature
