Re: [PATCH v8 3/8] ALSA: usb: babyfacepro: add mic preamp, phantom/pad and input trim

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ALSA: usb: babyfacepro: bf_trim_apply() and bf_phase_apply() clobber xpoints
- [Medium] ALSA: usb: babyfacepro: hardcoded Mic 1 names in babyface_create_preamp()
- [Low] ALSA: usb: babyfacepro: unchecked snd_ctl_new1() in babyface_create_preamp()
- [Low] ALSA: usb: babyfacepro: bf_phase_info() and bf_bool_info() duplicate code
--

commit 82ef805ee77606164dc3814bf736734ca60721d5
Author: Ismaïl Bahloul <i.bahloul01@gmail.com>

ALSA: usb: babyfacepro: add mic preamp, phantom/pad and input trim

This commit adds ALSA mixer controls for the input side of the RME Babyface
Pro, including phantom power, pad, instrument reference level, mic/instrument
preamp gains, phase invert, stereo split, and input trim.

> diff --git a/sound/usb/babyfacepro/babyfacepro-ctl.c b/sound/usb/babyfacepro/babyfacepro-ctl.c
> index 942d4732cbe07..fe6266ea118e6 100644
> --- a/sound/usb/babyfacepro/babyfacepro-ctl.c
> +++ b/sound/usb/babyfacepro/babyfacepro-ctl.c
> @@ -654,3 +654,627 @@ int babyface_create_masters(struct snd_usb_babyface *chip)

[ ... ]

> +int bf_phase_apply(struct snd_usb_babyface *chip, int mic, bool invert)
> +{
> +	const struct bf_source *s = &bf_sources[mic];
> +	int out, ret;
> +	u16 flag;
> +
> +	for (out = 0; out < 6; out++) {
> +		u16 plain = chip->xpoint[out][mic][0];
> +		u16 value = invert ? (u16)~plain : plain;
> +
> +		/* The AN1/2 block is written without the rotating bits, as
> +		 * bf_xpoint_write() does.
> +		 */
> +		flag = 0;
> +		if (out) {
> +			flag = bf_flag_cycle[chip->flag_cnt];
> +			chip->flag_cnt = (chip->flag_cnt + 1) & 3;
> +		}
> +		ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, value,
> +				      (BF_REG_XPOINT_BASE_L +
> +				       BF_REG_XPOINT_STRIDE * out + s->idx_l) |
> +				      flag);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	return 0;
> +}

[ ... ]

> +int bf_split_apply(struct snd_usb_babyface *chip, int pb, bool split)
> +{
> +	const struct bf_source *s = &bf_sources[8 + pb];
> +	u16 l = split ? 0x2000 : chip->xpoint[0][8 + pb][0];
> +	u16 r = split ? 0x0000 : chip->xpoint[0][8 + pb][1];
> +	int ret;
> +
> +	/* The AN1/2 block.  The same values also went to a "standard map"
> +	 * half at 0x0068, which is AS1/2's submix, not part of AN1/2: it
> +	 * overwrote AS1/2's own crosspoint for this playback pair.
> +	 */
> +	ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, l,
> +			      BF_REG_XPOINT_BASE_L + s->idx_l);
> +	if (ret < 0)
> +		return ret;
> +	return bf_vendor_write(chip, BF_REQ_CROSSPOINT, r,
> +			       BF_REG_XPOINT_BASE_R + s->idx_r);
> +}

[ ... ]

> +int bf_trim_apply(struct snd_usb_babyface *chip, int mic, int trim_db2)
> +{
> +	int base = mic & ~1;
> +	int sib = base + 1;
> +	const struct bf_source *sb = &bf_sources[base];
> +	const struct bf_source *ss = &bf_sources[sib];
> +	u16 trim_raw = bf_master_16bit(trim_db2);
> +	int ret;
> +
> +	ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, trim_raw,
> +			      BF_REG_LOWMAP_BASE_L + sb->idx_l);
> +	if (ret < 0)
> +		return ret;
> +	ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, trim_raw,
> +			      BF_REG_LOWMAP_BASE_R + sb->idx_r);
> +	if (ret < 0)
> +		return ret;
> +	ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, trim_raw,
> +			      BF_REG_LOWMAP_BASE_L + ss->idx_l);
> +	if (ret < 0)
> +		return ret;
> +	return bf_vendor_write(chip, BF_REQ_CROSSPOINT, trim_raw,
> +			       BF_REG_LOWMAP_BASE_R + ss->idx_r);
> +}

