All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ASoC: core: Remove only the registered component in devm functions
@ 2020-07-07  7:42 ` Maxime Ripard
  0 siblings, 0 replies; 8+ messages in thread
From: Maxime Ripard @ 2020-07-07  7:42 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
	Lars-Peter Clausen
  Cc: alsa-devel, linux-kernel, Dave Stevenson, Tim Gover, Phil Elwell,
	Maxime Ripard

The ASoC devm_ functions that register a component
(devm_snd_soc_register_component and devm_snd_dmaengine_pcm_register) will
clean their component by running snd_soc_unregister_component.

snd_soc_unregister_component will then remove all the components for the
device that was used to register the component in the first place.

However, some drivers register several components (such as a DAI and a
dmaengine PCM) on the same device, and if the dmaengine PCM is registered
first, then the DAI will be cleaned up first and
snd_dmaengine_pcm_unregister will be called next.

snd_dmaengine_pcm_unregister will then lookup the dmaengine PCM component
on the device, and if there's one unregister that component and release its
dmaengine channels. That doesn't happen in practice though since the first
call to snd_soc_unregister_component removed all the components, so we
never get the chance to release the dmaengine channels.

In order to fix this, instead of removing all the components for a given
device, we can simply remove the component that was registered in the first
place. We should have the same number of component registration than we
have components, so it should work just fine.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---

This was observed on a RaspberryPi that uses the vc4_hdmi driver
(drivers/gpu/drm/vc4/vc4_hdmi.c). This driver will register a dmaengine PCM
and two components. If the MIPI-DSI controller is enabled, it will create
an EPROBE_DEFER across the entire display pipeline, leading to multiple
bind/unbind cycles in the other display drivers including vc4_hdmi, leading
to multiple warnings since we request the same dmaengine channels on the
same device without ever releasing them.

It's not really clear to me when that bug was introduced exactly, since it
can only be seen on a rather unusual setup, and with all the drivers
built-in (otherwise we probably wouldn't get an EPROBE_DEFER for DSI), but
it still looks like something that should probably go to stable?

Changes from v1:
  - Rebased on top of next-20200706
---
 include/sound/soc.h                   |  2 ++
 sound/soc/soc-core.c                  | 27 +++++++++++++++++++++++++++
 sound/soc/soc-devres.c                |  8 +++++---
 sound/soc/soc-generic-dmaengine-pcm.c |  2 +-
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 6791b7570a67..59235e553630 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -426,6 +426,8 @@ int devm_snd_soc_register_component(struct device *dev,
 			 const struct snd_soc_component_driver *component_driver,
 			 struct snd_soc_dai_driver *dai_drv, int num_dai);
 void snd_soc_unregister_component(struct device *dev);
+void snd_soc_unregister_component_by_driver(struct device *dev,
+			 const struct snd_soc_component_driver *component_driver);
 struct snd_soc_component *snd_soc_lookup_component_nolocked(struct device *dev,
 							    const char *driver_name);
 struct snd_soc_component *snd_soc_lookup_component(struct device *dev,
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index adedadcb0efb..7c58e45c1c3f 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2513,6 +2513,33 @@ int snd_soc_register_component(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(snd_soc_register_component);
 
+/**
+ * snd_soc_unregister_component_by_driver - Unregister component using a given driver
+ * from the ASoC core
+ *
+ * @dev: The device to unregister
+ * @component_driver: The component driver to unregister
+ */
+void snd_soc_unregister_component_by_driver(struct device *dev,
+					    const struct snd_soc_component_driver *component_driver)
+{
+	struct snd_soc_component *component;
+
+	if (!component_driver)
+		return;
+
+	mutex_lock(&client_mutex);
+	component = snd_soc_lookup_component_nolocked(dev, component_driver->name);
+	if (!component)
+		goto out;
+
+	snd_soc_del_component_unlocked(component);
+
+out:
+	mutex_unlock(&client_mutex);
+}
+EXPORT_SYMBOL_GPL(snd_soc_unregister_component_by_driver);
+
 /**
  * snd_soc_unregister_component - Unregister all related component
  * from the ASoC core
diff --git a/sound/soc/soc-devres.c b/sound/soc/soc-devres.c
index 11e5d7962370..4534a1c03e8e 100644
--- a/sound/soc/soc-devres.c
+++ b/sound/soc/soc-devres.c
@@ -48,7 +48,9 @@ EXPORT_SYMBOL_GPL(devm_snd_soc_register_dai);
 
 static void devm_component_release(struct device *dev, void *res)
 {
-	snd_soc_unregister_component(*(struct device **)res);
+	const struct snd_soc_component_driver **cmpnt_drv = res;
+
+	snd_soc_unregister_component_by_driver(dev, *cmpnt_drv);
 }
 
 /**
@@ -65,7 +67,7 @@ int devm_snd_soc_register_component(struct device *dev,
 			 const struct snd_soc_component_driver *cmpnt_drv,
 			 struct snd_soc_dai_driver *dai_drv, int num_dai)
 {
-	struct device **ptr;
+	const struct snd_soc_component_driver **ptr;
 	int ret;
 
 	ptr = devres_alloc(devm_component_release, sizeof(*ptr), GFP_KERNEL);
@@ -74,7 +76,7 @@ int devm_snd_soc_register_component(struct device *dev,
 
 	ret = snd_soc_register_component(dev, cmpnt_drv, dai_drv, num_dai);
 	if (ret == 0) {
-		*ptr = dev;
+		*ptr = cmpnt_drv;
 		devres_add(dev, ptr);
 	} else {
 		devres_free(ptr);
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index 80a4e71f2d95..61844403f181 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -478,7 +478,7 @@ void snd_dmaengine_pcm_unregister(struct device *dev)
 
 	pcm = soc_component_to_pcm(component);
 
-	snd_soc_unregister_component(dev);
+	snd_soc_unregister_component_by_driver(dev, component->driver);
 	dmaengine_pcm_release_chan(pcm);
 	kfree(pcm);
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH] ASoC: core: Remove only the registered component in devm functions
@ 2020-07-07  7:42 ` Maxime Ripard
  0 siblings, 0 replies; 8+ messages in thread
