[RFC] ASoC: soc-component: count snd_soc_component_open/close()
diff mbox series

Message ID 878skyt6om.wl-kuninori.morimoto.gx@renesas.com
State New
Headers show
Series
  • [RFC] ASoC: soc-component: count snd_soc_component_open/close()
Related show

Commit Message

Kuninori Morimoto Feb. 20, 2020, 12:59 a.m. UTC
ASoC component open/close and snd_soc_component_module_get/put are
called once for each component, but we need it for each substream.
To solve this issue, this patch counts open / get,
and call close / put accordingly.

Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once")
Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
I tidyuped code.
I hope it can solve the issue.

 include/sound/soc-component.h |  5 +++--
 sound/soc/soc-component.c     | 35 ++++++++++++++++++++++-------------
 2 files changed, 25 insertions(+), 15 deletions(-)

Comments

Sridharan, Ranjani Feb. 20, 2020, 1:25 a.m. UTC | #1
On Wed, Feb 19, 2020 at 5:01 PM Kuninori Morimoto <
kuninori.morimoto.gx@renesas.com> wrote:

> ASoC component open/close and snd_soc_component_module_get/put are
> called once for each component, but we need it for each substream.
> To solve this issue, this patch counts open / get,
> and call close / put accordingly.
>
> Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close()
> once")
> Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> I tidyuped code.
> I hope it can solve the issue.
>
>  include/sound/soc-component.h |  5 +++--
>  sound/soc/soc-component.c     | 35 ++++++++++++++++++++++-------------
>  2 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
> index 1866ecc8e94b..4e78925858c0 100644
> --- a/include/sound/soc-component.h
> +++ b/include/sound/soc-component.h
> @@ -181,10 +181,11 @@ struct snd_soc_component {
>         const char *debugfs_prefix;
>  #endif
>
> +       u8 opened;
> +       u8 module;
> +
>         /* bit field */
>         unsigned int suspended:1; /* is in suspend PM state */
> -       unsigned int opened:1;
> -       unsigned int module:1;
>  };
>
>  #define for_each_component_dais(component, dai)\
> diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
> index ee00c09df5e7..bdd36be1fb70 100644
> --- a/sound/soc/soc-component.c
> +++ b/sound/soc/soc-component.c
> @@ -297,14 +297,16 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack);
>  int snd_soc_component_module_get(struct snd_soc_component *component,
>                                  int upon_open)
>  {
> -       if (component->module)
> -               return 0;
> +       if (unlikely(component->module == 0xff)) {
> +               dev_warn(component->dev, "too many module get (%s)\n",
> component->name);
> +               return -EBUSY;
> +       }
>
>         if (component->driver->module_get_upon_open == !!upon_open &&
>             !try_module_get(component->dev->driver->owner))
>                 return -ENODEV;
>
> -       component->module = 1;
> +       component->module++;
>
Thanks, Morimoto-san for the alternate fix. I understand the rationale for
having a count for component->opened, but what is the rationale for the
module count? What it is intended to protect?

Thanks,
Ranjani
Kuninori Morimoto Feb. 20, 2020, 1:41 a.m. UTC | #2
Hi Sridharan

>     ASoC component open/close and snd_soc_component_module_get/put are
>     called once for each component, but we need it for each substream.
>     To solve this issue, this patch counts open / get,
>     and call close / put accordingly.
>
>     Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once")
>     Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
>     Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>     ---
(snip)
>     @@ -297,14 +297,16 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack);
>      int snd_soc_component_module_get(struct snd_soc_component *component,
>                                      int upon_open)
>      {
>     -       if (component->module)
>     -               return 0;
>     +       if (unlikely(component->module == 0xff)) {
>     +               dev_warn(component->dev, "too many module get (%s)\n", component->name);
>     +               return -EBUSY;
>     +       }
>
>             if (component->driver->module_get_upon_open == !!upon_open &&
>                 !try_module_get(component->dev->driver->owner))
>                     return -ENODEV;
>
>     -       component->module = 1;
>     +       component->module++;
>
> Thanks, Morimoto-san for the alternate fix. I understand the rationale for having a count for component->opened, but what is the rationale for the module count? What it is
> intended to protect?

I think same as open ?
It protects calling put() from not-get-component.
Because module_put() has WARN_ON(ret < 0).

Thank you for your help !!
Best regards
---
Kuninori Morimoto
Sridharan, Ranjani Feb. 20, 2020, 1:57 a.m. UTC | #3
On Wed, Feb 19, 2020 at 5:42 PM Kuninori Morimoto <
kuninori.morimoto.gx@renesas.com> wrote:

>
> Hi Sridharan
>
> >     ASoC component open/close and snd_soc_component_module_get/put are
> >     called once for each component, but we need it for each substream.
> >     To solve this issue, this patch counts open / get,
> >     and call close / put accordingly.
> >
> >     Fixes: dd03907bf129 ("ASoC: soc-pcm: call
> snd_soc_component_open/close() once")
> >     Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> >     Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >     ---
> (snip)
> >     @@ -297,14 +297,16 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack);
> >      int snd_soc_component_module_get(struct snd_soc_component
> *component,
> >                                      int upon_open)
> >      {
> >     -       if (component->module)
> >     -               return 0;
> >     +       if (unlikely(component->module == 0xff)) {
> >     +               dev_warn(component->dev, "too many module get
> (%s)\n", component->name);
> >     +               return -EBUSY;
> >     +       }
> >
> >             if (component->driver->module_get_upon_open == !!upon_open &&
> >                 !try_module_get(component->dev->driver->owner))
> >                     return -ENODEV;
> >
> >     -       component->module = 1;
> >     +       component->module++;
> >
> > Thanks, Morimoto-san for the alternate fix. I understand the rationale
> for having a count for component->opened, but what is the rationale for the
> module count? What it is
> > intended to protect?
>
> I think same as open ?
> It protects calling put() from not-get-component.
> Because module_put() has WARN_ON(ret < 0).
>
Can we use the module_refcount instead of adding a new field?

Thanks,
Ranjani
Kuninori Morimoto Feb. 20, 2020, 3:01 a.m. UTC | #4
Hi Sridharan

>     >     ASoC component open/close and snd_soc_component_module_get/put are
>     >     called once for each component, but we need it for each substream.
>     >     To solve this issue, this patch counts open / get,
>     >     and call close / put accordingly.
>     >
>     >     Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once")
>     >     Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
>     >     Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>     >     ---
(snip)
>     I think same as open ?
>     It protects calling put() from not-get-component.
>     Because module_put() has WARN_ON(ret < 0).
> 
> Can we use the module_refcount instead of adding a new field?

Ahh, maybe yes.
I can try to use it in non-RFC patch.

Thank you for your help !!
Best regards
---
Kuninori Morimoto

Patch
diff mbox series

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 1866ecc8e94b..4e78925858c0 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -181,10 +181,11 @@  struct snd_soc_component {
 	const char *debugfs_prefix;
 #endif
 
+	u8 opened;
+	u8 module;
+
 	/* bit field */
 	unsigned int suspended:1; /* is in suspend PM state */
-	unsigned int opened:1;
-	unsigned int module:1;
 };
 
 #define for_each_component_dais(component, dai)\
diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
index ee00c09df5e7..bdd36be1fb70 100644
--- a/sound/soc/soc-component.c
+++ b/sound/soc/soc-component.c
@@ -297,14 +297,16 @@  EXPORT_SYMBOL_GPL(snd_soc_component_set_jack);
 int snd_soc_component_module_get(struct snd_soc_component *component,
 				 int upon_open)
 {
-	if (component->module)
-		return 0;
+	if (unlikely(component->module == 0xff)) {
+		dev_warn(component->dev, "too many module get (%s)\n", component->name);
+		return -EBUSY;
+	}
 
 	if (component->driver->module_get_upon_open == !!upon_open &&
 	    !try_module_get(component->dev->driver->owner))
 		return -ENODEV;
 
-	component->module = 1;
+	component->module++;
 
 	return 0;
 }
@@ -312,11 +314,13 @@  int snd_soc_component_module_get(struct snd_soc_component *component,
 void snd_soc_component_module_put(struct snd_soc_component *component,
 				  int upon_open)
 {
-	if (component->module &&
-	    component->driver->module_get_upon_open == !!upon_open)
+	if (!component->module)
+		return;
+
+	if (component->driver->module_get_upon_open == !!upon_open)
 		module_put(component->dev->driver->owner);
 
-	component->module = 0;
+	component->module--;
 }
 
 int snd_soc_component_open(struct snd_soc_component *component,
@@ -324,12 +328,15 @@  int snd_soc_component_open(struct snd_soc_component *component,
 {
 	int ret = 0;
 
-	if (!component->opened &&
-	    component->driver->open)
+	if (unlikely(component->opened == 0xff)) {
+		dev_warn(component->dev, "too many open (%s)\n", component->name);
+		return -EBUSY;
+	}
+
+	if (component->driver->open)
 		ret = component->driver->open(component, substream);
 
-	if (ret == 0)
-		component->opened = 1;
+	component->opened++;
 
 	return ret;
 }
@@ -339,11 +346,13 @@  int snd_soc_component_close(struct snd_soc_component *component,
 {
 	int ret = 0;
 
-	if (component->opened &&
-	    component->driver->close)
+	if (!component->opened)
+		return;
+
+	if (component->driver->close)
 		ret = component->driver->close(component, substream);
 
-	component->opened = 0;
+	component->opened--;
 
 	return ret;
 }