Re: [RFC 17/37] ASoC: Intel: avs: Dynamic firmware resources management

From: Mark Brown <broonie@kernel.org>
To: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: alsa-devel@alsa-project.org, upstream@semihalf.com,
	harshapriya.n@intel.com, yung-chuan.liao@linux.intel.com,
	rad@semihalf.com, pierre-louis.bossart@linux.intel.com,
	tiwai@suse.com, hdegoede@redhat.com,
	ranjani.sridharan@linux.intel.com,
	amadeuszx.slawinski@linux.intel.com, cujomalainey@chromium.org,
	peter.ujfalusi@linux.intel.com, lma@semihalf.com
Subject: Re: [RFC 17/37] ASoC: Intel: avs: Dynamic firmware resources management
Date: Tue, 21 Dec 2021 14:40:09 +0000	[thread overview]
Message-ID: <YcHjcnD0Ts7AYDy1@sirena.org.uk> (raw)
In-Reply-To: <20211208111301.1817725-18-cezary.rojewski@intel.com>

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

On Wed, Dec 08, 2021 at 12:12:41PM +0100, Cezary Rojewski wrote:

> +int avs_dsp_get_core(struct avs_dev *adev, u32 core_id)
> +{

...

> +	if (atomic_add_return(1, ref) == 1) {
> +		ret = avs_dsp_enable(adev, mask);
> +		if (ret)
> +			goto err_enable_dsp;
> +	}

> +int avs_dsp_put_core(struct avs_dev *adev, u32 core_id)
> +{

...

> +	ref = &adev->core_refs[core_id];
> +	if (atomic_dec_and_test(ref)) {
> +		ret = avs_dsp_disable(adev, mask);

This looks wrong - there's nothing that ensures that we don't get
a sequence like:

	CPU0		CPU1
	decrement
			increment
			enable DSP
	disable DSP

that I can see here?  Either there's a lock missing which ensures
that the actual DSP management is in sync with the refcount or
there's no need for the use of atomics since the wider lock will
ensure that only one thing could be updating at once.  In general
I'd expect something heavier weight than atomics.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]