From 703180998f088cc83ff8e58f5881c65963d557a8 Mon Sep 17 00:00:00 2001 From: Eric Laurent Date: Fri, 19 Jun 2015 17:49:17 -0700 Subject: [PATCH] audio HAL: reverse mutex locking order Do not use main audio HAL mutex but a specific stream mutex when preventing control thread starvation by playback or capture threads. This will prevent systematic locking of main HAL mutex in read ot write and avoid glitches. Bug: 21880828. Bug: 18489202. Change-Id: I3054e0d93b823bd9d2fb84a49e9c5bbbe728262a --- modules/usbaudio/audio_hal.c | 77 ++++++++++++++++++++++++------------ 1 file changed, 51 insertions(+), 26 deletions(-) diff --git a/modules/usbaudio/audio_hal.c b/modules/usbaudio/audio_hal.c index 872fa934..bbea5f5e 100644 --- a/modules/usbaudio/audio_hal.c +++ b/modules/usbaudio/audio_hal.c @@ -80,6 +80,7 @@ struct stream_out { struct audio_stream_out stream; pthread_mutex_t lock; /* see note below on mutex acquisition order */ + pthread_mutex_t pre_lock; /* acquire before lock to avoid DOS by playback thread */ bool standby; struct audio_device *dev; /* hardware information - only using this for the lock */ @@ -103,7 +104,8 @@ struct stream_out { struct stream_in { struct audio_stream_in stream; - pthread_mutex_t lock; /* see note below on mutex acquisition order */ + pthread_mutex_t lock; /* see note below on mutex acquisition order */ + pthread_mutex_t pre_lock; /* acquire before lock to avoid DOS by capture thread */ bool standby; struct audio_device *dev; /* hardware information - only using this for the lock */ @@ -125,6 +127,13 @@ struct stream_in { size_t conversion_buffer_size; /* in bytes */ }; +/* + * NOTE: when multiple mutexes have to be acquired, always take the + * stream_in or stream_out mutex first, followed by the audio_device mutex. + * stream pre_lock is always acquired before stream lock to prevent starvation of control thread by + * higher priority playback or capture thread. + */ + /* * Extract the card and device numbers from the supplied key/value pairs. * kvpairs A null-terminated string containing the key/value pairs or card and device. @@ -203,6 +212,20 @@ static char * device_get_parameters(alsa_device_profile * profile, const char * return result_str; } +void lock_input_stream(struct stream_in *in) +{ + pthread_mutex_lock(&in->pre_lock); + pthread_mutex_lock(&in->lock); + pthread_mutex_unlock(&in->pre_lock); +} + +void lock_output_stream(struct stream_out *out) +{ + pthread_mutex_lock(&out->pre_lock); + pthread_mutex_lock(&out->lock); + pthread_mutex_unlock(&out->pre_lock); +} + /* * HAl Functions */ @@ -260,16 +283,14 @@ static int out_standby(struct audio_stream *stream) { struct stream_out *out = (struct stream_out *)stream; - pthread_mutex_lock(&out->dev->lock); - pthread_mutex_lock(&out->lock); - + lock_output_stream(out); if (!out->standby) { + pthread_mutex_lock(&out->dev->lock); proxy_close(&out->proxy); + pthread_mutex_unlock(&out->dev->lock); out->standby = true; } - pthread_mutex_unlock(&out->lock); - pthread_mutex_unlock(&out->dev->lock); return 0; } @@ -295,9 +316,9 @@ static int out_set_parameters(struct audio_stream *stream, const char *kvpairs) return ret_value; } + lock_output_stream(out); /* Lock the device because that is where the profile lives */ pthread_mutex_lock(&out->dev->lock); - pthread_mutex_lock(&out->lock); if (!profile_is_cached_for(out->profile, card, device)) { /* cannot read pcm device info if playback is active */ @@ -316,8 +337,8 @@ static int out_set_parameters(struct audio_stream *stream, const char *kvpairs) } } - pthread_mutex_unlock(&out->lock); pthread_mutex_unlock(&out->dev->lock); + pthread_mutex_unlock(&out->lock); return ret_value; } @@ -325,8 +346,8 @@ static int out_set_parameters(struct audio_stream *stream, const char *kvpairs) static char * out_get_parameters(const struct audio_stream *stream, const char *keys) { struct stream_out *out = (struct stream_out *)stream; + lock_output_stream(out); pthread_mutex_lock(&out->dev->lock); - pthread_mutex_lock(&out->lock); char * params_str = device_get_parameters(out->profile, keys); @@ -360,17 +381,16 @@ static ssize_t out_write(struct audio_stream_out *stream, const void* buffer, si int ret; struct stream_out *out = (struct stream_out *)stream; - pthread_mutex_lock(&out->dev->lock); - pthread_mutex_lock(&out->lock); + lock_output_stream(out); if (out->standby) { + pthread_mutex_lock(&out->dev->lock); ret = start_output_stream(out); + pthread_mutex_unlock(&out->dev->lock); if (ret != 0) { - pthread_mutex_unlock(&out->dev->lock); goto err; } out->standby = false; } - pthread_mutex_unlock(&out->dev->lock); alsa_device_proxy* proxy = &out->proxy; const void * write_buff = buffer; @@ -480,6 +500,9 @@ static int adev_open_output_stream(struct audio_hw_device *dev, out->stream.get_presentation_position = out_get_presentation_position; out->stream.get_next_write_timestamp = out_get_next_write_timestamp; + pthread_mutex_init(&out->lock, (const pthread_mutexattr_t *) NULL); + pthread_mutex_init(&out->pre_lock, (const pthread_mutexattr_t *) NULL); + out->dev = adev; pthread_mutex_lock(&adev->lock); out->profile = &adev->out_profile; @@ -639,16 +662,15 @@ static int in_standby(struct audio_stream *stream) { struct stream_in *in = (struct stream_in *)stream; - pthread_mutex_lock(&in->dev->lock); - pthread_mutex_lock(&in->lock); - + lock_input_stream(in); if (!in->standby) { + pthread_mutex_lock(&in->dev->lock); proxy_close(&in->proxy); + pthread_mutex_unlock(&in->dev->lock); in->standby = true; } pthread_mutex_unlock(&in->lock); - pthread_mutex_unlock(&in->dev->lock); return 0; } @@ -676,8 +698,8 @@ static int in_set_parameters(struct audio_stream *stream, const char *kvpairs) return ret_value; } + lock_input_stream(in); pthread_mutex_lock(&in->dev->lock); - pthread_mutex_lock(&in->lock); if (card >= 0 && device >= 0 && !profile_is_cached_for(in->profile, card, device)) { /* cannot read pcm device info if playback is active */ @@ -696,8 +718,8 @@ static int in_set_parameters(struct audio_stream *stream, const char *kvpairs) } } - pthread_mutex_unlock(&in->lock); pthread_mutex_unlock(&in->dev->lock); + pthread_mutex_unlock(&in->lock); return ret_value; } @@ -706,13 +728,13 @@ static char * in_get_parameters(const struct audio_stream *stream, const char *k { struct stream_in *in = (struct stream_in *)stream; + lock_input_stream(in); pthread_mutex_lock(&in->dev->lock); - pthread_mutex_lock(&in->lock); char * params_str = device_get_parameters(in->profile, keys); - pthread_mutex_unlock(&in->lock); pthread_mutex_unlock(&in->dev->lock); + pthread_mutex_unlock(&in->lock); return params_str; } @@ -750,16 +772,16 @@ static ssize_t in_read(struct audio_stream_in *stream, void* buffer, size_t byte struct stream_in * in = (struct stream_in *)stream; - pthread_mutex_lock(&in->dev->lock); - pthread_mutex_lock(&in->lock); + lock_input_stream(in); if (in->standby) { - if (start_input_stream(in) != 0) { - pthread_mutex_unlock(&in->dev->lock); + pthread_mutex_lock(&in->dev->lock); + ret = start_input_stream(in); + pthread_mutex_unlock(&in->dev->lock); + if (ret != 0) { goto err; } in->standby = false; } - pthread_mutex_unlock(&in->dev->lock); alsa_device_profile * profile = in->profile; @@ -858,6 +880,9 @@ static int adev_open_input_stream(struct audio_hw_device *dev, in->stream.read = in_read; in->stream.get_input_frames_lost = in_get_input_frames_lost; + pthread_mutex_init(&in->lock, (const pthread_mutexattr_t *) NULL); + pthread_mutex_init(&in->pre_lock, (const pthread_mutexattr_t *) NULL); + in->dev = (struct audio_device *)dev; pthread_mutex_lock(&in->dev->lock);