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
  

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);

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 */

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,

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); }

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);

On 7/5/2023 2:30 PM, Johan Hovold wrote:

Suppress probe deferral error messages when loading topologies and creating frontend links to avoid spamming the logs when a component has not yet been registered:

 snd-sc8280xp sound: ASoC: adding FE link failed
 snd-sc8280xp sound: ASoC: topology: could not load header: -517

Note that dev_err_probe() is not used as the topology component can be probed and removed while the underlying platform device remains bound to its driver.

I'm not sure that I understand that argument... what's wrong with dev_err_probe(tplg->dev, err, "ASoC: adding FE link failed\n"); instead of dev_err(tplg->dev, "ASoC: adding FE link failed\n"); ? Personally I would prefer dev_err_probe() to be used as it also provides debug message.

Signed-off-by: Johan Hovold johan+linaro@kernel.org

sound/soc/soc-topology.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index d0aca6b9058b..696c9647debe 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1751,7 +1751,8 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg,

ret = snd_soc_add_pcm_runtimes(tplg->comp->card, link, 1); if (ret < 0) {

• dev_err(tplg->dev, "ASoC: adding FE link failed\n");
  

• if (ret != -EPROBE_DEFER)
  

• 	dev_err(tplg->dev, "ASoC: adding FE link failed\n");
  

  goto err; }

@@ -2514,8 +2515,11 @@ static int soc_tplg_process_headers(struct soc_tplg *tplg) /* load the header object */ ret = soc_tplg_load_header(tplg, hdr); if (ret < 0) {

• 		dev_err(tplg->dev,
  

• 			"ASoC: topology: could not load header: %d\n", ret);
  

• 		if (ret != -EPROBE_DEFER) {
  

• 			dev_err(tplg->dev,
  

• 				"ASoC: topology: could not load header: %d\n",
  

• 				ret);
  

• 		}
  	return ret;
  }