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