* [PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing
@ 2019-05-17 6:08 Kuninori Morimoto
2019-05-17 13:22 ` Pierre-Louis Bossart
0 siblings, 1 reply; 5+ messages in thread
From: Kuninori Morimoto @ 2019-05-17 6:08 UTC (permalink / raw)
To: Mark Brown, Pierre-Louis Bossart, Liam Girdwood, Jaroslav Kysela,
Vinod Koul
Cc: Linux-ALSA
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,
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing
2019-05-17 6:08 [PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing Kuninori Morimoto
@ 2019-05-17 13:22 ` Pierre-Louis Bossart
2019-05-18 17:54 ` Ranjani Sridharan
0 siblings, 1 reply; 5+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-17 13:22 UTC (permalink / raw)
To: Kuninori Morimoto, Mark Brown, Liam Girdwood, Jaroslav Kysela,
Vinod Koul
Cc: Linux-ALSA
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,
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing
2019-05-17 13:22 ` Pierre-Louis Bossart
@ 2019-05-18 17:54 ` Ranjani Sridharan
2019-05-18 18:11 ` Pierre-Louis Bossart
0 siblings, 1 reply; 5+ messages in thread
From: Ranjani Sridharan @ 2019-05-18 17:54 UTC (permalink / raw)
To: Pierre-Louis Bossart, Kuninori Morimoto, Mark Brown,
Liam Girdwood, Jaroslav Kysela, Vinod Koul
Cc: Linux-ALSA
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing
2019-05-18 17:54 ` Ranjani Sridharan
@ 2019-05-18 18:11 ` Pierre-Louis Bossart
2019-05-20 1:01 ` Kuninori Morimoto
0 siblings, 1 reply; 5+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-18 18:11 UTC (permalink / raw)
To: Ranjani Sridharan, Kuninori Morimoto, Mark Brown, Liam Girdwood,
Jaroslav Kysela, Vinod Koul
Cc: Linux-ALSA
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing
2019-05-18 18:11 ` Pierre-Louis Bossart
@ 2019-05-20 1:01 ` Kuninori Morimoto
0 siblings, 0 replies; 5+ messages in thread
From: Kuninori Morimoto @ 2019-05-20 1:01 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Linux-ALSA, Liam Girdwood, Ranjani Sridharan, Vinod Koul,
Mark Brown
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-05-20 1:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-17 6:08 [PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing Kuninori Morimoto
2019-05-17 13:22 ` Pierre-Louis Bossart
2019-05-18 17:54 ` Ranjani Sridharan
2019-05-18 18:11 ` Pierre-Louis Bossart
2019-05-20 1:01 ` Kuninori Morimoto
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.