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

[PATCH v2] 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, SAR and button-detection registers from a previous
headset session remain cached after the hardware reset.
regcache_sync() on rebind restores this stale state while jack_type
is zero, leaving the SAR source on button mode.  Clear the headset
state by calling rt5668_headset_detect(component, 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 until devres
frees the private data.  Register a devm action to disable the IRQ
and cancel both delayed works on any I2C teardown path.  Guard
disable_irq() in rt5668_i2c_shutdown() with the irq_disabled flag
to avoid an unbalanced disable when rt5668_remove() already ran.

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 v2:
- Clear SAR/button register state via rt5668_headset_detect(component, 0)
  before codec reset to prevent stale detection state on rebind
- Guard disable_irq() in rt5668_i2c_shutdown() with irq_disabled flag to
  avoid unbalanced disable when rt5668_remove() already ran
 sound/soc/codecs/rt5668.c | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/rt5668.c b/sound/soc/codecs/rt5668.c
index 29d70802a046a..c8aeaf25a7fd8 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,24 @@ 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);
 
+	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 +2596,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 +2611,11 @@ static void rt5668_i2c_shutdown(struct i2c_client *client)
 {
 	struct rt5668_priv *rt5668 = i2c_get_clientdata(client);
 
+	if (rt5668->irq_acquired && !rt5668->irq_disabled)
+		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 v2] ASoC: rt5668: fix teardown order and stale state across card rebind

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] ASoC: rt5668: duplicate IRQ teardown and missing irq_disabled
  updates
--

commit db40c33702a3afc67ec1edbbd986308c05f58e4d
Author: Tristan Madani <tristan@talencesecurity.com>

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

This commit clears headset state and marks the regmap cache dirty on
component removal, and adds a devres action and shutdown check to disable
the IRQ and cancel delayed works across teardown paths.

> diff --git a/sound/soc/codecs/rt5668.c b/sound/soc/codecs/rt5668.c
> index 29d70802a046a..c8aeaf25a7fd8 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);
> +}
[ ... ]
> @@ -2230,17 +2243,24 @@ 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);
[ ... ]
> @@ -2584,6 +2611,11 @@ static void rt5668_i2c_shutdown(struct i2c_client *client)
>  {
>  	struct rt5668_priv *rt5668 = i2c_get_clientdata(client);
>  
> +	if (rt5668->irq_acquired && !rt5668->irq_disabled)
> +		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]
Can this call disable_irq() twice if the device or card is unbound after
rt5668_i2c_shutdown() runs?

rt5668_i2c_shutdown() and rt5668_i2c_disable_irq_and_cancel_work() do not
set rt5668->irq_disabled = true after disabling the IRQ, and
rt5668_remove() does not check !rt5668->irq_disabled.  Setting
rt5668->irq_disabled = true inside
rt5668_i2c_disable_irq_and_cancel_work() and calling that helper from
both rt5668_remove() and rt5668_i2c_shutdown() would avoid duplicating
this teardown sequence and keep irq_disabled consistent.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107161024.1233587-1-tristmd@gmail.com?part=1