At Tue, 27 Aug 2013 15:17:41 +0530, Vinod Koul wrote:
On Tue, Aug 27, 2013 at 12:25:23PM +0200, Takashi Iwai wrote:
At Tue, 27 Aug 2013 12:10:32 +0530, Vinod Koul wrote:
Since we dont have lock over the function, we need to aquire mutex when checking and modfying states in drain and partial_drain handlers
Signed-off-by: Vinod Koul vinod.koul@intel.com Cc: stable@vger.kernel.org
sound/core/compress_offload.c | 22 +++++++++++++++++++--- 1 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 868aefd..e640f8c 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream) return retval; }
+/* this fn is called without lock being held and we change stream states here
- so while using the stream state auquire the lock but relase before invoking
- DSP as the call will possibly take a while
- */
static int snd_compr_drain(struct snd_compr_stream *stream) { int retval;
- mutex_lock(&stream->device->lock); if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
stream->runtime->state == SNDRV_PCM_STATE_SETUP)return -EPERM;
stream->runtime->state == SNDRV_PCM_STATE_SETUP) {retval = -EPERM;goto ret;- }
- mutex_unlock(&stream->device->lock); retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
Why release the lock here? The trigger callback is called within this mutex lock in other places.
This is the main part :)
Since the drain states will take a while (order of few seconds) to execute so we will be blocked. Thats why we cant have lock here.
What's about other places calling the trigger ops within lock? You can't mix things.
The point of lock is to sync the stream states here.
Without the lock, it's racy. What if another thread calls the same function at the same time?
We are not modfying anything. During drain and partial drain we need to allow other trigger ops like pause, stop tog o thru so drop the lock here for these two ops only!
Well, the biggest problem is that there is no proper design for which ops take a lock and which not. The trigger callback is basically to trigger the action. There should be no long-time blocking there. (Otherwise you'll definitely loose a gunfight :)
Takashi