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@xxxxxxxxx>
> > > Cc: <stable@xxxxxxxxxxxxxxx>
> > > ---
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html