| Message ID | 20191209103435.GA55498@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: gadget: u_audio: Initialize capture memory | expand |
On Mon, 09 Dec 2019 11:34:35 +0100, Lionel Koenig wrote: > > Using USB audio gadget in capture mode with an non blocking API may > result in getting uninitialized memory samples. That ensure that the > memory is initialized to 0 when the stream is prepared. > > Signed-off-by: Lionel Koenig <lionel.koenig@gmail.com> Thanks for the patch. The change itself looks right, but I believe that this should be better handled in ALSA PCM core side, as the problem itself is fairly generic. Also, the more appropriate place to perform memory clear is in hw_params callback, not in prepare. The former is usually called only once when the buffer is allocated, while the latter may be called repeatedly to make the stream ready for start (e.g. after resume). Below is a totally untested patch. It re-uses the existing function to fill silence for the given stream more generically. Let me know if this works. If it's OK, I'll submit and merge with a proper change log. thanks, Takashi --- diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 2236b5e0c1f2..3c63324b8bb7 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -30,9 +30,6 @@ #define trace_applptr(substream, prev, curr) #endif -static int fill_silence_frames(struct snd_pcm_substream *substream, - snd_pcm_uframes_t off, snd_pcm_uframes_t frames); - /* * fill ring buffer with silence * runtime->silence_start: starting pointer to silence area @@ -100,7 +97,7 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram ofs = runtime->silence_start % runtime->buffer_size; while (frames > 0) { transfer = ofs + frames > runtime->buffer_size ? runtime->buffer_size - ofs : frames; - err = fill_silence_frames(substream, ofs, transfer); + err = snd_pcm_fill_silence_frames(substream, ofs, transfer); snd_BUG_ON(err < 0); runtime->silence_filled += transfer; frames -= transfer; @@ -1945,8 +1942,6 @@ static int fill_silence(struct snd_pcm_substream *substream, int channel, { struct snd_pcm_runtime *runtime = substream->runtime; - if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK) - return 0; if (substream->ops->fill_silence) return substream->ops->fill_silence(substream, channel, hwoff, bytes); @@ -2030,10 +2025,10 @@ static int noninterleaved_copy(struct snd_pcm_substream *substream, } /* fill silence on the given buffer position; - * called from snd_pcm_playback_silence() + * called from snd_pcm_playback_silence() and snd_pcm_hw_params() */ -static int fill_silence_frames(struct snd_pcm_substream *substream, - snd_pcm_uframes_t off, snd_pcm_uframes_t frames) +int snd_pcm_fill_silence_frames(struct snd_pcm_substream *substream, + snd_pcm_uframes_t off, snd_pcm_uframes_t frames) { if (substream->runtime->access == SNDRV_PCM_ACCESS_RW_INTERLEAVED || substream->runtime->access == SNDRV_PCM_ACCESS_MMAP_INTERLEAVED) diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h index 384efd002984..ac4f455b1fff 100644 --- a/sound/core/pcm_local.h +++ b/sound/core/pcm_local.h @@ -34,6 +34,8 @@ int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream); void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_uframes_t new_hw_ptr); +int snd_pcm_fill_silence_frames(struct snd_pcm_substream *substream, + snd_pcm_uframes_t off, snd_pcm_uframes_t frames); static inline snd_pcm_uframes_t snd_pcm_avail(struct snd_pcm_substream *substream) diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 1fe581167b7b..d6f270c639b4 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -739,6 +739,9 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, while (runtime->boundary * 2 <= LONG_MAX - runtime->buffer_size) runtime->boundary *= 2; + /* clear the buffer once for avoiding information leaks */ + snd_pcm_fill_silence_frames(substream, 0, runtime->period_size); + snd_pcm_timer_resolution_change(substream); snd_pcm_set_state(substream, SNDRV_PCM_STATE_SETUP);
On Mon, Dec 09, 2019 at 02:07:58PM +0100, Takashi Iwai wrote: > Below is a totally untested patch. It re-uses the existing function > to fill silence for the given stream more generically. > > Let me know if this works. If it's OK, I'll submit and merge with a > proper change log. I tested the provided patch. It does work as I expected. Thanks a lot for that. Best regards, Lionel
diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c index 7ec6a996af26..6d708494ca77 100644 --- a/drivers/usb/gadget/function/u_audio.c +++ b/drivers/usb/gadget/function/u_audio.c @@ -317,6 +317,14 @@ static int uac_pcm_open(struct snd_pcm_substream *substream) return 0; } +static int uac_pcm_prepare(struct snd_pcm_substream *substream) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + + memset(runtime->dma_area, 0, runtime->dma_bytes); + return 0; +} + /* ALSA cries without these function pointers */ static int uac_pcm_null(struct snd_pcm_substream *substream) { @@ -331,7 +339,7 @@ static const struct snd_pcm_ops uac_pcm_ops = { .hw_free = uac_pcm_hw_free, .trigger = uac_pcm_trigger, .pointer = uac_pcm_pointer, - .prepare = uac_pcm_null, + .prepare = uac_pcm_prepare, }; static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep)
Using USB audio gadget in capture mode with an non blocking API may result in getting uninitialized memory samples. That ensure that the memory is initialized to 0 when the stream is prepared. Signed-off-by: Lionel Koenig <lionel.koenig@gmail.com> --- drivers/usb/gadget/function/u_audio.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)