Re: [PATCH v7 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 6 potential issue(s) to consider:
- [High] ALSA: usb: babyfacepro: bf_trim_apply() and bf_split_apply() clobber xpoints
- [High] ALSA: usb: babyfacepro: unchecked snd_ctl_new1() in babyface_create_trim()
- [Medium] ALSA: usb: babyfacepro: hardcoded Mic 1 names in babyface_create_preamp()
- [Medium] ALSA: usb: babyfacepro: unsynced ref_level on readback in babyface_probe()
- [Low] ALSA: usb: babyfacepro: unlocked preamp read in bf_phantom_put()
- [Low] ALSA: usb: babyfacepro: unused dead function bf_gain_db()
--

commit b41b007f1eed261ac67b9aeaa1416892ba18f7af
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 microphone phantom power, pad,
instrument reference level, preamp capture gain, phase invert, stereo split,
and input trim on the RME Babyface Pro.

> diff --git a/sound/usb/babyfacepro/babyfacepro-ctl.c b/sound/usb/babyfacepro/babyfacepro-ctl.c
> index 9b41dcde5308a..60604242be883 100644
> --- a/sound/usb/babyfacepro/babyfacepro-ctl.c
> +++ b/sound/usb/babyfacepro/babyfacepro-ctl.c
> @@ -654,3 +654,629 @@ 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 : 0x1000;
> +	u16 r = split ? 0x0000 : 0x1000;
> +	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);
> +}

[Severity: High]
Won't bf_trim_apply() and bf_split_apply() clobber the active AN1/2
crosspoint fader levels, panning, and phase inversion?

In babyfacepro.h, BF_REG_LOWMAP_BASE_L (0x0000) and BF_REG_LOWMAP_BASE_R
(0x001a) are aliases for BF_REG_XPOINT_BASE_L and BF_REG_XPOINT_BASE_R,
which are the crosspoint registers for output pair 0 (the AN1/2 main analog
output).

Although the commit message states that trim combines with the current fader
value on the crosspoint fader curve, bf_trim_apply() ignores
chip->xpoint[0][...] completely. Instead, it converts trim_db2 using
bf_master_16bit() (where 0 dB is 0x2000 and the return value is clamped to
[1, 0x4000]) and writes trim_raw directly to all four L and R crosspoint
registers of the input pair on AN1/2.

By default in babyface_write_default_mixer(), analog inputs AN1-4 have
chip->xpoint[0][0..3][0/1] set to 0 so live microphones are not routed into
the main speakers. Writing to Trim Volume via bf_trim_put() ->
bf_trim_apply() (even setting it to a value and back to 0 dB during alsactl
restore) writes 0x2000 into all four AN1/2 crosspoint registers for that
input pair, unmuting both live inputs into both L and R of the main speakers
while the AN1..AN4 Playback Volume controls still report 0. Even at -65 dB,
bf_master_16bit() clamps to 1 rather than 0.

In addition, bf_trim_apply() overwrites any active phase inversion in
chip->phase[] on out = 0 (including inside bf_state_apply_flags(), where the
trim loop runs after the phase loop and overwrites the negated L register
just written by bf_phase_apply() for out = 0). In bf_phase_apply() itself,
only BF_REG_XPOINT_BASE_L is inverted while BF_REG_XPOINT_BASE_R is left
untouched, and when plain == 0 (muted), ~plain writes 0xffff.

Similarly, when disabling stereo split (split == false) via bf_split_put() ->
bf_split_apply(), hardcoded 0x1000 (-6 dB) is written to the AN1/2 L and R
crosspoint registers instead of restoring the cached crosspoint fader values
in chip->xpoint[0][8 + pb][0/1] (which default to BF_FADER_0DB = 0x16a0).
Conversely, any subsequent bf_xpoint_put() -> bf_xpoint_write() writes plain
chip->xpoint[out][src][0/1] values, wiping out active phase, split, or trim
state on the hardware while their control caches remain set.

[ ... ]

