[kv2019i]

Fix a bug in sof_pcm_hw_free() where some cleanup actions were
skipped if STREAM_PCM_FREE IPC was already succesfully sent to
DSP when the stream was stopped or suspended. This is incorrect
as hw_free should clean up also other resources, including pcm
lib page allocations, period elapsed work queue and call to
platform hw_free.

Fixes: c29d96c ("ASoC: SOF: reset DMA state in prepare")
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com

[plbossart]

Error handling looks suspicious to me

[plbossart]

if ret < 0 you skip the follow-up cleanup...

[plbossart]

and the test below after cancel_work_sync may use an uninitialized value since ret is not set

[kv2019i]

@plbossart Oops sorry, my bad, I was meaning to remove the check on L269-270. But, but, I just replied to #1169 and it seems I have had incorrect assumptions about the FW ABI. Reflecting the same discussion here (and e.g. implementation of snd_pcm_hw_free() in pcm_native.c), the correct action would indeed seem to be capture the error but not give any of the following clean-up steps. I will update patch accordingly.

[kv2019i]

FYI @plbossart . This is pretty bad as we never run the end of hw_params in current code. It seems we have been a bit lucky in that we are not leaking memory due to the preallocated DMA buffers, so the missing call to snd_pcm_lib_free_pages() has had limited impact. I've been testing today with kmemleak and see no reports (without this patch). I also tested running a stress test of aplay and have been monitoring kernel memory stats and they seem normal. Phew. Patch updated with fixes, I'll continue testing tomorrow.

[keyonjie]

FYI @plbossart . This is pretty bad as we never run the end of hw_params in current code. It

Hi @kv2019i do you mean that the invoking to sof_pcm_dsp_params() will return failure in your cases?

seems we have been a bit lucky in that we are not leaking memory due to the preallocated DMA buffers, so the missing call to snd_pcm_lib_free_pages() has had limited impact. I've been testing today with kmemleak and see no reports (without this patch). I also tested running a stress test of aplay and have been monitoring kernel memory stats and they seem normal. Phew. Patch updated with fixes, I'll continue testing tomorrow.

[kv2019i]

@keyonjie wrote

Hi @kv2019i do you mean that the invoking to sof_pcm_dsp_params() will return failure in
your cases?

No. I mean that in current code, in typical use-case dsp_pcm_free() is already run successfully in sof_pcm_trigger(STOP). When PCM device is closed, code goes to he_free, but as prepared state is false (as dsp_pcm_free() is already run), we skip clearing DSP PCM state, but the problem is that we also skip other actions in hw_free() which is not correct.

[keyonjie]

@keyonjie wrote

Hi @kv2019i do you mean that the invoking to sof_pcm_dsp_params() will return failure in
your cases?

No. I mean that in current code, in typical use-case dsp_pcm_free() is already run successfully in sof_pcm_trigger(STOP). When PCM device is closed, code goes to he_free, but as prepared state is false (as dsp_pcm_free() is already run), we skip clearing DSP PCM state, but the problem is that we also skip other actions in hw_free() which is not correct.

um, I got your point now, sending PCM_FREE IPC at trigger stop was not good idea IMHO, but I don't recall why we do it in that way (to fix some suspend/resume issue?).

[kv2019i]

@keyonjie wrote:

um, I got your point now, sending PCM_FREE IPC at trigger stop was not good idea IMHO, but I don't recall why we do it in that way (to fix some suspend/resume issue?).

When ALSA does a stop and start (without close/open cycle), the DMA must be reprogrammed (see my patch "ASoC: SOF: reset DMA state in prepare" from June 19). Thus we need to do PCM_FREE_IPC at stop, so that in the start flow we are in the expected state. This broke start/stop tests on a number of apps. Doing at stop seemed better as it's less time critical here (versus adding more IPC hops to the start path where we'd like to get the stream started as soon as possible).

[keyonjie]

@keyonjie wrote:

um, I got your point now, sending PCM_FREE IPC at trigger stop was not good idea IMHO, but I don't recall why we do it in that way (to fix some suspend/resume issue?).

When ALSA does a stop and start (without close/open cycle), the DMA must be reprogrammed

Why? the hw_params is not changed, why DMA need to be reprogrammed?

(see my patch "ASoC: SOF: reset DMA state in prepare" from June 19). Thus we need to do PCM_FREE_IPC at stop, so that in the start flow we are in the expected state. This broke start/stop tests on a number of apps. Doing at stop seemed better as it's less time critical here (versus adding more IPC hops to the start path where we'd like to get the stream started as soon as possible).

[kv2019i]

@keyonjie wrote:

When ALSA does a stop and start (without close/open cycle), the DMA must be reprogrammed

Why? the hw_params is not changed, why DMA need to be reprogrammed?

Did you read the patch commit message I referred to? :) It has a lengthy description.

Please check the summary. The ALSA PCM model expects hw_ptr/appl_ptr to be reset when device goes to PREPARED state and back to RUNNING. In Intel DSP, we have no other means to reset the position than to FREE_PCM and then resent the params. And the linked DMA needs to be reset as well, or otherwise playback will fail. PAUSE/UNPAUSE is a different flow.

[plbossart]

Thanks @kv2019i