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