| Message ID | 20210412113457.328012-3-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ti-vpe: cal: prepare for multistream support | expand |
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:31PM +0300, Tomi Valkeinen wrote: > cal_camerarx_create() doesn't handle error returned from > cal_camerarx_sd_init_cfg() This looks good. > and it always runs all the cleanup/free > functions in the error code path. The latter doesn't cause any issues at > the moment as media_entity_cleanup() is an empty function. But this was by design. Do you think we could keep media_entity_cleanup() idempotent ? That would simplify error paths (as shown here). > Fix the error handling. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal-camerarx.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > index cbe6114908de..820fb483c402 100644 > --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > @@ -812,7 +812,7 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, > if (IS_ERR(phy->base)) { > cal_err(cal, "failed to ioremap\n"); > ret = PTR_ERR(phy->base); > - goto error; > + goto err_free_phy; > } > > cal_dbg(1, cal, "ioresource %s at %pa - %pa\n", > @@ -820,11 +820,11 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, > > ret = cal_camerarx_regmap_init(cal, phy); > if (ret) > - goto error; > + goto err_free_phy; > > ret = cal_camerarx_parse_dt(phy); > if (ret) > - goto error; > + goto err_free_phy; > > /* Initialize the V4L2 subdev and media entity. */ > sd = &phy->subdev; > @@ -840,18 +840,21 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, > ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(phy->pads), > phy->pads); > if (ret) > - goto error; > + goto err_entity_cleanup; > > - cal_camerarx_sd_init_cfg(sd, NULL); > + ret = cal_camerarx_sd_init_cfg(sd, NULL); > + if (ret) > + goto err_entity_cleanup; > > ret = v4l2_device_register_subdev(&cal->v4l2_dev, sd); > if (ret) > - goto error; > + goto err_entity_cleanup; > > return phy; > > -error: > +err_entity_cleanup: > media_entity_cleanup(&phy->subdev.entity); > +err_free_phy: > kfree(phy); > return ERR_PTR(ret); > }
On 18/04/2021 02:05, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Apr 12, 2021 at 02:34:31PM +0300, Tomi Valkeinen wrote: >> cal_camerarx_create() doesn't handle error returned from >> cal_camerarx_sd_init_cfg() > > This looks good. > >> and it always runs all the cleanup/free >> functions in the error code path. The latter doesn't cause any issues at >> the moment as media_entity_cleanup() is an empty function. > > But this was by design. Do you think we could keep > media_entity_cleanup() idempotent ? That would simplify error paths (as > shown here). It isn't documented. I can change the doc for media_entity_cleanup to state it can be called multiple times, if that was the intention, and simplify the error handling here. Tomi
Hi Tomi, On Mon, Apr 19, 2021 at 11:24:01AM +0300, Tomi Valkeinen wrote: > On 18/04/2021 02:05, Laurent Pinchart wrote: > > On Mon, Apr 12, 2021 at 02:34:31PM +0300, Tomi Valkeinen wrote: > >> cal_camerarx_create() doesn't handle error returned from > >> cal_camerarx_sd_init_cfg() > > > > This looks good. > > > >> and it always runs all the cleanup/free > >> functions in the error code path. The latter doesn't cause any issues at > >> the moment as media_entity_cleanup() is an empty function. > > > > But this was by design. Do you think we could keep > > media_entity_cleanup() idempotent ? That would simplify error paths (as > > shown here). > > It isn't documented. I can change the doc for media_entity_cleanup to > state it can be called multiple times, if that was the intention, and > simplify the error handling here. That would be my preference. media_entity_cleanup() isn't performance-sensitive, so I'd favour ease of use and simplicity of error handling in drivers.
diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c index cbe6114908de..820fb483c402 100644 --- a/drivers/media/platform/ti-vpe/cal-camerarx.c +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c @@ -812,7 +812,7 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, if (IS_ERR(phy->base)) { cal_err(cal, "failed to ioremap\n"); ret = PTR_ERR(phy->base); - goto error; + goto err_free_phy; } cal_dbg(1, cal, "ioresource %s at %pa - %pa\n", @@ -820,11 +820,11 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, ret = cal_camerarx_regmap_init(cal, phy); if (ret) - goto error; + goto err_free_phy; ret = cal_camerarx_parse_dt(phy); if (ret) - goto error; + goto err_free_phy; /* Initialize the V4L2 subdev and media entity. */ sd = &phy->subdev; @@ -840,18 +840,21 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(phy->pads), phy->pads); if (ret) - goto error; + goto err_entity_cleanup; - cal_camerarx_sd_init_cfg(sd, NULL); + ret = cal_camerarx_sd_init_cfg(sd, NULL); + if (ret) + goto err_entity_cleanup; ret = v4l2_device_register_subdev(&cal->v4l2_dev, sd); if (ret) - goto error; + goto err_entity_cleanup; return phy; -error: +err_entity_cleanup: media_entity_cleanup(&phy->subdev.entity); +err_free_phy: kfree(phy); return ERR_PTR(ret); }
cal_camerarx_create() doesn't handle error returned from cal_camerarx_sd_init_cfg() and it always runs all the cleanup/free functions in the error code path. The latter doesn't cause any issues at the moment as media_entity_cleanup() is an empty function. Fix the error handling. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/platform/ti-vpe/cal-camerarx.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)