Hi Laurent,
On 15/06/2020 00:58, Laurent Pinchart wrote:
> The cal_camerarx_create() function allocates resources with devm_*, and
> thus doesn't need any manual cleanup. Those won't hold true for long, as
> we will need to store resources that have no devm_* allocation variant
> in cal_camerarx. Furthermore, devm_kzalloc() is the wrong memory
> allocation API for structures that can be accessed from userspace, as
> device nodes can be kept open across device removal.
I still think that should be fixed by making devm_ allocations able to
be reference counted so that it doesn't remove until until references
(from open file handles) are released ;-) but that's completely out of
scope here...
>
> Add a cal_camerarx_destroy() function to destroy a CAMERARX instance
> explicitly, and switch to kzalloc() for memory allocation.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/platform/ti-vpe/cal.c | 35 ++++++++++++++++++++++++-----
> 1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index b8c7ad8e39cd..c9fef333c532 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -931,7 +931,7 @@ static struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
> struct cal_camerarx *phy;
> int ret;
>
> - phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
> + phy = kzalloc(sizeof(*phy), GFP_KERNEL);
> if (!phy)
> return ERR_PTR(-ENOMEM);
>
> @@ -946,7 +946,8 @@ static struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
> phy->base = devm_ioremap_resource(&pdev->dev, phy->res);
> if (IS_ERR(phy->base)) {
> cal_err(cal, "failed to ioremap\n");
> - return ERR_CAST(phy->base);
> + ret = PTR_ERR(phy->base);
> + goto error;
> }
>
> cal_dbg(1, cal, "ioresource %s at %pa - %pa\n",
> @@ -954,9 +955,21 @@ static struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
>
> ret = cal_camerarx_regmap_init(cal, phy);
> if (ret)
> - return ERR_PTR(ret);
> + goto error;
>
> return phy;
> +
> +error:
> + kfree(phy);
> + return ERR_PTR(ret);
> +}
> +
> +static void cal_camerarx_destroy(struct cal_camerarx *phy)
> +{
> + if (!phy)
> + return;
> +
> + kfree(phy);
> }
>
> static int cal_camerarx_init_regmap(struct cal_dev *cal)
> @@ -2252,15 +2265,17 @@ static int cal_probe(struct platform_device *pdev)
> /* Create CAMERARX PHYs. */
> for (i = 0; i < cal_data_get_num_csi2_phy(cal); ++i) {
> cal->phy[i] = cal_camerarx_create(cal, i);
> - if (IS_ERR(cal->phy[i]))
> - return PTR_ERR(cal->phy[i]);
> + if (IS_ERR(cal->phy[i])) {
> + ret = PTR_ERR(cal->phy[i]);
here, cal->phy[i] is an error value, but you then iterate them and call
cal_camerarx_destroy() on this value.
Perhaps you should set the following here, before jumping:
cal->phy[i] = NULL;
> + goto error_camerarx;
> + }
> }
>
> /* Register the V4L2 device. */
> ret = v4l2_device_register(&pdev->dev, &cal->v4l2_dev);
> if (ret) {
> cal_err(cal, "Failed to register V4L2 device\n");
> - return ret;
> + goto error_camerarx;
> }
>
> /* Create contexts. */
> @@ -2301,6 +2316,11 @@ static int cal_probe(struct platform_device *pdev)
>
> error_v4l2:
> v4l2_device_unregister(&cal->v4l2_dev);
> +
> +error_camerarx:
> + for (i = 0; i < ARRAY_SIZE(cal->phy); i++)
> + cal_camerarx_destroy(cal->phy[i]);
> +
> return ret;
> }
>
> @@ -2329,6 +2349,9 @@ static int cal_remove(struct platform_device *pdev)
>
> v4l2_device_unregister(&cal->v4l2_dev);
>
> + for (i = 0; i < ARRAY_SIZE(cal->phy); i++)
> + cal_camerarx_destroy(cal->phy[i]);
> +
> pm_runtime_put_sync(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
>
>
--
Regards
--
Kieran