From: Maxime Ripard @ 2020-07-07  7:42 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
	Lars-Peter Clausen
  Cc: alsa-devel, Tim Gover, Dave Stevenson, linux-kernel,
	Maxime Ripard, Phil Elwell

The ASoC devm_ functions that register a component
(devm_snd_soc_register_component and devm_snd_dmaengine_pcm_register) will
clean their component by running snd_soc_unregister_component.

snd_soc_unregister_component will then remove all the components for the
device that was used to register the component in the first place.

However, some drivers register several components (such as a DAI and a
dmaengine PCM) on the same device, and if the dmaengine PCM is registered
first, then the DAI will be cleaned up first and
snd_dmaengine_pcm_unregister will be called next.

snd_dmaengine_pcm_unregister will then lookup the dmaengine PCM component
on the device, and if there's one unregister that component and release its
dmaengine channels. That doesn't happen in practice though since the first
call to snd_soc_unregister_component removed all the components, so we
never get the chance to release the dmaengine channels.

In order to fix this, instead of removing all the components for a given
device, we can simply remove the component that was registered in the first
place. We should have the same number of component registration than we
have components, so it should work just fine.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---

This was observed on a RaspberryPi that uses the vc4_hdmi driver
(drivers/gpu/drm/vc4/vc4_hdmi.c). This driver will register a dmaengine PCM
and two components. If the MIPI-DSI controller is enabled, it will create
an EPROBE_DEFER across the entire display pipeline, leading to multiple
bind/unbind cycles in the other display drivers including vc4_hdmi, leading
to multiple warnings since we request the same dmaengine channels on the
same device without ever releasing them.

