Re: [PATCH v7 6/8] ALSA: usb: babyfacepro: add the front-panel poll and controls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] ALSA: usb: babyfacepro: MIX wheel stuck due to raw fader quantization
- [High] ALSA: usb: babyfacepro: unlocked panel_select races in bf_panel_tick()
- [High] ALSA: usb: babyfacepro: unchecked snd_ctl_new1() NULL dereference
- [Medium] ALSA: usb: babyfacepro: broken SELECT handling in bf_panel_mix_wheel()
- [Medium] ALSA: usb: babyfacepro: panel_saw_fader reset clobbered in bf_panel_tick()
- [Medium] ALSA: usb: babyfacepro: snd_card_register() called before panel init
- [Medium] ALSA: usb: babyfacepro: panel_start reset on resume breaks select put
- [Medium] ALSA: usb: babyfacepro: tick-based SELECT hold in bf_panel_tick()
- [Medium] ALSA: usb: babyfacepro: OUT wheel and resync overwrite active Phones DIM state
- [Medium] ALSA: usb: babyfacepro: stale panel_master_last after master updates
- [Low] ALSA: usb: babyfacepro: undocumented panel_poll_ms and urb_split params
--

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

ALSA: usb: babyfacepro: add the front-panel poll and controls

This commit adds periodic polling of the Babyface Pro front-panel state
via vendor request 0x17 and exposes the decoded buttons, rotary wheel,
and channel selection state as mixer controls. It also implements host-driven
handling for output volume, balance, preamp gain, phantom power, and
monitoring fader levels in response to front-panel input.

> diff --git a/sound/usb/babyfacepro/babyfacepro-ctl.c b/sound/usb/babyfacepro/babyfacepro-ctl.c
> index 39584ec719b23..cf41093dcbcaf 100644
> --- a/sound/usb/babyfacepro/babyfacepro-ctl.c
> +++ b/sound/usb/babyfacepro/babyfacepro-ctl.c
> @@ -611,6 +612,7 @@ int babyface_create_xpoints(struct snd_usb_babyface *chip)
>  			err = snd_ctl_add(chip->card, kctl);
>  			if (err < 0)
>  				return err;
> +			chip->xpoint_kctl[out][src] = kctl;
>  		}
>  	}

[Severity: High]
What happens in babyface_create_xpoints() if snd_ctl_new1() returns NULL
on allocation failure?

Before calling snd_ctl_add(chip->card, kctl) (which checks for a NULL
pointer), babyface_create_xpoints() writes directly into kctl->id.name:

    kctl = snd_ctl_new1(&(struct snd_kcontrol_new){
        ...
    }, chip);
    strscpy(kctl->id.name, bf_sources[src].name,
            sizeof(kctl->id.name));
    strlcat(kctl->id.name, " Playback Volume",
            sizeof(kctl->id.name));
    err = snd_ctl_add(chip->card, kctl);

The same pattern also appears in babyface_create_masters() and
babyface_create_trim(), which were introduced earlier in this series and
call strscpy() or strlcat() on kctl->id.name before snd_ctl_add().

Should these functions check for !kctl and return -ENOMEM before modifying
kctl->id.name?

