[PATCH 1/8] soundwire: fix enumeration completion

[PATCH 1/8] soundwire: fix enumeration completion

[Johan Hovold]

The soundwire subsystem uses two completion structures that allow
drivers to wait for soundwire device to become enumerated on the bus and
initialised by their drivers, respectively.

The code implementing the signalling is currently broken as it does not
signal all current and future waiters and also uses the wrong
reinitialisation function, which can potentially lead to memory
corruption if there are still waiters on the queue.

Not signalling future waiters specifically breaks sound card probe
deferrals as codec drivers can not tell that the soundwire device is
already attached when being reprobed. Some codec runtime PM
implementations suffer from similar problems as waiting for enumeration
during resume can also timeout despite the device already having been
enumerated.

Fixes: fb9469e54fa7 ("soundwire: bus: fix race condition with enumeration_complete signaling")
Fixes: a90def068127 ("soundwire: bus: fix race condition with initialization_complete signaling")
Cc: stable@vger.kernel.org      # 5.7
Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/soundwire/bus.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 1ea6a64f8c4a..66e5dba919fa 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -908,8 +908,8 @@ static void sdw_modify_slave_status(struct sdw_slave *slave,
 			"initializing enumeration and init completion for Slave %d\n",
 			slave->dev_num);
 
-		init_completion(&slave->enumeration_complete);
-		init_completion(&slave->initialization_complete);
+		reinit_completion(&slave->enumeration_complete);
+		reinit_completion(&slave->initialization_complete);
 
 	} else if ((status == SDW_SLAVE_ATTACHED) &&
 		   (slave->status == SDW_SLAVE_UNATTACHED)) {
@@ -917,7 +917,7 @@ static void sdw_modify_slave_status(struct sdw_slave *slave,
 			"signaling enumeration completion for Slave %d\n",
 			slave->dev_num);
 
-		complete(&slave->enumeration_complete);
+		complete_all(&slave->enumeration_complete);
 	}
 	slave->status = status;
 	mutex_unlock(&bus->bus_lock);
@@ -1941,7 +1941,7 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
 				"signaling initialization completion for Slave %d\n",
 				slave->dev_num);
 
-			complete(&slave->initialization_complete);
+			complete_all(&slave->initialization_complete);
 
 			/*
 			 * If the manager became pm_runtime active, the peripherals will be
-- 
2.39.3

[PATCH 2/8] ASoC: qdsp6: audioreach: fix topology probe deferral

[Johan Hovold]

Propagate errors when failing to load the topology component so that
probe deferrals can be handled.

Fixes: 36ad9bf1d93d ("ASoC: qdsp6: audioreach: add topology support")
Cc: stable@vger.kernel.org      # 5.17
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 sound/soc/qcom/qdsp6/topology.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/topology.c b/sound/soc/qcom/qdsp6/topology.c
index cccc59b570b9..130b22a34fb3 100644
--- a/sound/soc/qcom/qdsp6/topology.c
+++ b/sound/soc/qcom/qdsp6/topology.c
@@ -1277,8 +1277,8 @@ int audioreach_tplg_init(struct snd_soc_component *component)
 
 	ret = snd_soc_tplg_component_load(component, &audioreach_tplg_ops, fw);
 	if (ret < 0) {
-		dev_err(dev, "tplg component load failed%d\n", ret);
-		ret = -EINVAL;
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "tplg component load failed: %d\n", ret);
 	}
 
 	release_firmware(fw);
-- 
2.39.3

[PATCH 3/8] ASoC: codecs: wcd938x: fix missing clsh ctrl error handling

[Johan Hovold]

Allocation of the clash control structure may fail so add the missing
error handling to avoid dereferencing an error pointer.

Fixes: 8d78602aa87a ("ASoC: codecs: wcd938x: add basic driver")
Cc: stable@vger.kernel.org	# 5.14
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 sound/soc/codecs/wcd938x.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
index faa15a5ed2c8..2e342398d027 100644
--- a/sound/soc/codecs/wcd938x.c
+++ b/sound/soc/codecs/wcd938x.c
@@ -3106,6 +3106,10 @@ static int wcd938x_soc_codec_probe(struct snd_soc_component *component)
 						 WCD938X_ID_MASK);
 
 	wcd938x->clsh_info = wcd_clsh_ctrl_alloc(component, WCD938X);
+	if (IS_ERR(wcd938x->clsh_info)) {
+		pm_runtime_put(dev);
+		return PTR_ERR(wcd938x->clsh_info);
+	}
 
 	wcd938x_io_init(wcd938x);
 	/* Set all interrupts as edge triggered */
-- 
2.39.3

[PATCH 4/8] ASoC: codecs: wcd938x: fix resource leaks on component remove

[Johan Hovold]

Make sure to release allocated resources on component probe failure and
on remove.

This is specifically needed to allow probe deferrals of the sound card
which otherwise fails when reprobing the codec component:

    snd-sc8280xp sound: ASoC: failed to instantiate card -517
    genirq: Flags mismatch irq 289. 00002001 (HPHR PDM WD INT) vs. 00002001 (HPHR PDM WD INT)
    wcd938x_codec audio-codec: Failed to request HPHR WD interrupt (-16)
    genirq: Flags mismatch irq 290. 00002001 (HPHL PDM WD INT) vs. 00002001 (HPHL PDM WD INT)
    wcd938x_codec audio-codec: Failed to request HPHL WD interrupt (-16)
    genirq: Flags mismatch irq 291. 00002001 (AUX PDM WD INT) vs. 00002001 (AUX PDM WD INT)
    wcd938x_codec audio-codec: Failed to request Aux WD interrupt (-16)
    genirq: Flags mismatch irq 292. 00002001 (mbhc sw intr) vs. 00002001 (mbhc sw intr)
    wcd938x_codec audio-codec: Failed to request mbhc interrupts -16

