[ranj063]

period_elapsed_work is initialized during pcm open. So,
cancel it during pcm close to keep it balanced.

Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com

[plbossart]

well if you look at the history of #849 the INIT_WORK was done in hw_params and the cancel_ in hw_free(). Then later we moved the INIT_WORK to the open because if hw_params failed we would still call _cancel in hw_free.

I am not clear on what exactly the issue is, and why we now see an issue with code that's been around since mid-May. I don't mind moving the code but I'd like to know why it's needed.

[ranj063]

I am not clear on what exactly the issue is, and why we now see an issue with code that's been around since mid-May. I don't mind moving the code but I'd like to know why it's needed.

@plbossart this is aimed at fixing issue #1183 . I am still waiting for the device test results as I do not have a CML to test. But it looks like the failure is because of the period_elapsed_work() getting called after the DSp is in D3.

[plbossart]

I am not clear on what exactly the issue is, and why we now see an issue with code that's been around since mid-May. I don't mind moving the code but I'd like to know why it's needed.

@plbossart this is aimed at fixing issue #1183 . I am still waiting for the device test results as I do not have a CML to test. But it looks like the failure is because of the period_elapsed_work() getting called after the DSp is in D3.

I see. So initially we tried to cancel a non-existent workqueue, and now we call a function with also a non-existent workqueue... How many engineers does it take to screw a lightbulb ...

[ranj063]

CI is lost here. @fredoh9 is helping me with it.

[ranj063]

SOFCI TEST

[keyonjie]

@plbossart @ranj063 what I can imagine is that there might be cases that hw_free() is not called after hw_param() is called, e.g.:
pcm_open()->hw_params()->[trigger()]->pcm_close(), in this senarios, we might hit the issue that the delayed work is not cancelled.

Hi @ranj063 can you do some debugging and get logs to prove this, which can help us to understand why your patch is needed.

[ranj063]

Hi @ranj063 can you do some debugging and get logs to prove this, which can help us to understand why your patch is needed.

@keyonjie please have a look at the logs attached in #1183 . I came up with this patch based on the logs there.

[keyonjie]

@ranj063 I mean we need to make clear about what happens in #1183, how does it happen, then we can know if the patch here can fix the issue and make it robust in different scenarios.

According to the commit message, "period_elapsed_work is initialized during pcm open. So, cancel it during pcm close to keep it balanced.", I think we don't need to make "the work initialization" and its cancellation balanced? As the cancellation is not de-initialization, you can still run schedule_work directly after it's cancelled. I think the cancellation itself should be balanced with the schedule_work()?

[plbossart]

@ranj063 I mean we need to make clear about what happens in #1183, how does it happen, then we can know if the patch here can fix the issue and make it robust in different scenarios.

According to the commit message, "period_elapsed_work is initialized during pcm open. So, cancel it during pcm close to keep it balanced.", I think we don't need to make "the work initialization" and its cancellation balanced? As the cancellation is not de-initialization, you can still run schedule_work directly after it's cancelled. I think the cancellation itself should be balanced with the schedule_work()?

@keyonjie care to submit an alternate solution? IIRC you contributed the first PR on this so you are probably the one who should be looking into this issue. Thanks

[ranj063]

@ranj063 I mean we need to make clear about what happens in #1183, how does it happen, then we can know if the patch here can fix the issue and make it robust in different scenarios.

@keyonjie i remember Kai had a theory about this. @kv2019i could you please chime in as to why hw_free is sometimes not called when pulseaudio is probing pcm's?

[keyonjie]

@ranj063 I mean we need to make clear about what happens in #1183, how does it happen, then we can know if the patch here can fix the issue and make it robust in different scenarios.
According to the commit message, "period_elapsed_work is initialized during pcm open. So, cancel it during pcm close to keep it balanced.", I think we don't need to make "the work initialization" and its cancellation balanced? As the cancellation is not de-initialization, you can still run schedule_work directly after it's cancelled. I think the cancellation itself should be balanced with the schedule_work()?

@keyonjie care to submit an alternate solution? IIRC you contributed the first PR on this so you are probably the one who should be looking into this issue. Thanks

Hi @plbossart @ranj063 how about keeping the one in hw_free() and only adding a new cancellation in pcm_close()?

For long-term solution, I am thinking of abandoning the use of this work (I realize it could be buggy now), but we need to figure out a better solution to the trigger stop IPC timeout issue at PCM draining done.

[plbossart]

Hi @plbossart @ranj063 how about keeping the one in hw_free() and only adding a new cancellation in pcm_close()?

Sorry, what? you want to have two cancellations? That sounds bad to me.

For long-term solution, I am thinking of abandoning the use of this work (I realize it could be buggy now), but we need to figure out a better solution to the trigger stop IPC timeout issue at PCM draining done.

I also never liked this solution, so let's try and find the real fix rather than piecemeal code changes for a problem that remains.

[kv2019i]

@ranj063 I think the patch look good. We should definitely ensure cancel_work_sync() is called at close, or otherwise INIT_WORK() can overwrite state of the previous workqueue and thus can lead to random behaviour.
UPDATE: upon a second look, I don't see how this can fix the reported issue, so probably best to hold until we have clear rootcause

[kv2019i]

@ranj063 I mean we need to make clear about what happens in #1183, how does it happen, then we can know if the patch here can fix the issue and make it robust in different scenarios.

@keyonjie i remember Kai had a theory about this. @kv2019i could you please chime in as to why hw_free is sometimes not called when pulseaudio is probing pcm's?

@ranj063 I don't recall the context, but definitely there is no requirement for the application to call hw_params, so current code (without this patch) seems suspicious as we are repeatedly calling INIT_WORK() without matching cancel. This would seem harmless, but is not something done by other drivers.

What I don't fully understand, why is the elapsed work function called when it is cancelled. In most logs where this is seen, there's no playback happening, just creating the topologies. Now I'm wondering whether the use of workqueues is in general safe here (@keyonjie). Looking at other drivers, most work methods have a state check and action is only executed if state is correct. In our current implementation, period elapsed work function assumes it is run only if period has elapsed. Maybe this is not safe at all and the worker should explicitly check whether stream state is valid.

[kv2019i]

Changing my mind on this patch. Rootcause found.

[kv2019i]

@kv2019i wrote:

What I don't fully understand, why is the elapsed work function called when it is cancelled.
In most logs where this is seen, there's no playback happening, just creating the topologies.

@plbossart @keyonjie @ranj063
Good news and bad news. Good news is that I found the rootcause for issue 1183 and it's not related to the workqueue at all. Rather we have foobar ALSA handle around when we load FW and the FW-load interrupts sometimes trigger a ALSA PCM period callback on a completely foobar ALSA stream handled. Patch to fix this -> https://github.com/thesofproject/linux/pull/1193/commits

Bad news is that I found a more dangerous bug in hw_free. This might be triggered by my patches done in July or some combination of those and recent patches. I'll be sending another patch for these. UPDATE: patch for this #1194