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:
signature.asc
Description: PGP signature
