| Message ID | 20210524110909.672432-22-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ti-vpe: cal: multistream & embedded data support | expand |
Hi Tomi, Thank you for the patch. 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@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 ? > } > > if (cal_mc_api)
On 04/06/2021 16:47, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > 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@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. Tomi
Hi Tomi, 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@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.
On 07/06/2021 11:00, Laurent Pinchart wrote: > Hi Tomi, > > 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@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
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@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]); 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.
On 09/06/2021 15:36, Laurent Pinchart wrote: > 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@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]); > > 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. Yes, that's a bit nicer. >> + >> + return ret; >> } >> >> if (cal_mc_api) > > You also need to cal_ctx_v4l2_unregister() if the call in the next line > fails. Ah, indeed. Thanks! Tomi
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; } if (cal_mc_api)
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(-)