[alsa-devel] [PATCH 0/4] Internal PCM fixes / cleanups
Hi,
while reading the internal PCM (BE) handling code again, I noticed a few potential bugs. Here are fix and cleanup patches.
Takashi
===
Takashi Iwai (4): ALSA: pcm: Minor refactoring in snd_pcm_attach_substream() ALSA: pcm: Don't add internal PCMs to PCM device list ALSA: pcm: Don't notify internal PCMs ALSA: pcm: Don't ignore internal PCMs in snd_pcm_dev_disconnect()
sound/core/pcm.c | 92 +++++++++++++++++++++++++------------------------------- 1 file changed, 41 insertions(+), 51 deletions(-)
No functional changes at all.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm.c | 41 +++++++++++++++-------------------------- 1 file changed, 15 insertions(+), 26 deletions(-)
diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 0345e53a340c..89206e9c3578 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -919,6 +919,9 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
if (snd_BUG_ON(!pcm || !rsubstream)) return -ENXIO; + if (snd_BUG_ON(stream != SNDRV_PCM_STREAM_PLAYBACK || + stream != SNDRV_PCM_STREAM_CAPTURE)) + return -EINVAL; *rsubstream = NULL; pstr = &pcm->streams[stream]; if (pstr->substream == NULL || pstr->substream_count == 0) @@ -927,25 +930,14 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream, card = pcm->card; prefer_subdevice = snd_ctl_get_preferred_subdevice(card, SND_CTL_SUBDEV_PCM);
- switch (stream) { - case SNDRV_PCM_STREAM_PLAYBACK: - if (pcm->info_flags & SNDRV_PCM_INFO_HALF_DUPLEX) { - for (substream = pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream; substream; substream = substream->next) { - if (SUBSTREAM_BUSY(substream)) - return -EAGAIN; - } - } - break; - case SNDRV_PCM_STREAM_CAPTURE: - if (pcm->info_flags & SNDRV_PCM_INFO_HALF_DUPLEX) { - for (substream = pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream; substream; substream = substream->next) { - if (SUBSTREAM_BUSY(substream)) - return -EAGAIN; - } + if (pcm->info_flags & SNDRV_PCM_INFO_HALF_DUPLEX) { + int opposite = !stream; + + for (substream = pcm->streams[opposite].substream; substream; + substream = substream->next) { + if (SUBSTREAM_BUSY(substream)) + return -EAGAIN; } - break; - default: - return -EINVAL; }
if (file->f_flags & O_APPEND) { @@ -968,15 +960,12 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream, return 0; }
- if (prefer_subdevice >= 0) { - for (substream = pstr->substream; substream; substream = substream->next) - if (!SUBSTREAM_BUSY(substream) && substream->number == prefer_subdevice) - goto __ok; - } - for (substream = pstr->substream; substream; substream = substream->next) - if (!SUBSTREAM_BUSY(substream)) + for (substream = pstr->substream; substream; substream = substream->next) { + if (!SUBSTREAM_BUSY(substream) && + (prefer_subdevice == -1 || + substream->number == prefer_subdevice)) break; - __ok: + } if (substream == NULL) return -EAGAIN;
At Fri, 20 Feb 2015 17:23:25 +0100, Takashi Iwai wrote:
No functional changes at all.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/core/pcm.c | 41 +++++++++++++++-------------------------- 1 file changed, 15 insertions(+), 26 deletions(-)
diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 0345e53a340c..89206e9c3578 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -919,6 +919,9 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
if (snd_BUG_ON(!pcm || !rsubstream)) return -ENXIO;
- if (snd_BUG_ON(stream != SNDRV_PCM_STREAM_PLAYBACK ||
stream != SNDRV_PCM_STREAM_CAPTURE))
Gah, this must be &&. Fixed in the topic/pcm-internal branch.
Takashi
An internal PCM object shouldn't be added to the PCM device list, as it's never accessed directly from the user-space, and it has no proc or any similar accesses. Currently, it's excluded in snd_pcm_get() and snd_pcm_next(), but it's easier not to add such an object to the list.
Actually, the whole snd_pcm_dev_register() can be skipped for an internal PCM. So this patch changes the code there, but also addresses the uninitialized list_head access.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 89206e9c3578..92bbb5143b83 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -49,8 +49,6 @@ static struct snd_pcm *snd_pcm_get(struct snd_card *card, int device) struct snd_pcm *pcm;
list_for_each_entry(pcm, &snd_pcm_devices, list) { - if (pcm->internal) - continue; if (pcm->card == card && pcm->device == device) return pcm; } @@ -62,8 +60,6 @@ static int snd_pcm_next(struct snd_card *card, int device) struct snd_pcm *pcm;
list_for_each_entry(pcm, &snd_pcm_devices, list) { - if (pcm->internal) - continue; if (pcm->card == card && pcm->device > device) return pcm->device; else if (pcm->card->number > card->number) @@ -76,6 +72,9 @@ static int snd_pcm_add(struct snd_pcm *newpcm) { struct snd_pcm *pcm;
+ if (newpcm->internal) + return 0; + list_for_each_entry(pcm, &snd_pcm_devices, list) { if (pcm->card == newpcm->card && pcm->device == newpcm->device) return -EBUSY; @@ -782,6 +781,9 @@ static int _snd_pcm_new(struct snd_card *card, const char *id, int device, pcm->card = card; pcm->device = device; pcm->internal = internal; + mutex_init(&pcm->open_mutex); + init_waitqueue_head(&pcm->open_wait); + INIT_LIST_HEAD(&pcm->list); if (id) strlcpy(pcm->id, id, sizeof(pcm->id)); if ((err = snd_pcm_new_stream(pcm, SNDRV_PCM_STREAM_PLAYBACK, playback_count)) < 0) { @@ -792,8 +794,6 @@ static int _snd_pcm_new(struct snd_card *card, const char *id, int device, snd_pcm_free(pcm); return err; } - mutex_init(&pcm->open_mutex); - init_waitqueue_head(&pcm->open_wait); if ((err = snd_device_new(card, SNDRV_DEV_PCM, pcm, &ops)) < 0) { snd_pcm_free(pcm); return err; @@ -1075,15 +1075,16 @@ static int snd_pcm_dev_register(struct snd_device *device) if (snd_BUG_ON(!device || !device->device_data)) return -ENXIO; pcm = device->device_data; + if (pcm->internal) + return 0; + mutex_lock(®ister_mutex); err = snd_pcm_add(pcm); - if (err) { - mutex_unlock(®ister_mutex); - return err; - } + if (err) + goto unlock; for (cidx = 0; cidx < 2; cidx++) { int devtype = -1; - if (pcm->streams[cidx].substream == NULL || pcm->internal) + if (pcm->streams[cidx].substream == NULL) continue; switch (cidx) { case SNDRV_PCM_STREAM_PLAYBACK: @@ -1098,9 +1099,8 @@ static int snd_pcm_dev_register(struct snd_device *device) &snd_pcm_f_ops[cidx], pcm, &pcm->streams[cidx].dev); if (err < 0) { - list_del(&pcm->list); - mutex_unlock(®ister_mutex); - return err; + list_del_init(&pcm->list); + goto unlock; }
for (substream = pcm->streams[cidx].substream; substream; substream = substream->next) @@ -1110,8 +1110,9 @@ static int snd_pcm_dev_register(struct snd_device *device) list_for_each_entry(notify, &snd_pcm_notify_list, list) notify->n_register(pcm);
+ unlock: mutex_unlock(®ister_mutex); - return 0; + return err; }
static int snd_pcm_dev_disconnect(struct snd_device *device)
Notifier shouldn't listen to the changes of internal PCMs.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 92bbb5143b83..3cf345426171 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -888,8 +888,9 @@ static int snd_pcm_free(struct snd_pcm *pcm)
if (!pcm) return 0; - list_for_each_entry(notify, &snd_pcm_notify_list, list) { - notify->n_unregister(pcm); + if (!pcm->internal) { + list_for_each_entry(notify, &snd_pcm_notify_list, list) + notify->n_unregister(pcm); } if (pcm->private_free) pcm->private_free(pcm); @@ -1129,7 +1130,7 @@ static int snd_pcm_dev_disconnect(struct snd_device *device) mutex_lock(&pcm->open_mutex); wake_up(&pcm->open_wait); list_del_init(&pcm->list); - for (cidx = 0; cidx < 2; cidx++) + for (cidx = 0; cidx < 2; cidx++) { for (substream = pcm->streams[cidx].substream; substream; substream = substream->next) { snd_pcm_stream_lock_irq(substream); if (substream->runtime) { @@ -1139,8 +1140,10 @@ static int snd_pcm_dev_disconnect(struct snd_device *device) } snd_pcm_stream_unlock_irq(substream); } - list_for_each_entry(notify, &snd_pcm_notify_list, list) { - notify->n_disconnect(pcm); + } + if (!pcm->internal) { + list_for_each_entry(notify, &snd_pcm_notify_list, list) + notify->n_disconnect(pcm); } for (cidx = 0; cidx < 2; cidx++) { snd_unregister_device(&pcm->streams[cidx].dev);
Some codes in snd_pcm_dev_disconnect() are still valid even for internal PCMs, but they are skipped because of the check of list_empty(&pcm->list) at the beginning. Remove this check and put pcm->internal checks appropriately for internal PCM object to process through this function.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 3cf345426171..64e726658ca1 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -1124,9 +1124,6 @@ static int snd_pcm_dev_disconnect(struct snd_device *device) int cidx;
mutex_lock(®ister_mutex); - if (list_empty(&pcm->list)) - goto unlock; - mutex_lock(&pcm->open_mutex); wake_up(&pcm->open_wait); list_del_init(&pcm->list); @@ -1146,14 +1143,14 @@ static int snd_pcm_dev_disconnect(struct snd_device *device) notify->n_disconnect(pcm); } for (cidx = 0; cidx < 2; cidx++) { - snd_unregister_device(&pcm->streams[cidx].dev); + if (!pcm->internal) + snd_unregister_device(&pcm->streams[cidx].dev); if (pcm->streams[cidx].chmap_kctl) { snd_ctl_remove(pcm->card, pcm->streams[cidx].chmap_kctl); pcm->streams[cidx].chmap_kctl = NULL; } } mutex_unlock(&pcm->open_mutex); - unlock: mutex_unlock(®ister_mutex); return 0; }
On Fri, 2015-02-20 at 17:23 +0100, Takashi Iwai wrote:
Hi,
while reading the internal PCM (BE) handling code again, I noticed a few potential bugs. Here are fix and cleanup patches.
Takashi
===
Takashi Iwai (4): ALSA: pcm: Minor refactoring in snd_pcm_attach_substream() ALSA: pcm: Don't add internal PCMs to PCM device list ALSA: pcm: Don't notify internal PCMs ALSA: pcm: Don't ignore internal PCMs in snd_pcm_dev_disconnect()
sound/core/pcm.c | 92 +++++++++++++++++++++++++------------------------------- 1 file changed, 41 insertions(+), 51 deletions(-)
All
Acked-by: Liam Girdwood liam.r.girdwood@linux.intel.com
At Fri, 20 Feb 2015 18:42:23 +0000, Liam Girdwood wrote:
On Fri, 2015-02-20 at 17:23 +0100, Takashi Iwai wrote:
Hi,
while reading the internal PCM (BE) handling code again, I noticed a few potential bugs. Here are fix and cleanup patches.
Takashi
===
Takashi Iwai (4): ALSA: pcm: Minor refactoring in snd_pcm_attach_substream() ALSA: pcm: Don't add internal PCMs to PCM device list ALSA: pcm: Don't notify internal PCMs ALSA: pcm: Don't ignore internal PCMs in snd_pcm_dev_disconnect()
sound/core/pcm.c | 92 +++++++++++++++++++++++++------------------------------- 1 file changed, 41 insertions(+), 51 deletions(-)
All
Acked-by: Liam Girdwood liam.r.girdwood@linux.intel.com
Thanks, added your ack to topic/pcm-internal branch.
Takashi
participants (2)
-
Liam Girdwood
-
Takashi Iwai