> +int babyface_create_trim(struct snd_usb_babyface *chip)
> +{
> +	struct snd_kcontrol *kctl;
> +	int src, err;
> +
> +	for (src = 0; src < 4; src++) {
> +		kctl = snd_ctl_new1(&(struct snd_kcontrol_new){
> +			.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +			.name = "Phase Switch",
> +			.index = src,
> +			.info = bf_phase_info,
> +			.get = bf_phase_get,
> +			.put = bf_phase_put,
> +			.private_value = src,
> +		}, chip);
> +		strscpy(kctl->id.name, bf_sources[src].name, sizeof(kctl->id.name));
> +		strlcat(kctl->id.name, " Phase Switch", sizeof(kctl->id.name));
> +		err = snd_ctl_add(chip->card, kctl);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	for (src = 0; src < 6; src++) {
> +		kctl = snd_ctl_new1(&(struct snd_kcontrol_new){
> +			.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +			.name = "Stereo Split Switch",
> +			.index = src,
> +			.info = bf_phase_info, /* plain boolean, same shape */
> +			.get = bf_split_get,
> +			.put = bf_split_put,
> +			.private_value = src,
> +		}, chip);
> +		strscpy(kctl->id.name, bf_sources[8 + src].name, sizeof(kctl->id.name));
> +		strlcat(kctl->id.name, " Stereo Split Switch", sizeof(kctl->id.name));
> +		err = snd_ctl_add(chip->card, kctl);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	for (src = 0; src < 4; src++) {
> +		kctl = snd_ctl_new1(&(struct snd_kcontrol_new){
> +			.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +			.name = "Trim Volume",
> +			.index = src,
> +			.access = SNDRV_CTL_ELEM_ACCESS_READWRITE |
> +				  SNDRV_CTL_ELEM_ACCESS_TLV_READ,
> +			.info = bf_trim_info,
> +			.get = bf_trim_get,
> +			.put = bf_trim_put,
> +			.tlv.p = bf_trim_tlv,
> +			.private_value = src,
> +		}, chip);
> +		chip->trim_kctl[src] = kctl;
> +		strscpy(kctl->id.name, bf_sources[src].name, sizeof(kctl->id.name));
> +		strlcat(kctl->id.name, " Trim Volume", sizeof(kctl->id.name));
> +		err = snd_ctl_add(chip->card, kctl);
> +		if (err < 0)
> +			return err;
> +	}
> +	return 0;
> +}

[Severity: High]
Can snd_ctl_new1() return NULL here and cause a NULL pointer dereference in
babyface_create_trim()?

In all three loops in babyface_create_trim(), kctl->id.name is passed to
strscpy() and strlcat() before snd_ctl_add() checks whether kctl is NULL.
(Note that babyface_create_xpoints() and babyface_create_masters() from the
earlier patch in this series have the same unchecked kctl->id.name
dereference after snd_ctl_new1().)

Also, in the Trim Volume loop, chip->trim_kctl[src] = kctl is assigned
before snd_ctl_add(chip->card, kctl). Since snd_ctl_add() frees kctl on
failure, wouldn't an error in snd_ctl_add() leave a dangling pointer in
chip->trim_kctl[src]?

[ ... ]

> +static int bf_phantom_put(struct snd_kcontrol *kctl,
> +			  struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_usb_babyface *chip = snd_kcontrol_chip(kctl);
> +	u16 bit = kctl->private_value;
> +	bool on = ucontrol->value.integer.value[0];
> +	bool cur = !!(chip->preamp & bit);
> +	u16 old = chip->preamp;
> +	int ret = 0;
> +
> +	mutex_lock(&chip->mutex);
> +	if (on == cur)
> +		goto out;
> +	chip->preamp = on ? (chip->preamp | bit) : (chip->preamp & ~bit);
> +	ret = bf_preamp_state_write(chip);
> +	if (ret < 0) {
> +		/* Do not leave the cache claiming a state the device
> +		 * never took.
> +		 */
> +		chip->preamp = old;
> +		goto out;
> +	}
> +	ret = 1;
> +out:
> +	mutex_unlock(&chip->mutex);
> +	return ret;
> +}

[Severity: Low]
Should cur and old be read from chip->preamp after acquiring chip->mutex in
bf_phantom_put()?

Currently, cur and old are sampled before mutex_lock(&chip->mutex), whereas
other writers (such as bf_reflevel_put() and the front-panel work handler
added later in the series) update chip->preamp under chip->mutex. Reading
chip->preamp before taking the lock could cause the change check or the error
rollback to use a stale value.

[ ... ]

> +int bf_gain_max_db(int mic)
> +{
> +	return mic < 2 ? BF_GAIN_MAX_DB : 9;
> +}
> +
> +int bf_gain_db(int mic, u8 raw)
> +{
> +	if (mic >= 2)
> +		return raw / 2;
> +	return 3 * (raw & BF_GAIN_COARSE_MASK) + (raw >> BF_GAIN_FINE_SHIFT);
> +}

[Severity: Low]
Is bf_gain_db() needed?

It is defined here and declared in babyfacepro.h, but it has no callers in
this patch or across the rest of the patch series because chip->gain[]
stores the value in dB and converts to the raw register encoding via
bf_gain_raw() on write.

[ ... ]

> +int babyface_create_preamp(struct snd_usb_babyface *chip)
> +{
> +	struct snd_kcontrol *kctl;
> +	int i, err;
> +
> +	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);
> +		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);
> +		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;
> +
> +	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);
> +		err = snd_ctl_add(chip->card, kctl);
> +		if (err < 0)
> +			return err;
> +	}
> +	return 0;
> +}

