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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
