Re: [PATCH] ALSA: hdsp: allow sample rate changes from hw_params

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: hdsp: race on other->runtime->state in hdsp_speed_change_blocked()
--

commit 6c483175b4a8f7fb76733a5685d3d140f485defd
Author: Rafael Gago <rafael.gago@gmail.com>

ALSA: hdsp: allow sample rate changes from hw_params

This commit updates hdsp_set_rate() to accept the requesting PCM substream
and adds hdsp_speed_change_blocked() so that speed mode transitions from
snd_hdsp_hw_params() are only blocked when the opposite substream is open and
past SNDRV_PCM_STATE_OPEN.

> diff --git a/sound/pci/rme9652/hdsp.c b/sound/pci/rme9652/hdsp.c
> index 7dce548b9f363..a3dfa937d7e54 100644
> --- a/sound/pci/rme9652/hdsp.c
> +++ b/sound/pci/rme9652/hdsp.c
> @@ -1095,7 +1095,35 @@ static void hdsp_set_dds_value(struct hdsp *hdsp, int rate)
>  	hdsp_write(hdsp, HDSP_freqReg, hdsp->dds_value);
>  }
>  
> -static int hdsp_set_rate(struct hdsp *hdsp, int rate, int called_internally)
> +/*
> + * A speed mode change moves the channels to different DMA buffer slots
> + * (hdsp->channel_map) and changes the channel count. It is only safe if
> + * no substream other than the one asking for it (if any) was set up with
> + * the old layout. A substream of another process is already turned away
> + * by snd_hdsp_hw_params().
> + */
> +static bool hdsp_speed_change_blocked(struct hdsp *hdsp,
> +				      struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_substream *other;
> +
> +	if (!substream)
> +		return hdsp->capture_pid >= 0 || hdsp->playback_pid >= 0;
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		other = hdsp->capture_substream;
> +	else
> +		other = hdsp->playback_substream;
> +
> +	return other && other->runtime->state != SNDRV_PCM_STATE_OPEN;
> +}

[Severity: Medium]
Can two concurrent snd_pcm_hw_params() calls on the playback and capture
substreams race here and switch hdsp->channel_map underneath a substream that
has just finished snd_hdsp_hw_params()?

When a single thread opens both directions, hdsp->playback_pid and
hdsp->capture_pid match, so snd_hdsp_hw_params() allows both substreams to
proceed to hdsp_set_rate(). In sound/core/pcm_native.c:snd_pcm_hw_params(),
each substream only holds its own runtime->buffer_mutex, and runtime->state
is not updated to SNDRV_PCM_STATE_SETUP until after substream->ops->hw_params
returns:

sound/core/pcm_native.c:snd_pcm_hw_params() {
	...
	if (substream->ops->hw_params != NULL) {
		err = substream->ops->hw_params(substream, params);
		if (err < 0)
			goto _error;
	}

	runtime->access = params_access(params);
	runtime->format = params_format(params);
	runtime->subformat = params_subformat(params);
	runtime->channels = params_channels(params);
	runtime->rate = params_rate(params);
	...
	snd_pcm_set_state(substream, SNDRV_PCM_STATE_SETUP);
}

If one thread calls snd_pcm_hw_params() on playback at single speed (for
example 48 kHz with ss_out_channels), snd_hdsp_hw_params() sets the
single-speed hdsp->channel_map and releases hdsp->lock while
playback_substream->runtime->state is still SNDRV_PCM_STATE_OPEN.

Before that first thread reaches snd_pcm_set_state(), a second thread (or
forked process sharing the open file descriptors) calling snd_pcm_hw_params()
on capture at double or quad speed (96 kHz or 192 kHz) will still observe
playback_substream->runtime->state == SNDRV_PCM_STATE_OPEN in
hdsp_speed_change_blocked() (which is also read without holding the other
substream's PCM stream lock or buffer_mutex). It will then overwrite
hdsp->channel_map with the double-speed or quad-speed map while the playback
substream finishes snd_pcm_hw_params() with runtime->channels set to
ss_out_channels.

Subsequent transfers on the higher channel numbers in snd_hdsp_playback_copy(),
snd_hdsp_capture_copy(), or snd_hdsp_hw_silence() will then look up
hdsp->channel_map[channel] == -1 via hdsp_channel_buffer_location() and
trigger snd_BUG_ON(!channel_buf):

sound/pci/rme9652/hdsp.c:snd_hdsp_playback_copy() {
	...
	channel_buf = hdsp_channel_buffer_location (hdsp, substream->pstr->stream, channel);
	if (snd_BUG_ON(!channel_buf))
		return -EIO;
	...
}

Would tracking whether each direction has configured hw_params in struct hdsp
under hdsp->lock (and clearing it in snd_hdsp_hw_free() and release) avoid
relying on other->runtime->state across the lock boundary?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-hdsp-sound-v1-1-f39f185400b5@gmail.com?part=1