[PATCH 0/8] video: fbdev: au1200fb: Fix error handling path of 'au1200fb_drv_probe()'

[PATCH 0/8] video: fbdev: au1200fb: Fix error handling path of 'au1200fb_drv_probe()'

[Christophe JAILLET]

This patch serie tries to fix several issues found in the error handling
code of 'au1200fb_drv_probe()'.
The 5 first patches fixes various issues (double free, missing error code,
un-released resources on error, incorrect IRQ releasing and incomplete
error handling path)

The 3 last patches are just cleanups.


I've spilt the serie in 8 steps that look logical to me. They could
also be merged together if preferred.


These patches are provided as-is and ARE NOT even compile-tested (sorry in
advance if a patch is broken) because I don't have a cross compiler for MIPS
and won't install one.


This serie already goes further that the fixes I usually provide, so
please excuse me if I missed something or if it is somehow broken and/or
incomplete.

---
V1 previously posted is patch 3/8 of this serie

Christophe JAILLET (8):
  video: fbdev: au1200fb: Fix a potential double free
  video: fbdev: au1200fb: Return an error code if a memory allocation
    fails
  video: fbdev: au1200fb: Release some resources if a memory allocation
    fails
  video: fbdev: au1200fb: Fix error handling path
  video: fbdev: au1200fb: Fix error handling path
  video: fbdev: au1200fb: Remove some dead code
  video: fbdev: au1200fb: Propagate an error code
  video: fbdev: au1200fb: Style clean up

 drivers/video/fbdev/au1200fb.c | 43 ++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

-- 
2.11.0

[PATCH 1/8] video: fbdev: au1200fb: Fix a potential double free

[Christophe JAILLET]

If 'fb_alloc_cmap()' fails, 'fbi->pseudo_palette' is freed and an error
code is returned by 'au1200fb_init_fbinfo()'.
The only caller, 'au1200fb_drv_probe()' goes to an error handling path
where resources allocated in 'fb_alloc_cmap()' are freed.
This leads to a double free of 'fbi->pseudo_palette'.

Fix it by letting the caller free all resources in case of failure in
'au1200fb_init_fbinfo()'.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/video/fbdev/au1200fb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
index 5f04b4096c42..a5facc2ad90b 100644
--- a/drivers/video/fbdev/au1200fb.c
+++ b/drivers/video/fbdev/au1200fb.c
@@ -1553,7 +1553,6 @@ static int au1200fb_init_fbinfo(struct au1200fb_device *fbdev)
 	if (fb_alloc_cmap(&fbi->cmap, AU1200_LCD_NBR_PALETTE_ENTRIES, 0) < 0) {
 		print_err("Fail to allocate colormap (%d entries)",
 			   AU1200_LCD_NBR_PALETTE_ENTRIES);
-		kfree(fbi->pseudo_palette);
 		return -EFAULT;
 	}
 
-- 
2.11.0

[PATCH 2/8] video: fbdev: au1200fb: Return an error code if a memory allocation fails

[Christophe JAILLET]

'ret' is known to be 0 at this point.
In case of memory allocation error in 'framebuffer_alloc()', return
-ENOMEM instead.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/video/fbdev/au1200fb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
index a5facc2ad90b..7fa41026984d 100644
--- a/drivers/video/fbdev/au1200fb.c
+++ b/drivers/video/fbdev/au1200fb.c
@@ -1680,8 +1680,10 @@ static int au1200fb_drv_probe(struct platform_device *dev)
 
 		fbi = framebuffer_alloc(sizeof(struct au1200fb_device),
 					&dev->dev);
-		if (!fbi)
+		if (!fbi) {
+			ret = -ENOMEM;
 			goto failed;
+		}
 
 		_au1200fb_infos[plane] = fbi;
 		fbdev = fbi->par;
-- 
2.11.0

[PATCH 3/8] video: fbdev: au1200fb: Release some resources if a memory allocation fails

[Christophe JAILLET]

