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
