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

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

[Rafael Gago via B4 Relay]

From: Rafael Gago <rafael.gago@gmail.com>

hdsp_set_rate() refuses to switch between single, double and quad
speed whenever capture_pid or playback_pid is set. Both are set in the
PCM open callbacks, so the substream that asks for the new rate in
hw_params always blocks itself:

  cannot change speed mode (capture PID = -1, playback PID = 4242)

As a result no application can move an HDSP from 48 kHz to 96 kHz (or
to 192 kHz on the H9632) through the normal ALSA API. Rate changes
within one speed mode work, and a speed mode change only works through
the "Sample Clock Source" control while no PCM is open, which is what
users have to script around today.

The restriction exists because a speed mode change moves the channels
to other DMA buffer slots (hdsp->channel_map) and changes the channel
count, which must not happen under a substream that was already set up
with the old layout. The substream calling hw_params is about to be set
up anyway, so it is not such a substream.

Pass the requesting substream to hdsp_set_rate() in place of the
called_internally flag, and from hw_params only refuse the speed mode
change if the other direction is open and past the OPEN state, that is,
it has hw_params of its own. A substream of another process is still
turned away by the existing check in snd_hdsp_hw_params(), and requests
from the controls and from card initialization (substream == NULL) keep
the old rule of no open substreams at all.

A duplex client therefore opens both directions, sets hw_params on the
first one (which may change the speed mode) and then on the second one
at the same rate. To change the speed mode later it has to hw_free or
close the other direction first.

Tested on an HDSP 9632 (v7.0 based kernel, same hdsp.c as v7.0) with
aplay/arecord -M on hw:DSP: one stream moves the clock between 44.1k,
48k, 88.2k, 96k and 192k in both directions; alsaloop runs duplex in one
process at 48k/12ch, 96k/8ch and 192k/4ch; while another process plays
at 96k, a capture at 96k works and one at 48k or 192k is still refused;
the "Sample Clock Source" control is still refused while a stream is
open. REAPER on raw ALSA now changes the project rate between speed
modes without presetting the clock first. Also built with W=1 on
current mainline without warnings.

Only the HDSP 9632 was available for testing; the Digiface, Multiface,
RPM and 9652 were not. The change is not model specific: the speed
mode switch itself is the same code the "Sample Clock Source" control
already runs on every model, and it still only happens while no stream
is transferring data. The per-model channel counts and channel maps
come from the driver's existing tables.

A Claude AI coding assistant (Claude Code) read the driver, wrote this
change, the changelog and the tests at the request of the submitter,
who reviewed them.

Signed-off-by: Rafael Gago <rafael.gago@gmail.com>
---
 sound/pci/rme9652/hdsp.c | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/sound/pci/rme9652/hdsp.c b/sound/pci/rme9652/hdsp.c
index 7dce548b9..a3dfa937d 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;
+}
+
+/*
+ * substream is the stream asking for the rate in hw_params, or NULL for a
+ * request from a control or from card initialization.
+ */
+static int hdsp_set_rate(struct hdsp *hdsp, int rate,
+			 struct snd_pcm_substream *substream)
 {
 	int reject_if_open = 0;
 	int current_rate;
@@ -1107,7 +1135,7 @@ static int hdsp_set_rate(struct hdsp *hdsp, int rate, int called_internally)
 	*/
 
 	if (!(hdsp->control_register & HDSP_ClockModeMaster)) {
-		if (called_internally) {
+		if (!substream) {
 			/* request from ctl or card initialization */
 			dev_err(hdsp->card->dev,
 				"device is not running as a clock master: cannot set sample rate.\n");
@@ -1134,7 +1162,7 @@ static int hdsp_set_rate(struct hdsp *hdsp, int rate, int called_internally)
 	current_rate = hdsp->system_sample_rate;
 
 	/* Changing from a "single speed" to a "double speed" rate is
-	   not allowed if any substreams are open. This is because
+	   not allowed while another substream is set up. This is because
 	   such a change causes a shift in the location of
 	   the DMA buffers and a reduction in the number of available
 	   buffers.
@@ -1196,7 +1224,7 @@ static int hdsp_set_rate(struct hdsp *hdsp, int rate, int called_internally)
 		return -EINVAL;
 	}
 
-	if (reject_if_open && (hdsp->capture_pid >= 0 || hdsp->playback_pid >= 0)) {
+	if (reject_if_open && hdsp_speed_change_blocked(hdsp, substream)) {
 		dev_warn(hdsp->card->dev,
 			 "cannot change speed mode (capture PID = %d, playback PID = %d)\n",
 			    hdsp->capture_pid,
@@ -1939,7 +1967,7 @@ static int hdsp_set_clock_source(struct hdsp *hdsp, int mode)
 	switch (mode) {
 	case HDSP_CLOCK_SOURCE_AUTOSYNC:
 		if (hdsp_external_sample_rate(hdsp) != 0) {
-		    if (!hdsp_set_rate(hdsp, hdsp_external_sample_rate(hdsp), 1)) {
+		    if (!hdsp_set_rate(hdsp, hdsp_external_sample_rate(hdsp), NULL)) {
 			hdsp->control_register &= ~HDSP_ClockModeMaster;
 			hdsp_write(hdsp, HDSP_controlRegister, hdsp->control_register);
 			return 0;
@@ -1978,7 +2006,7 @@ static int hdsp_set_clock_source(struct hdsp *hdsp, int mode)
 	}
 	hdsp->control_register |= HDSP_ClockModeMaster;
 	hdsp_write(hdsp, HDSP_controlRegister, hdsp->control_register);
-	hdsp_set_rate(hdsp, rate, 1);
+	hdsp_set_rate(hdsp, rate, NULL);
 	return 0;
 }
 
@@ -3815,7 +3843,7 @@ static int snd_hdsp_set_defaults(struct hdsp *hdsp)
 	/* set a default rate so that the channel map is set up.
 	 */
 
-	hdsp_set_rate(hdsp, 48000, 1);
+	hdsp_set_rate(hdsp, 48000, NULL);
 
 	return 0;
 }
@@ -4048,7 +4076,7 @@ static int snd_hdsp_hw_params(struct snd_pcm_substream *substream,
 	 */
 
 	if (! hdsp->clock_source_locked) {
-		err = hdsp_set_rate(hdsp, params_rate(params), 0);
+		err = hdsp_set_rate(hdsp, params_rate(params), substream);
 		if (err < 0) {
 			_snd_pcm_hw_param_setempty(params, SNDRV_PCM_HW_PARAM_RATE);
 			return err;

---
base-commit: 231efd8f68c950ad3b774ff51c6d15fdd0443a5b
change-id: 20261101-hdsp-sound-1faea0a4f83f

Best regards,
--  
Rafael Gago <rafael.gago@gmail.com>

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

[sashiko-bot]

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