On 07/06/2021 11:00, Laurent Pinchart wrote:

Hi Tomi,

On Mon, Jun 07, 2021 at 10:44:17AM +0300, Tomi Valkeinen wrote:

quoted

On 04/06/2021 16:47, Laurent Pinchart wrote:

quoted

On Mon, May 24, 2021 at 02:08:52PM +0300, Tomi Valkeinen wrote:

quoted

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@ideasonboard.com>
---
   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@ideasonboard.com>

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]);
+
+		return ret;
  	}
  
  	if (cal_mc_api)

I'll squash this diff to this patch.

  Tomi