From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pierre-Louis Bossart Subject: Re: [PATCH][RFC] ASoC: soc-pcm: fixup try_module_get() calling timing Date: Sat, 18 May 2019 13:11:05 -0500 Message-ID: <8f6194bb-74f8-04bd-4cf6-7a16d3a7299c@linux.intel.com> References: <87h89t39t6.wl-kuninori.morimoto.gx@renesas.com> <7b48709fa2fa360d4425f07261e218b429383b9f.camel@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 36DD1F806E7 for ; Sat, 18 May 2019 20:11:08 +0200 (CEST) In-Reply-To: <7b48709fa2fa360d4425f07261e218b429383b9f.camel@linux.intel.com> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" To: Ranjani Sridharan , Kuninori Morimoto , Mark Brown , Liam Girdwood , Jaroslav Kysela , Vinod Koul Cc: Linux-ALSA List-Id: alsa-devel@alsa-project.org 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 >>> >>> 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 >>> --- >>> 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 >