[PATCH 0/7] drm/sun4i: Various fixes and cleanups part 1

[PATCH 0/7] drm/sun4i: Various fixes and cleanups part 1

[Chen-Yu Tsai]

Hi Maxime,

This is the first bunch of fixes for the sun4i drm driver. This is part
of the cleanup I am doing towards making the driver support multiple
display pipelines.

Patch 1 moves the drm_mode_config_cleanup call from sun4i_framebuffer_free
to be called directly in sun4i_drv_unbind. This is needed for patch 2, so
it doesn't get called twice.

Patch 2 adds proper clean up to the error return path in sun4i_drv_bind.

Patch 3 adds a check for drm_vblank_init's return value. It can fail if
no memory is available.

Patch 4 fixes the element size passed to kcalloc. Previously we were
allocating too much memory.

Patch 5 drops a useless assignment.

Patch 6 makes sun4i_layers_init actually store the created layers in the
list that it returns. This one was particularly nasty.

Patch 7 makes sun4i_crtc_init pass back error codes.

We probably don't need to tag stable for these. Patch 1 and 2 fix up
possible memory and object leakage, but unless the user keeps unloading
and loading the modules, it won't leak past a few times.

Regards
ChenYu


Chen-Yu Tsai (7):
  drm/sun4i: Move drm_mode_config_cleanup call to main driver
  drm/sun4i: Fix up error path cleanup for master bind function
  drm/sun4i: Check return value of drm_vblank_init
  drm/sun4i: Fix kcalloc element size in sun4i_layers_init
  drm/sun4i: Drop useless assignment in sun4i_layers_init
  drm/sun4i: Save newly created layer in layers array in
    sun4i_layers_init
  drm/sun4i: Make sun4i_crtc_init return ERR_PTR style error codes

 drivers/gpu/drm/sun4i/sun4i_crtc.c        |  4 ++--
 drivers/gpu/drm/sun4i/sun4i_drv.c         | 27 +++++++++++++++++++--------
 drivers/gpu/drm/sun4i/sun4i_framebuffer.c |  1 -
 drivers/gpu/drm/sun4i/sun4i_layer.c       |  5 +++--
 4 files changed, 24 insertions(+), 13 deletions(-)

-- 
2.11.0

[PATCH 1/7] drm/sun4i: Move drm_mode_config_cleanup call to main driver

[Chen-Yu Tsai]

drm_mode_config_cleanup is the complement of drm_mode_config_init, which
is called in the bind function of sun4i_drv. drm_mode_config_cleanup
should be put in the unbind function to match.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_drv.c         | 1 +
 drivers/gpu/drm/sun4i/sun4i_framebuffer.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 4ce665349f6b..99a0f1861be2 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -188,6 +188,7 @@ static void sun4i_drv_unbind(struct device *dev)
 	drm_dev_unregister(drm);
 	drm_kms_helper_poll_fini(drm);
 	sun4i_framebuffer_free(drm);
+	drm_mode_config_cleanup(drm);
 	drm_vblank_cleanup(drm);
 	drm_dev_unref(drm);
 }
diff --git a/drivers/gpu/drm/sun4i/sun4i_framebuffer.c b/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
index 8b6ce619ad81..0c72b54c047d 100644
--- a/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
@@ -50,5 +50,4 @@ void sun4i_framebuffer_free(struct drm_device *drm)
 	struct sun4i_drv *drv = drm->dev_private;
 
 	drm_fbdev_cma_fini(drv->fbdev);
-	drm_mode_config_cleanup(drm);
 }
-- 
2.11.0

Re: [PATCH 1/7] drm/sun4i: Move drm_mode_config_cleanup call to main driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

On Fri, Feb 17, 2017 at 11:13:24AM +0800, Chen-Yu Tsai wrote:
> drm_mode_config_cleanup is the complement of drm_mode_config_init, which
> is called in the bind function of sun4i_drv. drm_mode_config_cleanup
> should be put in the unbind function to match.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH 2/7] drm/sun4i: Fix up error path cleanup for master bind function

[Chen-Yu Tsai]

The master bind function calls numerous drm functions which initialize
underlying structures. It also tries to bind the various components
of the display pipeline, some of which may add additional drm objects.

This patch adds proper cleanup functions in the error path of the
master bind function.

This requires the patch "drm/sun4i: Move drm_mode_config_cleanup call
to main driver", which splits out drm_mode_config_cleanup from
sun4i_framebuffer_free so we can call it separately.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_drv.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 99a0f1861be2..8e777167bca4 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -136,7 +136,7 @@ static int sun4i_drv_bind(struct device *dev)
 	ret = component_bind_all(drm->dev, drm);
 	if (ret) {
 		dev_err(drm->dev, "Couldn't bind all pipelines components\n");
-		goto free_drm;
+		goto cleanup_mode_config;
 	}
 
 	/* Create our layers */
