Re: [PATCH v2] ASoC: soc-pcm: fixup try_module_get()/module_put() timing

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 5/19/19 8:42 PM, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> soc_pcm_components_open/close() try to call
> try_module_get()/module_put() based on
> component->driver->module_get_upon_open.
> 
> Here, the purpose why we need to call these functions are to
> checking module reference.
> Thus, we need to call try_module_open() even though it doesn't
> have .open callback.
> 
> The same reason, we need to call module_put() even though it
> doesn't have .close
> 
> This patch calls try_module_get()/module_put() regardless of
> .open/.close
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>


LGTM, this removes the assumptions on the implementation of .open and 
.close.

FWIW
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Thanks!

> ---
> v1 -> v2
> 
>   - merge both try_module_get()/module_put() patch into 1
> 
>   sound/soc/soc-pcm.c | 16 +++++++---------
>   1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 7737e00..e24eab3 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,
> @@ -477,11 +477,9 @@ static int soc_pcm_components_close(struct snd_pcm_substream *substream,
>   		if (component == last)
>   			break;
>   
> -		if (!component->driver->ops ||
> -		    !component->driver->ops->close)
> -			continue;
> -
> -		component->driver->ops->close(substream);
> +		if (component->driver->ops &&
> +		    component->driver->ops->close)
> +			component->driver->ops->close(substream);
>   
>   		if (component->driver->module_get_upon_open)
>   			module_put(component->dev->driver->owner);
>