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

[Takashi Sakamoto]

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 at 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