Fixes: 8d78602aa87a ("ASoC: codecs: wcd938x: add basic driver")
Cc: stable@vger.kernel.org	# 5.14
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 sound/soc/codecs/wcd938x.c | 55 +++++++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
index 2e342398d027..be38cad5f354 100644
--- a/sound/soc/codecs/wcd938x.c
+++ b/sound/soc/codecs/wcd938x.c
@@ -2636,6 +2636,14 @@ static int wcd938x_mbhc_init(struct snd_soc_component *component)
 
 	return 0;
 }
+
+static void wcd938x_mbhc_deinit(struct snd_soc_component *component)
+{
+	struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component);
+
+	wcd_mbhc_deinit(wcd938x->wcd_mbhc);
+}
+
 /* END MBHC */
 
 static const struct snd_kcontrol_new wcd938x_snd_controls[] = {
@@ -3131,20 +3139,26 @@ static int wcd938x_soc_codec_probe(struct snd_soc_component *component)
 	ret = request_threaded_irq(wcd938x->hphr_pdm_wd_int, NULL, wcd938x_wd_handle_irq,
 				   IRQF_ONESHOT | IRQF_TRIGGER_RISING,
 				   "HPHR PDM WD INT", wcd938x);
-	if (ret)
+	if (ret) {
 		dev_err(dev, "Failed to request HPHR WD interrupt (%d)\n", ret);
+		goto err_free_clsh_ctrl;
+	}
 
 	ret = request_threaded_irq(wcd938x->hphl_pdm_wd_int, NULL, wcd938x_wd_handle_irq,
 				   IRQF_ONESHOT | IRQF_TRIGGER_RISING,
 				   "HPHL PDM WD INT", wcd938x);
-	if (ret)
+	if (ret) {
 		dev_err(dev, "Failed to request HPHL WD interrupt (%d)\n", ret);
+		goto err_free_hphr_pdm_wd_int;
+	}
 
 	ret = request_threaded_irq(wcd938x->aux_pdm_wd_int, NULL, wcd938x_wd_handle_irq,
 				   IRQF_ONESHOT | IRQF_TRIGGER_RISING,
 				   "AUX PDM WD INT", wcd938x);
-	if (ret)
+	if (ret) {
 		dev_err(dev, "Failed to request Aux WD interrupt (%d)\n", ret);
+		goto err_free_hphl_pdm_wd_int;
+	}
 
 	/* Disable watchdog interrupt for HPH and AUX */
 	disable_irq_nosync(wcd938x->hphr_pdm_wd_int);
@@ -3159,7 +3173,7 @@ static int wcd938x_soc_codec_probe(struct snd_soc_component *component)
 			dev_err(component->dev,
 				"%s: Failed to add snd ctrls for variant: %d\n",
 				__func__, wcd938x->variant);
-			goto err;
+			goto err_free_aux_pdm_wd_int;
 		}
 		break;
 	case WCD9385:
@@ -3169,7 +3183,7 @@ static int wcd938x_soc_codec_probe(struct snd_soc_component *component)
 			dev_err(component->dev,
 				"%s: Failed to add snd ctrls for variant: %d\n",
 				__func__, wcd938x->variant);
-			goto err;
+			goto err_free_aux_pdm_wd_int;
 		}
 		break;
 	default:
@@ -3177,12 +3191,38 @@ static int wcd938x_soc_codec_probe(struct snd_soc_component *component)
 	}
 
 	ret = wcd938x_mbhc_init(component);
-	if (ret)
+	if (ret) {
 		dev_err(component->dev,  "mbhc initialization failed\n");
-err:
+		goto err_free_aux_pdm_wd_int;
+	}
+
+	return 0;
+
+err_free_aux_pdm_wd_int:
+	free_irq(wcd938x->aux_pdm_wd_int, wcd938x);
+err_free_hphl_pdm_wd_int:
+	free_irq(wcd938x->hphl_pdm_wd_int, wcd938x);
+err_free_hphr_pdm_wd_int:
+	free_irq(wcd938x->hphr_pdm_wd_int, wcd938x);
+err_free_clsh_ctrl:
+	wcd_clsh_ctrl_free(wcd938x->clsh_info);
+
 	return ret;
 }
 
