Re: [PATCH v5 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 ed495f70ecb1520022a86353ac80bee32a4816ff
Author: Tristan Madani <tristmd@gmail.com>

ASoC: rt5659: cancel jack_detect_work on component removal

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

> 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 v4 [1], when a sound card unbinds and rebinds, can
re-enabling the IRQ in rt5659_probe() restore jack detection or is it unsafe
while rt5659->hs_jack still holds a pointer from the previous card?

At the end of rt5659_remove(), regmap_write(rt5659->regmap, RT5659_RESET, 0)
resets all codec hardware registers to power-on defaults, clearing the GPIO1
IRQ pin mux (RT5659_GP1_PIN_IRQ in RT5659_GPIO_CTRL_1) and all jack-detection
and IRQ control registers (such as RT5659_EJD_CTRL_1, RT5659_RC_CLK_CTRL,
RT5659_GPIO_CTRL_3, RT5659_JD_CTRL_1, RT5659_IRQ_CTRL_1, and
RT5659_IRQ_CTRL_2) that were only programmed once in rt5659_i2c_probe().
Because RT5659_RESET is marked volatile in rt5659_volatile_register(), the
REGCACHE_RBTREE regmap cache is not marked dirty or resynced on re-probe,
leaving the codec unable to generate jack-detection interrupts after rebind.

In addition, rt5659_remove() does not clear rt5659->hs_jack or
rt5659->component. When a card rebinds, snd_soc_component_probe() invokes
rt5659_probe() and calls enable_irq() before the machine driver's DAI-link
init callback registers a new snd_soc_jack. If an interrupt fires before a
new jack is set (or if the new card does not call rt5659_set_jack_detect()),
rt5659_jack_detect_work() or rt5659_jack_detect_intel_hd_header() will pass
the dangling rt5659->hs_jack pointer from the old card to
snd_soc_jack_report().

> +
>  	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 v4 [2], what happens if the I2C device is unbound
while no ASoC sound card is bound to the component, or if
devm_snd_soc_register_component() fails in rt5659_i2c_probe()?

In rt5659_i2c_probe(), when rt5659->pdata.jd_src is RT5659_JD3 or
RT5659_JD_HDA_HEADER, rt5659->jack_detect_work is initialized and hardware
interrupts are enabled via devm_request_threaded_irq() and
RT5659_GP1_PIN_IRQ before devm_snd_soc_register_component() is called. Once
devm_request_threaded_irq() succeeds, any interrupt on i2c->irq invokes
rt5659_irq() and queues rt5659->jack_detect_work onto
system_power_efficient_wq with a 250 ms delay.

Because disable_irq() and cancel_delayed_work_sync() are only placed in
rt5659_remove() (the snd_soc_component_driver.remove callback), and the ASoC
core in snd_soc_del_component_unlocked() -> snd_soc_component_remove() only
invokes component->driver->remove() when a card has probed the component,
unbinding the I2C device without a bound card (or failing in
devm_snd_soc_register_component()) never calls rt5659_remove(). Devres then
frees the IRQ, regmap, and struct rt5659_priv while rt5659->jack_detect_work
can still be pending, leading to a use-after-free when the worker runs.

Similarly, rt5659_i2c_shutdown() writes RT5659_RESET without disabling the
IRQ or cancelling rt5659->jack_detect_work. Should the work cancellation and
IRQ teardown also be tied to the I2C device lifecycle (for example via
devm_delayed_work_autocancel() or a devm action) and handled in
rt5659_i2c_shutdown()?

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

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