| Message ID | 878skyt6om.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] ASoC: soc-component: count snd_soc_component_open/close() | expand |
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
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
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
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
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; }
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(-)