[ ... ]

> +static int bf_trim_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_usb_babyface *chip = snd_kcontrol_chip(kctl);
> +	int mic = kctl->private_value;
> +	int sib = mic ^ 1;
> +	int db = ucontrol->value.integer.value[0];
> +	int ret = 0;
> +
> +	if (db < -65 || db > 6)
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &chip->mutex) {
> +		if (db == chip->trim[mic])
> +			return 0;
> +		ret = bf_trim_apply(chip, mic, db * 2);
> +		if (ret < 0)
> +			return ret;

[Severity: High]
As reported by Sashiko on v7 [1], does bf_trim_apply() clobber the AN1/2
crosspoint state and unmute live analog inputs into the main outputs?

In sound/usb/babyfacepro/babyfacepro.h, BF_REG_LOWMAP_BASE_L (0x0000) and
BF_REG_LOWMAP_BASE_R (0x001a) are the exact same register addresses as
BF_REG_XPOINT_BASE_L and BF_REG_XPOINT_BASE_R for output pair 0 (the AN1/2
main output). In babyface_write_default_mixer(), analog inputs AN1-4 have
chip->xpoint[0][0..3][0/1] initialized to 0 so live microphones are not
routed directly into the speakers.

When bf_trim_put() calls bf_trim_apply(), it converts trim_db2 via
bf_master_16bit() (clamped to [1, 0x4000] where 0 dB is 0x2000, compared to
BF_FADER_0DB = 0x16a0 on the crosspoint curve) and writes trim_raw directly
to all four L and R crosspoint registers of the input pair (base and sib) on
AN1/2, ignoring chip->xpoint[0][base/sib][0/1]. Touching Trim Volume at all
(even at -65 dB, or returning it to 0 dB which writes 0x2000 instead of
restoring chip->xpoint[0][base/sib][0/1]) unconditionally routes both inputs
of the pair to both L and R of AN1/2, overriding any muted state, fader
level, or panning.

Because chip->xpoint[0][base/sib][0/1] remains 0 in the cache while the
hardware holds 0x2000, a subsequent write of 0 to the crosspoint control hits
the early return in bf_xpoint_put():

sound/usb/babyfacepro/babyfacepro-ctl.c:bf_xpoint_put() {
	guard(mutex)(&chip->mutex);
	if (l == chip->xpoint[out][src][0] && r == chip->xpoint[out][src][1])
		return 0;

	ret = bf_xpoint_write(chip, out, src, l, r);
}

and cannot re-mute the hardware.

Additionally, because bf_phase_apply(), bf_split_apply(), bf_trim_apply(),
and bf_xpoint_put() write to the shared crosspoint registers without
combining their state:
- bf_trim_apply() (both in bf_trim_put() and in bf_state_apply_flags() where
  the trim loop runs after the phase loop) overwrites the negated out = 0 L
  register written by bf_phase_apply().
- bf_phase_apply() only negates BF_REG_XPOINT_BASE_L (leaving R un-inverted,
  and writing ~0 = 0xffff when plain == 0).
- Any subsequent bf_xpoint_put() writes plain l/r values, silently wiping out
  active phase, stereo split, or trim settings in hardware while their cached
  control states remain active.

[ ... ]

> +static int bf_bool_info(struct snd_kcontrol *kctl,
> +			struct snd_ctl_elem_info *uinfo)
> +{
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
> +	uinfo->count = 1;
> +	uinfo->value.integer.min = 0;
> +	uinfo->value.integer.max = 1;
> +	return 0;
> +}

[Severity: Low]
This isn't a bug, but bf_bool_info() is identical to bf_phase_info() added
earlier in sound/usb/babyfacepro/babyfacepro-ctl.c (and to the existing
bf_mute_info()). Could these controls use the standard ALSA helper
snd_ctl_boolean_mono_info() from include/sound/control.h instead of
duplicating the callback?

[ ... ]

> +int babyface_create_preamp(struct snd_usb_babyface *chip)
> +{
> +	struct snd_kcontrol *kctl;
> +	int i, err;
> +
> +	/* The control name is the type and .index is the channel (0 =
> +	 * Mic 1/AN1, 1 = Mic 2/AN2), the convention the companion mixer
> +	 * and existing asound.state files use.
> +	 */
> +	for (i = 0; i < 2; i++) {
> +		u16 bit = i == 0 ? BF_PREAMP_48V_MIC1 : BF_PREAMP_48V_MIC2;
> +
> +		kctl = snd_ctl_new1(&(struct snd_kcontrol_new){
> +			.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +			.name = "Phantom Power Mic 1",
> +			.index = i,
> +			.info = bf_bool_info,
> +			.get = bf_phantom_get,
> +			.put = bf_phantom_put,
> +			.private_value = bit,
> +		}, chip);
> +		if (!kctl)
> +			return -ENOMEM;
> +		err = snd_ctl_add(chip->card, kctl);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	for (i = 0; i < 2; i++) {
> +		u16 bit = i == 0 ? BF_PREAMP_PAD_MIC1 : BF_PREAMP_PAD_MIC2;
> +
> +		kctl = snd_ctl_new1(&(struct snd_kcontrol_new){
> +			.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +			.name = "Pad Mic 1",
> +			.index = i,
> +			.info = bf_bool_info,
> +			.get = bf_phantom_get,
> +			.put = bf_phantom_put,
> +			.private_value = bit,
> +		}, chip);
> +		if (!kctl)
> +			return -ENOMEM;
> +		err = snd_ctl_add(chip->card, kctl);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	kctl = snd_ctl_new1(&(struct snd_kcontrol_new){
> +		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +		.name = "Instrument Ref Level",
> +		.info = bf_reflevel_info,
> +		.get = bf_reflevel_get,
> +		.put = bf_reflevel_put,
> +	}, chip);
> +	err = snd_ctl_add(chip->card, kctl);
> +	if (err < 0)
> +		return err;

[Severity: Low]
If snd_ctl_new1() returns NULL for the "Instrument Ref Level" control in
babyface_create_preamp(), passing NULL to snd_ctl_add() will return -EINVAL
instead of -ENOMEM. Should this check if (!kctl) return -ENOMEM; before
calling snd_ctl_add() like the other control creation calls in this function?

> +
> +	for (i = 0; i < 4; i++) {
> +		kctl = snd_ctl_new1(&(struct snd_kcontrol_new){
> +			.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +			.name = "Mic 1 Capture Volume",
> +			.index = i,
> +			.access = SNDRV_CTL_ELEM_ACCESS_READWRITE |
> +				  SNDRV_CTL_ELEM_ACCESS_TLV_READ,
> +			.info = bf_gain_info,
> +			.get = bf_gain_get,
> +			.put = bf_gain_put,
> +			.tlv.p = bf_gain_tlv,
> +			.private_value = i,
> +		}, chip);

[Severity: Medium]
As reported by Sashiko on v7 [1], why do all three loops in
babyface_create_preamp() hardcode "Mic 1" in .name ("Phantom Power Mic 1",
"Pad Mic 1", and "Mic 1 Capture Volume") across all loop iterations?

When i > 0, this registers Mic 2 phantom power and pad as "Phantom Power
Mic 1",1 and "Pad Mic 1",1, and registers Mic 2, Instrument 3, and Instrument
4 capture gains as "Mic 1 Capture Volume" with indices 1, 2, and 3. Should
these either use a channel-neutral name when indexing by i, or format the
per-channel name as babyface_create_trim() does?

[1] https://lore.kernel.org/r/20261027113140.134257-4-i.bahloul01@gmail.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105151553.223722-1-i.bahloul01@gmail.com?part=3