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

From: Cezary Rojewski <cezary.rojewski@intel.com>
To: Mark Brown <broonie@kernel.org>
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 18:07:57 +0100	[thread overview]
Message-ID: <30e45868-ce11-9cfd-9a13-51d37ad311e1@intel.com> (raw)
In-Reply-To: <YcHjcnD0Ts7AYDy1@sirena.org.uk>

On 2021-12-21 3:40 PM, Mark Brown wrote:
> 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.

Keen eye, Mark. In fact, you're right in both statements:

- assuming there is no wider lock, existing usage of atomics won't 
prevent possible race for enable/disable of DSP carried as a consequence 
to ->core_refs manipulation

- there is a wider lock indeed, and that's why we haven't encountered 
the problem I guess. It's ->path_mutex, a member of struct avs_dev. Said 
mutex is introduced in:
[PATCH 19/37] ASoC: Intel: avs: Path management

along with its usage. By the usage I mean the following:
avs_dsp_put_core() and avs_dsp_get_core() are called only within 
avs_dsp_init_module() and avs_dsp_delete_module(). The latter two are 
part of 'struct avs_path *' instances creation and deletion procedure: 
avs_path_create() and avs_path_free(). Both avs_path_create() and 
avs_path_free() lock ->path_mutex before doing anything.

I admit that answer to question: "which approach fits best here?" will 
probably need to wait for the Christmas break to be over. While myself 
I'm in favour of synchronizing avs_dsp_put_core() and avs_dsp_get_core() 
locally as it scales better into the future and we won't get caught 
unprepared when avs_path_create() and avs_path_free() stop being the 
only places for their usage, such decision need to be made by the team 
as a whole.

One more thing came into my mind during this discussion: 
avs_dsp_put_core() and avs_dsp_get_core() should probably be 'static' - 
as it was said earlier, these are not used outside of the dsp.c file.

Once again, good finding Mark, thank you.

Regards,
Czarek