[PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing

[PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing

[Kuninori Morimoto]

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

Re: [PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing

[Pierre-Louis Bossart]

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,
> 

Re: [PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing

[Ranjani Sridharan]

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

Re: [PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing

[Pierre-Louis Bossart]

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
> 

Re: [PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing

[Kuninori Morimoto]

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