Re: [PATCH] Samsung backlight driver

[Dmitry Torokhov]

On Fri, Aug 14, 2009 at 03:48:35PM -0700, Greg KH wrote:
> Here's an updated version, with two more laptop models supported, and
> the comments from Dmitry added in.
> 

You are leaking pci device reference in error unwinding path....

Here you go, just fold it together with yours.

-- 
Dmitry

Fix PCI device reference leak in error handling path; move more code
into __init/__exit sections; simplify searching for video card and
use less globals.

Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx>
---

 drivers/platform/x86/samsung-backlight.c |  100 +++++++++++-------------------
 1 files changed, 36 insertions(+), 64 deletions(-)


diff --git a/drivers/platform/x86/samsung-backlight.c b/drivers/platform/x86/samsung-backlight.c
index accc20e..8df5fc5 100644
--- a/drivers/platform/x86/samsung-backlight.c
+++ b/drivers/platform/x86/samsung-backlight.c
@@ -21,23 +21,24 @@
 #define MAX_BRIGHT	0xff
 #define OFFSET		0xf4
 
+static int offset = OFFSET;
+module_param(offset, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(offset, "The offset into the PCI device for the brightness control");
+
 static struct pci_dev *pci_device;
 static struct backlight_device *backlight_device;
-static int offset = OFFSET;
-static u8 current_brightness;
 
-static void read_brightness(void)
+static u8 read_brightness(void)
 {
-	if (!pci_device)
-		return;
-	pci_read_config_byte(pci_device, offset, &current_brightness);
+	u8 brightness;
+
+	pci_read_config_byte(pci_device, offset, &brightness);
+	return brightness;
 }
 
-static void set_brightness(void)
+static void set_brightness(u8 brightness)
 {
-	if (!pci_device)
-		return;
-	pci_write_config_byte(pci_device, offset, current_brightness);
+	pci_write_config_byte(pci_device, offset, brightness);
 }
 
 static int get_brightness(struct backlight_device *bd)
@@ -47,11 +48,7 @@ static int get_brightness(struct backlight_device *bd)
 
 static int update_status(struct backlight_device *bd)
 {
-	if (!pci_device)
-		return -ENODEV;
-
-	current_brightness = bd->props.brightness;
-	set_brightness();
+	set_brightness(bd->props.brightness);
 	return 0;
 }
 
@@ -60,51 +57,7 @@ static struct backlight_ops backlight_ops = {
 	.update_status	= update_status,
 };
 
-static int find_video_card(void)
-{
-	struct pci_dev *dev = NULL;
-
-	while ((dev = pci_get_device(0x8086, 0x27ae, dev)) != NULL) {
-		/*
-		 * Found one, so let's save it off and break
-		 * Note that the reference is still raised on
-		 * the PCI device here.
-		 */
-		pci_device = dev;
-		break;
-	}
-
-	if (!pci_device)
-		return -ENODEV;
-
-	/* create a backlight device to talk to this one */
-	backlight_device = backlight_device_register("samsung",
-						     &pci_device->dev,
-						     NULL, &backlight_ops);
-	if (IS_ERR(backlight_device))
-		return PTR_ERR(backlight_device);
-	read_brightness();
-	backlight_device->props.max_brightness = MAX_BRIGHT;
-	backlight_device->props.brightness = current_brightness;
-	backlight_device->props.power = FB_BLANK_UNBLANK;
-	backlight_update_status(backlight_device);
-	return 0;
-}
-
-static void remove_video_card(void)
-{
-	if (!pci_device)
-		return;
-
-	backlight_device_unregister(backlight_device);
-	backlight_device = NULL;
-
-	/* we are done with the PCI device, put it back */
-	pci_dev_put(pci_device);
-	pci_device = NULL;
-}
-
-static int dmi_check_cb(const struct dmi_system_id *id)
+static int __init dmi_check_cb(const struct dmi_system_id *id)
 {
 	printk(KERN_INFO KBUILD_MODNAME ": found laptop model '%s'\n",
 		id->ident);
@@ -147,12 +100,33 @@ static int __init samsung_init(void)
 	if (!dmi_check_system(samsung_dmi_table))
 		return -ENODEV;
 
-	return find_video_card();
+	pci_device = pci_get_device(0x8086, 0x27ae, NULL);
+	if (!pci_device)
+		return -ENODEV;
+
+	/* create a backlight device to talk to this one */
+	backlight_device = backlight_device_register("samsung",
+						     &pci_device->dev,
+						     NULL, &backlight_ops);
+	if (IS_ERR(backlight_device)) {
+		pci_dev_put(pci_device);
+		return PTR_ERR(backlight_device);
+	}
+
+	backlight_device->props.max_brightness = MAX_BRIGHT;
+	backlight_device->props.brightness = read_brightness();
+	backlight_device->props.power = FB_BLANK_UNBLANK;
+	backlight_update_status(backlight_device);
+
+	return 0;
 }
 
 static void __exit samsung_exit(void)
 {
-	remove_video_card();
+	backlight_device_unregister(backlight_device);
+
+	/* we are done with the PCI device, put it back */
+	pci_dev_put(pci_device);
 }
 
 module_init(samsung_init);
@@ -161,8 +135,6 @@ module_exit(samsung_exit);
 MODULE_AUTHOR("Greg Kroah-Hartman <gregkh@xxxxxxx>");
 MODULE_DESCRIPTION("Samsung Backlight driver");
 MODULE_LICENSE("GPL");
-module_param(offset, int, S_IRUGO | S_IWUSR);
-MODULE_PARM_DESC(offset, "The offset into the PCI device for the brightness control");
 MODULE_ALIAS("dmi:*:svnSAMSUNGELECTRONICSCO.,LTD.:pnN130:*:rnN130:*");
 MODULE_ALIAS("dmi:*:svnSAMSUNGELECTRONICSCO.,LTD.:pnNC10:*:rnNC10:*");
 MODULE_ALIAS("dmi:*:svnSAMSUNGELECTRONICSCO.,LTD.:pnSQ45S70S:*:rnSQ45S70S:*");
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/