[PATCH] ASoC: rt5668: fix teardown order and stale state across card rebind

[PATCH] ASoC: rt5668: fix teardown order and stale state across card rebind

[Tristan Madani]

The cleanup in rt5668_remove() does not reset hs_jack, jack_type, or
the component pointer, and does not mark the regmap cache dirty after
the codec reset.  On card rebind this leaves a dangling hs_jack pointer
and causes regmap_update_bits() to skip re-programming jack-detection
registers because the cache still holds the pre-reset values.

If the I2C device unbinds without a card bound, rt5668_remove() is
never called and the threaded IRQ keeps queuing jack_detect_work, which
self-rearms every 15 ms until devres frees the private data.
rt5668_i2c_shutdown() has the same gap: it resets the codec without
disabling the IRQ or cancelling the works.

Store the I2C client pointer in rt5668_priv so all paths can reference
it directly.  Clear stale driver state and mark the regmap cache dirty
in rt5668_remove(), and sync the cache in rt5668_probe() to restore
register values on rebind.  Register a devm action to disable the IRQ
and cancel both delayed works on any I2C teardown path.  Apply the same
cleanup in rt5668_i2c_shutdown().

Fixes: d59fb2856223 ("ASoC: rt5668: add rt5668B codec driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 sound/soc/codecs/rt5668.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/rt5668.c b/sound/soc/codecs/rt5668.c
index 29d70802a046a..2ceae9d9f40f6 100644
--- a/sound/soc/codecs/rt5668.c
+++ b/sound/soc/codecs/rt5668.c
@@ -45,6 +45,7 @@ struct rt5668_priv {
 	struct rt5668_platform_data pdata;
 	struct gpio_desc *ldo1_en;
 	struct regmap *regmap;
+	struct i2c_client *i2c;
 	struct snd_soc_jack *hs_jack;
 	struct regulator_bulk_data supplies[RT5668_NUM_SUPPLIES];
 	struct delayed_work jack_detect_work;
@@ -2213,14 +2214,26 @@ static int rt5668_set_bias_level(struct snd_soc_component *component,
 	return 0;
 }
 
+static void rt5668_i2c_disable_irq_and_cancel_work(void *data)
+{
+	struct rt5668_priv *rt5668 = data;
+
+	if (rt5668->irq_acquired && !rt5668->irq_disabled)
+		disable_irq(rt5668->i2c->irq);
+	cancel_delayed_work_sync(&rt5668->jack_detect_work);
+	cancel_delayed_work_sync(&rt5668->jd_check_work);
+}
+
 static int rt5668_probe(struct snd_soc_component *component)
 {
 	struct rt5668_priv *rt5668 = snd_soc_component_get_drvdata(component);
 
 	rt5668->component = component;
 
+	regcache_sync(rt5668->regmap);
+
 	if (rt5668->irq_disabled) {
-		enable_irq(to_i2c_client(component->dev)->irq);
+		enable_irq(rt5668->i2c->irq);
 		rt5668->irq_disabled = false;
 	}
 
@@ -2230,17 +2243,21 @@ static int rt5668_probe(struct snd_soc_component *component)
 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);
+		disable_irq(rt5668->i2c->irq);
 		rt5668->irq_disabled = true;
 	}
 
 	cancel_delayed_work_sync(&rt5668->jack_detect_work);
 	cancel_delayed_work_sync(&rt5668->jd_check_work);
 
+	rt5668->hs_jack = NULL;
+	rt5668->jack_type = 0;
+	rt5668->component = NULL;
+
 	rt5668_reset(rt5668->regmap);
+	regcache_mark_dirty(rt5668->regmap);
 }
 
 #ifdef CONFIG_PM
@@ -2576,6 +2593,13 @@ static int rt5668_i2c_probe(struct i2c_client *i2c)
 			rt5668->irq_acquired = true;
 	}
 
+	rt5668->i2c = i2c;
+	ret = devm_add_action_or_reset(&i2c->dev,
+				      rt5668_i2c_disable_irq_and_cancel_work,
+				      rt5668);
+	if (ret)
+		return ret;
+
 	return devm_snd_soc_register_component(&i2c->dev, &soc_component_dev_rt5668,
 			rt5668_dai, ARRAY_SIZE(rt5668_dai));
 }
@@ -2584,6 +2608,11 @@ static void rt5668_i2c_shutdown(struct i2c_client *client)
 {
 	struct rt5668_priv *rt5668 = i2c_get_clientdata(client);
 
+	if (rt5668->irq_acquired)
+		disable_irq(client->irq);
+	cancel_delayed_work_sync(&rt5668->jack_detect_work);
+	cancel_delayed_work_sync(&rt5668->jd_check_work);
+
 	rt5668_reset(rt5668->regmap);
 }
 
