I've been hitting a race during boot which breaks probe of the sound
card on the Lenovo ThinkPad X13s as I've previously reported here:
https://lore.kernel.org/all/[email protected]/
The immediate issue appeared to be a probe deferral that was turned into
a hard failure, but addressing that in itself only made things worse as
it exposed further bugs.
I was hoping someone more familiar with the code in question would look
into this, but as this affects users of the X13s and breaks audio on my
machine every fifth boot or so, I decided to investigate it myself.
As expected, the Qualcomm codec drivers are broken and specifically leak
resources on component remove, which in turn breaks sound card probe
deferrals.
The source of the deferral itself appears to be legitimate and was
simply due to some audio component not yet having been registered due to
random changes in timing during boot.
These issues can most easily be reproduced by simply blacklisting the
q6apm_dai module and loading it manually after boot.
The sound card probe deferral also exposes a bug in the soundwire
subsystem, which uses completion structures for signalling that a device
has been enumerated on the bus and initialised. The way this is
implemented prevents reprobed codec drivers from learning that the
soundwire devices are still attached, which causes probe to fail.
Included are also two patches that suppresses error messages on
component probe deferral to avoid spamming the logs during boot.
These patches should preferably all go through the ASoC tree even if
merging the soundwire fix separately also works.
Note the ASoC tree already has the following related fixes:
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
Johan
Johan Hovold (8):
soundwire: fix enumeration completion
ASoC: qdsp6: audioreach: fix topology probe deferral
ASoC: codecs: wcd938x: fix missing clsh ctrl error handling
ASoC: codecs: wcd938x: fix resource leaks on component remove
ASoC: codecs: wcd934x: fix resource leaks on component remove
ASoC: codecs: wcd-mbhc-v2: fix resource leaks on component remove
ASoC: topology: suppress probe deferral errors
ASoC: core: suppress probe deferral errors
drivers/soundwire/bus.c | 8 ++---
sound/soc/codecs/wcd-mbhc-v2.c | 57 ++++++++++++++++++++++---------
sound/soc/codecs/wcd934x.c | 12 +++++++
sound/soc/codecs/wcd938x.c | 59 +++++++++++++++++++++++++++++----
sound/soc/qcom/qdsp6/topology.c | 4 +--
sound/soc/soc-core.c | 6 ++--
sound/soc/soc-topology.c | 10 ++++--
7 files changed, 122 insertions(+), 34 deletions(-)
--
2.39.3
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: [email protected] # 5.14
Cc: Srinivas Kandagatla <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
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
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: [email protected] # 5.14
Cc: Srinivas Kandagatla <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
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
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: [email protected] # 5.14
Cc: Srinivas Kandagatla <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
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
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.
Signed-off-by: Johan Hovold <[email protected]>
---
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;
}
--
2.39.3
Suppress probe deferral error messages when probing link components to
avoid spamming the logs, for example, if a required component has not
yet been registered:
snd-sc8280xp sound: ASoC: failed to instantiate card -517
Note that dev_err_probe() is not used as the card can be unbound and
rebound while the underlying platform device remains bound to its
driver.
Signed-off-by: Johan Hovold <[email protected]>
---
sound/soc/soc-core.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index b48efc3a08d2..b9cb5c4e3685 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1988,8 +1988,10 @@ static int snd_soc_bind_card(struct snd_soc_card *card)
/* probe all components used by DAI links on this card */
ret = soc_probe_link_components(card);
if (ret < 0) {
- dev_err(card->dev,
- "ASoC: failed to instantiate card %d\n", ret);
+ if (ret != -EPROBE_DEFER) {
+ dev_err(card->dev,
+ "ASoC: failed to instantiate card %d\n", ret);
+ }
goto probe_end;
}
--
2.39.3
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: [email protected] # 5.14
Cc: Srinivas Kandagatla <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
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
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: [email protected] # 5.17
Cc: Srinivas Kandagatla <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
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
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: [email protected] # 5.7
Cc: Pierre-Louis Bossart <[email protected]>
Cc: Rander Wang <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
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
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: [email protected] # 5.7
> Cc: Pierre-Louis Bossart <[email protected]>
> Cc: Rander Wang <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> 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 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
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 <[email protected]>
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 <[email protected]>
> ---
> 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;
> }
>
On Wed, Jul 05, 2023 at 05:07:22PM +0200, Amadeusz Sławiński wrote:
> 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");
> ?
In short, it is not correct to use dev_err_probe() here as this is not a
probe function.
dev_err_probe() is tied to driver core and will specifically allocate
and associate an error message with the struct device on probe
deferrals, which is later freed when the struct device is bound to a
driver (or released).
For ASoC drivers, the struct device is typically bound to a driver long
before the components they register are "probed". I use quotation marks
as this is not probing in the driver model sense of the word and the
underlying struct device is already bound to a driver when the component
is "probed".
> Personally I would prefer dev_err_probe() to be used as it also provides
> debug message.
Yeah, but it would be conceptually wrong to do so (besides the fact that
it would waste some memory).
Johan
On 7/6/2023 8:14 AM, Johan Hovold wrote:
> On Wed, Jul 05, 2023 at 05:07:22PM +0200, Amadeusz Sławiński wrote:
>> 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");
>> ?
>
> In short, it is not correct to use dev_err_probe() here as this is not a
> probe function.
>
> dev_err_probe() is tied to driver core and will specifically allocate
> and associate an error message with the struct device on probe
> deferrals, which is later freed when the struct device is bound to a
> driver (or released).
>
I guess you mean call to: device_set_deferred_probe_reason(dev, &vaf);
perhaps functionality could be extended to allow to skip this call and
just do prints? Or just add separate dev_err_defer function without this
step, although it would be best if they could share parts of code.
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: [email protected] # 5.14
> Cc: Srinivas Kandagatla <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Srinivas Kandagatla <[email protected]>
> 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:
> 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: [email protected] # 5.17
> Cc: Srinivas Kandagatla <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Srinivas Kandagatla <[email protected]>
> 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: [email protected] # 5.14
> Cc: Srinivas Kandagatla <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Srinivas Kandagatla <[email protected]>
> 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 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: [email protected] # 5.14
> Cc: Srinivas Kandagatla <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Srinivas Kandagatla <[email protected]>
> 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: [email protected] # 5.14
> Cc: Srinivas Kandagatla <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Srinivas Kandagatla <[email protected]>
--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 Wed, 05 Jul 2023 14:30:10 +0200, Johan Hovold wrote:
> I've been hitting a race during boot which breaks probe of the sound
> card on the Lenovo ThinkPad X13s as I've previously reported here:
>
> https://lore.kernel.org/all/[email protected]/
>
> The immediate issue appeared to be a probe deferral that was turned into
> a hard failure, but addressing that in itself only made things worse as
> it exposed further bugs.
>
> [...]
Applied, thanks!
[1/8] soundwire: fix enumeration completion
commit: 27e0c9f08ac622db7b907c126249dd23367867ab
Best regards,
--
~Vinod
On Thu, Jul 06, 2023 at 09:25:26AM +0200, Amadeusz Sławiński wrote:
> On 7/6/2023 8:14 AM, Johan Hovold wrote:
> > In short, it is not correct to use dev_err_probe() here as this is not a
> > probe function.
> >
> > dev_err_probe() is tied to driver core and will specifically allocate
> > and associate an error message with the struct device on probe
> > deferrals, which is later freed when the struct device is bound to a
> > driver (or released).
> I guess you mean call to: device_set_deferred_probe_reason(dev, &vaf);
> perhaps functionality could be extended to allow to skip this call and
> just do prints? Or just add separate dev_err_defer function without this
> step, although it would be best if they could share parts of code.
Feel free to suggest adding such a function if you think it's
worthwhile. It doesn't exist today it should not be a prerequisite for
suppressing these error messages.
Johan
On Wed, 05 Jul 2023 14:30:10 +0200, Johan Hovold wrote:
> I've been hitting a race during boot which breaks probe of the sound
> card on the Lenovo ThinkPad X13s as I've previously reported here:
>
> https://lore.kernel.org/all/[email protected]/
>
> The immediate issue appeared to be a probe deferral that was turned into
> a hard failure, but addressing that in itself only made things worse as
> it exposed further bugs.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[2/8] ASoC: qdsp6: audioreach: fix topology probe deferral
commit: 46ec420573cefa1fc98025e7e6841bdafd6f1e20
[3/8] ASoC: codecs: wcd938x: fix missing clsh ctrl error handling
commit: ed0dd9205bf69593edb495cb4b086dbae96a3f05
[4/8] ASoC: codecs: wcd938x: fix resource leaks on component remove
commit: a3406f87775fee986876e03f93a84385f54d5999
[5/8] ASoC: codecs: wcd934x: fix resource leaks on component remove
commit: 798590cc7d3c2b5f3a7548d96dd4d8a081c1bc39
[6/8] ASoC: codecs: wcd-mbhc-v2: fix resource leaks on component remove
commit: a5475829adcc600bc69ee9ff7c9e3e43fb4f8d30
[7/8] ASoC: topology: suppress probe deferral errors
commit: b6c3bdda3a7e43acfcec711ce20e7cfe44744740
[8/8] ASoC: core: suppress probe deferral errors
commit: f09b6e96796056633453cb0d07b720d09f1efc68
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark