From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4E769C48BCF for ; Wed, 9 Jun 2021 12:36:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2F05F613AD for ; Wed, 9 Jun 2021 12:36:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232514AbhFIMiO (ORCPT ); Wed, 9 Jun 2021 08:38:14 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:42860 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230377AbhFIMiN (ORCPT ); Wed, 9 Jun 2021 08:38:13 -0400 Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 0077546E; Wed, 9 Jun 2021 14:36:17 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1623242178; bh=rJibpfdexbpvixry90lwtpoTXgxdRSVSW4rBSufZ0oQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=t5zw5mxB0aiCWNaUt2z0cmOFQMb6hiRhV8MJdiva73lfom2GW46zvUrxWkn8wr8i7 zBL2KaeVOA2B8VcHWAyGy7+W+ted6J1HXdz80Wjuxgush0BGIvKbTAhiJlt/DbKs5f WwQI0HBXDqfTJDDwgeBgs76sKIidOUgXEQGG2vVY= Date: Wed, 9 Jun 2021 15:36:00 +0300 From: Laurent Pinchart To: Tomi Valkeinen Cc: Pratyush Yadav , Lokesh Vutla , linux-media@vger.kernel.org Subject: Re: [PATCH v3 21/38] media: ti-vpe: cal: handle cal_ctx_v4l2_register error Message-ID: References: <20210524110909.672432-1-tomi.valkeinen@ideasonboard.com> <20210524110909.672432-22-tomi.valkeinen@ideasonboard.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Hi Tomi, On Mon, Jun 07, 2021 at 11:53:54AM +0300, Tomi Valkeinen wrote: > On 07/06/2021 11:00, Laurent Pinchart wrote: > > On Mon, Jun 07, 2021 at 10:44:17AM +0300, Tomi Valkeinen wrote: > >> On 04/06/2021 16:47, Laurent Pinchart wrote: > >>> On Mon, May 24, 2021 at 02:08:52PM +0300, Tomi Valkeinen wrote: > >>>> cal_async_notifier_complete() doesn't handle errors returned from > >>>> cal_ctx_v4l2_register(). Add the error handling. > >>>> > >>>> Signed-off-by: Tomi Valkeinen > >>>> --- > >>>> drivers/media/platform/ti-vpe/cal.c | 8 ++++++-- > >>>> 1 file changed, 6 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > >>>> index ba8821a3b262..9e051c2e84a9 100644 > >>>> --- a/drivers/media/platform/ti-vpe/cal.c > >>>> +++ b/drivers/media/platform/ti-vpe/cal.c > >>>> @@ -743,8 +743,12 @@ static int cal_async_notifier_complete(struct v4l2_async_notifier *notifier) > >>>> int ret = 0; > >>>> > >>>> for (i = 0; i < ARRAY_SIZE(cal->ctx); ++i) { > >>>> - if (cal->ctx[i]) > >>>> - cal_ctx_v4l2_register(cal->ctx[i]); > >>>> + if (!cal->ctx[i]) > >>>> + continue; > >>>> + > >>>> + ret = cal_ctx_v4l2_register(cal->ctx[i]); > >>>> + if (ret) > >>>> + return ret; > >>> > >>> This part looks good, so > >>> > >>> Reviewed-by: Laurent Pinchart > >>> > >>> Don't we need to call cal_ctx_v4l2_unregister() in the error path of > >>> cal_async_notifier_register() though ? > >> > >> Hmm, can you elaborate? I don't understand where and why we need to call > >> the unregister. > > > > cal_async_notifier_complete() can be called synchronously by > > v4l2_async_notifier_register() if all async subdevs are present. If > > cal_ctx_v4l2_register() fails for one contexts, the failure will be > > propagated to cal_async_notifier_register(), then to > > cal_media_register(), and cal_probe(). Unless I'm mistaken, the contexts > > for which cal_ctx_v4l2_register() succeeded will not be cleaned > > properly. > > Right. I think this can be solved easily by unrolling in the complete callback: > > @@ -748,7 +748,16 @@ static int cal_async_notifier_complete(struct v4l2_async_notifier *notifier) > > ret = cal_ctx_v4l2_register(cal->ctx[i]); > if (ret) > - return ret; > + break; > + } > + > + if (ret) { > + unsigned int j; > + > + for (j = 0; j < i; ++j) > + cal_ctx_v4l2_unregister(cal->ctx[j]); This could also be written for ( ; i > 0; --i) cal_ctx_v4l2_unregister(cal->ctx[i-1]); to avoid an additional variable, it's up to you. > + > + return ret; > } > > if (cal_mc_api) You also need to cal_ctx_v4l2_unregister() if the call in the next line fails. > I'll squash this diff to this patch. -- Regards, Laurent Pinchart