> @@ -834,6 +836,83 @@ static int bf_split_put(struct snd_kcontrol *kctl,
>  	return ret;
>  }
>  
> +/* MIX-mode monitoring level (fader curve).
> + * Calibrated crosspoint-fader curve (AN1->AN1/2, cap_calib.pcap
> + * 2026-08-22; the same table as tuxmix-core/src/usb.rs FADER_CURVE).
> + * dB stored x2 (half-dB grid): the MIX wheel steps +/-0.5 dB per click
> + * on this curve (cap_mix.pcap).  0x0000 = -inf (digital mute),
> + * 0x0003 = -62 dB, ... 0x2D41 = +6 dB.  Raw values interpolate linearly
> + * between the 1-dB points.
> + */
> +#define BF_FADER_DB2_INF	(-130)	/* -65 dB = the wheel's -inf floor */
> +
> +static const struct bf_fader_pt {
> +	s16 db2;	/* dB x 2 */
> +	u16 raw;
> +} bf_fader_curve[] = {
> +	{ -124, 0x0003 }, { -122, 0x0004 }, { -120, 0x0005 },
> +	{ -118, 0x0006 }, { -116, 0x0007 }, { -114, 0x0008 },
> +	{ -112, 0x0009 }, { -110, 0x000a }, { -108, 0x000b },
> +	{ -106, 0x000d }, { -104, 0x000e }, { -102, 0x0010 },
> +	{ -100, 0x0012 }, {  -98, 0x0014 }, {  -96, 0x0017 },
> +	{  -94, 0x0019 }, {  -92, 0x001d }, {  -90, 0x0020 },
> +	{  -88, 0x0024 }, {  -86, 0x0029 }, {  -84, 0x002e },
> +	{  -82, 0x0033 }, {  -80, 0x003a }, {  -78, 0x0041 },
> +	{  -76, 0x0049 }, {  -74, 0x0051 }, {  -72, 0x005b },
> +	{  -70, 0x0067 }, {  -68, 0x0073 }, {  -66, 0x0081 },
> +	{  -64, 0x0091 }, {  -62, 0x00a3 }, {  -60, 0x00b7 },
> +	{  -58, 0x00cd }, {  -56, 0x00e6 }, {  -54, 0x0102 },
> +	{  -52, 0x0122 }, {  -50, 0x0145 }, {  -48, 0x016d },
> +	{  -46, 0x019a }, {  -44, 0x01cc }, {  -42, 0x0204 },
> +	{  -40, 0x0243 }, {  -38, 0x028a }, {  -36, 0x02d9 },
> +	{  -34, 0x0332 }, {  -32, 0x0396 }, {  -30, 0x0406 },
> +	{  -28, 0x0483 }, {  -26, 0x0510 }, {  -24, 0x05af },
> +	{  -22, 0x0660 }, {  -20, 0x0727 }, {  -18, 0x0807 },
> +	{  -16, 0x0902 }, {  -14, 0x0a1b }, {  -12, 0x0b57 },
> +	{  -10, 0x0cb9 }, {   -8, 0x0e47 }, {   -6, 0x1004 },
> +	{   -4, 0x11f9 }, {   -2, 0x142a }, {    0, 0x16a0 },
> +	{    2, 0x1963 }, {    4, 0x1c7c }, {    6, 0x1ff6 },
> +	{    8, 0x23dc }, {   10, 0x283d }, {   12, 0x2d41 },
> +};
> +
> +/* Fader raw -> dBx2 (linear interpolation; raw 0 = -inf). */
> +static int bf_fader_raw_to_db2(u16 raw)
> +{
> +	int i;
> +
> +	if (raw == 0 || raw < bf_fader_curve[0].raw)
> +		return BF_FADER_DB2_INF;
> +	for (i = 0; i < ARRAY_SIZE(bf_fader_curve) - 1; i++) {
> +		if (raw <= bf_fader_curve[i + 1].raw) {
> +			u32 num = (u32)(raw - bf_fader_curve[i].raw) *
> +				  (u32)(bf_fader_curve[i + 1].db2 - bf_fader_curve[i].db2);
> +			u32 den = bf_fader_curve[i + 1].raw - bf_fader_curve[i].raw;
> +
> +			return bf_fader_curve[i].db2 + (int)((num + den / 2) / den);
> +		}
> +	}
> +	return bf_fader_curve[ARRAY_SIZE(bf_fader_curve) - 1].db2;
> +}
> +
> +/* dBx2 -> fader raw (linear interpolation; below -62 dB = mute 0). */
> +static u16 bf_fader_db2_to_raw(int db2)
> +{
> +	int i;
> +
> +	if (db2 <= bf_fader_curve[0].db2)
> +		return db2 < bf_fader_curve[0].db2 ? 0 : bf_fader_curve[0].raw;
> +	for (i = 0; i < ARRAY_SIZE(bf_fader_curve) - 1; i++) {
> +		if (db2 <= bf_fader_curve[i + 1].db2) {
> +			u32 num = (u32)(db2 - bf_fader_curve[i].db2) *
> +				  (u32)(bf_fader_curve[i + 1].raw - bf_fader_curve[i].raw);
> +			u32 den = bf_fader_curve[i + 1].db2 - bf_fader_curve[i].db2;
> +
> +			return bf_fader_curve[i].raw + (u16)((num + den / 2) / den);
> +		}
> +	}
> +	return bf_fader_curve[ARRAY_SIZE(bf_fader_curve) - 1].raw;
> +}

[Severity: High]
Can the monitoring fader wheel in bf_panel_mix_wheel() get stuck at mute
(raw == 0) when turning up, and stuck at -52 dB (0x000e) or -54 dB (0x000b)
when turning down?

In bf_panel_mix_wheel(), each poll tick converts chip->panel_mix_raw to db2,
adds delta, converts back to raw, and stores only raw in chip->panel_mix_raw:

    db2 = bf_fader_raw_to_db2(chip->panel_mix_raw);
    db2 = clamp(db2 + delta, BF_FADER_DB2_INF, 12);
    raw = bf_fader_db2_to_raw(db2);
    chip->panel_mix_raw = raw;

In babyface_write_default_mixer(), all hardware input crosspoints start at
raw = 0. When panel_mix_raw is 0, bf_fader_raw_to_db2(0) returns
BF_FADER_DB2_INF (-130), whereas bf_fader_curve[0].db2 is -124 and
bf_fader_db2_to_raw(db2) returns 0 for all db2 < -124. If a user turns the
wheel clockwise by 1 to 5 clicks within a single 20 ms poll window,
db2 = -130 + delta is in [-129, -125], so bf_fader_db2_to_raw(db2) returns 0
and chip->panel_mix_raw stays 0. On the next poll tick, bf_fader_raw_to_db2(0)
starts from -130 again, discarding the clicks unless at least 6 clicks occur
in a single 20 ms interval.

Similarly, when turning the wheel down by a single click (delta = -1) at any
segment in bf_fader_curve[] where bf_fader_curve[i + 1].raw -
bf_fader_curve[i].raw == 1 across a db2 span of 2 (such as {-106, 0x000d} to
{-104, 0x000e}, and {-124, 0x0003} through {-108, 0x000b}),
db2 = bf_fader_curve[i + 1].db2 - 1 evaluates in bf_fader_db2_to_raw() to:

    bf_fader_curve[i].raw + ((1 + 1) / 2) = bf_fader_curve[i + 1].raw

which rounds back up to the starting raw value and prevents single-click
decrements from moving past those points.

Would tracking the monitoring fader state in half-dB (db2) rather than
re-deriving it from the quantized raw value on every tick avoid losing
half-dB steps?

[ ... ]

