Re: [PATCH v5 1/4] ASoC: rt5668: cancel delayed works on component removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ASoC: rt5668: uncancelled delayed work on I2C unbind in rt5668_remove()
- [High] ASoC: rt5668: stale jack and regcache state across rebind in rt5668_remove()
--

commit 17edcee2252c0f564a19636e40414b1293748590
Author: Tristan Madani <tristmd@gmail.com>

ASoC: rt5668: cancel delayed works on component removal

This commit disables the codec IRQ and cancels the jack_detect_work and
jd_check_work delayed works in rt5668_remove(), and re-enables the IRQ in
rt5668_probe() when a sound card rebinds.

> diff --git a/sound/soc/codecs/rt5668.c b/sound/soc/codecs/rt5668.c
> index cd6245d6b91af..29d70802a046a 100644
> --- a/sound/soc/codecs/rt5668.c
> +++ b/sound/soc/codecs/rt5668.c

[ ... ]

> @@ -2216,12 +2219,26 @@ static int rt5668_probe(struct snd_soc_component *component)
>  
>  	rt5668->component = component;
>  
> +	if (rt5668->irq_disabled) {
> +		enable_irq(to_i2c_client(component->dev)->irq);
> +		rt5668->irq_disabled = false;
> +	}
> +
>  	return 0;
>  }
>  
>  static void rt5668_remove(struct snd_soc_component *component)
>  {
>  	struct rt5668_priv *rt5668 = snd_soc_component_get_drvdata(component);
> +	struct i2c_client *i2c = to_i2c_client(component->dev);
> +
> +	if (rt5668->irq_acquired) {
> +		disable_irq(i2c->irq);
> +		rt5668->irq_disabled = true;
> +	}
> +
> +	cancel_delayed_work_sync(&rt5668->jack_detect_work);
> +	cancel_delayed_work_sync(&rt5668->jd_check_work);

[Severity: High]
As reported by Sashiko on v2 [1], what happens if the I2C device is unbound
when no ASoC sound card is bound to the component, or if
devm_snd_soc_register_component() fails in rt5668_i2c_probe()?

In rt5668_i2c_probe(), struct rt5668_priv is allocated via devm_kzalloc(),
rt5668->jack_detect_work and rt5668->jd_check_work are initialized, and
i2c->irq is immediately requested and enabled via devm_request_threaded_irq()
before any sound card binds:

sound/soc/codecs/rt5668.c:rt5668_i2c_probe() {
	...
	INIT_DELAYED_WORK(&rt5668->jack_detect_work,
				rt5668_jack_detect_handler);
	INIT_DELAYED_WORK(&rt5668->jd_check_work,
				rt5668_jd_check_handler);

	mutex_init(&rt5668->calibrate_mutex);

	if (i2c->irq) {
		ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL,
			rt5668_irq, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
			| IRQF_ONESHOT, "rt5668", rt5668);
		if (ret)
			dev_err(&i2c->dev, "Failed to request IRQ: %d\n", ret);
		else
			rt5668->irq_acquired = true;
	}

	return devm_snd_soc_register_component(&i2c->dev, &soc_component_dev_rt5668,
			rt5668_dai, ARRAY_SIZE(rt5668_dai));
}

If i2c->irq fires before a sound card is bound or instantiated, rt5668_irq()
queues rt5668->jack_detect_work, and rt5668_jack_detect_handler() continuously
re-arms itself every 15 ms:

sound/soc/codecs/rt5668.c:rt5668_jack_detect_handler() {
	...
	if (!rt5668->component ||
	    !snd_soc_card_is_instantiated(rt5668->component->card)) {
		/* card not yet ready, try later */
		mod_delayed_work(system_power_efficient_wq,
				 &rt5668->jack_detect_work, msecs_to_jiffies(15));
		return;
	}
	...
}

However, rt5668_remove() is the snd_soc_component_driver.remove callback, and
the ASoC core only invokes it when a card was bound to the component:

devm_component_release()
  -> snd_soc_unregister_component_by_driver()
    -> snd_soc_del_component_unlocked()
      -> if (card) snd_soc_unbind_card(card)
        -> soc_cleanup_card_resources()
          -> snd_soc_dapm_shutdown() / snd_soc_component_remove()

Because rt5668_i2c_driver has no .remove callback and registers no devm action
to disable the IRQ and cancel the delayed works (and rt5668_i2c_shutdown()
also resets the codec without stopping them), won't unbinding the I2C device
without an instantiated card cause devres to free struct rt5668_priv while
rt5668->jack_detect_work remains pending and self-rearming?

>  
>  	rt5668_reset(rt5668->regmap);
>  }

[Severity: High]
As reported by Sashiko on v4 [2], since rt5668_probe() now calls enable_irq()
to support sound card unbind/rebind cycles, does rt5668_remove() leave the
regmap cache and jack state out of sync with the hardware after calling
rt5668_reset()?

In rt5668_remove(), rt5668_reset(rt5668->regmap) writes 0 to the volatile
RT5668_RESET register, resetting all codec registers in hardware to power-on
defaults without resetting or dirtying the REGCACHE_MAPLE regmap cache and
without clearing rt5668->hs_jack, rt5668->jack_type, or rt5668->component.

When a sound card subsequently binds again, can this cause the following
issues?

1. If the previous card's struct snd_soc_jack was freed when that card unbound
   without clearing rt5668->hs_jack, rt5668->hs_jack is left dangling. When
   rt5668_probe() immediately calls enable_irq(), if an interrupt triggers
   rt5668_jack_detect_handler() before a new jack is registered (or if the new
   card does not call snd_soc_component_set_jack()), snd_soc_jack_report() is
   called with the dangling rt5668->hs_jack pointer.

2. When the newly bound card calls rt5668_set_jack_detect(), the
   snd_soc_component_update_bits() and regmap_update_bits() calls on cached
   non-volatile registers (RT5668_CBJ_CTRL_3, RT5668_SAR_IL_CMD_1,
   RT5668_GPIO_CTRL_1, RT5668_RC_CLK_CTRL, RT5668_PWR_ANLG_2, and
   RT5668_IRQ_CTRL_2) see those bits already set in the stale regmap cache
   from the first bind and skip writing them to the reset hardware, leaving
   the physical IRQ output pin and jack detection disabled in the codec.

3. If a headset was plugged in when the card unbound (rt5668->jack_type != 0),
   rt5668_jack_detect_handler() on rebind sees rt5668->jack_type != 0 and
   skips initial headset detection via rt5668_headset_detect():

sound/soc/codecs/rt5668.c:rt5668_jack_detect_handler() {
	...
		if (rt5668->jack_type == 0) {
			/* jack was out, report jack type */
			rt5668->jack_type =
				rt5668_headset_detect(rt5668->component, 1);
		} else {
			/* jack is already in, report button event */
			rt5668->jack_type = SND_JACK_HEADSET;
			btn_type = rt5668_button_detect(rt5668->component);
	...
}

[1] https://lore.kernel.org/r/sashiko-outbox-190014@kernel.org
[2] https://lore.kernel.org/r/20261030170416.3561618-2-tristan@talencesecurity.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030174837.1839402-1-tristan@talencesecurity.com?part=1