[alsa-devel] [PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_components_open() try to call try_module_get() based on component->driver->module_get_upon_open. But, it should be always called, not relatead to .open callback. It should be called at (A) istead of (B)
=> (A) if (!component->driver->ops || !component->driver->ops->open) continue; => (B) if (component->driver->module_get_upon_open && !try_module_get(component->dev->driver->owner)) { ... }
ret = component->driver->ops->open(substream);
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- Mark, Pierre-Louis, Vinod, Liam
I think this patch is correct, but I'm not sure. I'm happy if someone can confirm it.
sound/soc/soc-pcm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 7737e00..7b4cda6 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -440,10 +440,6 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream, component = rtdcom->component; *last = component;
- if (!component->driver->ops || - !component->driver->ops->open) - continue; - if (component->driver->module_get_upon_open && !try_module_get(component->dev->driver->owner)) { dev_err(component->dev, @@ -452,6 +448,10 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream, return -ENODEV; }
+ if (!component->driver->ops || + !component->driver->ops->open) + continue; + ret = component->driver->ops->open(substream); if (ret < 0) { dev_err(component->dev,
On 5/17/19 1:08 AM, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_components_open() try to call try_module_get() based on component->driver->module_get_upon_open. But, it should be always called, not relatead to .open callback. It should be called at (A) istead of (B)
=> (A) if (!component->driver->ops || !component->driver->ops->open) continue; => (B) if (component->driver->module_get_upon_open && !try_module_get(component->dev->driver->owner)) { ... }
ret = component->driver->ops->open(substream);
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Mark, Pierre-Louis, Vinod, Liam
I think this patch is correct, but I'm not sure. I'm happy if someone can confirm it.
The try_module_get()/module_put() mechanism is based on the assumption that the .open and .close callbacks are both mandatory.
open flow: if (!component->driver->ops || !component->driver->ops->open) continue;
if (component->driver->module_get_upon_open && !try_module_get(component->dev->driver->owner)) { ret = -ENODEV; goto module_err; }
ret = component->driver->ops->open(substream);
close flow: if (!component->driver->ops || !component->driver->ops->close) continue;
component->driver->ops->close(substream);
if (component->driver->module_get_upon_open) module_put(component->dev->driver->owner);
it'd be odd to allow the refcount to be increased when there is no .open, since if there is no .close either then the refcount never decreases.
sound/soc/soc-pcm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 7737e00..7b4cda6 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -440,10 +440,6 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream, component = rtdcom->component; *last = component;
if (!component->driver->ops ||
!component->driver->ops->open)
continue;
- if (component->driver->module_get_upon_open && !try_module_get(component->dev->driver->owner)) { dev_err(component->dev,
@@ -452,6 +448,10 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream, return -ENODEV; }
if (!component->driver->ops ||
!component->driver->ops->open)
continue;
- ret = component->driver->ops->open(substream); if (ret < 0) { dev_err(component->dev,
On Fri, 2019-05-17 at 08:22 -0500, Pierre-Louis Bossart wrote:
On 5/17/19 1:08 AM, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_components_open() try to call try_module_get() based on component->driver->module_get_upon_open. But, it should be always called, not relatead to .open callback. It should be called at (A) istead of (B)
=> (A) if (!component->driver->ops || !component->driver->ops->open) continue; => (B) if (component->driver->module_get_upon_open && !try_module_get(component->dev->driver->owner)) { ... }
ret = component->driver->ops->open(substream);
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Mark, Pierre-Louis, Vinod, Liam
I think this patch is correct, but I'm not sure. I'm happy if someone can confirm it.
The try_module_get()/module_put() mechanism is based on the assumption that the .open and .close callbacks are both mandatory.
Hi Pierre, But is this enforced? We could end up doing a try_module_get() without checking if there is a close callback in which case we'd never do the module_put(), isnt it?
Thanks, Ranjani
open flow: if (!component->driver->ops || !component->driver->ops->open) continue;
if (component->driver->module_get_upon_open && !try_module_get(component->dev->driver->owner)) { ret = -ENODEV; goto module_err; } ret = component->driver->ops->open(substream);
close flow: if (!component->driver->ops || !component->driver->ops->close) continue;
component->driver->ops->close(substream); if (component->driver->module_get_upon_open) module_put(component->dev->driver->owner);
it'd be odd to allow the refcount to be increased when there is no .open, since if there is no .close either then the refcount never decreases.
sound/soc/soc-pcm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 7737e00..7b4cda6 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -440,10 +440,6 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream, component = rtdcom->component; *last = component;
if (!component->driver->ops ||
!component->driver->ops->open)
continue;
- if (component->driver->module_get_upon_open && !try_module_get(component->dev->driver->owner)) { dev_err(component->dev,
@@ -452,6 +448,10 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream, return -ENODEV; }
if (!component->driver->ops ||
!component->driver->ops->open)
continue;
- ret = component->driver->ops->open(substream); if (ret < 0) { dev_err(component->dev,
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 5/18/19 12:54 PM, Ranjani Sridharan wrote:
On Fri, 2019-05-17 at 08:22 -0500, Pierre-Louis Bossart wrote:
On 5/17/19 1:08 AM, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_components_open() try to call try_module_get() based on component->driver->module_get_upon_open. But, it should be always called, not relatead to .open callback. It should be called at (A) istead of (B)
=> (A) if (!component->driver->ops || !component->driver->ops->open) continue; => (B) if (component->driver->module_get_upon_open && !try_module_get(component->dev->driver->owner)) { ... }
ret = component->driver->ops->open(substream);
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Mark, Pierre-Louis, Vinod, Liam
I think this patch is correct, but I'm not sure. I'm happy if someone can confirm it.
The try_module_get()/module_put() mechanism is based on the assumption that the .open and .close callbacks are both mandatory.
Hi Pierre, But is this enforced? We could end up doing a try_module_get() without checking if there is a close callback in which case we'd never do the module_put(), isnt it?
My initial feedback was that changing the open case only wouldn't work.
We need to enforce that both the open/close callbacks are required and leave the code as is, or we apply both of Morimoto-san's patches (which unfortunately have the same subject to cover the two cases) and both open and close are optional - though I am having a hard time figuring out case where we we'd use one and the other.
Thanks, Ranjani
open flow: if (!component->driver->ops || !component->driver->ops->open) continue;
if (component->driver->module_get_upon_open && !try_module_get(component->dev->driver->owner)) { ret = -ENODEV; goto module_err; } ret = component->driver->ops->open(substream);
close flow: if (!component->driver->ops || !component->driver->ops->close) continue;
component->driver->ops->close(substream); if (component->driver->module_get_upon_open) module_put(component->dev->driver->owner);
it'd be odd to allow the refcount to be increased when there is no .open, since if there is no .close either then the refcount never decreases.
sound/soc/soc-pcm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 7737e00..7b4cda6 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -440,10 +440,6 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream, component = rtdcom->component; *last = component;
if (!component->driver->ops ||
!component->driver->ops->open)
continue;
if (component->driver->module_get_upon_open && !try_module_get(component->dev->driver->owner)) { dev_err(component->dev,
@@ -452,6 +448,10 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream, return -ENODEV; }
if (!component->driver->ops ||
!component->driver->ops->open)
continue;
ret = component->driver->ops->open(substream); if (ret < 0) { dev_err(component->dev,
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi Pierre-Louis
Thank you for your feedback
=> (A) if (!component->driver->ops || !component->driver->ops->open) continue; => (B) if (component->driver->module_get_upon_open && !try_module_get(component->dev->driver->owner)) { ... }
(snip)
The try_module_get()/module_put() mechanism is based on the assumption that the .open and .close callbacks are both mandatory.
Hi Pierre, But is this enforced? We could end up doing a try_module_get() without checking if there is a close callback in which case we'd never do the module_put(), isnt it?
My initial feedback was that changing the open case only wouldn't work.
We need to enforce that both the open/close callbacks are required and leave the code as is, or we apply both of Morimoto-san's patches (which unfortunately have the same subject to cover the two cases) and both open and close are optional - though I am having a hard time figuring out case where we we'd use one and the other.
If my understanding is correct, the reason why we need to call try_module_get()/module_put() is to checking used component. The component will be used anyway even though it doesn't have .open, I think. So, we need to call these anyway. But yes it should change both .open/.close in the same patch. Then, .open/.close is just optional. I will repost patch.
Thank you for your help !! Best regards --- Kuninori Morimoto
participants (3)
-
Kuninori Morimoto
-
Pierre-Louis Bossart
-
Ranjani Sridharan