[alsa-devel] [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 at 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 at 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 at alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel