Re: [alsa-devel] [PATCH 22/25] ALSA: firewire-tascam: add PCM functionality

On Aug 25 2015 06:12, Takashi Iwai wrote:

On Sat, 22 Aug 2015 11:19:38 +0200, Takashi Sakamoto wrote:

...

This commit adds PCM functionality to transmit/receive PCM samples.

When one of PCM substreams are running or external clock source is selected, current sampling rate is used. Else, the sampling rate is changed as an userspace application requests.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp

sound/firewire/tascam/Makefile | 2 +- sound/firewire/tascam/tascam-pcm.c | 287 +++++++++++++++++++++++++++++++++++++ sound/firewire/tascam/tascam.c | 4 + sound/firewire/tascam/tascam.h | 2 + 4 files changed, 294 insertions(+), 1 deletion(-) create mode 100644 sound/firewire/tascam/tascam-pcm.c

diff --git a/sound/firewire/tascam/Makefile b/sound/firewire/tascam/Makefile index 0a1c387..f075b9b 100644 --- a/sound/firewire/tascam/Makefile +++ b/sound/firewire/tascam/Makefile @@ -1,3 +1,3 @@ snd-firewire-tascam-objs := tascam-proc.o amdtp-tascam.o tascam-stream.o \

• 	    tascam.o
  

• 	    tascam-pcm.o tascam.o
  

  obj-$(CONFIG_SND_FIREWIRE_TASCAM) += snd-firewire-tascam.o

diff --git a/sound/firewire/tascam/tascam-pcm.c b/sound/firewire/tascam/tascam-pcm.c new file mode 100644 index 0000000..b1aa980 --- /dev/null +++ b/sound/firewire/tascam/tascam-pcm.c @@ -0,0 +1,287 @@ +/*

• • tascam-pcm.c - a part of driver for TASCAM FireWire series
• • Copyright (c) 2015 Takashi Sakamoto
• • Licensed under the terms of the GNU General Public License, version 2.
• */

+#include "tascam.h"

+static void set_buffer_params(struct snd_pcm_hardware *hw) +{

• hw->period_bytes_min = 4 * hw->channels_min;
• hw->period_bytes_max = hw->period_bytes_min * 2048;
• hw->buffer_bytes_max = hw->period_bytes_max * 2;
• hw->periods_min = 2;
• hw->periods_max = UINT_MAX;

+}

+static int pcm_init_hw_params(struct snd_tscm *tscm,

• 	      struct snd_pcm_substream *substream)
  

+{

• static const struct snd_pcm_hardware hardware = {

• .info = SNDRV_PCM_INFO_BATCH |
  

• 	SNDRV_PCM_INFO_BLOCK_TRANSFER |
  

• 	SNDRV_PCM_INFO_INTERLEAVED |
  

• 	SNDRV_PCM_INFO_JOINT_DUPLEX |
  

• 	SNDRV_PCM_INFO_MMAP |
  

• 	SNDRV_PCM_INFO_MMAP_VALID,
  

• .rates = SNDRV_PCM_RATE_44100 |
  

• 	 SNDRV_PCM_RATE_48000 |
  

• 	 SNDRV_PCM_RATE_88200 |
  

• 	 SNDRV_PCM_RATE_96000,
  

• .rate_min = 44100,
  

• .rate_max = 96000,
  

• .channels_min = 10,
  

• .channels_max = 18,
  

• };
• struct snd_pcm_runtime *runtime = substream->runtime;
• struct amdtp_stream *stream;
• unsigned int pcm_channels;
• runtime->hw = hardware;
• if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {

• runtime->hw.formats = SNDRV_PCM_FMTBIT_S32;
  

• stream = &tscm->tx_stream;
  

• pcm_channels = tscm->spec->pcm_capture_analog_channels;
  

• if (tscm->spec->has_adat)
  

• 	pcm_channels += 8;
  

• if (tscm->spec->has_spdif)
  

• 	pcm_channels += 2;
  

• runtime->hw.channels_min =
  

• 			runtime->hw.channels_max = pcm_channels;
  

• } else {

• runtime->hw.formats =
  

• 		SNDRV_PCM_FMTBIT_S16 | SNDRV_PCM_FMTBIT_S32;
  

• stream = &tscm->rx_stream;
  

• pcm_channels = tscm->spec->pcm_playback_analog_channels;
  

• if (tscm->spec->has_adat)
  

• 	pcm_channels += 8;
  

• if (tscm->spec->has_spdif)
  

• 	pcm_channels += 2;
  

• runtime->hw.channels_min =
  

• 			runtime->hw.channels_max = pcm_channels;
  

• }
• set_buffer_params(&runtime->hw);
• return amdtp_tscm_add_pcm_hw_constraints(stream, runtime);

+}

+static int pcm_open(struct snd_pcm_substream *substream) +{

• struct snd_tscm *tscm = substream->private_data;
• enum snd_tscm_clock clock;
• unsigned int rate;
• int err;
• err = pcm_init_hw_params(tscm, substream);
• if (err < 0)

• return err;
  

• err = snd_tscm_stream_get_clock(tscm, &clock);
• if ((clock != SND_TSCM_CLOCK_INTERNAL) |

•    amdtp_stream_pcm_running(&tscm->rx_stream) |
  

•    amdtp_stream_pcm_running(&tscm->tx_stream)) {
  

Must be '||'.

...

• err = snd_tscm_stream_get_rate(tscm, &rate);
  

• if (err < 0)
  

• 	return err;
  

• substream->runtime->hw.rate_min = rate;
  

• substream->runtime->hw.rate_max = rate;
  

• }
• snd_pcm_set_sync(substream);
• return err;

+}

+static int pcm_close(struct snd_pcm_substream *substream) +{

• return 0;

+}

+static int pcm_capture_hw_params(struct snd_pcm_substream *substream,

• 		 struct snd_pcm_hw_params *hw_params)
  

+{

• struct snd_tscm *tscm = substream->private_data;
• if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN) {

• mutex_lock(&tscm->mutex);
  

• tscm->substreams_counter++;
  

• mutex_unlock(&tscm->mutex);
  

• }

This looks dangerous. For example...

...

• amdtp_tscm_set_pcm_format(&tscm->tx_stream, params_format(hw_params));
• return snd_pcm_lib_alloc_vmalloc_buffer(substream,

• 				params_buffer_bytes(hw_params));
  

... if the buffer allocation fails, this return an error. And snd_pcm_hw_params() calls hw_free() at the error path. But hw_free() decreases the counter only when the state is not OPEN. At this moment, however, the state is still OPEN, so the counter is unbalanced.

Indeed. I missed this case. I think moving the increment of substream counter aftre keeping buffer.

And Fireworks/BeBoB/OXFW/Dice drivers have the same issue.

...

+} +static int pcm_playback_hw_params(struct snd_pcm_substream *substream,

Put a blank line (also in many places in this file).

Thanks

Takashi Sakamoto