Re: [PATCH 06/14] ASoC: Intel: avs: Coredump and recovery flow

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

>>> +
>>> +    /* forcibly shutdown all cores */
>>> +    core_mask = GENMASK(adev->hw_cfg.dsp_cores - 1, 0);
>>> +    avs_dsp_core_disable(adev, core_mask);
>>> +
>>> +    /* attempt dsp reboot */
>>> +    ret = avs_dsp_boot_firmware(adev, true);
>>> +    if (ret < 0)
>>> +        dev_err(adev->dev, "dsp reboot failed: %d\n", ret);
>>> +
>>> +    pm_runtime_mark_last_busy(adev->dev);
>>> +    pm_runtime_enable(adev->dev);
>>> +    pm_request_autosuspend(adev->dev);
>>
>> there are zero users of this routine in the entire sound/ tree, can you clarify why this is needed or what you are trying to do?
> 
> 
> Unsure which routine you question here. I'll assume it's pm_request_autosuspend().
> 
> pm_request_audiosuspend() is being used to queue suspend once recovery completes. Recovery takes time and during that time all communication attempts with DSP will yield -EPERM. PM is also blocked for the device with pm_runtime_disable(), performed before scheduling the recovery work. Once recovery completes we do not just unblock the PM as that would cause immediate suspend. Instead, we "refresh" the *last busy* status and queue the suspend operation.

But since you already have autosuspend enabled, why would you need to explicitly queue a suspend operation? What happens if that last call is omitted, is there actually a functional difference?

Not objecting if that's required, but since no one else used it so far I wonder if we missed something or if this is overkill.