@@ -144,7 +144,7 @@ static int sun4i_drv_bind(struct device *dev)
 	if (IS_ERR(drv->layers)) {
 		dev_err(drm->dev, "Couldn't create the planes\n");
 		ret = PTR_ERR(drv->layers);
-		goto free_drm;
+		goto cleanup_mode_config;
 	}
 
 	/* Create our CRTC */
@@ -152,7 +152,7 @@ static int sun4i_drv_bind(struct device *dev)
 	if (!drv->crtc) {
 		dev_err(drm->dev, "Couldn't create the CRTC\n");
 		ret = -EINVAL;
-		goto free_drm;
+		goto cleanup_mode_config;
 	}
 	drm->irq_enabled = true;
 
@@ -164,7 +164,7 @@ static int sun4i_drv_bind(struct device *dev)
 	if (IS_ERR(drv->fbdev)) {
 		dev_err(drm->dev, "Couldn't create our framebuffer\n");
 		ret = PTR_ERR(drv->fbdev);
-		goto free_drm;
+		goto cleanup_mode_config;
 	}
 
 	/* Enable connectors polling */
@@ -172,10 +172,16 @@ static int sun4i_drv_bind(struct device *dev)
 
 	ret = drm_dev_register(drm, 0);
 	if (ret)
-		goto free_drm;
+		goto finish_poll;
 
 	return 0;
 
+finish_poll:
+	drm_kms_helper_poll_fini(drm);
+	sun4i_framebuffer_free(drm);
+cleanup_mode_config:
+	drm_mode_config_cleanup(drm);
+	drm_vblank_cleanup(drm);
 free_drm:
 	drm_dev_unref(drm);
 	return ret;
-- 
2.11.0

Re: [PATCH 2/7] drm/sun4i: Fix up error path cleanup for master bind function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 762 bytes --]

On Fri, Feb 17, 2017 at 11:13:25AM +0800, Chen-Yu Tsai wrote:
> The master bind function calls numerous drm functions which initialize
> underlying structures. It also tries to bind the various components
> of the display pipeline, some of which may add additional drm objects.
> 
> This patch adds proper cleanup functions in the error path of the
> master bind function.
> 
> This requires the patch "drm/sun4i: Move drm_mode_config_cleanup call
> to main driver", which splits out drm_mode_config_cleanup from
> sun4i_framebuffer_free so we can call it separately.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH 3/7] drm/sun4i: Check return value of drm_vblank_init

[Chen-Yu Tsai]

drm_vblank_init can fail due to insufficient memory. Ignoring the error
and proceeding may cause the kernel to dereference an invalid pointer
when vblank is enabled.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_drv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 8e777167bca4..63c46643fdd1 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -130,7 +130,11 @@ static int sun4i_drv_bind(struct device *dev)
 	}
 	drm->dev_private = drv;
 
-	drm_vblank_init(drm, 1);
+	/* drm_vblank_init calls kcalloc, which can fail */
+	ret = drm_vblank_init(drm, 1);
+	if (ret)
+		goto free_drm;
+
 	drm_mode_config_init(drm);
 
 	ret = component_bind_all(drm->dev, drm);
-- 
2.11.0

Re: [PATCH 3/7] drm/sun4i: Check return value of drm_vblank_init

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

On Fri, Feb 17, 2017 at 11:13:26AM +0800, Chen-Yu Tsai wrote:
> drm_vblank_init can fail due to insufficient memory. Ignoring the error
> and proceeding may cause the kernel to dereference an invalid pointer
> when vblank is enabled.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH 4/7] drm/sun4i: Fix kcalloc element size in sun4i_layers_init

[Chen-Yu Tsai]

In sun4i_layers_init we are allocating an array of pointers to struct
sun4i_layer:

	layers = devm_kcalloc(drm->dev, ARRAY_SIZE(sun4i_backend_planes),
	                      sizeof(**layers), GFP_KERNEL);

The element size should be the size of an individual element of the
array. Change it to sizeof(*layers) to avoid wasting a lot of memory.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_layer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index 5d53c977bca5..62552a356d66 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -140,7 +140,7 @@ struct sun4i_layer **sun4i_layers_init(struct drm_device *drm)
 	int i;
 
 	layers = devm_kcalloc(drm->dev, ARRAY_SIZE(sun4i_backend_planes),
-			      sizeof(**layers), GFP_KERNEL);
+			      sizeof(*layers), GFP_KERNEL);
 	if (!layers)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.11.0

Re: [PATCH 4/7] drm/sun4i: Fix kcalloc element size in sun4i_layers_init

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On Fri, Feb 17, 2017 at 11:13:27AM +0800, Chen-Yu Tsai wrote:
> In sun4i_layers_init we are allocating an array of pointers to struct
> sun4i_layer:
> 
> 	layers = devm_kcalloc(drm->dev, ARRAY_SIZE(sun4i_backend_planes),
> 	                      sizeof(**layers), GFP_KERNEL);
> 
> The element size should be the size of an individual element of the
> array. Change it to sizeof(*layers) to avoid wasting a lot of memory.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime
-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH 5/7] drm/sun4i: Drop useless assignment in sun4i_layers_init

