diff mbox series

[v2,057/108] media: ti-vpe: cal: Add cal_camerarx_destroy() to cleanup CAMERARX

Message ID 20200706183709.12238-58-laurent.pinchart@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series media: ti-vpe: cal: Add media controller support | expand

Commit Message

Laurent Pinchart July 6, 2020, 6:36 p.m. UTC 
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.

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@ideasonboard.com>
---
Changes since v1:

- Set cal->phy[i] to NULL in error path
---
 drivers/media/platform/ti-vpe/cal.c | 36 ++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 6 deletions(-)

Comments

Hans Verkuil July 10, 2020, 11:24 a.m. UTC | #1 
On 06/07/2020 20:36, 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.
> 
> 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@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Set cal->phy[i] to NULL in error path
> ---
>  drivers/media/platform/ti-vpe/cal.c | 36 ++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 5580913a1356..492141f07d69 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -932,7 +932,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);
>  
> @@ -947,7 +947,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",
> @@ -955,9 +956,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;

This isn't necessary since kfree already tests for this.

Regards,

	Hans

> +
> +	kfree(phy);
>  }
>  
>  static int cal_camerarx_init_regmap(struct cal_dev *cal)
> @@ -2253,15 +2266,18 @@ static int cal_probe(struct platform_device *pdev)
>  	/* Create CAMERARX PHYs. */
>  	for (i = 0; i < cal->data->num_csi2_phy; ++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]);
> +			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. */
> @@ -2302,6 +2318,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;
>  }
>  
> @@ -2330,6 +2351,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);
>  
>
Laurent Pinchart July 10, 2020, 11:51 a.m. UTC | #2 
Hi Hans,

On Fri, Jul 10, 2020 at 01:24:28PM +0200, Hans Verkuil wrote:
> On 06/07/2020 20:36, 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.
> > 
> > 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@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Set cal->phy[i] to NULL in error path
> > ---
> >  drivers/media/platform/ti-vpe/cal.c | 36 ++++++++++++++++++++++++-----
> >  1 file changed, 30 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> > index 5580913a1356..492141f07d69 100644
> > --- a/drivers/media/platform/ti-vpe/cal.c
> > +++ b/drivers/media/platform/ti-vpe/cal.c
> > @@ -932,7 +932,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);
> >  
> > @@ -947,7 +947,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",
> > @@ -955,9 +956,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;
> 
> This isn't necessary since kfree already tests for this.

The function later gets expanded to perform more cleanups that would
crash if phy was null. I'd rather keep the check here to show that
calling cal_camerarx_destroy() with a null pointer is allowed by design.

> > +
> > +	kfree(phy);
> >  }
> >  
> >  static int cal_camerarx_init_regmap(struct cal_dev *cal)
> > @@ -2253,15 +2266,18 @@ static int cal_probe(struct platform_device *pdev)
> >  	/* Create CAMERARX PHYs. */
> >  	for (i = 0; i < cal->data->num_csi2_phy; ++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]);
> > +			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. */
> > @@ -2302,6 +2318,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;
> >  }
> >  
> > @@ -2330,6 +2351,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);
> >
diff mbox series

Patch

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 5580913a1356..492141f07d69 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -932,7 +932,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);
 
@@ -947,7 +947,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",
@@ -955,9 +956,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)
@@ -2253,15 +2266,18 @@  static int cal_probe(struct platform_device *pdev)
 	/* Create CAMERARX PHYs. */
 	for (i = 0; i < cal->data->num_csi2_phy; ++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]);
+			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. */
@@ -2302,6 +2318,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;
 }
 
@@ -2330,6 +2351,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);