The wcd938x codec driver happily ignores error handling, something which
has bitten us in the past when we hit a probe deferral:
https://lore.kernel.org/lkml/[email protected]/
Fix up the remaining probe and component bind paths that left resources
allocated and registered after errors to avoid similar future issues.
Johan
Johan Hovold (7):
ASoC: codecs: wcd938x: drop bogus bind error handling
ASoC: codecs: wcd938x: fix unbind tear down order
ASoC: codecs: wcd938x: fix resource leaks on bind errors
ASoC: codecs: wcd938x: fix regulator leaks on probe errors
ASoC: codecs: wcd938x: fix runtime PM imbalance on remove
ASoC: codecs: wcd938x-sdw: fix use after free on driver unbind
ASoC: codecs: wcd938x-sdw: fix runtime PM imbalance on probe errors
sound/soc/codecs/wcd938x-sdw.c | 27 +++++++++++-
sound/soc/codecs/wcd938x.c | 76 +++++++++++++++++++++++++---------
2 files changed, 83 insertions(+), 20 deletions(-)
--
2.41.0
Add the missing code to release resources on bind errors, including the
references taken by wcd938x_sdw_device_get() which also need to be
dropped on unbind().
Fixes: 16572522aece ("ASoC: codecs: wcd938x-sdw: add SoundWire driver")
Cc: [email protected] # 5.14
Cc: Srinivas Kandagatla <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
sound/soc/codecs/wcd938x.c | 44 +++++++++++++++++++++++++++++---------
1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
index c617fc3ce489..7e0b07eeed77 100644
--- a/sound/soc/codecs/wcd938x.c
+++ b/sound/soc/codecs/wcd938x.c
@@ -3435,7 +3435,8 @@ static int wcd938x_bind(struct device *dev)
wcd938x->rxdev = wcd938x_sdw_device_get(wcd938x->rxnode);
if (!wcd938x->rxdev) {
dev_err(dev, "could not find slave with matching of node\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_unbind;
}
wcd938x->sdw_priv[AIF1_PB] = dev_get_drvdata(wcd938x->rxdev);
wcd938x->sdw_priv[AIF1_PB]->wcd938x = wcd938x;
@@ -3443,7 +3444,8 @@ static int wcd938x_bind(struct device *dev)
wcd938x->txdev = wcd938x_sdw_device_get(wcd938x->txnode);
if (!wcd938x->txdev) {
dev_err(dev, "could not find txslave with matching of node\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_put_rxdev;
}
wcd938x->sdw_priv[AIF1_CAP] = dev_get_drvdata(wcd938x->txdev);
wcd938x->sdw_priv[AIF1_CAP]->wcd938x = wcd938x;
@@ -3454,31 +3456,35 @@ static int wcd938x_bind(struct device *dev)
if (!device_link_add(wcd938x->rxdev, wcd938x->txdev, DL_FLAG_STATELESS |
DL_FLAG_PM_RUNTIME)) {
dev_err(dev, "could not devlink tx and rx\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_put_txdev;
}
if (!device_link_add(dev, wcd938x->txdev, DL_FLAG_STATELESS |
DL_FLAG_PM_RUNTIME)) {
dev_err(dev, "could not devlink wcd and tx\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_remove_rxtx_link;
}
if (!device_link_add(dev, wcd938x->rxdev, DL_FLAG_STATELESS |
DL_FLAG_PM_RUNTIME)) {
dev_err(dev, "could not devlink wcd and rx\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_remove_tx_link;
}
wcd938x->regmap = dev_get_regmap(&wcd938x->tx_sdw_dev->dev, NULL);
if (!wcd938x->regmap) {
dev_err(dev, "could not get TX device regmap\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_remove_rx_link;
}
ret = wcd938x_irq_init(wcd938x, dev);
if (ret) {
dev_err(dev, "%s: IRQ init failed: %d\n", __func__, ret);
- return ret;
+ goto err_remove_rx_link;
}
wcd938x->sdw_priv[AIF1_PB]->slave_irq = wcd938x->virq;
@@ -3487,17 +3493,33 @@ static int wcd938x_bind(struct device *dev)
ret = wcd938x_set_micbias_data(wcd938x);
if (ret < 0) {
dev_err(dev, "%s: bad micbias pdata\n", __func__);
- return ret;
+ goto err_remove_rx_link;
}
ret = snd_soc_register_component(dev, &soc_codec_dev_wcd938x,
wcd938x_dais, ARRAY_SIZE(wcd938x_dais));
- if (ret)
+ if (ret) {
dev_err(dev, "%s: Codec registration failed\n",
__func__);
+ goto err_remove_rx_link;
+ }
- return ret;
+ return 0;
+err_remove_rx_link:
+ device_link_remove(dev, wcd938x->rxdev);
+err_remove_tx_link:
+ device_link_remove(dev, wcd938x->txdev);
+err_remove_rxtx_link:
+ device_link_remove(wcd938x->rxdev, wcd938x->txdev);
+err_put_txdev:
+ put_device(wcd938x->txdev);
+err_put_rxdev:
+ put_device(wcd938x->rxdev);
+err_unbind:
+ component_unbind_all(dev, wcd938x);
+
+ return ret;
}
static void wcd938x_unbind(struct device *dev)
@@ -3508,6 +3530,8 @@ static void wcd938x_unbind(struct device *dev)
device_link_remove(dev, wcd938x->txdev);
device_link_remove(dev, wcd938x->rxdev);
device_link_remove(wcd938x->rxdev, wcd938x->txdev);
+ put_device(wcd938x->txdev);
+ put_device(wcd938x->rxdev);
component_unbind_all(dev, wcd938x);
}
--
2.41.0
Drop the bogus error handling for a soundwire device backcast during
bind() that cannot fail.
Fixes: 16572522aece ("ASoC: codecs: wcd938x-sdw: add SoundWire 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 deletions(-)
diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
index a3c680661377..cf1eaf678fc2 100644
--- a/sound/soc/codecs/wcd938x.c
+++ b/sound/soc/codecs/wcd938x.c
@@ -3448,10 +3448,6 @@ static int wcd938x_bind(struct device *dev)
wcd938x->sdw_priv[AIF1_CAP] = dev_get_drvdata(wcd938x->txdev);
wcd938x->sdw_priv[AIF1_CAP]->wcd938x = wcd938x;
wcd938x->tx_sdw_dev = dev_to_sdw_dev(wcd938x->txdev);
- if (!wcd938x->tx_sdw_dev) {
- dev_err(dev, "could not get txslave with matching of dev\n");
- return -EINVAL;
- }
/* As TX is main CSR reg interface, which should not be suspended first.
* expicilty add the dependency link */
--
2.41.0
Make sure to balance the runtime PM operations, including the disable
count, on probe errors and on driver unbind.
Fixes: 16572522aece ("ASoC: codecs: wcd938x-sdw: add SoundWire driver")
Cc: [email protected] # 5.14
Cc: Srinivas Kandagatla <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
sound/soc/codecs/wcd938x-sdw.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/wcd938x-sdw.c b/sound/soc/codecs/wcd938x-sdw.c
index 1baea04480e2..a1f04010da95 100644
--- a/sound/soc/codecs/wcd938x-sdw.c
+++ b/sound/soc/codecs/wcd938x-sdw.c
@@ -1278,7 +1278,18 @@ static int wcd9380_probe(struct sdw_slave *pdev,
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
- return component_add(dev, &wcd938x_sdw_component_ops);
+ ret = component_add(dev, &wcd938x_sdw_component_ops);
+ if (ret)
+ goto err_disable_rpm;
+
+ return 0;
+
+err_disable_rpm:
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+ pm_runtime_dont_use_autosuspend(dev);
+
+ return ret;
}
static int wcd9380_remove(struct sdw_slave *pdev)
@@ -1287,6 +1298,10 @@ static int wcd9380_remove(struct sdw_slave *pdev)
component_del(dev, &wcd938x_sdw_component_ops);
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+ pm_runtime_dont_use_autosuspend(dev);
+
return 0;
}
--
2.41.0
Make sure to balance the runtime PM operations, including the disable
count, on driver unbind.
Fixes: 16572522aece ("ASoC: codecs: wcd938x-sdw: add SoundWire driver")
Cc: [email protected] # 5.14
Cc: Srinivas Kandagatla <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
sound/soc/codecs/wcd938x.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
index 679c627f7eaa..d27b919c63b4 100644
--- a/sound/soc/codecs/wcd938x.c
+++ b/sound/soc/codecs/wcd938x.c
@@ -3620,9 +3620,15 @@ static int wcd938x_probe(struct platform_device *pdev)
static void wcd938x_remove(struct platform_device *pdev)
{
- struct wcd938x_priv *wcd938x = dev_get_drvdata(&pdev->dev);
+ struct device *dev = &pdev->dev;
+ struct wcd938x_priv *wcd938x = dev_get_drvdata(dev);
+
+ component_master_del(dev, &wcd938x_comp_ops);
+
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+ pm_runtime_dont_use_autosuspend(dev);
- component_master_del(&pdev->dev, &wcd938x_comp_ops);
regulator_bulk_disable(WCD938X_MAX_SUPPLY, wcd938x->supplies);
regulator_bulk_free(WCD938X_MAX_SUPPLY, wcd938x->supplies);
}
--
2.41.0
Make sure to deregister the component when the driver is being unbound
and before the underlying device-managed resources are freed.
Fixes: 16572522aece ("ASoC: codecs: wcd938x-sdw: add SoundWire driver")
Cc: [email protected] # 5.14
Cc: Srinivas Kandagatla <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
sound/soc/codecs/wcd938x-sdw.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/sound/soc/codecs/wcd938x-sdw.c b/sound/soc/codecs/wcd938x-sdw.c
index 6951120057e5..1baea04480e2 100644
--- a/sound/soc/codecs/wcd938x-sdw.c
+++ b/sound/soc/codecs/wcd938x-sdw.c
@@ -1281,6 +1281,15 @@ static int wcd9380_probe(struct sdw_slave *pdev,
return component_add(dev, &wcd938x_sdw_component_ops);
}
+static int wcd9380_remove(struct sdw_slave *pdev)
+{
+ struct device *dev = &pdev->dev;
+
+ component_del(dev, &wcd938x_sdw_component_ops);
+
+ return 0;
+}
+
static const struct sdw_device_id wcd9380_slave_id[] = {
SDW_SLAVE_ENTRY(0x0217, 0x10d, 0),
{},
@@ -1320,6 +1329,7 @@ static const struct dev_pm_ops wcd938x_sdw_pm_ops = {
static struct sdw_driver wcd9380_codec_driver = {
.probe = wcd9380_probe,
+ .remove = wcd9380_remove,
.ops = &wcd9380_slave_ops,
.id_table = wcd9380_slave_id,
.driver = {
--
2.41.0
Make sure to disable and free the regulators on probe errors and on
driver unbind.
Fixes: 16572522aece ("ASoC: codecs: wcd938x-sdw: add SoundWire driver")
Cc: [email protected] # 5.14
Cc: Srinivas Kandagatla <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
sound/soc/codecs/wcd938x.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
index 7e0b07eeed77..679c627f7eaa 100644
--- a/sound/soc/codecs/wcd938x.c
+++ b/sound/soc/codecs/wcd938x.c
@@ -3325,8 +3325,10 @@ static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device
return dev_err_probe(dev, ret, "Failed to get supplies\n");
ret = regulator_bulk_enable(WCD938X_MAX_SUPPLY, wcd938x->supplies);
- if (ret)
+ if (ret) {
+ regulator_bulk_free(WCD938X_MAX_SUPPLY, wcd938x->supplies);
return dev_err_probe(dev, ret, "Failed to enable supplies\n");
+ }
wcd938x_dt_parse_micbias_info(dev, wcd938x);
@@ -3592,13 +3594,13 @@ static int wcd938x_probe(struct platform_device *pdev)
ret = wcd938x_add_slave_components(wcd938x, dev, &match);
if (ret)
- return ret;
+ goto err_disable_regulators;
wcd938x_reset(wcd938x);
ret = component_master_add_with_match(dev, &wcd938x_comp_ops, match);
if (ret)
- return ret;
+ goto err_disable_regulators;
pm_runtime_set_autosuspend_delay(dev, 1000);
pm_runtime_use_autosuspend(dev);
@@ -3608,11 +3610,21 @@ static int wcd938x_probe(struct platform_device *pdev)
pm_runtime_idle(dev);
return 0;
+
+err_disable_regulators:
+ regulator_bulk_disable(WCD938X_MAX_SUPPLY, wcd938x->supplies);
+ regulator_bulk_free(WCD938X_MAX_SUPPLY, wcd938x->supplies);
+
+ return ret;
}
static void wcd938x_remove(struct platform_device *pdev)
{
+ struct wcd938x_priv *wcd938x = dev_get_drvdata(&pdev->dev);
+
component_master_del(&pdev->dev, &wcd938x_comp_ops);
+ regulator_bulk_disable(WCD938X_MAX_SUPPLY, wcd938x->supplies);
+ regulator_bulk_free(WCD938X_MAX_SUPPLY, wcd938x->supplies);
}
#if defined(CONFIG_OF)
--
2.41.0
On Tue, 03 Oct 2023 17:55:51 +0200, Johan Hovold wrote:
> The wcd938x codec driver happily ignores error handling, something which
> has bitten us in the past when we hit a probe deferral:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> Fix up the remaining probe and component bind paths that left resources
> allocated and registered after errors to avoid similar future issues.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/7] ASoC: codecs: wcd938x: drop bogus bind error handling
commit: bfbc79de60c53e5fed505390440b87ef59ee268c
[2/7] ASoC: codecs: wcd938x: fix unbind tear down order
commit: fa2f8a991ba4aa733ac1c3b1be0c86148aa4c52c
[3/7] ASoC: codecs: wcd938x: fix resource leaks on bind errors
commit: da29b94ed3547cee9d510d02eca4009f2de476cf
[4/7] ASoC: codecs: wcd938x: fix regulator leaks on probe errors
commit: 69a026a2357ee69983690d07976de44ef26ee38a
[5/7] ASoC: codecs: wcd938x: fix runtime PM imbalance on remove
commit: 3ebebb2c1eca92a15107b2d7aeff34196fd9e217
[6/7] ASoC: codecs: wcd938x-sdw: fix use after free on driver unbind
commit: f0dfdcbe706462495d47982eecd13a61aabd644d
[7/7] ASoC: codecs: wcd938x-sdw: fix runtime PM imbalance on probe errors
commit: c5c0383082eace13da2ffceeea154db2780165e7
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