-- 
2.47.3

Re: [PATCH] ASoC: rt5668: fix teardown order and stale state across card rebind

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ASoC: rt5668: stale jack and button state synced on rebind in
  rt5668_probe()
- [Low] ASoC: rt5668: duplicate disable_irq() in rt5668_i2c_shutdown()
  when unbound
It also noticed 2 related pre-existing issues this patch does not
introduce:
- [High] ASoC: rt5668: delayed work re-armed when hs_jack is NULL in
  rt5668_set_jack_detect() [1]
- [High] ASoC: rt5668: uninitialized mutex locked in rt5668_calibrate()
  [2]
--

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

ASoC: rt5668: fix teardown order and stale state across card rebind

This commit clears jack state and marks the regmap cache dirty in
rt5668_remove(), syncs the cache in rt5668_probe(), and registers a devm
action and shutdown handler to disable the IRQ and cancel delayed works.

> diff --git a/sound/soc/codecs/rt5668.c b/sound/soc/codecs/rt5668.c
> index 29d70802a046a..2ceae9d9f40f6 100644
> --- a/sound/soc/codecs/rt5668.c
> +++ b/sound/soc/codecs/rt5668.c
[ ... ]
> @@ -2213,14 +2214,26 @@ static int rt5668_set_bias_level(struct snd_soc_component *component,
>  	return 0;
>  }
>  
> +static void rt5668_i2c_disable_irq_and_cancel_work(void *data)
> +{
> +	struct rt5668_priv *rt5668 = data;
> +
> +	if (rt5668->irq_acquired && !rt5668->irq_disabled)
> +		disable_irq(rt5668->i2c->irq);
> +	cancel_delayed_work_sync(&rt5668->jack_detect_work);
> +	cancel_delayed_work_sync(&rt5668->jd_check_work);
> +}
> +
>  static int rt5668_probe(struct snd_soc_component *component)
>  {
>  	struct rt5668_priv *rt5668 = snd_soc_component_get_drvdata(component);
>  
>  	rt5668->component = component;
>  
> +	regcache_sync(rt5668->regmap);
> +
>  	if (rt5668->irq_disabled) {
> -		enable_irq(to_i2c_client(component->dev)->irq);
> +		enable_irq(rt5668->i2c->irq);
>  		rt5668->irq_disabled = false;
>  	}
>  
> @@ -2230,17 +2243,21 @@ static int rt5668_probe(struct snd_soc_component *component)
>  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);
> +		disable_irq(rt5668->i2c->irq);
>  		rt5668->irq_disabled = true;
>  	}
>  
>  	cancel_delayed_work_sync(&rt5668->jack_detect_work);
>  	cancel_delayed_work_sync(&rt5668->jd_check_work);
>  
> +	rt5668->hs_jack = NULL;
> +	rt5668->jack_type = 0;
> +	rt5668->component = NULL;

[Severity: High]
This problem wasn't introduced by this patch, but can
rt5668_set_jack_detect() re-arm rt5668->jack_detect_work after
rt5668_remove() has cancelled it [1]?

During card teardown, soc_cleanup_card_resources() runs
snd_soc_component_remove() (invoking rt5668_remove()) before
snd_card_free() -> snd_jack_dev_free(), where jack->private_free() calls
snd_soc_component_set_jack(component, NULL, NULL).  When pdata.jd_src is
RT5668_JD1, rt5668_set_jack_detect() does not check whether hs_jack is
NULL before enabling RT5668_JD1_EN and queuing rt5668->jack_detect_work:

sound/soc/codecs/rt5668.c:rt5668_set_jack_detect() {
	switch (rt5668->pdata.jd_src) {
	case RT5668_JD1:
		...
		regmap_update_bits(rt5668->regmap, RT5668_IRQ_CTRL_2,
			RT5668_JD1_EN_MASK | RT5668_JD1_POL_MASK,
			RT5668_JD1_EN | RT5668_JD1_POL_NOR);
		mod_delayed_work(system_power_efficient_wq,
			   &rt5668->jack_detect_work, msecs_to_jiffies(250));
		break;
}

When rt5668_jack_detect_handler() runs 250 ms later, rt5668->component is
NULL, so the handler reschedules itself every 15 ms until the I2C device
is unbound.

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

[Severity: Medium]
Does regcache_sync() in rt5668_probe() restore stale headset and
button-detection register state when rebinding after a 4-pole headset was
plugged in?

When a 4-pole headset is detected, rt5668_headset_detect(component, 1) and
rt5668_enable_push_button_irq(component, true) update non-volatile cached
registers: RT5668_CBJ_CTRL_1 (RT5668_TRIG_JD_HIGH), RT5668_SAR_IL_CMD_1
(RT5668_SAR_BUTT_DET_EN), RT5668_SAR_IL_CMD_13 (switching the SAR source
from RT5668_SAR_SOUR_TYPE to RT5668_SAR_SOUR_BTN), RT5668_4BTN_IL_CMD_2,
and RT5668_IRQ_CTRL_3 (RT5668_IL_IRQ_EN).

In rt5668_remove(), rt5668->jack_type is cleared to 0 and the hardware is
reset, but regcache_mark_dirty() preserves those non-default values in the
cache.  On card rebind, regcache_sync() writes them back to the codec
while rt5668->jack_type is 0.  Because neither rt5668_set_jack_detect()
nor the jack_insert branch of rt5668_headset_detect() switches
RT5668_SAR_IL_CMD_13 back to RT5668_SAR_SOUR_TYPE or disables push-button
IRQ, combo-jack type detection runs with the SAR source stuck on button
mode and leaves RT5668_IL_IRQ_EN enabled even if a 3-pole headphone is
connected.

Calling rt5668_headset_detect(component, 0) in rt5668_remove() before
rt5668_reset() (or reinitializing the cache to defaults) would clear the
cached headset and button state.

[ ... ]
> @@ -2576,6 +2593,13 @@ static int rt5668_i2c_probe(struct i2c_client *i2c)
>  			rt5668->irq_acquired = true;
>  	}
>  
> +	rt5668->i2c = i2c;
> +	ret = devm_add_action_or_reset(&i2c->dev,
> +				      rt5668_i2c_disable_irq_and_cancel_work,
> +				      rt5668);
> +	if (ret)
> +		return ret;
> +
>  	return devm_snd_soc_register_component(&i2c->dev, &soc_component_dev_rt5668,
>  			rt5668_dai, ARRAY_SIZE(rt5668_dai));
>  }

[Severity: High]
This problem wasn't introduced by this patch, but earlier in
rt5668_i2c_probe(), rt5668_calibrate() is called before
mutex_init(&rt5668->calibrate_mutex) [2]:

sound/soc/codecs/rt5668.c:rt5668_calibrate() {
	int value, count;

	guard(mutex)(&rt5668->calibrate_mutex);
	...
}

sound/soc/codecs/rt5668.c:rt5668_i2c_probe() {
	...
	rt5668_calibrate(rt5668);
	...
	mutex_init(&rt5668->calibrate_mutex);
}

guard(mutex) locks and unlocks a zero-initialized mutex during probe,
triggering CONFIG_DEBUG_MUTEXES / lockdep warnings and operating on an
uninitialized wait_list.

> @@ -2584,6 +2608,11 @@ static void rt5668_i2c_shutdown(struct i2c_client *client)
>  {
>  	struct rt5668_priv *rt5668 = i2c_get_clientdata(client);
>  
> +	if (rt5668->irq_acquired)
> +		disable_irq(client->irq);
> +	cancel_delayed_work_sync(&rt5668->jack_detect_work);
> +	cancel_delayed_work_sync(&rt5668->jd_check_work);
> +
>  	rt5668_reset(rt5668->regmap);
>  }

[Severity: Low]
If the sound card was unbound via rt5668_remove() before system shutdown,
rt5668->irq_disabled is already true; doesn't rt5668_i2c_shutdown() then
call disable_irq() a second time on the already-disabled IRQ?

Setting rt5668->irq_disabled = true in
rt5668_i2c_disable_irq_and_cancel_work() and calling that helper here
(and in rt5668_remove()) would avoid the unbalanced disable_irq() and the
duplicated teardown sequence.

[1] https://sashiko.dev/#/bug/linux-ba1194b0-1aa7-482d-b655-2fd164cde5ff
[2] https://sashiko.dev/#/bug/linux-52105661-89bd-4327-8228-f16b8d913d85

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

[PATCH v3] ASoC: rt5668: fix teardown order and stale state across card rebind

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

The cleanup in rt5668_remove() does not reset hs_jack, jack_type, or
the component pointer, and does not mark the regmap cache dirty after
the codec reset.  On card rebind this leaves a dangling hs_jack pointer
and causes regmap_update_bits() to skip re-programming jack-detection
registers because the cache still holds the pre-reset values.

Additionally, headset detection registers from a previous session
remain cached after the hardware reset.  regcache_sync() on rebind
restores this stale state, which can leave the SAR source on button
mode.  Clear the headset state by calling rt5668_headset_detect(0)
before the codec reset.