[Severity: Medium]
Are the control names in babyface_create_preamp() intentionally hardcoded to
"Mic 1" for all channels?

The three loops iterate over i = 0..1 for phantom power, i = 0..1 for pad,
and i = 0..3 for capture gain (covering Mic 1, Mic 2, Instrument 3, and
Instrument 4), but every iteration uses "Phantom Power Mic 1", "Pad Mic 1",
and "Mic 1 Capture Volume" with only .index = i varying. Should these Either
omit "Mic 1" when using .index = i, or use per-channel names?

> diff --git a/sound/usb/babyfacepro/babyfacepro.c b/sound/usb/babyfacepro/babyfacepro.c
> index de1659307fbc0..c2835e2d64851 100644
> --- a/sound/usb/babyfacepro/babyfacepro.c
> +++ b/sound/usb/babyfacepro/babyfacepro.c

[ ... ]

> @@ -1429,6 +1507,15 @@ static int babyface_probe(struct usb_interface *intf,
>  		goto error;
>  	}
>  
> +	/* Sync the preamp state from the 0x17 readback (byte 0 mirrors
> +	 * the 48V/PAD bits; it persists across power cycles).
> +	 */
> +	err = bf_vendor_read(chip, BF_REQ_PREAMP, BF_REG_PREAMP, st);
> +	if (err < 0)
> +		dev_dbg(&intf->dev, "preamp readback failed: %d\n", err);
> +	else
> +		chip->preamp = st[0];

[Severity: Medium]
Does assigning chip->preamp = st[0] in babyface_probe() leave
chip->ref_level out of sync with chip->preamp?

Earlier in babyface_probe(), chip->preamp is initialized to BF_PREAMP_BASE
(BF_PREAMP_REF_4DBU, 0x000c) while chip->ref_level is 0 (BF_REF_LEVEL_4DBU).
When bf_vendor_read() succeeds and st[0] has the BF_PREAMP_REF_MASK (0x000c)
bits clear, chip->preamp has its ref-level bits cleared (-10dBV) while
chip->ref_level remains 0 (+4dBu).

After that, bf_reflevel_get() reports +4dBu even though the ref-level bits in
chip->preamp are clear, any phantom power or pad toggle in bf_phantom_put()
writes -10dBV to the hardware, and trying to select +4dBu (item == 0) in
bf_reflevel_put() hits the early return:

	if ((int)item == chip->ref_level)
		goto out;

without setting BF_PREAMP_REF_4DBU in chip->preamp. Should babyface_probe()
either sync chip->ref_level from st[0] & BF_PREAMP_REF_MASK or preserve
BF_PREAMP_BASE when applying the readback byte?

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