It's not really clear to me when that bug was introduced exactly, since it
can only be seen on a rather unusual setup, and with all the drivers
built-in (otherwise we probably wouldn't get an EPROBE_DEFER for DSI), but
it still looks like something that should probably go to stable?

Changes from v1:
  - Rebased on top of next-20200706
---
 include/sound/soc.h                   |  2 ++
 sound/soc/soc-core.c                  | 27 +++++++++++++++++++++++++++
 sound/soc/soc-devres.c                |  8 +++++---
 sound/soc/soc-generic-dmaengine-pcm.c |  2 +-
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 6791b7570a67..59235e553630 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -426,6 +426,8 @@ int devm_snd_soc_register_component(struct device *dev,
 			 const struct snd_soc_component_driver *component_driver,
 			 struct snd_soc_dai_driver *dai_drv, int num_dai);
 void snd_soc_unregister_component(struct device *dev);
+void snd_soc_unregister_component_by_driver(struct device *dev,
+			 const struct snd_soc_component_driver *component_driver);
 struct snd_soc_component *snd_soc_lookup_component_nolocked(struct device *dev,
 							    const char *driver_name);
 struct snd_soc_component *snd_soc_lookup_component(struct device *dev,
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index adedadcb0efb..7c58e45c1c3f 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2513,6 +2513,33 @@ int snd_soc_register_component(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(snd_soc_register_component);
 
+/**
+ * snd_soc_unregister_component_by_driver - Unregister component using a given driver
+ * from the ASoC core
+ *
+ * @dev: The device to unregister
+ * @component_driver: The component driver to unregister
+ */
+void snd_soc_unregister_component_by_driver(struct device *dev,
+					    const struct snd_soc_component_driver *component_driver)
+{
+	struct snd_soc_component *component;
+
+	if (!component_driver)
+		return;
+
+	mutex_lock(&client_mutex);
+	component = snd_soc_lookup_component_nolocked(dev, component_driver->name);
+	if (!component)
+		goto out;
+
+	snd_soc_del_component_unlocked(component);
+
+out:
+	mutex_unlock(&client_mutex);
+}
+EXPORT_SYMBOL_GPL(snd_soc_unregister_component_by_driver);
+
 /**
  * snd_soc_unregister_component - Unregister all related component
  * from the ASoC core
diff --git a/sound/soc/soc-devres.c b/sound/soc/soc-devres.c
index 11e5d7962370..4534a1c03e8e 100644
--- a/sound/soc/soc-devres.c
+++ b/sound/soc/soc-devres.c
@@ -48,7 +48,9 @@ EXPORT_SYMBOL_GPL(devm_snd_soc_register_dai);
 
 static void devm_component_release(struct device *dev, void *res)
 {
-	snd_soc_unregister_component(*(struct device **)res);
+	const struct snd_soc_component_driver **cmpnt_drv = res;
+
+	snd_soc_unregister_component_by_driver(dev, *cmpnt_drv);
 }
 
 /**
@@ -65,7 +67,7 @@ int devm_snd_soc_register_component(struct device *dev,
 			 const struct snd_soc_component_driver *cmpnt_drv,
 			 struct snd_soc_dai_driver *dai_drv, int num_dai)
 {
-	struct device **ptr;
+	const struct snd_soc_component_driver **ptr;
 	int ret;
 
 	ptr = devres_alloc(devm_component_release, sizeof(*ptr), GFP_KERNEL);
@@ -74,7 +76,7 @@ int devm_snd_soc_register_component(struct device *dev,
 
 	ret = snd_soc_register_component(dev, cmpnt_drv, dai_drv, num_dai);
 	if (ret == 0) {
-		*ptr = dev;
+		*ptr = cmpnt_drv;
 		devres_add(dev, ptr);
 	} else {
 		devres_free(ptr);
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index 80a4e71f2d95..61844403f181 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -478,7 +478,7 @@ void snd_dmaengine_pcm_unregister(struct device *dev)
 
 	pcm = soc_component_to_pcm(component);
 
-	snd_soc_unregister_component(dev);
+	snd_soc_unregister_component_by_driver(dev, component->driver);
 	dmaengine_pcm_release_chan(pcm);
 	kfree(pcm);
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: core: Remove only the registered component in devm functions
  2020-07-07  7:42 ` Maxime Ripard
@ 2020-07-07 14:17   ` Mark Brown
  -1 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2020-07-07 14:17 UTC (permalink / raw)
  To: Liam Girdwood, Lars-Peter Clausen, Maxime Ripard,
	Jaroslav Kysela, Takashi Iwai
  Cc: Phil Elwell, Dave Stevenson, linux-kernel, Tim Gover, alsa-devel

On Tue, 7 Jul 2020 09:42:37 +0200, Maxime Ripard wrote:
> The ASoC devm_ functions that register a component
> (devm_snd_soc_register_component and devm_snd_dmaengine_pcm_register) will
> clean their component by running snd_soc_unregister_component.
> 
> snd_soc_unregister_component will then remove all the components for the
> device that was used to register the component in the first place.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: core: Remove only the registered component in devm functions
      commit: 58f30150ffd6d95efa524ff05bbcee4e95bfa870

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: core: Remove only the registered component in devm functions
@ 2020-07-07 14:17   ` Mark Brown
  0 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2020-07-07 14:17 UTC (permalink / raw)
  To: Liam Girdwood, Lars-Peter Clausen, Maxime Ripard,
	Jaroslav Kysela, Takashi Iwai
  Cc: linux-kernel, alsa-devel, Phil Elwell, Tim Gover, Dave Stevenson

On Tue, 7 Jul 2020 09:42:37 +0200, Maxime Ripard wrote:
> The ASoC devm_ functions that register a component
> (devm_snd_soc_register_component and devm_snd_dmaengine_pcm_register) will
> clean their component by running snd_soc_unregister_component.
> 
> snd_soc_unregister_component will then remove all the components for the
> device that was used to register the component in the first place.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: core: Remove only the registered component in devm functions
      commit: 58f30150ffd6d95efa524ff05bbcee4e95bfa870

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: core: Remove only the registered component in devm functions
  2020-07-03  7:49 ` Maxime Ripard
@ 2020-07-03 15:32   ` Mark Brown
  -1 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2020-07-03 15:32 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Lars-Peter Clausen,
	alsa-devel, Tim Gover, Dave Stevenson, linux-kernel, Phil Elwell

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

On Fri, Jul 03, 2020 at 09:49:35AM +0200, Maxime Ripard wrote:
> The ASoC devm_ functions that register a component
> (devm_snd_soc_register_component and devm_snd_dmaengine_pcm_register) will
> clean their component by running snd_soc_unregister_component.

This doesn't apply against current code, please check and resend.  Looks
like a genuine issue and sensible fix :/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: core: Remove only the registered component in devm functions
@ 2020-07-03 15:32   ` Mark Brown
  0 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2020-07-03 15:32 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: alsa-devel, Lars-Peter Clausen, Tim Gover, Dave Stevenson,
	linux-kernel, Takashi Iwai, Liam Girdwood, Phil Elwell

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

On Fri, Jul 03, 2020 at 09:49:35AM +0200, Maxime Ripard wrote:
> The ASoC devm_ functions that register a component
> (devm_snd_soc_register_component and devm_snd_dmaengine_pcm_register) will
> clean their component by running snd_soc_unregister_component.

This doesn't apply against current code, please check and resend.  Looks
like a genuine issue and sensible fix :/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] ASoC: core: Remove only the registered component in devm functions
@ 2020-07-03  7:49 ` Maxime Ripard
  0 siblings, 0 replies; 8+ messages in thread
From: Maxime Ripard @ 2020-07-03  7:49 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
	Lars-Peter Clausen
  Cc: alsa-devel, linux-kernel, Dave Stevenson, Tim Gover, Phil Elwell,
	Maxime Ripard

The ASoC devm_ functions that register a component
(devm_snd_soc_register_component and devm_snd_dmaengine_pcm_register) will
clean their component by running snd_soc_unregister_component.

snd_soc_unregister_component will then remove all the components for the
device that was used to register the component in the first place.

However, some drivers register several components (such as a DAI and a
dmaengine PCM) on the same device, and if the dmaengine PCM is registered
first, then the DAI will be cleaned up first and
snd_dmaengine_pcm_unregister will be called next.

snd_dmaengine_pcm_unregister will then lookup the dmaengine PCM component
on the device, and if there's one unregister that component and release its
dmaengine channels. That doesn't happen in practice though since the first
call to snd_soc_unregister_component removed all the components, so we
never get the chance to release the dmaengine channels.

In order to fix this, instead of removing all the components for a given
device, we can simply remove the component that was registered in the first
place. We should have the same number of component registration than we
have components, so it should work just fine.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---

This was observed on a RaspberryPi that uses the vc4_hdmi driver
(drivers/gpu/drm/vc4/vc4_hdmi.c). This driver will register a dmaengine PCM
and two components. If the MIPI-DSI controller is enabled, it will create
an EPROBE_DEFER across the entire display pipeline, leading to multiple
bind/unbind cycles in the other display drivers including vc4_hdmi, leading
to multiple warnings since we request the same dmaengine channels on the
same device without ever releasing them.

It's not really clear to me when that bug was introduced exactly, since it
can only be seen on a rather unusual setup, and with all the drivers
built-in (otherwise we probably wouldn't get an EPROBE_DEFER for DSI), but
it still looks like something that should probably go to stable?
---
 include/sound/soc.h                   |  2 ++
 sound/soc/soc-core.c                  | 27 +++++++++++++++++++++++++++
 sound/soc/soc-devres.c                |  8 +++++---
 sound/soc/soc-generic-dmaengine-pcm.c |  2 +-
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index ef5dd28e10a9..0b3b31803678 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -444,6 +444,8 @@ int devm_snd_soc_register_component(struct device *dev,
 			 const struct snd_soc_component_driver *component_driver,
 			 struct snd_soc_dai_driver *dai_drv, int num_dai);
 void snd_soc_unregister_component(struct device *dev);
+void snd_soc_unregister_component_by_driver(struct device *dev,
+			 const struct snd_soc_component_driver *component_driver);
 struct snd_soc_component *snd_soc_lookup_component(struct device *dev,
 						   const char *driver_name);
 
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 7b387202c5db..9d24cbd9111f 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2571,6 +2571,33 @@ int snd_soc_register_component(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(snd_soc_register_component);
 
+/**
+ * snd_soc_unregister_component_by_driver - Unregister component using a given driver
+ * from the ASoC core
+ *
+ * @dev: The device to unregister
+ * @component_driver: The component driver to unregister
+ */
+void snd_soc_unregister_component_by_driver(struct device *dev,
+					    const struct snd_soc_component_driver *component_driver)
+{
+	struct snd_soc_component *component;
+
+	if (!component_driver)
+		return;
+
+	mutex_lock(&client_mutex);
+	component = snd_soc_lookup_component_nolocked(dev, component_driver->name);
+	if (!component)
+		goto out;
+
+	snd_soc_del_component_unlocked(component);
+
+out:
+	mutex_unlock(&client_mutex);
+}
+EXPORT_SYMBOL_GPL(snd_soc_unregister_component_by_driver);
+
 /**
  * snd_soc_unregister_component - Unregister all related component
  * from the ASoC core
diff --git a/sound/soc/soc-devres.c b/sound/soc/soc-devres.c
index a9ea172a66a7..c6364caabc0e 100644
--- a/sound/soc/soc-devres.c
+++ b/sound/soc/soc-devres.c
@@ -11,7 +11,9 @@
 
 static void devm_component_release(struct device *dev, void *res)
 {
-	snd_soc_unregister_component(*(struct device **)res);
+	const struct snd_soc_component_driver **cmpnt_drv = res;
+
+	snd_soc_unregister_component_by_driver(dev, *cmpnt_drv);
 }
 
 /**
@@ -28,7 +30,7 @@ int devm_snd_soc_register_component(struct device *dev,
 			 const struct snd_soc_component_driver *cmpnt_drv,
 			 struct snd_soc_dai_driver *dai_drv, int num_dai)
 {
-	struct device **ptr;
+	const struct snd_soc_component_driver **ptr;
 	int ret;
 
 	ptr = devres_alloc(devm_component_release, sizeof(*ptr), GFP_KERNEL);
@@ -37,7 +39,7 @@ int devm_snd_soc_register_component(struct device *dev,
 
 	ret = snd_soc_register_component(dev, cmpnt_drv, dai_drv, num_dai);
 	if (ret == 0) {
-		*ptr = dev;
+		*ptr = cmpnt_drv;
 		devres_add(dev, ptr);
 	} else {
 		devres_free(ptr);
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index f728309a0833..ab600be9e69f 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -490,7 +490,7 @@ void snd_dmaengine_pcm_unregister(struct device *dev)
 
 	pcm = soc_component_to_pcm(component);
 
-	snd_soc_unregister_component(dev);
+	snd_soc_unregister_component_by_driver(dev, component->driver);
 	dmaengine_pcm_release_chan(pcm);
 	kfree(pcm);
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH] ASoC: core: Remove only the registered component in devm functions
@ 2020-07-03  7:49 ` Maxime Ripard
  0 siblings, 0 replies; 8+ messages in thread
From: Maxime Ripard @ 2020-07-03  7:49 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
	Lars-Peter Clausen
  Cc: alsa-devel, Tim Gover, Dave Stevenson, linux-kernel,
	Maxime Ripard, Phil Elwell

The ASoC devm_ functions that register a component
(devm_snd_soc_register_component and devm_snd_dmaengine_pcm_register) will
clean their component by running snd_soc_unregister_component.

snd_soc_unregister_component will then remove all the components for the
device that was used to register the component in the first place.

However, some drivers register several components (such as a DAI and a
dmaengine PCM) on the same device, and if the dmaengine PCM is registered
first, then the DAI will be cleaned up first and
snd_dmaengine_pcm_unregister will be called next.

snd_dmaengine_pcm_unregister will then lookup the dmaengine PCM component
on the device, and if there's one unregister that component and release its
dmaengine channels. That doesn't happen in practice though since the first
call to snd_soc_unregister_component removed all the components, so we
never get the chance to release the dmaengine channels.

In order to fix this, instead of removing all the components for a given
device, we can simply remove the component that was registered in the first
place. We should have the same number of component registration than we
have components, so it should work just fine.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---

This was observed on a RaspberryPi that uses the vc4_hdmi driver
(drivers/gpu/drm/vc4/vc4_hdmi.c). This driver will register a dmaengine PCM
and two components. If the MIPI-DSI controller is enabled, it will create
an EPROBE_DEFER across the entire display pipeline, leading to multiple
bind/unbind cycles in the other display drivers including vc4_hdmi, leading
to multiple warnings since we request the same dmaengine channels on the
same device without ever releasing them.

It's not really clear to me when that bug was introduced exactly, since it
can only be seen on a rather unusual setup, and with all the drivers
built-in (otherwise we probably wouldn't get an EPROBE_DEFER for DSI), but
it still looks like something that should probably go to stable?
---
 include/sound/soc.h                   |  2 ++
 sound/soc/soc-core.c                  | 27 +++++++++++++++++++++++++++
 sound/soc/soc-devres.c                |  8 +++++---
 sound/soc/soc-generic-dmaengine-pcm.c |  2 +-
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index ef5dd28e10a9..0b3b31803678 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -444,6 +444,8 @@ int devm_snd_soc_register_component(struct device *dev,
 			 const struct snd_soc_component_driver *component_driver,
 			 struct snd_soc_dai_driver *dai_drv, int num_dai);
 void snd_soc_unregister_component(struct device *dev);
+void snd_soc_unregister_component_by_driver(struct device *dev,
+			 const struct snd_soc_component_driver *component_driver);
 struct snd_soc_component *snd_soc_lookup_component(struct device *dev,
 						   const char *driver_name);
 
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 7b387202c5db..9d24cbd9111f 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2571,6 +2571,33 @@ int snd_soc_register_component(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(snd_soc_register_component);
 
+/**
+ * snd_soc_unregister_component_by_driver - Unregister component using a given driver
+ * from the ASoC core
+ *
+ * @dev: The device to unregister
+ * @component_driver: The component driver to unregister
+ */
+void snd_soc_unregister_component_by_driver(struct device *dev,
+					    const struct snd_soc_component_driver *component_driver)
+{
+	struct snd_soc_component *component;
+
+	if (!component_driver)
+		return;
+
+	mutex_lock(&client_mutex);
+	component = snd_soc_lookup_component_nolocked(dev, component_driver->name);
+	if (!component)
+		goto out;
+
+	snd_soc_del_component_unlocked(component);
+
+out:
+	mutex_unlock(&client_mutex);
+}
+EXPORT_SYMBOL_GPL(snd_soc_unregister_component_by_driver);
+
 /**
  * snd_soc_unregister_component - Unregister all related component
  * from the ASoC core
diff --git a/sound/soc/soc-devres.c b/sound/soc/soc-devres.c
index a9ea172a66a7..c6364caabc0e 100644
--- a/sound/soc/soc-devres.c
+++ b/sound/soc/soc-devres.c
@@ -11,7 +11,9 @@
 
 static void devm_component_release(struct device *dev, void *res)
 {
-	snd_soc_unregister_component(*(struct device **)res);
+	const struct snd_soc_component_driver **cmpnt_drv = res;
+
+	snd_soc_unregister_component_by_driver(dev, *cmpnt_drv);
 }
 
 /**
@@ -28,7 +30,7 @@ int devm_snd_soc_register_component(struct device *dev,
 			 const struct snd_soc_component_driver *cmpnt_drv,
 			 struct snd_soc_dai_driver *dai_drv, int num_dai)
 {
-	struct device **ptr;
+	const struct snd_soc_component_driver **ptr;
 	int ret;
 
 	ptr = devres_alloc(devm_component_release, sizeof(*ptr), GFP_KERNEL);
@@ -37,7 +39,7 @@ int devm_snd_soc_register_component(struct device *dev,
 
 	ret = snd_soc_register_component(dev, cmpnt_drv, dai_drv, num_dai);
 	if (ret == 0) {
-		*ptr = dev;
+		*ptr = cmpnt_drv;
 		devres_add(dev, ptr);
 	} else {
 		devres_free(ptr);
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index f728309a0833..ab600be9e69f 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -490,7 +490,7 @@ void snd_dmaengine_pcm_unregister(struct device *dev)
 
 	pcm = soc_component_to_pcm(component);
 
-	snd_soc_unregister_component(dev);
+	snd_soc_unregister_component_by_driver(dev, component->driver);
 	dmaengine_pcm_release_chan(pcm);
 	kfree(pcm);
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-07-07 14:18 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-07  7:42 [PATCH] ASoC: core: Remove only the registered component in devm functions Maxime Ripard
2020-07-07  7:42 ` Maxime Ripard
2020-07-07 14:17 ` Mark Brown
2020-07-07 14:17   ` Mark Brown
  -- strict thread matches above, loose matches on Subject: below --
2020-07-03  7:49 Maxime Ripard
2020-07-03  7:49 ` Maxime Ripard
2020-07-03 15:32 ` Mark Brown
2020-07-03 15:32   ` Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.