[Chen-Yu Tsai]

The assignment found in the main loop in sun4i_layers_init:

	struct sun4i_layer *layer = layers[i];

is useless as it gets overwritten by the next line:

	layer = sun4i_layer_init_one(drm, plane);

Drop the assignment.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_layer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index 62552a356d66..92ecc967dcb1 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -167,7 +167,7 @@ struct sun4i_layer **sun4i_layers_init(struct drm_device *drm)
 	 */
 	for (i = 0; i < ARRAY_SIZE(sun4i_backend_planes); i++) {
 		const struct sun4i_plane_desc *plane = &sun4i_backend_planes[i];
-		struct sun4i_layer *layer = layers[i];
+		struct sun4i_layer *layer;
 
 		layer = sun4i_layer_init_one(drm, plane);
 		if (IS_ERR(layer)) {
-- 
2.11.0

Re: [PATCH 5/7] drm/sun4i: Drop useless assignment in sun4i_layers_init

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 493 bytes --]

On Fri, Feb 17, 2017 at 11:13:28AM +0800, Chen-Yu Tsai wrote:
> The assignment found in the main loop in sun4i_layers_init:
> 
> 	struct sun4i_layer *layer = layers[i];
> 
> is useless as it gets overwritten by the next line:
> 
> 	layer = sun4i_layer_init_one(drm, plane);
> 
> Drop the assignment.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH 6/7] drm/sun4i: Save newly created layer in layers array in sun4i_layers_init

[Chen-Yu Tsai]

sun4i_layers_init allocates an array to store pointers to newly created
layers returned by sun4i_layer_init_one(), but fails to actually store
them. But it actually returns the empty array to unsuspecting users.

Save the pointers in the array, so that they may be used later.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_layer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index 92ecc967dcb1..41bc0f860f5c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -183,6 +183,7 @@ struct sun4i_layer **sun4i_layers_init(struct drm_device *drm)
 				   SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL(plane->pipe));
 
 		layer->id = i;
+		layers[i] = layer;
 	};
 
 	return layers;
-- 
2.11.0

Re: [PATCH 6/7] drm/sun4i: Save newly created layer in layers array in sun4i_layers_init

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]

On Fri, Feb 17, 2017 at 11:13:29AM +0800, Chen-Yu Tsai wrote:
> sun4i_layers_init allocates an array to store pointers to newly created
> layers returned by sun4i_layer_init_one(), but fails to actually store
> them. But it actually returns the empty array to unsuspecting users.
> 
> Save the pointers in the array, so that they may be used later.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH 7/7] drm/sun4i: Make sun4i_crtc_init return ERR_PTR style error codes

[Chen-Yu Tsai]

sun4i_crtc_init can fail for a number of reasons. Instead of returning
a NULL pointer when it fails, pass back the encountered error using
ERR_PTR.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_crtc.c | 4 ++--
 drivers/gpu/drm/sun4i/sun4i_drv.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c
index 4a192210574f..4e2e89c3104f 100644
--- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
@@ -121,7 +121,7 @@ struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm)
 
 	scrtc = devm_kzalloc(drm->dev, sizeof(*scrtc), GFP_KERNEL);
 	if (!scrtc)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 	scrtc->drv = drv;
 
 	ret = drm_crtc_init_with_planes(drm, &scrtc->crtc,
@@ -131,7 +131,7 @@ struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm)
 					NULL);
 	if (ret) {
 		dev_err(drm->dev, "Couldn't init DRM CRTC\n");
-		return NULL;
+		return ERR_PTR(ret);
 	}
 
 	drm_crtc_helper_add(&scrtc->crtc, &sun4i_crtc_helper_funcs);
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 63c46643fdd1..fc6ef4066c59 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -153,9 +153,9 @@ static int sun4i_drv_bind(struct device *dev)
 
 	/* Create our CRTC */
 	drv->crtc = sun4i_crtc_init(drm);
-	if (!drv->crtc) {
+	if (IS_ERR(drv->crtc)) {
 		dev_err(drm->dev, "Couldn't create the CRTC\n");
-		ret = -EINVAL;
+		ret = PTR_ERR(drv->crtc);
 		goto cleanup_mode_config;
 	}
 	drm->irq_enabled = true;
-- 
2.11.0

Re: [PATCH 7/7] drm/sun4i: Make sun4i_crtc_init return ERR_PTR style error codes

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

On Fri, Feb 17, 2017 at 11:13:30AM +0800, Chen-Yu Tsai wrote:
> sun4i_crtc_init can fail for a number of reasons. Instead of returning
> a NULL pointer when it fails, pass back the encountered error using
> ERR_PTR.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]