We should go through the error handling code instead of returning -ENOMEM
directly.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/video/fbdev/au1200fb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
index 7fa41026984d..cf54168d44dc 100644
--- a/drivers/video/fbdev/au1200fb.c
+++ b/drivers/video/fbdev/au1200fb.c
@@ -1702,7 +1702,8 @@ static int au1200fb_drv_probe(struct platform_device *dev)
 		if (!fbdev->fb_mem) {
 			print_err("fail to allocate frambuffer (size: %dK))",
 				  fbdev->fb_len / 1024);
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto failed;
 		}
 
 		/*
-- 
2.11.0

[PATCH 4/8] video: fbdev: au1200fb: Fix error handling path

[Christophe JAILLET]

'au1200fb_drv_probe()' can not fail after a succesful call to
'request_irq()'. So there is no point to call 'free_irq()' in the error
handling path.

Moreover, the hard coded AU1200_LCD_INT looks boggus since commit
1630d85a8312 ("au1200fb: fix hardcoded IRQ")

So, remove it.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/video/fbdev/au1200fb.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
index cf54168d44dc..0d8ed0ef9183 100644
--- a/drivers/video/fbdev/au1200fb.c
+++ b/drivers/video/fbdev/au1200fb.c
@@ -1766,8 +1766,6 @@ static int au1200fb_drv_probe(struct platform_device *dev)
 			fb_dealloc_cmap(&fbi->cmap);
 		kfree(fbi->pseudo_palette);
 	}
-	if (plane == 0)
-		free_irq(AU1200_LCD_INT, (void*)dev);
 	return ret;
 }
 
-- 
2.11.0

[PATCH 5/8] video: fbdev: au1200fb: Fix error handling path

[Christophe JAILLET]

Rewrite the exit path based on 'au1200fb_drv_remove()'.
We can safely iterate for all already handled planes. Even if not
completely initialized, the functions that are called will silently accept
the 'fb_info' structure that is passed.

As soon as we find a NULL in the '_au1200fb_infos' array, we know that we
have released all what we needed to release. So we can 'break'.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/video/fbdev/au1200fb.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
index 0d8ed0ef9183..e531543bc707 100644
--- a/drivers/video/fbdev/au1200fb.c
+++ b/drivers/video/fbdev/au1200fb.c
@@ -1760,11 +1760,19 @@ static int au1200fb_drv_probe(struct platform_device *dev)
 	return 0;
 
 failed:
-	/* NOTE: This only does the current plane/window that failed; others are still active */
-	if (fbi) {
+	for (plane = 0; plane < device_count; ++plane) {
+		fbi = _au1200fb_infos[plane];
+		if (!fbi)
+			break;
+
+		/* Clean up all probe data */
+		unregister_framebuffer(fbi);
 		if (fbi->cmap.len != 0)
 			fb_dealloc_cmap(&fbi->cmap);
 		kfree(fbi->pseudo_palette);
+
+		framebuffer_release(fbi);
+		_au1200fb_infos[plane] = NULL;
 	}
 	return ret;
 }
-- 
2.11.0

Re: [PATCH 5/8] video: fbdev: au1200fb: Fix error handling path

[walter harms]

Am 16.10.2017 21:04, schrieb Christophe JAILLET:
> Rewrite the exit path based on 'au1200fb_drv_remove()'.
> We can safely iterate for all already handled planes. Even if not
> completely initialized, the functions that are called will silently accept
> the 'fb_info' structure that is passed.
> 
> As soon as we find a NULL in the '_au1200fb_infos' array, we know that we
> have released all what we needed to release. So we can 'break'.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/video/fbdev/au1200fb.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
> index 0d8ed0ef9183..e531543bc707 100644
> --- a/drivers/video/fbdev/au1200fb.c
> +++ b/drivers/video/fbdev/au1200fb.c
> @@ -1760,11 +1760,19 @@ static int au1200fb_drv_probe(struct platform_device *dev)
>  	return 0;
>  
>  failed:
> -	/* NOTE: This only does the current plane/window that failed; others are still active */
> -	if (fbi) {
> +	for (plane = 0; plane < device_count; ++plane) {
> +		fbi = _au1200fb_infos[plane];
> +		if (!fbi)
> +			break;
> +
> +		/* Clean up all probe data */
> +		unregister_framebuffer(fbi);
>  		if (fbi->cmap.len != 0)
>  			fb_dealloc_cmap(&fbi->cmap);

While you are here ... fb_dealloc_cmap() is a collection of free's()
the check for fbi->cmap.len can be omitted (or moved to fb_dealloc_cmap).

re,
 wh

>  		kfree(fbi->pseudo_palette);
> +
> +		framebuffer_release(fbi);
> +		_au1200fb_infos[plane] = NULL;
>  	}
>  	return ret;
>  }

