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

[Takashi Iwai <tiwai@suse.de>]

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.


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

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


Takashi