+static void wcd938x_soc_codec_remove(struct snd_soc_component *component)
+{
+	struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component);
+
+	wcd938x_mbhc_deinit(component);
+
+	free_irq(wcd938x->aux_pdm_wd_int, wcd938x);
+	free_irq(wcd938x->hphl_pdm_wd_int, wcd938x);
+	free_irq(wcd938x->hphr_pdm_wd_int, wcd938x);
+
+	wcd_clsh_ctrl_free(wcd938x->clsh_info);
+}
+
 static int wcd938x_codec_set_jack(struct snd_soc_component *comp,
 				  struct snd_soc_jack *jack, void *data)
 {
@@ -3199,6 +3239,7 @@ static int wcd938x_codec_set_jack(struct snd_soc_component *comp,
 static const struct snd_soc_component_driver soc_codec_dev_wcd938x = {
 	.name = "wcd938x_codec",
 	.probe = wcd938x_soc_codec_probe,
+	.remove = wcd938x_soc_codec_remove,
 	.controls = wcd938x_snd_controls,
 	.num_controls = ARRAY_SIZE(wcd938x_snd_controls),
 	.dapm_widgets = wcd938x_dapm_widgets,
-- 
2.39.3

[PATCH 5/8] ASoC: codecs: wcd934x: fix resource leaks on component remove

[Johan Hovold]

Make sure to release allocated MBHC resources also on component remove.

This is specifically needed to allow probe deferrals of the sound card
which otherwise fails when reprobing the codec component.

Fixes: 9fb9b1690f0b ("ASoC: codecs: wcd934x: add mbhc support")
Cc: stable@vger.kernel.org      # 5.14
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 sound/soc/codecs/wcd934x.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c
index a17cd75b969b..1b6e376f3833 100644
--- a/sound/soc/codecs/wcd934x.c
+++ b/sound/soc/codecs/wcd934x.c
@@ -3044,6 +3044,17 @@ static int wcd934x_mbhc_init(struct snd_soc_component *component)
 
 	return 0;
 }
+
+static void wcd934x_mbhc_deinit(struct snd_soc_component *component)
+{
+	struct wcd934x_codec *wcd = snd_soc_component_get_drvdata(component);
+
+	if (!wcd->mbhc)
+		return;
+
+	wcd_mbhc_deinit(wcd->mbhc);
+}
+
 static int wcd934x_comp_probe(struct snd_soc_component *component)
 {
 	struct wcd934x_codec *wcd = dev_get_drvdata(component->dev);
@@ -3077,6 +3088,7 @@ static void wcd934x_comp_remove(struct snd_soc_component *comp)
 {
 	struct wcd934x_codec *wcd = dev_get_drvdata(comp->dev);
 
+	wcd934x_mbhc_deinit(comp);
 	wcd_clsh_ctrl_free(wcd->clsh_ctrl);
 }
 
-- 
2.39.3

[PATCH 6/8] ASoC: codecs: wcd-mbhc-v2: fix resource leaks on component remove

[Johan Hovold]

The MBHC resources must be released on component probe failure and
removal so can not be tied to the lifetime of the component device.

This is specifically needed to allow probe deferrals of the sound card
which otherwise fails when reprobing the codec component:

    snd-sc8280xp sound: ASoC: failed to instantiate card -517
    genirq: Flags mismatch irq 299. 00002001 (mbhc sw intr) vs. 00002001 (mbhc sw intr)
    wcd938x_codec audio-codec: Failed to request mbhc interrupts -16
    wcd938x_codec audio-codec: mbhc initialization failed
    wcd938x_codec audio-codec: ASoC: error at snd_soc_component_probe on audio-codec: -16
    snd-sc8280xp sound: ASoC: failed to instantiate card -16

Fixes: 0e5c9e7ff899 ("ASoC: codecs: wcd: add multi button Headset detection support")
Cc: stable@vger.kernel.org      # 5.14
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 sound/soc/codecs/wcd-mbhc-v2.c | 57 ++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/sound/soc/codecs/wcd-mbhc-v2.c b/sound/soc/codecs/wcd-mbhc-v2.c
index 1911750f7445..5da1934527f3 100644
--- a/sound/soc/codecs/wcd-mbhc-v2.c
+++ b/sound/soc/codecs/wcd-mbhc-v2.c
@@ -1454,7 +1454,7 @@ struct wcd_mbhc *wcd_mbhc_init(struct snd_soc_component *component,
 		return ERR_PTR(-EINVAL);
 	}
 
-	mbhc = devm_kzalloc(dev, sizeof(*mbhc), GFP_KERNEL);
+	mbhc = kzalloc(sizeof(*mbhc), GFP_KERNEL);
 	if (!mbhc)
 		return ERR_PTR(-ENOMEM);
 
@@ -1474,61 +1474,76 @@ struct wcd_mbhc *wcd_mbhc_init(struct snd_soc_component *component,
 
 	INIT_WORK(&mbhc->correct_plug_swch, wcd_correct_swch_plug);
 
-	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->mbhc_sw_intr, NULL,
+	ret = request_threaded_irq(mbhc->intr_ids->mbhc_sw_intr, NULL,
 					wcd_mbhc_mech_plug_detect_irq,
 					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
 					"mbhc sw intr", mbhc);
 	if (ret)
-		goto err;
+		goto err_free_mbhc;
 
-	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->mbhc_btn_press_intr, NULL,
+	ret = request_threaded_irq(mbhc->intr_ids->mbhc_btn_press_intr, NULL,
 					wcd_mbhc_btn_press_handler,
 					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
 					"Button Press detect", mbhc);
 	if (ret)
-		goto err;
+		goto err_free_sw_intr;
 
-	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->mbhc_btn_release_intr, NULL,
+	ret = request_threaded_irq(mbhc->intr_ids->mbhc_btn_release_intr, NULL,
 					wcd_mbhc_btn_release_handler,
 					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
 					"Button Release detect", mbhc);
 	if (ret)
-		goto err;
+		goto err_free_btn_press_intr;
 
-	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->mbhc_hs_ins_intr, NULL,
+	ret = request_threaded_irq(mbhc->intr_ids->mbhc_hs_ins_intr, NULL,
 					wcd_mbhc_adc_hs_ins_irq,
 					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
 					"Elect Insert", mbhc);
 	if (ret)
-		goto err;
+		goto err_free_btn_release_intr;
 
 	disable_irq_nosync(mbhc->intr_ids->mbhc_hs_ins_intr);
 
-	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->mbhc_hs_rem_intr, NULL,
+	ret = request_threaded_irq(mbhc->intr_ids->mbhc_hs_rem_intr, NULL,
 					wcd_mbhc_adc_hs_rem_irq,
 					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
 					"Elect Remove", mbhc);
 	if (ret)
-		goto err;
+		goto err_free_hs_ins_intr;
 
 	disable_irq_nosync(mbhc->intr_ids->mbhc_hs_rem_intr);
 
-	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->hph_left_ocp, NULL,
+	ret = request_threaded_irq(mbhc->intr_ids->hph_left_ocp, NULL,
 					wcd_mbhc_hphl_ocp_irq,
 					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
 					"HPH_L OCP detect", mbhc);
 	if (ret)
-		goto err;
+		goto err_free_hs_rem_intr;
 
-	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->hph_right_ocp, NULL,
+	ret = request_threaded_irq(mbhc->intr_ids->hph_right_ocp, NULL,
 					wcd_mbhc_hphr_ocp_irq,
 					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
 					"HPH_R OCP detect", mbhc);
 	if (ret)
-		goto err;
+		goto err_free_hph_left_ocp;
 
 	return mbhc;
-err:
+
+err_free_hph_left_ocp:
+	free_irq(mbhc->intr_ids->hph_left_ocp, mbhc);
+err_free_hs_rem_intr:
+	free_irq(mbhc->intr_ids->mbhc_hs_rem_intr, mbhc);
+err_free_hs_ins_intr:
+	free_irq(mbhc->intr_ids->mbhc_hs_ins_intr, mbhc);
+err_free_btn_release_intr:
+	free_irq(mbhc->intr_ids->mbhc_btn_release_intr, mbhc);
+err_free_btn_press_intr:
+	free_irq(mbhc->intr_ids->mbhc_btn_press_intr, mbhc);
+err_free_sw_intr:
+	free_irq(mbhc->intr_ids->mbhc_sw_intr, mbhc);
+err_free_mbhc:
+	kfree(mbhc);
+
 	dev_err(dev, "Failed to request mbhc interrupts %d\n", ret);
 
 	return ERR_PTR(ret);
@@ -1537,9 +1552,19 @@ EXPORT_SYMBOL(wcd_mbhc_init);
 
 void wcd_mbhc_deinit(struct wcd_mbhc *mbhc)
 {
+	free_irq(mbhc->intr_ids->hph_right_ocp, mbhc);
+	free_irq(mbhc->intr_ids->hph_left_ocp, mbhc);
+	free_irq(mbhc->intr_ids->mbhc_hs_rem_intr, mbhc);
+	free_irq(mbhc->intr_ids->mbhc_hs_ins_intr, mbhc);
+	free_irq(mbhc->intr_ids->mbhc_btn_release_intr, mbhc);
+	free_irq(mbhc->intr_ids->mbhc_btn_press_intr, mbhc);
+	free_irq(mbhc->intr_ids->mbhc_sw_intr, mbhc);
+
 	mutex_lock(&mbhc->lock);
 	wcd_cancel_hs_detect_plug(mbhc,	&mbhc->correct_plug_swch);
 	mutex_unlock(&mbhc->lock);
+
+	kfree(mbhc);
 }
 EXPORT_SYMBOL(wcd_mbhc_deinit);
 
-- 
2.39.3

Re: [PATCH 1/8] soundwire: fix enumeration completion

[Pierre-Louis Bossart]

On 7/5/23 14:30, Johan Hovold wrote:
> The soundwire subsystem uses two completion structures that allow
> drivers to wait for soundwire device to become enumerated on the bus and
> initialised by their drivers, respectively.
> 
> The code implementing the signalling is currently broken as it does not
> signal all current and future waiters and also uses the wrong
> reinitialisation function, which can potentially lead to memory
> corruption if there are still waiters on the queue.

That change sounds good, but I am not following the two paragraphs below.

> Not signalling future waiters specifically breaks sound card probe
> deferrals as codec drivers can not tell that the soundwire device is
> already attached when being reprobed. 

What makes you say that? There is a test in the probe and the codec
driver will absolutely be notified, see bus_type.c

	if (drv->ops && drv->ops->update_status) {
		ret = drv->ops->update_status(slave, slave->status);
		if (ret < 0)
			dev_warn(dev, "%s: update_status failed with status %d\n", __func__,
ret);
	}

> Some codec runtime PM
> implementations suffer from similar problems as waiting for enumeration
> during resume can also timeout despite the device already having been
> enumerated.

I am not following this either. Are you saying the wait_for_completion()
times out because of the init_completion/reinit_completion confusion, or
something else.

> Fixes: fb9469e54fa7 ("soundwire: bus: fix race condition with enumeration_complete signaling")
> Fixes: a90def068127 ("soundwire: bus: fix race condition with initialization_complete signaling")
> Cc: stable@vger.kernel.org      # 5.7
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Cc: Rander Wang <rander.wang@linux.intel.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/soundwire/bus.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 1ea6a64f8c4a..66e5dba919fa 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -908,8 +908,8 @@ static void sdw_modify_slave_status(struct sdw_slave *slave,
>  			"initializing enumeration and init completion for Slave %d\n",
>  			slave->dev_num);
>  
> -		init_completion(&slave->enumeration_complete);
> -		init_completion(&slave->initialization_complete);
> +		reinit_completion(&slave->enumeration_complete);
> +		reinit_completion(&slave->initialization_complete);
>  
>  	} else if ((status == SDW_SLAVE_ATTACHED) &&
>  		   (slave->status == SDW_SLAVE_UNATTACHED)) {
> @@ -917,7 +917,7 @@ static void sdw_modify_slave_status(struct sdw_slave *slave,
>  			"signaling enumeration completion for Slave %d\n",
>  			slave->dev_num);
>  
> -		complete(&slave->enumeration_complete);
> +		complete_all(&slave->enumeration_complete);
>  	}
>  	slave->status = status;
>  	mutex_unlock(&bus->bus_lock);
> @@ -1941,7 +1941,7 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
>  				"signaling initialization completion for Slave %d\n",
>  				slave->dev_num);
>  
> -			complete(&slave->initialization_complete);
> +			complete_all(&slave->initialization_complete);
>  
>  			/*
>  			 * If the manager became pm_runtime active, the peripherals will be

Re: [PATCH 1/8] soundwire: fix enumeration completion

[Johan Hovold]

On Wed, Jul 05, 2023 at 02:53:17PM +0200, Pierre-Louis Bossart wrote:
> On 7/5/23 14:30, Johan Hovold wrote:
> > The soundwire subsystem uses two completion structures that allow
> > drivers to wait for soundwire device to become enumerated on the bus and
> > initialised by their drivers, respectively.
> > 
> > The code implementing the signalling is currently broken as it does not
> > signal all current and future waiters and also uses the wrong
> > reinitialisation function, which can potentially lead to memory
> > corruption if there are still waiters on the queue.
> 
> That change sounds good, but I am not following the two paragraphs below.
> 
> > Not signalling future waiters specifically breaks sound card probe
> > deferrals as codec drivers can not tell that the soundwire device is
> > already attached when being reprobed. 
> 
> What makes you say that? There is a test in the probe and the codec
> driver will absolutely be notified, see bus_type.c
> 
> 	if (drv->ops && drv->ops->update_status) {
> 		ret = drv->ops->update_status(slave, slave->status);
> 		if (ret < 0)
> 			dev_warn(dev, "%s: update_status failed with status %d\n", __func__,
> ret);
> 	}

I'm talking about signalling the codec driver using the soundwire device
via the completion structs. Unless the underling device is detached and
reattached, trying to wait for completion a second time will currently
timeout instead of returning immediately.

This affects codecs like rt5682, which wait for completion in component
probe (see rt5682_probe()).

> > Some codec runtime PM
> > implementations suffer from similar problems as waiting for enumeration
> > during resume can also timeout despite the device already having been
> > enumerated.
> 
> I am not following this either. Are you saying the wait_for_completion()
> times out because of the init_completion/reinit_completion confusion, or
> something else.

It times out because the completion counter is not saturated unless you
use complete_all().

Drivers that wait unconditionally in resume, will time out the second
time they are runtime resumed unless the underlying device has been
detached and reattached in the meantime (e.g. wsa881x_runtime_resume()).

Johan

Re: [PATCH 1/8] soundwire: fix enumeration completion

[Pierre-Louis Bossart]

On 7/5/23 16:30, Johan Hovold wrote:
> On Wed, Jul 05, 2023 at 02:53:17PM +0200, Pierre-Louis Bossart wrote:
>> On 7/5/23 14:30, Johan Hovold wrote:
>>> The soundwire subsystem uses two completion structures that allow
>>> drivers to wait for soundwire device to become enumerated on the bus and
>>> initialised by their drivers, respectively.
>>>
>>> The code implementing the signalling is currently broken as it does not
>>> signal all current and future waiters and also uses the wrong
>>> reinitialisation function, which can potentially lead to memory
>>> corruption if there are still waiters on the queue.
>>
>> That change sounds good, but I am not following the two paragraphs below.
>>
>>> Not signalling future waiters specifically breaks sound card probe
>>> deferrals as codec drivers can not tell that the soundwire device is
>>> already attached when being reprobed. 
>>
>> What makes you say that? There is a test in the probe and the codec
>> driver will absolutely be notified, see bus_type.c
>>
>> 	if (drv->ops && drv->ops->update_status) {
>> 		ret = drv->ops->update_status(slave, slave->status);
>> 		if (ret < 0)
>> 			dev_warn(dev, "%s: update_status failed with status %d\n", __func__,
>> ret);
>> 	}
> 
> I'm talking about signalling the codec driver using the soundwire device
> via the completion structs. Unless the underling device is detached and
> reattached, trying to wait for completion a second time will currently
> timeout instead of returning immediately.
> 
> This affects codecs like rt5682, which wait for completion in component
> probe (see rt5682_probe()).
> 
>>> Some codec runtime PM
>>> implementations suffer from similar problems as waiting for enumeration
>>> during resume can also timeout despite the device already having been
>>> enumerated.
>>
>> I am not following this either. Are you saying the wait_for_completion()
>> times out because of the init_completion/reinit_completion confusion, or
>> something else.
> 
> It times out because the completion counter is not saturated unless you
> use complete_all().
> 
> Drivers that wait unconditionally in resume, will time out the second
> time they are runtime resumed unless the underlying device has been
> detached and reattached in the meantime (e.g. wsa881x_runtime_resume()).

Makes sense. The default on Intel platforms is to reset the bus in all
resume cases, that forces the attachment so we never saw the issue.

For this patch:

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Re: [PATCH 3/8] ASoC: codecs: wcd938x: fix missing clsh ctrl error handling

[Srinivas Kandagatla]

On 05/07/2023 13:30, Johan Hovold wrote:
> Allocation of the clash control structure may fail so add the missing
> error handling to avoid dereferencing an error pointer.
> 
> Fixes: 8d78602aa87a ("ASoC: codecs: wcd938x: add basic driver")
> Cc: stable@vger.kernel.org	# 5.14
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>


>   sound/soc/codecs/wcd938x.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
> index faa15a5ed2c8..2e342398d027 100644
> --- a/sound/soc/codecs/wcd938x.c
> +++ b/sound/soc/codecs/wcd938x.c
> @@ -3106,6 +3106,10 @@ static int wcd938x_soc_codec_probe(struct snd_soc_component *component)
>   						 WCD938X_ID_MASK);
>   
>   	wcd938x->clsh_info = wcd_clsh_ctrl_alloc(component, WCD938X);
> +	if (IS_ERR(wcd938x->clsh_info)) {
> +		pm_runtime_put(dev);
> +		return PTR_ERR(wcd938x->clsh_info);
> +	}
>   
>   	wcd938x_io_init(wcd938x);
>   	/* Set all interrupts as edge triggered */

Re: [PATCH 2/8] ASoC: qdsp6: audioreach: fix topology probe deferral

[Srinivas Kandagatla]

On 05/07/2023 13:30, Johan Hovold wrote:
> Propagate errors when failing to load the topology component so that
> probe deferrals can be handled.
> 
> Fixes: 36ad9bf1d93d ("ASoC: qdsp6: audioreach: add topology support")
> Cc: stable@vger.kernel.org      # 5.17
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>


>   sound/soc/qcom/qdsp6/topology.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/qcom/qdsp6/topology.c b/sound/soc/qcom/qdsp6/topology.c
> index cccc59b570b9..130b22a34fb3 100644
> --- a/sound/soc/qcom/qdsp6/topology.c
> +++ b/sound/soc/qcom/qdsp6/topology.c
> @@ -1277,8 +1277,8 @@ int audioreach_tplg_init(struct snd_soc_component *component)
>   
>   	ret = snd_soc_tplg_component_load(component, &audioreach_tplg_ops, fw);
>   	if (ret < 0) {
> -		dev_err(dev, "tplg component load failed%d\n", ret);
> -		ret = -EINVAL;
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "tplg component load failed: %d\n", ret);
>   	}
>   
>   	release_firmware(fw);

Re: [PATCH 4/8] ASoC: codecs: wcd938x: fix resource leaks on component remove

[Srinivas Kandagatla]

On 05/07/2023 13:30, Johan Hovold wrote:
> Make sure to release allocated resources on component probe failure and
> on remove.
> 
> This is specifically needed to allow probe deferrals of the sound card
> which otherwise fails when reprobing the codec component:
> 
>      snd-sc8280xp sound: ASoC: failed to instantiate card -517
>      genirq: Flags mismatch irq 289. 00002001 (HPHR PDM WD INT) vs. 00002001 (HPHR PDM WD INT)
>      wcd938x_codec audio-codec: Failed to request HPHR WD interrupt (-16)
>      genirq: Flags mismatch irq 290. 00002001 (HPHL PDM WD INT) vs. 00002001 (HPHL PDM WD INT)
>      wcd938x_codec audio-codec: Failed to request HPHL WD interrupt (-16)
>      genirq: Flags mismatch irq 291. 00002001 (AUX PDM WD INT) vs. 00002001 (AUX PDM WD INT)
>      wcd938x_codec audio-codec: Failed to request Aux WD interrupt (-16)
>      genirq: Flags mismatch irq 292. 00002001 (mbhc sw intr) vs. 00002001 (mbhc sw intr)
>      wcd938x_codec audio-codec: Failed to request mbhc interrupts -16
> 
> Fixes: 8d78602aa87a ("ASoC: codecs: wcd938x: add basic driver")
> Cc: stable@vger.kernel.org	# 5.14
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>


>   sound/soc/codecs/wcd938x.c | 55 +++++++++++++++++++++++++++++++++-----
>   1 file changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
> index 2e342398d027..be38cad5f354 100644
> --- a/sound/soc/codecs/wcd938x.c
> +++ b/sound/soc/codecs/wcd938x.c
> @@ -2636,6 +2636,14 @@ static int wcd938x_mbhc_init(struct snd_soc_component *component)
>   
>   	return 0;
>   }
> +
> +static void wcd938x_mbhc_deinit(struct snd_soc_component *component)
> +{
> +	struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component);
> +
> +	wcd_mbhc_deinit(wcd938x->wcd_mbhc);
> +}
> +
>   /* END MBHC */
>   
>   static const struct snd_kcontrol_new wcd938x_snd_controls[] = {
> @@ -3131,20 +3139,26 @@ static int wcd938x_soc_codec_probe(struct snd_soc_component *component)
>   	ret = request_threaded_irq(wcd938x->hphr_pdm_wd_int, NULL, wcd938x_wd_handle_irq,
>   				   IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>   				   "HPHR PDM WD INT", wcd938x);
> -	if (ret)
> +	if (ret) {
>   		dev_err(dev, "Failed to request HPHR WD interrupt (%d)\n", ret);
> +		goto err_free_clsh_ctrl;
> +	}
>   
>   	ret = request_threaded_irq(wcd938x->hphl_pdm_wd_int, NULL, wcd938x_wd_handle_irq,
>   				   IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>   				   "HPHL PDM WD INT", wcd938x);
> -	if (ret)
> +	if (ret) {
>   		dev_err(dev, "Failed to request HPHL WD interrupt (%d)\n", ret);
> +		goto err_free_hphr_pdm_wd_int;
> +	}
>   
>   	ret = request_threaded_irq(wcd938x->aux_pdm_wd_int, NULL, wcd938x_wd_handle_irq,
>   				   IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>   				   "AUX PDM WD INT", wcd938x);
> -	if (ret)
> +	if (ret) {
>   		dev_err(dev, "Failed to request Aux WD interrupt (%d)\n", ret);
> +		goto err_free_hphl_pdm_wd_int;
> +	}
>   
>   	/* Disable watchdog interrupt for HPH and AUX */
>   	disable_irq_nosync(wcd938x->hphr_pdm_wd_int);
> @@ -3159,7 +3173,7 @@ static int wcd938x_soc_codec_probe(struct snd_soc_component *component)
>   			dev_err(component->dev,
>   				"%s: Failed to add snd ctrls for variant: %d\n",
>   				__func__, wcd938x->variant);
> -			goto err;
> +			goto err_free_aux_pdm_wd_int;
>   		}
>   		break;
>   	case WCD9385:
> @@ -3169,7 +3183,7 @@ static int wcd938x_soc_codec_probe(struct snd_soc_component *component)
>   			dev_err(component->dev,
>   				"%s: Failed to add snd ctrls for variant: %d\n",
>   				__func__, wcd938x->variant);
> -			goto err;
> +			goto err_free_aux_pdm_wd_int;
>   		}
>   		break;
>   	default:
> @@ -3177,12 +3191,38 @@ static int wcd938x_soc_codec_probe(struct snd_soc_component *component)
>   	}
>   
>   	ret = wcd938x_mbhc_init(component);
> -	if (ret)
> +	if (ret) {
>   		dev_err(component->dev,  "mbhc initialization failed\n");
> -err:
> +		goto err_free_aux_pdm_wd_int;
> +	}
> +
> +	return 0;
> +
> +err_free_aux_pdm_wd_int:
> +	free_irq(wcd938x->aux_pdm_wd_int, wcd938x);
> +err_free_hphl_pdm_wd_int:
> +	free_irq(wcd938x->hphl_pdm_wd_int, wcd938x);
> +err_free_hphr_pdm_wd_int:
> +	free_irq(wcd938x->hphr_pdm_wd_int, wcd938x);
> +err_free_clsh_ctrl:
> +	wcd_clsh_ctrl_free(wcd938x->clsh_info);
> +
>   	return ret;
>   }
>   
> +static void wcd938x_soc_codec_remove(struct snd_soc_component *component)
> +{
> +	struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component);
> +
> +	wcd938x_mbhc_deinit(component);
> +
> +	free_irq(wcd938x->aux_pdm_wd_int, wcd938x);
> +	free_irq(wcd938x->hphl_pdm_wd_int, wcd938x);
> +	free_irq(wcd938x->hphr_pdm_wd_int, wcd938x);
> +
> +	wcd_clsh_ctrl_free(wcd938x->clsh_info);
> +}
> +
>   static int wcd938x_codec_set_jack(struct snd_soc_component *comp,
>   				  struct snd_soc_jack *jack, void *data)
>   {
> @@ -3199,6 +3239,7 @@ static int wcd938x_codec_set_jack(struct snd_soc_component *comp,
>   static const struct snd_soc_component_driver soc_codec_dev_wcd938x = {
>   	.name = "wcd938x_codec",
>   	.probe = wcd938x_soc_codec_probe,
> +	.remove = wcd938x_soc_codec_remove,
>   	.controls = wcd938x_snd_controls,
>   	.num_controls = ARRAY_SIZE(wcd938x_snd_controls),
>   	.dapm_widgets = wcd938x_dapm_widgets,

Re: [PATCH 5/8] ASoC: codecs: wcd934x: fix resource leaks on component remove

[Srinivas Kandagatla]

On 05/07/2023 13:30, Johan Hovold wrote:
> Make sure to release allocated MBHC resources also on component remove.
> 
> This is specifically needed to allow probe deferrals of the sound card
> which otherwise fails when reprobing the codec component.
> 
> Fixes: 9fb9b1690f0b ("ASoC: codecs: wcd934x: add mbhc support")
> Cc: stable@vger.kernel.org      # 5.14
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

>   sound/soc/codecs/wcd934x.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c
> index a17cd75b969b..1b6e376f3833 100644
> --- a/sound/soc/codecs/wcd934x.c
> +++ b/sound/soc/codecs/wcd934x.c
> @@ -3044,6 +3044,17 @@ static int wcd934x_mbhc_init(struct snd_soc_component *component)
>   
>   	return 0;
>   }
> +
> +static void wcd934x_mbhc_deinit(struct snd_soc_component *component)
> +{
> +	struct wcd934x_codec *wcd = snd_soc_component_get_drvdata(component);
> +
> +	if (!wcd->mbhc)
> +		return;
> +
> +	wcd_mbhc_deinit(wcd->mbhc);
> +}
> +
>   static int wcd934x_comp_probe(struct snd_soc_component *component)
>   {
>   	struct wcd934x_codec *wcd = dev_get_drvdata(component->dev);
> @@ -3077,6 +3088,7 @@ static void wcd934x_comp_remove(struct snd_soc_component *comp)
>   {
>   	struct wcd934x_codec *wcd = dev_get_drvdata(comp->dev);
>   
> +	wcd934x_mbhc_deinit(comp);
>   	wcd_clsh_ctrl_free(wcd->clsh_ctrl);
>   }
>   

Re: [PATCH 6/8] ASoC: codecs: wcd-mbhc-v2: fix resource leaks on component remove

[Srinivas Kandagatla]

On 05/07/2023 13:30, Johan Hovold wrote:
> The MBHC resources must be released on component probe failure and
> removal so can not be tied to the lifetime of the component device.
> 
> This is specifically needed to allow probe deferrals of the sound card
> which otherwise fails when reprobing the codec component:
> 
>      snd-sc8280xp sound: ASoC: failed to instantiate card -517
>      genirq: Flags mismatch irq 299. 00002001 (mbhc sw intr) vs. 00002001 (mbhc sw intr)
>      wcd938x_codec audio-codec: Failed to request mbhc interrupts -16
>      wcd938x_codec audio-codec: mbhc initialization failed
>      wcd938x_codec audio-codec: ASoC: error at snd_soc_component_probe on audio-codec: -16
>      snd-sc8280xp sound: ASoC: failed to instantiate card -16
> 
> Fixes: 0e5c9e7ff899 ("ASoC: codecs: wcd: add multi button Headset detection support")
> Cc: stable@vger.kernel.org      # 5.14
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

--srini
>   sound/soc/codecs/wcd-mbhc-v2.c | 57 ++++++++++++++++++++++++----------
>   1 file changed, 41 insertions(+), 16 deletions(-)
> 
> diff --git a/sound/soc/codecs/wcd-mbhc-v2.c b/sound/soc/codecs/wcd-mbhc-v2.c
> index 1911750f7445..5da1934527f3 100644
> --- a/sound/soc/codecs/wcd-mbhc-v2.c
> +++ b/sound/soc/codecs/wcd-mbhc-v2.c
> @@ -1454,7 +1454,7 @@ struct wcd_mbhc *wcd_mbhc_init(struct snd_soc_component *component,
>   		return ERR_PTR(-EINVAL);
>   	}
>   
> -	mbhc = devm_kzalloc(dev, sizeof(*mbhc), GFP_KERNEL);
> +	mbhc = kzalloc(sizeof(*mbhc), GFP_KERNEL);
>   	if (!mbhc)
>   		return ERR_PTR(-ENOMEM);
>   
> @@ -1474,61 +1474,76 @@ struct wcd_mbhc *wcd_mbhc_init(struct snd_soc_component *component,
>   
>   	INIT_WORK(&mbhc->correct_plug_swch, wcd_correct_swch_plug);
>   
> -	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->mbhc_sw_intr, NULL,
> +	ret = request_threaded_irq(mbhc->intr_ids->mbhc_sw_intr, NULL,
>   					wcd_mbhc_mech_plug_detect_irq,
>   					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>   					"mbhc sw intr", mbhc);
>   	if (ret)
> -		goto err;
> +		goto err_free_mbhc;
>   
> -	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->mbhc_btn_press_intr, NULL,
> +	ret = request_threaded_irq(mbhc->intr_ids->mbhc_btn_press_intr, NULL,
>   					wcd_mbhc_btn_press_handler,
>   					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>   					"Button Press detect", mbhc);
>   	if (ret)
> -		goto err;
> +		goto err_free_sw_intr;
>   
> -	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->mbhc_btn_release_intr, NULL,
> +	ret = request_threaded_irq(mbhc->intr_ids->mbhc_btn_release_intr, NULL,
>   					wcd_mbhc_btn_release_handler,
>   					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>   					"Button Release detect", mbhc);
>   	if (ret)
> -		goto err;
> +		goto err_free_btn_press_intr;
>   
> -	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->mbhc_hs_ins_intr, NULL,
> +	ret = request_threaded_irq(mbhc->intr_ids->mbhc_hs_ins_intr, NULL,
>   					wcd_mbhc_adc_hs_ins_irq,
>   					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>   					"Elect Insert", mbhc);
>   	if (ret)
> -		goto err;
> +		goto err_free_btn_release_intr;
>   
>   	disable_irq_nosync(mbhc->intr_ids->mbhc_hs_ins_intr);
>   
> -	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->mbhc_hs_rem_intr, NULL,
> +	ret = request_threaded_irq(mbhc->intr_ids->mbhc_hs_rem_intr, NULL,
>   					wcd_mbhc_adc_hs_rem_irq,
>   					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>   					"Elect Remove", mbhc);
>   	if (ret)
> -		goto err;
> +		goto err_free_hs_ins_intr;
>   
>   	disable_irq_nosync(mbhc->intr_ids->mbhc_hs_rem_intr);
>   
> -	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->hph_left_ocp, NULL,
> +	ret = request_threaded_irq(mbhc->intr_ids->hph_left_ocp, NULL,
>   					wcd_mbhc_hphl_ocp_irq,
>   					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>   					"HPH_L OCP detect", mbhc);
>   	if (ret)
> -		goto err;
> +		goto err_free_hs_rem_intr;
>   
> -	ret = devm_request_threaded_irq(dev, mbhc->intr_ids->hph_right_ocp, NULL,
> +	ret = request_threaded_irq(mbhc->intr_ids->hph_right_ocp, NULL,
>   					wcd_mbhc_hphr_ocp_irq,
>   					IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>   					"HPH_R OCP detect", mbhc);
>   	if (ret)
> -		goto err;
> +		goto err_free_hph_left_ocp;
>   
>   	return mbhc;
> -err:
> +
> +err_free_hph_left_ocp:
> +	free_irq(mbhc->intr_ids->hph_left_ocp, mbhc);
> +err_free_hs_rem_intr:
> +	free_irq(mbhc->intr_ids->mbhc_hs_rem_intr, mbhc);
> +err_free_hs_ins_intr:
> +	free_irq(mbhc->intr_ids->mbhc_hs_ins_intr, mbhc);
> +err_free_btn_release_intr:
> +	free_irq(mbhc->intr_ids->mbhc_btn_release_intr, mbhc);
> +err_free_btn_press_intr:
> +	free_irq(mbhc->intr_ids->mbhc_btn_press_intr, mbhc);
> +err_free_sw_intr:
> +	free_irq(mbhc->intr_ids->mbhc_sw_intr, mbhc);
> +err_free_mbhc:
> +	kfree(mbhc);
> +
>   	dev_err(dev, "Failed to request mbhc interrupts %d\n", ret);
>   
>   	return ERR_PTR(ret);
> @@ -1537,9 +1552,19 @@ EXPORT_SYMBOL(wcd_mbhc_init);
>   
>   void wcd_mbhc_deinit(struct wcd_mbhc *mbhc)
>   {
> +	free_irq(mbhc->intr_ids->hph_right_ocp, mbhc);
> +	free_irq(mbhc->intr_ids->hph_left_ocp, mbhc);
> +	free_irq(mbhc->intr_ids->mbhc_hs_rem_intr, mbhc);
> +	free_irq(mbhc->intr_ids->mbhc_hs_ins_intr, mbhc);
> +	free_irq(mbhc->intr_ids->mbhc_btn_release_intr, mbhc);
> +	free_irq(mbhc->intr_ids->mbhc_btn_press_intr, mbhc);
> +	free_irq(mbhc->intr_ids->mbhc_sw_intr, mbhc);
> +
>   	mutex_lock(&mbhc->lock);
>   	wcd_cancel_hs_detect_plug(mbhc,	&mbhc->correct_plug_swch);
>   	mutex_unlock(&mbhc->lock);
> +
> +	kfree(mbhc);
>   }
>   EXPORT_SYMBOL(wcd_mbhc_deinit);
>