Re: [PATCH v3 2/4] ASoC: rt5659: cancel jack_detect_work on component removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ASoC: rt5659: uncancelled jack_detect_work on I2C teardown without card
- [High] ASoC: rt5659: broken jack detect and dangling hs_jack in rt5659_probe()
--

commit 2bb6cdac1d7b83279322bc0304f15b740f1928f7
Author: Tristan Madani <tristmd@gmail.com>

ASoC: rt5659: cancel jack_detect_work on component removal

This commit disables the codec IRQ and cancels pending jack_detect_work in
rt5659_remove() before resetting the codec, and re-enables the IRQ in
rt5659_probe() when a component is rebound.

> diff --git a/sound/soc/codecs/rt5659.c b/sound/soc/codecs/rt5659.c
> index af8051fa69f89..68f7d5673e190 100644
> --- a/sound/soc/codecs/rt5659.c
> +++ b/sound/soc/codecs/rt5659.c
> @@ -3667,6 +3667,11 @@ static int rt5659_probe(struct snd_soc_component *component)
>  
>  	rt5659->component = component;
>  
> +	if (rt5659->irq_disabled) {
> +		enable_irq(to_i2c_client(component->dev)->irq);
> +		rt5659->irq_disabled = false;
> +	}

[Severity: High]
As reported by Sashiko on v2 [1], does re-enabling the IRQ in rt5659_probe()
restore jack detection or leave a dangling rt5659->hs_jack pointer on card
rebind?

At the end of rt5659_remove(), writing RT5659_RESET resets all codec hardware
registers to defaults, wiping out the GPIO1 IRQ pin mux (RT5659_GP1_PIN_IRQ in
RT5659_GPIO_CTRL_1) and the jack-detect and IRQ registers (RT5659_EJD_CTRL_1,
RT5659_RC_CLK_CTRL, RT5659_JD_CTRL_1, RT5659_IRQ_CTRL_1, RT5659_IRQ_CTRL_2,
RT5659_PWR_ANLG_1, and RT5659_PWR_ANLG_2) that were only configured in
rt5659_i2c_probe(). Because RT5659_RESET is volatile in
rt5659_volatile_register(), the REGCACHE_RBTREE regmap cache is also not
marked dirty or resynced on re-probe, so the codec cannot generate jack
detection interrupts after rebind.

In addition, rt5659_remove() does not clear rt5659->hs_jack or
rt5659->component. When a sound card unbinds and its card-owned struct
snd_soc_jack is freed, a subsequent card bind invokes
snd_soc_component_probe() -> rt5659_probe() before the machine driver's init
callback registers a new jack. Calling enable_irq() here while rt5659->hs_jack
still points to the freed jack from the previous card allows any interrupt to
schedule jack_detect_work, which passes the dangling rt5659->hs_jack pointer
to snd_soc_jack_report():

sound/soc/codecs/rt5659.c:rt5659_jack_detect_intel_hd_header() {
	...
	if (!rt5659->hs_jack)
		return;
	...
	snd_soc_jack_report(rt5659->hs_jack, rt5659->jack_type,
		SND_JACK_HEADSET);
}

>  
>  	switch (rt5659->pdata.jd_src) {
>  	case RT5659_JD_HDA_HEADER:
>  		break;
> @@ -3684,6 +3689,15 @@ static int rt5659_probe(struct snd_soc_component *component)
>  static void rt5659_remove(struct snd_soc_component *component)
>  {
>  	struct rt5659_priv *rt5659 = snd_soc_component_get_drvdata(component);
> +	struct i2c_client *i2c = to_i2c_client(component->dev);
> +
> +	if (rt5659->irq_acquired) {
> +		disable_irq(i2c->irq);
> +		rt5659->irq_disabled = true;
> +	}
> +
> +	if (rt5659->jack_detect_work_inited)
> +		cancel_delayed_work_sync(&rt5659->jack_detect_work);
>  
>  	regmap_write(rt5659->regmap, RT5659_RESET, 0);
>  }

[Severity: High]
As reported by Sashiko on v2 [2], can rt5659->jack_detect_work still remain
queued and cause a use-after-free of struct rt5659_priv and rt5659->regmap
when the I2C device is unbound without a bound sound card?

In rt5659_i2c_probe(), when rt5659->pdata.jd_src is RT5659_JD3 or
RT5659_JD_HDA_HEADER, the driver initializes rt5659->jack_detect_work,
configures the jack-detection interrupt registers, registers rt5659_irq() via
devm_request_threaded_irq(), and enables RT5659_GP1_PIN_IRQ before calling
devm_snd_soc_register_component(). Once the IRQ is registered, an interrupt on
i2c->irq runs rt5659_irq() and queues rt5659->jack_detect_work on
system_power_efficient_wq with a 250 ms delay.

However, disable_irq() and cancel_delayed_work_sync() are only placed in
rt5659_remove(), which is the snd_soc_component_driver remove callback. In the
ASoC core, component removal skips the driver remove callback whenever no card
is bound to the component:

sound/soc/soc-core.c:snd_soc_del_component_unlocked() {
	...
	if (card) {
		instantiated = card->instantiated;
		snd_soc_unbind_card(card);
		if (instantiated)
			list_add(&card->list, &unbind_card_list);
	}
}

sound/soc/soc-component.c:snd_soc_component_remove() {
	if (!component->card)
		return;

	if (probed && component->driver->remove)
		component->driver->remove(component);
	...
}

If devm_snd_soc_register_component() fails in rt5659_i2c_probe(), or if the
I2C device is unbound while component->card is NULL, rt5659_remove() is never
called and devres frees the IRQ, regmap, and struct rt5659_priv while
rt5659->jack_detect_work is still pending or running. Similarly,
rt5659_i2c_shutdown() resets the codec via RT5659_RESET without disabling the
IRQ or cancelling rt5659->jack_detect_work.

Would registering the delayed work with devm_delayed_work_autocancel() (or
handling teardown at the I2C driver level) ensure the work is always cancelled
after the IRQ is freed on all unbind paths?

[1] https://lore.kernel.org/r/20261029212504.2997364-3-tristmd@gmail.com
[2] https://lore.kernel.org/r/20261029212504.2997364-3-tristmd@gmail.com

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