Messages in this thread | | | Date | Mon, 1 Jun 2015 17:10:47 +0100 | From | Mark Brown <> | Subject | Re: [PATCH v7 1/4] ASoC: arizona: Export functions to control subsystem DVFS |
| |
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. [unhandled content-type:application/pgp-signature] | |