Re: [PATCH v7 1/4] ASoC: arizona: Export functions to control subsystem DVFS

Mark Brown
2015-06-01T16:11:02Z

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]