ASoC: SOF: pcm: remove warning - initialize workqueue on open

[plbossart]

If the SOF hw_params() fail, typically with an IPC error thrown by the
firmware, the period_elapsed workqueue is not initialized, but we
still cancel it in hw_free(), which results in a kernel warning.

Move the initialization to the .open callback. Tested on Broadwell
(Samus) and IceLake.

Fixes: e2803e6 ("ASoC: SOF: PCM: add period_elapsed work to fix
race condition in interrupt context")

GitHub issue: #932
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

[keyonjie]

Maybe we should move INIT_WORK() to the end of sof_pcm_open(), cancel_work_sync() to end of trigger stop(it makes more sense to flush the pcm_elapse() work at trigger stop), then we can avoid introducing the new flag.

[lyakh]

Wouldn't it be easier to just move the INIT_WORK() call up in sof_pcm_hw_params() above allocating buffer pages? Seems like a much simpler option to me.

[lyakh]

I'd rather use the "bool" type here, especially since you use "true" and "false" to set it.

[plbossart]

checkpatch.pl yells when we use boolean in structures. I am tempted to make the change though since there is no risk of alignment issues here,

[bardliao]

@lyakh @plbossart We did a job to remove the bool in struct before. #270

[plbossart]

Wouldn't it be easier to just move the INIT_WORK() call up in sof_pcm_hw_params() above allocating buffer pages? Seems like a much simpler option to me.

I thought about this but there are no many things that can go wrong in hw_params that I wonder if it's really necessary to initialize something we may or may not use.
I am fine with this change if @keyonjie agrees.

[ranj063]

I thought about this but there are no many things that can go wrong in hw_params that I wonder if it's really necessary to initialize something we may or may not use.
I am fine with this change if @keyonjie agrees.

@plbossart @keyonjie I thought about this too when I reviewed this PR. I think the intention is clearly stated so even though it looks awkward, I think it makes sense.

On the other hand, if we are going to initialize the work anyway, why do it hw_params? Can we just do it when spcm is created?

[keyonjie]

I thought about this but there are no many things that can go wrong in hw_params that I wonder if it's really necessary to initialize something we may or may not use.
I am fine with this change if @keyonjie agrees.

@plbossart @keyonjie I thought about this too when I reviewed this PR. I think the intention is clearly stated so even though it looks awkward, I think it makes sense.

On the other hand, if we are going to initialize the work anyway, why do it hw_params? Can we just do it when spcm is created?

Yes, I prefer to move INIT_WORK() to the end of sof_pcm_open(), cancel_work_sync() to end of trigger stop(it makes more sense to flush the pcm_elapse() work at trigger stop), then we can avoid introducing the new flag also.

[plbossart]

I thought about this but there are no many things that can go wrong in hw_params that I wonder if it's really necessary to initialize something we may or may not use.
I am fine with this change if @keyonjie agrees.

@plbossart @keyonjie I thought about this too when I reviewed this PR. I think the intention is clearly stated so even though it looks awkward, I think it makes sense.
On the other hand, if we are going to initialize the work anyway, why do it hw_params? Can we just do it when spcm is created?

Yes, I prefer to move INIT_WORK() to the end of sof_pcm_open(), cancel_work_sync() to end of trigger stop(it makes more sense to flush the pcm_elapse() work at trigger stop), then we can avoid introducing the new flag also.

This is really asking for trouble. Open and trigger-stop are not exactly mirrors. It's hw_params/hw_free or open/close. Anything else is just going to break all the state machines.

[keyonjie]

This is really asking for trouble. Open and trigger-stop are not exactly mirrors. It's hw_params/hw_free or open/close. Anything else is just going to break all the state machines.

Pierre, because INIT_WORK() and cancel_work_sync() are not mirrors IMO, usually we INIT_WORK() one time, and then go to loop of schedule_work()->[Optional, do it when we try to drain the unfinished work, cancel_work()]->schedule_work()->[cancel_work()].

So, with this understanding, I am fine with @ranj063 that we can even move INIT_WORK() to where as early as spcm is created.

[lyakh]

I thought about this but there are no many things that can go wrong in hw_params that I wonder if it's really necessary to initialize something we may or may not use.
I am fine with this change if @keyonjie agrees.

@plbossart @keyonjie I thought about this too when I reviewed this PR. I think the intention is clearly stated so even though it looks awkward, I think it makes sense.

On the other hand, if we are going to initialize the work anyway, why do it hw_params? Can we just do it when spcm is created?

@plbossart @ranj063 yes, that's even better: put INIT_WORK() in sof_pcm_open() right together with the rest of the spcm->stream[substream->stream] initialisation.

[plbossart]

updated with change to open only. also updated commit message.

[keyonjie]

LGTM.