| Message ID | 4db811899ccd7b5315080790a627974e3569c7cc.1311839940.git.andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hi Andy, On Thursday 28 July 2011 09:59:38 Andy Shevchenko wrote: > Potentially the adp1653_init_controls could return an error. In our case > the error was ignored, meanwhile it means incorrect initialization of V4L2 > controls. Additionally we have to free control handler structures in case > of apd1653_init_controls or media_entity_init failure. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Sakari Ailus <sakari.ailus@iki.fi> > --- > drivers/media/video/adp1653.c | 11 +++++++++-- > 1 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c > index 8ad89ff..279d75d 100644 > --- a/drivers/media/video/adp1653.c > +++ b/drivers/media/video/adp1653.c > @@ -429,12 +429,19 @@ static int adp1653_probe(struct i2c_client *client, > flash->subdev.internal_ops = &adp1653_internal_ops; > flash->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > - adp1653_init_controls(flash); > + ret = adp1653_init_controls(flash); > + if (ret) > + goto free_and_quit; > > ret = media_entity_init(&flash->subdev.entity, 0, NULL, 0); > if (ret < 0) > - kfree(flash); > + goto free_and_quit; > > + return 0; > + > +free_and_quit: > + v4l2_ctrl_handler_free(&flash->ctrls); > + kfree(flash); > return ret; What about ret = adp1653_init_controls(flash); if (ret) goto done; ret = media_entity_init(&flash->subdev.entity, 0, NULL, 0); done: if (ret < 0) { v4l2_ctrl_handler_free(&flash->ctrls); kfree(flash); } return ret; > }
On Thu, 2011-07-28 at 16:34 +0200, Laurent Pinchart wrote: > > - adp1653_init_controls(flash); > > + ret = adp1653_init_controls(flash); > > + if (ret) > > + goto free_and_quit; > > > > ret = media_entity_init(&flash->subdev.entity, 0, NULL, 0); > > if (ret < 0) > > - kfree(flash); > > + goto free_and_quit; > > > > + return 0; > > + > > +free_and_quit: > > + v4l2_ctrl_handler_free(&flash->ctrls); > > + kfree(flash); > > return ret; > What about > ret = adp1653_init_controls(flash); > if (ret) > goto done; > > ret = media_entity_init(&flash->subdev.entity, 0, NULL, 0); > > done: > if (ret < 0) { > v4l2_ctrl_handler_free(&flash->ctrls); > kfree(flash); > } > > return ret; There is no difference at first glance. However, your variant is less straight to understand for my opinion.
diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c index 8ad89ff..279d75d 100644 --- a/drivers/media/video/adp1653.c +++ b/drivers/media/video/adp1653.c @@ -429,12 +429,19 @@ static int adp1653_probe(struct i2c_client *client, flash->subdev.internal_ops = &adp1653_internal_ops; flash->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; - adp1653_init_controls(flash); + ret = adp1653_init_controls(flash); + if (ret) + goto free_and_quit; ret = media_entity_init(&flash->subdev.entity, 0, NULL, 0); if (ret < 0) - kfree(flash); + goto free_and_quit; + return 0; + +free_and_quit: + v4l2_ctrl_handler_free(&flash->ctrls); + kfree(flash); return ret; }
Potentially the adp1653_init_controls could return an error. In our case the error was ignored, meanwhile it means incorrect initialization of V4L2 controls. Additionally we have to free control handler structures in case of apd1653_init_controls or media_entity_init failure. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Sakari Ailus <sakari.ailus@iki.fi> --- drivers/media/video/adp1653.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)