| Message ID | 20230302100755.191164-3-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/5] media: ti: cal: Clean up mbus formats uses | expand |
Hi Tomi On Thu, Mar 02, 2023 at 12:07:52PM +0200, Tomi Valkeinen wrote: > We don't do a proper job at freeing resources in cal_camerarx_create's > error paths. > > Fix these, and also switch the phy allcation from kzalloc to > devm_kzalloc to simplify the code further. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti/cal/cal-camerarx.c | 23 +++++++++++--------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c > index 267089b0fea0..97208d542f9e 100644 > --- a/drivers/media/platform/ti/cal/cal-camerarx.c > +++ b/drivers/media/platform/ti/cal/cal-camerarx.c > @@ -864,7 +864,7 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, > unsigned int i; > int ret; > > - phy = kzalloc(sizeof(*phy), GFP_KERNEL); > + phy = devm_kzalloc(cal->dev, sizeof(*phy), GFP_KERNEL); > if (!phy) > return ERR_PTR(-ENOMEM); > > @@ -882,7 +882,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_destroy_mutex; I have your previous version applied, I'm probably on a different base as I don't see any phy->mutex at all! > } > > cal_dbg(1, cal, "ioresource %s at %pa - %pa\n", > @@ -890,11 +890,11 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, > > ret = cal_camerarx_regmap_init(cal, phy); > if (ret) > - goto error; > + goto err_destroy_mutex; > > ret = cal_camerarx_parse_dt(phy); > if (ret) > - goto error; > + goto err_destroy_mutex; > > /* Initialize the V4L2 subdev and media entity. */ > sd = &phy->subdev; > @@ -911,21 +911,25 @@ 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_node_put; > > ret = cal_camerarx_sd_init_cfg(sd, NULL); > if (ret) > - goto error; > + 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); > - kfree(phy); > +err_node_put: > + of_node_put(phy->source_ep_node); > + of_node_put(phy->source_node); good, these where leaked indeed! Missing mutex apart the patch is good to me Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > +err_destroy_mutex: > + mutex_destroy(&phy->mutex); > return ERR_PTR(ret); > } > > @@ -939,5 +943,4 @@ void cal_camerarx_destroy(struct cal_camerarx *phy) > of_node_put(phy->source_ep_node); > of_node_put(phy->source_node); > mutex_destroy(&phy->mutex); > - kfree(phy); > } > -- > 2.34.1 >
On 02/03/2023 12:50, Jacopo Mondi wrote: > Hi Tomi > > On Thu, Mar 02, 2023 at 12:07:52PM +0200, Tomi Valkeinen wrote: >> We don't do a proper job at freeing resources in cal_camerarx_create's >> error paths. >> >> Fix these, and also switch the phy allcation from kzalloc to >> devm_kzalloc to simplify the code further. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/ti/cal/cal-camerarx.c | 23 +++++++++++--------- >> 1 file changed, 13 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c >> index 267089b0fea0..97208d542f9e 100644 >> --- a/drivers/media/platform/ti/cal/cal-camerarx.c >> +++ b/drivers/media/platform/ti/cal/cal-camerarx.c >> @@ -864,7 +864,7 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, >> unsigned int i; >> int ret; >> >> - phy = kzalloc(sizeof(*phy), GFP_KERNEL); >> + phy = devm_kzalloc(cal->dev, sizeof(*phy), GFP_KERNEL); >> if (!phy) >> return ERR_PTR(-ENOMEM); >> >> @@ -882,7 +882,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_destroy_mutex; > > I have your previous version applied, I'm probably on a different base > as I don't see any phy->mutex at all! The "media: ti: cal: Use subdev state" drops the mutex, but in v3 that patch comes after this one. So here we still have the mutex, but it'll go away in the next patch. Tomi
diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c index 267089b0fea0..97208d542f9e 100644 --- a/drivers/media/platform/ti/cal/cal-camerarx.c +++ b/drivers/media/platform/ti/cal/cal-camerarx.c @@ -864,7 +864,7 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, unsigned int i; int ret; - phy = kzalloc(sizeof(*phy), GFP_KERNEL); + phy = devm_kzalloc(cal->dev, sizeof(*phy), GFP_KERNEL); if (!phy) return ERR_PTR(-ENOMEM); @@ -882,7 +882,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_destroy_mutex; } cal_dbg(1, cal, "ioresource %s at %pa - %pa\n", @@ -890,11 +890,11 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal, ret = cal_camerarx_regmap_init(cal, phy); if (ret) - goto error; + goto err_destroy_mutex; ret = cal_camerarx_parse_dt(phy); if (ret) - goto error; + goto err_destroy_mutex; /* Initialize the V4L2 subdev and media entity. */ sd = &phy->subdev; @@ -911,21 +911,25 @@ 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_node_put; ret = cal_camerarx_sd_init_cfg(sd, NULL); if (ret) - goto error; + 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); - kfree(phy); +err_node_put: + of_node_put(phy->source_ep_node); + of_node_put(phy->source_node); +err_destroy_mutex: + mutex_destroy(&phy->mutex); return ERR_PTR(ret); } @@ -939,5 +943,4 @@ void cal_camerarx_destroy(struct cal_camerarx *phy) of_node_put(phy->source_ep_node); of_node_put(phy->source_node); mutex_destroy(&phy->mutex); - kfree(phy); }
We don't do a proper job at freeing resources in cal_camerarx_create's error paths. Fix these, and also switch the phy allcation from kzalloc to devm_kzalloc to simplify the code further. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/platform/ti/cal/cal-camerarx.c | 23 +++++++++++--------- 1 file changed, 13 insertions(+), 10 deletions(-)