[PATCH 6/8] video: fbdev: au1200fb: Remove some dead code

[Christophe JAILLET]

There is no need to shut gcc up. It should not complain.
Axe 'fbdev', it is never used in this function.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/video/fbdev/au1200fb.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
index e531543bc707..970ce761ff89 100644
--- a/drivers/video/fbdev/au1200fb.c
+++ b/drivers/video/fbdev/au1200fb.c
@@ -1667,10 +1667,6 @@ static int au1200fb_drv_probe(struct platform_device *dev)
 	printk(DRIVER_NAME ": Panel %d %s\n", panel_index, panel->name);
 	printk(DRIVER_NAME ": Win %d %s\n", window_index, win->name);
 
-	/* shut gcc up */
-	ret = 0;
-	fbdev = NULL;
-
 	for (plane = 0; plane < device_count; ++plane) {
 		bpp = winbpp(win->w[plane].mode_winctrl1);
 		if (win->w[plane].xres == 0)
@@ -1780,7 +1776,6 @@ static int au1200fb_drv_probe(struct platform_device *dev)
 static int au1200fb_drv_remove(struct platform_device *dev)
 {
 	struct au1200fb_platdata *pd = platform_get_drvdata(dev);
-	struct au1200fb_device *fbdev;
 	struct fb_info *fbi;
 	int plane;
 
@@ -1789,7 +1784,6 @@ static int au1200fb_drv_remove(struct platform_device *dev)
 
 	for (plane = 0; plane < device_count; ++plane)	{
 		fbi = _au1200fb_infos[plane];
-		fbdev = fbi->par;
 
 		/* Clean up all probe data */
 		unregister_framebuffer(fbi);
-- 
2.11.0

Re: [PATCH 6/8] video: fbdev: au1200fb: Remove some dead code

[Dan Carpenter]

On Mon, Oct 16, 2017 at 09:04:52PM +0200, Christophe JAILLET wrote:
> There is no need to shut gcc up. It should not complain.
> Axe 'fbdev', it is never used in this function.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/video/fbdev/au1200fb.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
> index e531543bc707..970ce761ff89 100644
> --- a/drivers/video/fbdev/au1200fb.c
> +++ b/drivers/video/fbdev/au1200fb.c
> @@ -1667,10 +1667,6 @@ static int au1200fb_drv_probe(struct platform_device *dev)
>  	printk(DRIVER_NAME ": Panel %d %s\n", panel_index, panel->name);
>  	printk(DRIVER_NAME ": Win %d %s\n", window_index, win->name);
>  
> -	/* shut gcc up */
> -	ret = 0;

Heh...  It looks like they were deliberately silencing the warning about
the bug you fixed in patch 2/8.  :P

regards,
dan carpenter

[PATCH 7/8] video: fbdev: au1200fb: Propagate an error code

[Christophe JAILLET]

We should propagate the error code returned by 'fb_alloc_cmap()' instead
of returning -EFAULT.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/video/fbdev/au1200fb.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
index 970ce761ff89..687ea2a8f810 100644
--- a/drivers/video/fbdev/au1200fb.c
+++ b/drivers/video/fbdev/au1200fb.c
@@ -1518,7 +1518,7 @@ static irqreturn_t au1200fb_handle_irq(int irq, void* dev_id)
 static int au1200fb_init_fbinfo(struct au1200fb_device *fbdev)
 {
 	struct fb_info *fbi = fbdev->fb_info;
-	int bpp;
+	int bpp, ret;
 
 	fbi->fbops = &au1200fb_fb_ops;
 
@@ -1550,10 +1550,11 @@ static int au1200fb_init_fbinfo(struct au1200fb_device *fbdev)
 		return -ENOMEM;
 	}
 
-	if (fb_alloc_cmap(&fbi->cmap, AU1200_LCD_NBR_PALETTE_ENTRIES, 0) < 0) {
+	ret = fb_alloc_cmap(&fbi->cmap, AU1200_LCD_NBR_PALETTE_ENTRIES, 0);
+	if (ret < 0) {
 		print_err("Fail to allocate colormap (%d entries)",
 			   AU1200_LCD_NBR_PALETTE_ENTRIES);
-		return -EFAULT;
+		return ret;
 	}
 
 	strncpy(fbi->fix.id, "AU1200", sizeof(fbi->fix.id));
-- 
2.11.0

[PATCH 8/8] video: fbdev: au1200fb: Style clean up

[Christophe JAILLET]

Style clean-up.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/video/fbdev/au1200fb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
index 687ea2a8f810..87d5a62bf6ca 100644
--- a/drivers/video/fbdev/au1200fb.c
+++ b/drivers/video/fbdev/au1200fb.c
@@ -1546,14 +1546,13 @@ static int au1200fb_init_fbinfo(struct au1200fb_device *fbdev)
 	}
 
 	fbi->pseudo_palette = kcalloc(16, sizeof(u32), GFP_KERNEL);
-	if (!fbi->pseudo_palette) {
+	if (!fbi->pseudo_palette)
 		return -ENOMEM;
-	}
 
 	ret = fb_alloc_cmap(&fbi->cmap, AU1200_LCD_NBR_PALETTE_ENTRIES, 0);
 	if (ret < 0) {
 		print_err("Fail to allocate colormap (%d entries)",
-			   AU1200_LCD_NBR_PALETTE_ENTRIES);
+			  AU1200_LCD_NBR_PALETTE_ENTRIES);
 		return ret;
 	}
 
@@ -1717,7 +1716,8 @@ static int au1200fb_drv_probe(struct platform_device *dev)
 		print_dbg("phys=0x%08x, size=%dK", fbdev->fb_phys, fbdev->fb_len / 1024);
 
 		/* Init FB data */
-		if ((ret = au1200fb_init_fbinfo(fbdev)) < 0)
+		ret = au1200fb_init_fbinfo(fbdev);
+		if (ret < 0)
 			goto failed;
 
 		/* Register new framebuffer */
-- 
2.11.0

Re: [PATCH 0/8] video: fbdev: au1200fb: Fix error handling path of 'au1200fb_drv_probe()'

[Bartlomiej Zolnierkiewicz]

On Monday, October 16, 2017 09:04:46 PM Christophe JAILLET wrote:
> This patch serie tries to fix several issues found in the error handling
> code of 'au1200fb_drv_probe()'.
> The 5 first patches fixes various issues (double free, missing error code,
> un-released resources on error, incorrect IRQ releasing and incomplete
> error handling path)
> 
> The 3 last patches are just cleanups.
> 
> 
> I've spilt the serie in 8 steps that look logical to me. They could
> also be merged together if preferred.
> 
> 
> These patches are provided as-is and ARE NOT even compile-tested (sorry in
> advance if a patch is broken) because I don't have a cross compiler for MIPS
> and won't install one.
> 
> 
> This serie already goes further that the fixes I usually provide, so
> please excuse me if I missed something or if it is somehow broken and/or
> incomplete.
> 
> ---
> V1 previously posted is patch 3/8 of this serie
> 
> Christophe JAILLET (8):
>   video: fbdev: au1200fb: Fix a potential double free
>   video: fbdev: au1200fb: Return an error code if a memory allocation
>     fails
>   video: fbdev: au1200fb: Release some resources if a memory allocation
>     fails
>   video: fbdev: au1200fb: Fix error handling path
>   video: fbdev: au1200fb: Fix error handling path
>   video: fbdev: au1200fb: Remove some dead code
>   video: fbdev: au1200fb: Propagate an error code
>   video: fbdev: au1200fb: Style clean up
> 
>  drivers/video/fbdev/au1200fb.c | 43 ++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)

Thanks, I queued all patches for 4.15.

I also did some minor fixes to the patch #4 while merging it:

* patch summary was changed to "video: fbdev: au1200fb: Fix incorrect IRQ
  freeing") to be different from the patch summary of patch #5

* patch description was changed to silence issues reported by checkpatch.pl:

WARNING: 'succesful' may be misspelled - perhaps 'successful'?
#4: 
'au1200fb_drv_probe()' can not fail after a succesful call to

ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 1630d85a8312 ("au1200fb: fix hardcoded IRQ")'
#9: 
1630d85a8312 ("au1200fb: fix hardcoded IRQ")

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics