LKML Archive on
help / color / mirror / Atom feed
* [PATCH][RESEND] Fix several memory leaks in cr_backlight_probe() - take2
       [not found] ` <1185116146.6819.29.camel@localhost.localdomain>
@ 2007-08-04 18:30   ` Jesper Juhl
  0 siblings, 0 replies; only message in thread
From: Jesper Juhl @ 2007-08-04 18:30 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Richard Purdie, Alan Hourihane, Thomas Hellstrom,
	Linux Kernel Mailing List, Jesper Juhl

This is a resend of a patch I originally submitted on 23/7-2007 23:32. 
That patch was an updated version of one originally submitted on 21-Jul-2007 00:56.

The original patch made a brief appearance in -mm, but was dropped 
due to concerns raised by Richard Purdie that it didn't fix all issues.

Here are Richard's comments:
> The problems you've spotted are valid however the error paths are still
> broken both before and after this patch since there is no call to
> backlight_device_unregister() and/or lcd_device_unregister() in the
> probe function.

The following patch fixes the same problems as the original and also 
tries to fix the stuff that Richard pointed out.
Hopefully this time it is perfect :-)

Here's the new version of the patch : 

After fixing the too small memory allocation in cr_backlight_probe()
from drivers/video/backlight/cr_bllcd.c
(commit e3bbb3f05339de438faf54124f25c92e6fe4ac2e) I noticed that the
Coverity checker also thought there were a few memory leaks in there.
I took a closer look and confirmed that there were indeed several

At the start of the function we allocate storage for a
'struct cr_panel' and store the pointer in a variable named 'crp'.

Then we call pci_get_device() and pci_read_config_byte() and if
either of them fail we return without freeing the memory allocated
for the 'struct cr_panel'. These two leaks are easy to fix since we
don't even use 'crp' for anything up to this point, so I simply
moved the allocation further down in the function so it only happens
just before we actually need it.

A bit further down we call backlight_device_register() and store the
result in 'crp->cr_backlight_device'. In case of error we return
'crp->cr_backlight_device' from the function, thus leaking 'crp'
itself. The same thing happens with the call to lcd_device_register().
To fix these two leaks I declare two new pointers to hold the return
values, so that in case of error we can return the pointer (as before)
but without leaking 'crp'.

This version of the patch also adds missing 
backlight_device_unregister() / lcd_device_unregister() / pci_dev_put() 
calls to error paths.
  Thanks to Richard Purdie <> for noticing.

Please consider merging.

Note: Due to lack of hardware the patch has only been compile tested.

Signed-off-by: Jesper Juhl <>

 drivers/video/backlight/cr_bllcd.c |   35 ++++++++++++++++++++---------------
 1 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/video/backlight/cr_bllcd.c b/drivers/video/backlight/cr_bllcd.c
index b7904da..92e201e 100644
--- a/drivers/video/backlight/cr_bllcd.c
+++ b/drivers/video/backlight/cr_bllcd.c
@@ -171,13 +171,11 @@ static struct lcd_ops cr_lcd_ops = {
 static int cr_backlight_probe(struct platform_device *pdev)
+	struct backlight_device *bdp;
+	struct lcd_device *ldp;
 	struct cr_panel *crp;
 	u8 dev_en;
-	crp = kzalloc(sizeof(*crp), GFP_KERNEL);
-	if (crp == NULL)
-		return -ENOMEM;
 	lpc_dev = pci_get_device(PCI_VENDOR_ID_INTEL,
 	if (!lpc_dev) {
@@ -193,27 +191,34 @@ static int cr_backlight_probe(struct platform_device *pdev)
 		return -ENODEV;
-	crp->cr_backlight_device = backlight_device_register("cr-backlight",
-							     &pdev->dev, NULL,
-							     &cr_backlight_ops);
-	if (IS_ERR(crp->cr_backlight_device)) {
+	bdp = backlight_device_register("cr-backlight",
+					&pdev->dev, NULL, &cr_backlight_ops);
+	if (IS_ERR(bdp)) {
-		return PTR_ERR(crp->cr_backlight_device);
+		return PTR_ERR(bdp);
-	crp->cr_lcd_device = lcd_device_register("cr-lcd",
-							&pdev->dev, NULL,
-							&cr_lcd_ops);
-	if (IS_ERR(crp->cr_lcd_device)) {
+	ldp = lcd_device_register("cr-lcd", &pdev->dev, NULL, &cr_lcd_ops);
+	if (IS_ERR(ldp)) {
+		backlight_device_unregister(bdp);
-		return PTR_ERR(crp->cr_backlight_device);
+		return PTR_ERR(bdp);
 	pci_read_config_dword(lpc_dev, CRVML_REG_GPIOBAR,
 	gpio_bar &= ~0x3F;
+	crp = kzalloc(sizeof(*crp), GFP_KERNEL);
+	if (!crp) {
+		lcd_device_unregister(ldp);
+		backlight_device_unregister(bdp);
+		pci_dev_put(lpc_dev);
+		return -ENOMEM;
+	}
+	crp->cr_backlight_device = bdp;
+	crp->cr_lcd_device = ldp;
 	crp->cr_backlight_device->props.power = FB_BLANK_UNBLANK;
 	crp->cr_backlight_device->props.brightness = 0;
 	crp->cr_backlight_device->props.max_brightness = 0;

^ permalink raw reply	[flat|nested] only message in thread

only message in thread, other threads:[~2007-08-04 18:32 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <>
     [not found] ` <1185116146.6819.29.camel@localhost.localdomain>
2007-08-04 18:30   ` [PATCH][RESEND] Fix several memory leaks in cr_backlight_probe() - take2 Jesper Juhl

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).