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

[Mark Brown <broonie@kernel.org>]

[-- 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 --]