> +static void bf_panel_mix_wheel(struct snd_usb_babyface *chip, int delta)
> +{
> +	int out = bf_panel_out_pair(chip);
> +	u8 targets[2];
> +	int n = 0;
> +	int db2;
> +	u16 raw, flag;
> +	int i;
> +
> +	/* SELECT-chosen channel(s) of the IN pair (manual sec. 5.1: SELECT
> +	 * steps left/right/both; none = nothing selected = no-op wheel).
> +	 * Source indices: AN1/AN2 = 0/1, AN3/AN4 = 2/3, AS1/2 = 4.
> +	 */
> +	if (READ_ONCE(chip->panel_in) == 3) {
> +		targets[0] = 4;		/* Opt: the AS1/2 pair */
> +		n = 1;
> +	} else if (READ_ONCE(chip->panel_select) != 3) {
> +		int base = READ_ONCE(chip->panel_in) == 2 ? 2 : 0;
> +
> +		targets[0] = base + (READ_ONCE(chip->panel_select) == 1 ? 1 : 0);
> +		n = 1;
> +		if (READ_ONCE(chip->panel_select) == 2)
> +			targets[n++] = base + 1;
> +	} else {
> +		bf_panel_sel_hint(chip);
> +	}
> +
> +	mutex_lock(&chip->mutex);
> +	db2 = bf_fader_raw_to_db2(chip->panel_mix_raw);
> +	db2 = clamp(db2 + delta, BF_FADER_DB2_INF, 12);
> +	raw = bf_fader_db2_to_raw(db2);
> +	chip->panel_mix_raw = raw;

[Severity: Medium]
How is channel selection handled in bf_panel_mix_wheel() and bf_panel_tick()
when panel_select is unknown/none or changes while mix mode is active?

There appear to be four related issues here:

1. When panel_in == 3 (Opt), bf_panel_mix_wheel() checks panel_in == 3 before
   checking panel_select != 3, so it modifies targets[0] = 4 even when
   panel_sel[2] ("Front Panel Selection" index 2) is unknown or none (3),
   without calling bf_panel_sel_hint().

2. When panel_in != 3 and panel_select == 3, the else branch calls
   bf_panel_sel_hint(chip) without returning, so bf_panel_mix_wheel() still
   acquires chip->mutex and modifies chip->panel_mix_raw when n == 0.

3. In bf_panel_tick(), chip->panel_mix_raw and BF_REG_PANEL_GAIN are only
   seeded on the mix_flash entry edge:

    out = bf_panel_out_pair(chip);
    ref = READ_ONCE(chip->panel_in) == 3 ? 4 :
          (READ_ONCE(chip->panel_in) == 2 ? 2 : 0) +
          (READ_ONCE(chip->panel_select) == 1 ? 1 : 0);
    chip->panel_mix_raw = chip->xpoint[out][ref][0];

   If the user presses the select button (or writes "Front Panel Selection"
   via bf_panel_select_put()) or switches the input or output pair while
   panel_mix is already true, chip->panel_mix_raw is not re-seeded from the
   newly selected channel's crosspoint, causing the next wheel click to jump
   the new channel to the previous channel's level.

4. On that mix_flash entry edge in bf_panel_tick(), when panel_select == 3
   (none/unknown), ref evaluates to base + 0 and writes BF_REG_PANEL_GAIN + ref
   for an unselected channel, whereas when panel_select == 2 (both), only
   base + 0 is written to BF_REG_PANEL_GAIN and base + 1 is omitted.

Should bf_panel_mix_wheel() return early when panel_select == 3 (including
for panel_in == 3), and should the fader reference level and display registers
stay synchronized with the active selection?

[ ... ]

> +/* The wheel's view of one side: its own tracked level while the master
> + * is still what the wheel wrote, else the master's level.
> + */
> +static int bf_out_wheel_level(struct snd_usb_babyface *chip, int out, int ch)
> +{
> +	u16 step = chip->master[out][ch];
> +
> +	if (step == chip->panel_master_last[out][ch])
> +		return chip->panel_out_hdb[out][ch];
> +	if (!step)
> +		return BF_OUT_WHEEL_MUTED;
> +	return step - BF_MASTER_STEP_0DB;
> +}

[Severity: Medium]
Can bf_out_wheel_level() use a stale sub-floor offset from
chip->panel_out_hdb[out][ch] after the master volume is updated by
bf_master_put() or bf_panel_balance_wheel()?

When the output wheel turns an output with a stereo balance offset down so
that the quieter side drops below BF_OUT_WHEEL_FLOOR (-180),
bf_panel_out_wheel() records:

    chip->panel_master_last[out][ch] = 0;
    chip->panel_out_hdb[out][ch] = db[ch]; /* e.g. -220 */

If the user then updates the master volume via bf_master_put() (for example,
setting both channels to 0) or adjusts balance via bf_panel_balance_wheel() ->
bf_panel_write_master(), neither path invalidates chip->panel_master_last[out]
to 0xffff (or clears chip->panel_out_resync). When the output wheel is later
turned up, bf_out_wheel_level() sees step == chip->panel_master_last[out][ch]
(0 == 0) and returns the old sub-floor panel_out_hdb[out][ch] instead of
BF_OUT_WHEEL_MUTED (-181), reintroducing the stale balance offset.

Should bf_master_put() and bf_panel_write_master() reset
chip->panel_master_last[out] to 0xffff?

[ ... ]

> +static void bf_panel_out_resync(struct snd_usb_babyface *chip)
> +{
> +	int out, ch;
> +	int db[2];
> +	u16 flag;
> +
> +	if (!chip->panel_out_resync ||
> +	    ktime_ms_delta(ktime_get(), chip->panel_out_wheel_t) <
> +	    BF_OUT_WHEEL_RESYNC_MS)
> +		return;
> +
> +	mutex_lock(&chip->mutex);
> +	chip->panel_out_resync = false;
> +	out = chip->panel_out_wheel_out;
> +	for (ch = 0; ch < 2; ch++) {
> +		if (chip->master[out][ch] != chip->panel_master_last[out][ch])
> +			goto unlock;
> +		db[ch] = chip->panel_out_hdb[out][ch];
> +		if (db[ch] < BF_OUT_WHEEL_8BIT_MIN)
> +			goto unlock;
> +	}
> +	if (!chip->muted[out]) {
> +		flag = bf_flag_cycle[chip->flag_cnt];
> +		chip->flag_cnt = (chip->flag_cnt + 1) & 3;
> +		bf_vendor_write(chip, BF_REQ_GAIN, BF_MASTER_8_0DB + db[0],
> +				BF_REG_MASTER_8 + 2 * out);
> +		bf_vendor_write(chip, BF_REQ_GAIN, BF_MASTER_8_0DB + db[1],
> +				BF_REG_MASTER_8 + 2 * out + 1);
> +		bf_vendor_write(chip, BF_REQ_CROSSPOINT,
> +				bf_master_reg16(chip->master[out][0]),
> +				(BF_REG_MASTER_16 + 2 * out) | flag);
> +		bf_vendor_write(chip, BF_REQ_CROSSPOINT,
> +				bf_master_reg16(chip->master[out][1]),
> +				(BF_REG_MASTER_16 + 2 * out + 1) | flag);
> +	}
> +unlock:
> +	mutex_unlock(&chip->mutex);
> +}

[Severity: Medium]
Does turning the output or balance wheel on Phones (out == 1) while
chip->dim is engaged, or engaging "Dim Switch" within the 150 ms resync
window, overwrite the hardware's -20 dB dim attenuation with the full
un-dimmed volume?

When "Dim Switch" is engaged via bf_dim_put() -> bf_dim_apply(chip, true),
the driver writes the fixed -20 dB values (BF_MASTER_MINUS20_8 and
BF_MASTER_MINUS20_16) to the Phones master registers while leaving
chip->master[1] holding the un-dimmed level.

If a user turns the output wheel on Phones (out == 1) and "Dim Switch" is
then engaged within the 150 ms BF_OUT_WHEEL_RESYNC_MS window,
bf_dim_apply(chip, true) does not change chip->master[1]. When
bf_panel_out_resync() runs 150 ms after the click,
chip->master[1][ch] == chip->panel_master_last[1][ch] and !chip->muted[1]
are still true, so bf_panel_out_resync() writes the un-dimmed 8-bit and
16-bit levels to the hardware, wiping out the -20 dB dim attenuation.

Similarly, in bf_panel_write_master() and bf_panel_out_wheel_write():

    chip->master[out][0] = l;
    chip->master[out][1] = r;
    /* A Phones change while DIM is engaged re-bases the restore. */
    if (chip->dim && out == 1) {
        chip->dim_saved[0] = l;
        chip->dim_saved[1] = r;
    }

both functions update chip->dim_saved when chip->dim && out == 1, but still
write the un-dimmed l and r values to BF_REG_MASTER_8 and BF_REG_MASTER_16
while chip->dim remains true.

Should bf_panel_write_master(), bf_panel_out_wheel_write(), and
bf_panel_out_resync() avoid writing un-dimmed master levels to out == 1 while
chip->dim is active (or clear chip->dim if the wheel is meant to cancel it)?

[ ... ]

> +static void bf_panel_set_phantom(struct snd_usb_babyface *chip)
> +{
> +	u16 bits = 0;
> +	u16 old;
> +	int m;
> +
> +	if (READ_ONCE(chip->panel_mix) || READ_ONCE(chip->panel_in) != 1)
> +		return;
> +	if (READ_ONCE(chip->panel_select) == 3) {
> +		bf_panel_sel_hint(chip);
> +		return;
> +	}
> +	if (READ_ONCE(chip->panel_select) != 1)
> +		bits |= BF_PREAMP_48V_MIC1;
> +	if (READ_ONCE(chip->panel_select) != 0)
> +		bits |= BF_PREAMP_48V_MIC2;
> +
> +	mutex_lock(&chip->mutex);
> +	old = chip->preamp;
> +	/* One channel selected: toggle it.  Both selected: ALIGN both to
> +	 * the same state, so repeated SET presses cycle all-on <-> all-off
> +	 * (a mixed phantom state cannot persist with both selected).
> +	 */
> +	if (READ_ONCE(chip->panel_select) == 2) {
> +		if ((chip->preamp & bits) == bits)
> +			chip->preamp &= ~bits;
> +		else
> +			chip->preamp |= bits;
> +	} else {
> +		chip->preamp ^= bits;
> +	}

[Severity: High]
Can a concurrent write in bf_panel_select_put() race with
bf_panel_set_phantom(), bf_panel_gain_wheel(), and bf_panel_mix_wheel() and
cause +48V phantom power or gain to be applied to the wrong channels?

bf_panel_select_put() updates chip->panel_select_armed,
chip->panel_sel[pair], chip->panel_select_known, and chip->panel_select
(via bf_panel_sel_load()) without taking chip->mutex, while
bf_panel_set_phantom() re-evaluates READ_ONCE(chip->panel_select) four
separate times instead of snapshotting it once under a lock.

If chip->panel_select is 0 or 1 when bf_panel_set_phantom() checks:

    if (READ_ONCE(chip->panel_select) == 3)

and bf_panel_select_put() concurrently sets chip->panel_select to 3
(none/unknown) before the next two reads:

    if (READ_ONCE(chip->panel_select) != 1)
        bits |= BF_PREAMP_48V_MIC1;
    if (READ_ONCE(chip->panel_select) != 0)
        bits |= BF_PREAMP_48V_MIC2;

then both conditions evaluate to true (setting both BF_PREAMP_48V_MIC1 and
BF_PREAMP_48V_MIC2), and READ_ONCE(chip->panel_select) == 2 evaluates to
false, toggling +48V phantom power on both microphone inputs when no channel
is selected.

Similarly, bf_panel_gain_wheel() and bf_panel_mix_wheel() re-read
READ_ONCE(chip->panel_select) multiple times across the != 3, == 1, and == 2
checks, which can select channel 0 if panel_select changes to 3 or push
duplicate channel 1 entries if panel_select changes from 1 to 2.

Should panel_select state updates in bf_panel_select_put() and reads in the
panel worker be serialized (or at least read once into a local variable)?

[ ... ]

> +		if (READ_ONCE(chip->panel_mix))
> +			bf_panel_mix_wheel(chip, delta);
> +		else if (chip->panel_sel_hold >= 10 && cls == 1)
> +			bf_panel_balance_wheel(chip, delta);
> +		else if (cls == 1)
> +			bf_panel_out_wheel(chip, delta);
> +		else if (cls == 2)
> +			bf_panel_gain_wheel(chip, delta);
> +	}
> +	bf_panel_out_resync(chip);
> +
> +	/* Selections - keep the previous when the field is not in range
> +	 * (the fader-mode readback drops the IN position bits).
> +	 */
> +	in = bf_panel_in_decode((st[2] >> BF_PANEL_IN_SHIFT) & 0x7);
> +	if (in && in != READ_ONCE(chip->panel_in)) {
> +		WRITE_ONCE(chip->panel_in, in);
> +		/* The unit keeps one SELECT selection per IN pair, and an
> +		 * IN switch does not clear it (hardware-verified
> +		 * 2026-10-02: back on Ch 1/2 after a trip through Ch 3/4,
> +		 * the first SELECT press showed the L it had left, while
> +		 * Ch 3/4 showed nothing).  Switch to the new pair's.
> +		 */
> +		bf_panel_sel_load(chip);
> +		/* The LEDs go dark on an IN switch: the next SELECT press
> +		 * only shows the selection again (no step), the one after
> +		 * that steps it (user-verified 2026-08-28).
> +		 */
> +		chip->panel_select_armed = false;
> +		bf_panel_notify(chip, BF_PANEL_KCTL_IN);
> +	}
> +	out = bf_panel_out_decode(st[1] & 0x07);
> +	if (out && out != READ_ONCE(chip->panel_out)) {
> +		WRITE_ONCE(chip->panel_out, out);
> +		bf_panel_notify(chip, BF_PANEL_KCTL_OUT);
> +	}
> +
> +	/* SELECT press cycles the channel selection L -> R -> both -> none
> +	 * -> L (manual sec. 5.1).  The state is NOT in the readback
> +	 * (panelprobe 2026-08-24), so it is tracked host-side.
> +	 */
> +	if (st[3] == BF_PANEL_FLASH_SELECT &&
> +	    chip->panel_prev[3] != BF_PANEL_FLASH_SELECT &&
> +	    chip->panel_select_known) {
> +		if (!chip->panel_select_armed) {
> +			/* Disarmed (IN switch since the last step): the press
> +			 * only shows the selection - the device steps it on
> +			 * the NEXT press (user-verified 2026-08-28).
> +			 */
> +			chip->panel_select_armed = true;
> +		} else {
> +			int sel = (READ_ONCE(chip->panel_select) + 1) & 3;
> +			struct snd_kcontrol *kctl =
> +				chip->panel_select_kctl[READ_ONCE(chip->panel_in) - 1];
> +
> +			WRITE_ONCE(chip->panel_select, sel);
> +			chip->panel_sel[READ_ONCE(chip->panel_in) - 1] = sel;
> +			if (kctl)
> +				snd_ctl_notify(chip->card, SNDRV_CTL_EVENT_MASK_VALUE,
> +					       &kctl->id);
> +		}
> +	}
> +	/* SELECT hold (the OUT-balance gesture, manual sec. 5.1 "Output
> +	 * Balance"): a tap flashes byte3 0x50 for ~2-3 frames at 20 Hz
> +	 * (~100-150 ms - selhold_probe2), a hold keeps it sustained, and
> +	 * byte0 does NOT gain the 0x80 engaged bit - so the duration is
> +	 * the only discriminator: >= 10 ticks (200 ms at 50 Hz) = held.
> +	 */
> +	if (st[3] == BF_PANEL_FLASH_SELECT)
> +		chip->panel_sel_hold++;
> +	else
> +		chip->panel_sel_hold = 0;

[Severity: Medium]
Does counting 10 poll ticks in chip->panel_sel_hold accurately measure a
200 ms hold when fast polling is active or when panel_poll_ms is changed?

bf_panel_tick() checks chip->panel_sel_hold >= 10 assuming a fixed 20 ms
poll interval. However, during the 200 ms fast-poll window after an output
wheel click (chip->panel_fast_until), babyface_panel_work() polls every
BF_PANEL_FAST_POLL_MS (5 ms), so 10 ticks elapse in 50 ms (shorter than the
100-150 ms flash of a normal tap). Conversely, if the user sets the
panel_poll_ms module parameter up to 1000 ms, 10 ticks require holding the
button for up to 10 seconds.

In addition, because the rising-edge check:

    if (st[3] == BF_PANEL_FLASH_SELECT &&
        chip->panel_prev[3] != BF_PANEL_FLASH_SELECT &&
        chip->panel_select_known)

runs unconditionally on the first tick of BF_PANEL_FLASH_SELECT, holding
select in output mode to adjust balance also steps chip->panel_select on the
initial press edge.

Would measuring the hold duration with jiffies or ktime_t (and avoiding
stepping the input channel selection on an output-mode balance hold) work
more reliably?

[ ... ]

> +	mix_flash = st[3] == BF_PANEL_FLASH_MIX &&
> +		    chip->panel_prev[3] != BF_PANEL_FLASH_MIX;
> +	fader_now = (st[2] >> 4) == 0x0;
> +
> +	if (mix_flash) {
> +		if (READ_ONCE(chip->panel_mix)) {
> +			bf_vendor_write(chip, BF_REQ_PREAMP, 0x0400, 0x8000);
> +			bf_vendor_write(chip, BF_REQ_PREAMP, 0x0400, 0x8080);
> +			WRITE_ONCE(chip->panel_mix, false);
> +			WRITE_ONCE(chip->panel_saw_fader, false);
> +		} else {
> +			int ref, out;
> +			int m;
> +
> +			bf_vendor_write(chip, BF_REQ_PREAMP, 0x8480, 0x8c80);
> +			WRITE_ONCE(chip->panel_mix, true);

[ ... ]

> +		}
> +		bf_panel_notify(chip, BF_PANEL_KCTL_MIX);
> +	}
> +	if (fader_now) {
> +		WRITE_ONCE(chip->panel_saw_fader, true);
> +	} else if (READ_ONCE(chip->panel_mix) && READ_ONCE(chip->panel_saw_fader) &&
> +		   st[3] != BF_PANEL_FLASH_MIX) {
> +		/* device left fader mode by itself (IN/OUT/SET press) */
> +		bf_vendor_write(chip, BF_REQ_PREAMP, 0x0400, 0x8000);
> +		bf_vendor_write(chip, BF_REQ_PREAMP, 0x0400, 0x8080);
> +		WRITE_ONCE(chip->panel_mix, false);
> +		WRITE_ONCE(chip->panel_saw_fader, false);
> +		bf_panel_notify(chip, BF_PANEL_KCTL_MIX);
> +	}

[Severity: Medium]
When exiting mix mode via a second press of the mix button, doesn't the
subsequent if (fader_now) check immediately overwrite chip->panel_saw_fader
back to true?

While mix mode is engaged, the device is in fader mode, so fader_now
((st[2] >> 4) == 0x0) is true in the readback that reports the second
BF_PANEL_FLASH_MIX press. The mix_flash exit branch sets:

    WRITE_ONCE(chip->panel_mix, false);
    WRITE_ONCE(chip->panel_saw_fader, false);

but immediately after the if (mix_flash) block, if (fader_now) executes
unconditionally and sets chip->panel_saw_fader back to true. Because the
mix_flash entry branch does not clear panel_saw_fader to false either, the
next press of the mix button starts with panel_saw_fader == true before the
device has switched st[2] >> 4 to 0x0, which can trigger the device-driven
exit branch prematurely if the 0x44 flash clears before fader_now becomes
true.

Should if (fader_now) be gated on READ_ONCE(chip->panel_mix) (or cleared on
mix entry)?

[ ... ]

> +void babyface_panel_start(struct snd_usb_babyface *chip)
> +{
> +	chip->panel_seen = false;
> +	/* The SELECT channel selection is not in the readback, so the
> +	 * driver follows it from the presses.  The unit keeps one per IN
> +	 * pair across a driver reload and even a power cycle, with the LEDs
> +	 * dark until the next SELECT press, which shows it again instead of
> +	 * stepping (hardware-verified 2026-09-29: after an unplug, the
> +	 * first press lit both channels).  What panel_sel[] holds comes
> +	 * from before a suspend, from alsactl after a boot, or is not
> +	 * known, and an unknown selection targets nothing: SET, the IN
> +	 * wheel and the MIX wheel then do nothing rather than act on the
> +	 * wrong channel, until "Front Panel Select" is set to what the LEDs
> +	 * show.
> +	 */
> +	bf_panel_sel_load(chip);
> +	chip->panel_select_armed = false;
> +	chip->panel_start = jiffies;

[ ... ]

> +static int bf_panel_select_put(struct snd_kcontrol *kctl,
> +			       struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_usb_babyface *chip = snd_kcontrol_chip(kctl);
> +	unsigned int v = ucontrol->value.enumerated.item[0];
> +	int pair = kctl->private_value;
> +	/* alsactl restores the values of its last store shortly after
> +	 * every probe.
> +	 */
> +	bool restore = time_is_after_jiffies(chip->panel_start + 3 * HZ);
> +	s8 sel = v < 4 ? v : -1;
> +
> +	if (v > 4)
> +		return -EINVAL;
> +	/* After a boot they are what there is.  After a re-probe the unit
> +	 * may have taken SELECT presses since that store, and a wrong
> +	 * selection makes SET and the wheels act on a channel that is not
> +	 * the lit one: leave the pair unknown, as for a never-seen pair.
> +	 */
> +	if (restore && chip->reprobe)
> +		return 0;
> +	if (pair == READ_ONCE(chip->panel_in) - 1 && !restore && sel >= 0)
> +		/* Set while the LEDs show the selection: the next press steps
> +		 * it.
> +		 */
> +		chip->panel_select_armed = true;
> +	if (sel == chip->panel_sel[pair])
> +		return 0;
> +	chip->panel_sel[pair] = sel;
> +	if (pair == READ_ONCE(chip->panel_in) - 1)
> +		bf_panel_sel_load(chip);
> +	return 1;
> +}

[Severity: Medium]
Does resetting chip->panel_start = jiffies in babyface_panel_start() on
system resume cause bf_panel_select_put() to misclassify writes for 3 seconds
after every resume?

babyface_resume() calls babyface_panel_start(chip), which resets
chip->panel_start = jiffies. For 3 seconds after every system resume,
restore = time_is_after_jiffies(chip->panel_start + 3 * HZ) in
bf_panel_select_put() is true, so user or application writes to
"Front Panel Selection" are silently ignored if chip->reprobe is true, or
fail to set chip->panel_select_armed = true if chip->reprobe is false.

Conversely, during boot or probe, if alsactl restore runs more than 3 seconds
after probe, restore evaluates to false, bypassing the chip->reprobe check
and setting chip->panel_select_armed = true while the unit's selection
indicators are still dark, which desynchronizes the host-tracked selection on
the next physical button press.

Should chip->panel_start only be set at probe time, or is there a more
direct way to handle initial state restoration without a 3-second timer
window?

> diff --git a/sound/usb/babyfacepro/babyfacepro.c b/sound/usb/babyfacepro/babyfacepro.c
> index e0eb0ce9b92f3..a1aafebb2037c 100644
> --- a/sound/usb/babyfacepro/babyfacepro.c
> +++ b/sound/usb/babyfacepro/babyfacepro.c
> @@ -1479,8 +1527,12 @@ module_param(frames_per_urb, int, 0444);
>  MODULE_PARM_DESC(frames_per_urb, "Largest URB in audio frames, 8..1024 (256 = default); the period chooses the size below it.");
>  module_param(urb_frames_min, int, 0444);
>  MODULE_PARM_DESC(urb_frames_min, "Smallest URB in audio frames, 8..1024 (32 = default, 16 = low-latency floor).");
> +module_param(urb_split, int, 0444);
> +MODULE_PARM_DESC(urb_split, "Split the period into two URBs, queueing 1.5 periods instead of 2: 0 = auto (default), 1 = never, 2 = always.");
>  module_param(nurbs, int, 0444);
>  MODULE_PARM_DESC(nurbs, "Most URBs in flight per direction, 2..16 (8 = default).");
> +module_param(panel_poll_ms, int, 0444);
> +MODULE_PARM_DESC(panel_poll_ms, "Front-panel poll interval in ms, 10..1000 (20 = default, matches Windows' ~50 Hz).");

[Severity: Low]
Should the new panel_poll_ms and urb_split module parameters be documented
in Documentation/admin-guide/kernel-parameters.txt?

Also, urb_split belongs to the pcm period-splitting logic from the earlier
streaming patch rather than the front-panel changes described in this commit.

> @@ -1731,12 +1787,24 @@ static int babyface_probe(struct usb_interface *intf,
>  		goto error;
>  	}
>  
> +	err = babyface_create_panel(chip);
> +	if (err < 0) {
> +		dev_err(&intf->dev, "front-panel control creation failed: %d\n", err);
> +		goto error;
> +	}
> +
>  	err = snd_card_register(chip->card);
>  	if (err < 0) {
>  		dev_err(&intf->dev, "snd_card_register failed: %d\n", err);
>  		goto error;
>  	}
>  
> +	/* The panel poll mirrors the physical buttons/wheel into the
> +	 * Front Panel controls; it runs for the whole card lifetime.
> +	 */
> +	chip->reprobe = bf_seen_before(dev);
> +	babyface_panel_start(chip);

[Severity: Medium]
Can userspace access "Front Panel Selection" via bf_panel_select_put()
between snd_card_register() and babyface_panel_start()?

snd_card_register(chip->card) exposes the control device to userspace and
triggers udev's alsactl restore before chip->reprobe = bf_seen_before(dev)
and babyface_panel_start(chip) have run. If bf_panel_select_put() executes
during that window on a re-probed device, chip->reprobe is still false and
chip->panel_start is still 0, so the if (restore && chip->reprobe) check in
bf_panel_select_put() is bypassed and babyface_panel_start() subsequently
overwrites chip->panel_select_armed.

Should chip->reprobe and the initial panel state in babyface_panel_start()
be initialized before calling snd_card_register()?

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