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 <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> >>>> ---
> >>>> 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 <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >>>
> >>> 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