If the I2C device unbinds without a card bound, rt5668_remove() is
never called and the IRQ keeps queuing jack_detect_work and
jd_check_work until devres frees the private data.  Register a devm
action to disable the IRQ, set irq_disabled, and cancel both delayed
works on any I2C teardown path.

Consolidate the IRQ disable and work cancellation into the devm
helper and call it from rt5668_remove() and rt5668_i2c_shutdown()
as well, so every path goes through the same sequence and
irq_disabled stays consistent.  This prevents an unbalanced
disable_irq() when multiple teardown paths run in succession.

Store the I2C client pointer in rt5668_priv so all paths can
reference it directly.

Fixes: d59fb2856223 ("ASoC: rt5668: add rt5668B codec driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
Changes in v3:
  - Set irq_disabled in the devm teardown helper and call it from
    rt5668_remove() and rt5668_i2c_shutdown() to prevent double
    disable_irq() when multiple teardown paths run
Changes in v2:
  - Clear headset detection state via rt5668_headset_detect(0) before
    codec reset to prevent stale SAR/button state on rebind
  - Guard disable_irq() in rt5668_i2c_shutdown() with irq_disabled
    flag to avoid unbalanced disable

 sound/soc/codecs/rt5668.c | 41 +++++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/rt5668.c b/sound/soc/codecs/rt5668.c
index 29d70802a046a..5bfe5d9546925 100644
--- a/sound/soc/codecs/rt5668.c
+++ b/sound/soc/codecs/rt5668.c
@@ -45,6 +45,7 @@ struct rt5668_priv {
 	struct rt5668_platform_data pdata;
 	struct gpio_desc *ldo1_en;
 	struct regmap *regmap;
+	struct i2c_client *i2c;
 	struct snd_soc_jack *hs_jack;
 	struct regulator_bulk_data supplies[RT5668_NUM_SUPPLIES];
 	struct delayed_work jack_detect_work;
@@ -2213,14 +2214,28 @@ static int rt5668_set_bias_level(struct snd_soc_component *component,
 	return 0;
 }
 
+static void rt5668_i2c_disable_irq_and_cancel_work(void *data)
+{
+	struct rt5668_priv *rt5668 = data;
+
+	if (rt5668->irq_acquired && !rt5668->irq_disabled) {
+		disable_irq(rt5668->i2c->irq);
+		rt5668->irq_disabled = true;
+	}
+	cancel_delayed_work_sync(&rt5668->jack_detect_work);
+	cancel_delayed_work_sync(&rt5668->jd_check_work);
+}
+
 static int rt5668_probe(struct snd_soc_component *component)
 {
 	struct rt5668_priv *rt5668 = snd_soc_component_get_drvdata(component);
 
 	rt5668->component = component;
 
+	regcache_sync(rt5668->regmap);
+
 	if (rt5668->irq_disabled) {
-		enable_irq(to_i2c_client(component->dev)->irq);
+		enable_irq(rt5668->i2c->irq);
 		rt5668->irq_disabled = false;
 	}
 
@@ -2230,17 +2245,18 @@ static int rt5668_probe(struct snd_soc_component *component)
 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;
-	}
+	rt5668_i2c_disable_irq_and_cancel_work(rt5668);
 
-	cancel_delayed_work_sync(&rt5668->jack_detect_work);
-	cancel_delayed_work_sync(&rt5668->jd_check_work);
+	if (rt5668->jack_type)
+		rt5668_headset_detect(component, 0);
+
+	rt5668->hs_jack = NULL;
+	rt5668->jack_type = 0;
+	rt5668->component = NULL;
 
 	rt5668_reset(rt5668->regmap);
+	regcache_mark_dirty(rt5668->regmap);
 }
 
 #ifdef CONFIG_PM
@@ -2576,6 +2592,13 @@ static int rt5668_i2c_probe(struct i2c_client *i2c)
 			rt5668->irq_acquired = true;
 	}
 
+	rt5668->i2c = i2c;
+	ret = devm_add_action_or_reset(&i2c->dev,
+				      rt5668_i2c_disable_irq_and_cancel_work,
+				      rt5668);
+	if (ret)
+		return ret;
+
 	return devm_snd_soc_register_component(&i2c->dev, &soc_component_dev_rt5668,
 			rt5668_dai, ARRAY_SIZE(rt5668_dai));
 }
@@ -2584,6 +2607,8 @@ static void rt5668_i2c_shutdown(struct i2c_client *client)
 {
 	struct rt5668_priv *rt5668 = i2c_get_clientdata(client);
 
+	rt5668_i2c_disable_irq_and_cancel_work(rt5668);
+
 	rt5668_reset(rt5668->regmap);
 }
 
-- 
2.47.3