Backlight & lcd fixes/cleanups

PATCH In review Dmitry Torokhov 4 participants 19 messages Aug 11, 2006 – Aug 29, 2006

Subsystems (4)

USB Framebuffer layer Backlight class/subsystem Linux for power macintosh

lore

Share & tools

lore

Dmitry Torokhov

author

2006-08-11 05:09 UTC #0 lore raw

cover Backlight & lcd fixes/cleanups

Hi Richard,

I was looking into intergating backlight support into acpi video driver (went nowhere so far) and I stumbled across some issues in backlight/lcd core support which this patch series tries to address.

Please review and consider applying. Compile-tested only.

--
Dmitry

Dmitry Torokhov

author

2006-08-11 05:09 UTC #1 lore raw

PATCH 1/6 Convert to use default class device attributes

Backlight: convert to use default class device attributes

Use provided by the driver core method of creating set of default attributes for all devices belonging to a given class instead of using homegrown code.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/video/backlight/backlight.c |   49 ++++++++----------------------------
 drivers/video/backlight/lcd.c       |   44 +++++++-------------------------
 2 files changed, 22 insertions(+), 71 deletions(-)

drivers/video/backlight/backlight.c +12 ~~-37~~

Index: work/drivers/video/backlight/backlight.c
===================================================================
@@ -130,25 +130,19 @@ static void backlight_class_release(stru
 	kfree(bd);
 }
 
-static struct class backlight_class = {
-	.name = "backlight",
-	.release = backlight_class_release,
+static struct class_device_attribute bl_class_device_attributes[] = {
+	__ATTR(power, 0644, backlight_show_power, backlight_store_power),
+	__ATTR(brightness, 0644,
+		backlight_show_brightness, backlight_store_brightness),
+	__ATTR(actual_brightness, 0444, backlight_show_actual_brightness, NULL),
+	__ATTR(max_brightness, 0444, backlight_show_max_brightness, NULL),
+	__ATTR_NULL
 };
 
-#define DECLARE_ATTR(_name,_mode,_show,_store)			\
-{							 	\
-	.attr	= { .name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },	\
-	.show	= _show,					\
-	.store	= _store,					\
-}
-
-static struct class_device_attribute bl_class_device_attributes[] = {
-	DECLARE_ATTR(power, 0644, backlight_show_power, backlight_store_power),
-	DECLARE_ATTR(brightness, 0644, backlight_show_brightness,
-		     backlight_store_brightness),
-	DECLARE_ATTR(actual_brightness, 0444, backlight_show_actual_brightness,
-		     NULL),
-	DECLARE_ATTR(max_brightness, 0444, backlight_show_max_brightness, NULL),
+static struct class backlight_class = {
+	.name			= "backlight",
+	.release		= backlight_class_release,
+	.class_dev_attrs	= bl_class_device_attributes,
 };
 
 /* This callback gets called when something important happens inside a

@@ -193,7 +187,7 @@ static int fb_notifier_callback(struct n
 struct backlight_device *backlight_device_register(const char *name, void *devdata,
 						   struct backlight_properties *bp)
 {
-	int i, rc;
+	int rc;
 	struct backlight_device *new_bd;
 
 	pr_debug("backlight_device_alloc: name=%s\n", name);

@@ -222,19 +216,6 @@ error:		kfree(new_bd);
 	if (unlikely(rc))
 		goto error;
 
-	for (i = 0; i < ARRAY_SIZE(bl_class_device_attributes); i++) {
-		rc = class_device_create_file(&new_bd->class_dev,
-					      &bl_class_device_attributes[i]);
-		if (unlikely(rc)) {
-			while (--i >= 0)
-				class_device_remove_file(&new_bd->class_dev,
-							 &bl_class_device_attributes[i]);
-			class_device_unregister(&new_bd->class_dev);
-			/* No need to kfree(new_bd) since release() method was called */
-			return ERR_PTR(rc);
-		}
-	}
-
 	return new_bd;
 }
 EXPORT_SYMBOL(backlight_device_register);

@@ -247,17 +228,11 @@ EXPORT_SYMBOL(backlight_device_register)
  */
 void backlight_device_unregister(struct backlight_device *bd)
 {
-	int i;
-
 	if (!bd)
 		return;
 
 	pr_debug("backlight_device_unregister: name=%s\n", bd->class_dev.class_id);
 
-	for (i = 0; i < ARRAY_SIZE(bl_class_device_attributes); i++)
-		class_device_remove_file(&bd->class_dev,
-					 &bl_class_device_attributes[i]);
-
 	down(&bd->sem);
 	if (likely(bd->props && bd->props->update_status)) {
 		bd->props->brightness = 0;

drivers/video/backlight/lcd.c +10 ~~-34~~

75 lines not on this page · raw message

Dmitry Torokhov

author

2006-08-11 05:09 UTC #2 lore raw

PATCH 2/6 Fix error handling when registering new device

In reply to Dmitry Torokhov

Backlight: fix error handling when registering new device

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/video/backlight/backlight.c |    8 +++++---
 drivers/video/backlight/lcd.c       |    8 +++++---
 2 files changed, 10 insertions(+), 6 deletions(-)

drivers/video/backlight/backlight.c +5 -3

Index: work/drivers/video/backlight/backlight.c
===================================================================
@@ -205,7 +205,7 @@ struct backlight_device *backlight_devic
 
 	rc = class_device_register(&new_bd->class_dev);
 	if (unlikely(rc)) {
-error:		kfree(new_bd);
+		kfree(new_bd);
 		return ERR_PTR(rc);
 	}

@@ -213,8 +213,10 @@ error:		kfree(new_bd);
 	new_bd->fb_notif.notifier_call = fb_notifier_callback;
 
 	rc = fb_register_client(&new_bd->fb_notif);
-	if (unlikely(rc))
-		goto error;
+	if (unlikely(rc)) {
+		class_device_unregister(&new_bd->class_dev);
+		return ERR_PTR(rc);
+	}
 
 	return new_bd;
 }

drivers/video/backlight/lcd.c +5 -3

Index: work/drivers/video/backlight/lcd.c
===================================================================
@@ -177,7 +177,7 @@ struct lcd_device *lcd_device_register(c
 
 	rc = class_device_register(&new_ld->class_dev);
 	if (unlikely(rc)) {
-error:		kfree(new_ld);
+		kfree(new_ld);
 		return ERR_PTR(rc);
 	}

@@ -185,8 +185,10 @@ error:		kfree(new_ld);
 	new_ld->fb_notif.notifier_call = fb_notifier_callback;
 
 	rc = fb_register_client(&new_ld->fb_notif);
-	if (unlikely(rc))
-		goto error;
+	if (unlikely(rc)) {
+		class_device_unregister(&new_ld->class_dev);
+		return ERR_PTR(rc);
+	}
 
 	return new_ld;
 }

Dmitry Torokhov

author

2006-08-11 05:09 UTC #3 lore raw

PATCH 3/6 Get rid of excessive amount of likely()s

In reply to Dmitry Torokhov

Backlight: get rid of excessive amount of likely()s

There are no hot paths in the backlight core; do not litter the code with likely()s and rely on compiler to do the right thing.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/video/backlight/backlight.c |   45 +++++++++++++++++-------------------
 drivers/video/backlight/lcd.c       |   37 ++++++++++++-----------------
 2 files changed, 38 insertions(+), 44 deletions(-)

drivers/video/backlight/backlight.c +22 ~~-23~~

Index: work/drivers/video/backlight/backlight.c
===================================================================
@@ -20,7 +20,7 @@ static ssize_t backlight_show_power(stru
 	struct backlight_device *bd = to_backlight_device(cdev);
 
 	down(&bd->sem);
-	if (likely(bd->props))
+	if (bd->props)
 		rc = sprintf(buf, "%d\n", bd->props->power);
 	up(&bd->sem);

@@ -41,10 +41,10 @@ static ssize_t backlight_store_power(str
 		return -EINVAL;
 
 	down(&bd->sem);
-	if (likely(bd->props)) {
+	if (bd->props) {
 		pr_debug("backlight: set power to %d\n", power);
 		bd->props->power = power;
-		if (likely(bd->props->update_status))
+		if (bd->props->update_status)
 			bd->props->update_status(bd);
 		rc = count;
 	}

@@ -59,7 +59,7 @@ static ssize_t backlight_show_brightness
 	struct backlight_device *bd = to_backlight_device(cdev);
 
 	down(&bd->sem);
-	if (likely(bd->props))
+	if (bd->props)
 		rc = sprintf(buf, "%d\n", bd->props->brightness);
 	up(&bd->sem);

@@ -80,14 +80,14 @@ static ssize_t backlight_store_brightnes
 		return -EINVAL;
 
 	down(&bd->sem);
-	if (likely(bd->props)) {
+	if (bd->props) {
 		if (brightness > bd->props->max_brightness)
 			rc = -EINVAL;
 		else {
 			pr_debug("backlight: set brightness to %d\n",
 				 brightness);
 			bd->props->brightness = brightness;
-			if (likely(bd->props->update_status))
+			if (bd->props->update_status)
 				bd->props->update_status(bd);
 			rc = count;
 		}

@@ -103,7 +103,7 @@ static ssize_t backlight_show_max_bright
 	struct backlight_device *bd = to_backlight_device(cdev);
 
 	down(&bd->sem);
-	if (likely(bd->props))
+	if (bd->props)
 		rc = sprintf(buf, "%d\n", bd->props->max_brightness);
 	up(&bd->sem);

@@ -117,7 +117,7 @@ static ssize_t backlight_show_actual_bri
 	struct backlight_device *bd = to_backlight_device(cdev);
 
 	down(&bd->sem);
-	if (likely(bd->props && bd->props->get_brightness))
+	if (bd->props && bd->props->get_brightness)
 		rc = sprintf(buf, "%d\n", bd->props->get_brightness(bd));
 	up(&bd->sem);

@@ -127,6 +127,7 @@ static ssize_t backlight_show_actual_bri
 static void backlight_class_release(struct class_device *dev)
 {
 	struct backlight_device *bd = to_backlight_device(dev);
+
 	kfree(bd);
 }

@@ -153,7 +154,7 @@ static int fb_notifier_callback(struct n
 				unsigned long event, void *data)
 {
 	struct backlight_device *bd;
-	struct fb_event *evdata =(struct fb_event *)data;
+	struct fb_event *evdata = data;
 
 	/* If we aren't interested in this event, skip it immediately ... */
 	if (event != FB_EVENT_BLANK)

@@ -161,13 +162,14 @@ static int fb_notifier_callback(struct n
 
 	bd = container_of(self, struct backlight_device, fb_notif);
 	down(&bd->sem);
-	if (bd->props)
+	if (bd->props) {
 		if (!bd->props->check_fb ||
 		    bd->props->check_fb(evdata->info)) {
 			bd->props->fb_blank = *(int *)evdata->data;
 			if (likely(bd->props && bd->props->update_status))
 				bd->props->update_status(bd);
 		}
+	}
 	up(&bd->sem);
 	return 0;
 }

@@ -187,35 +189,33 @@ static int fb_notifier_callback(struct n
 struct backlight_device *backlight_device_register(const char *name, void *devdata,
 						   struct backlight_properties *bp)
 {
-	int rc;
+	int err;
 	struct backlight_device *new_bd;
 
 	pr_debug("backlight_device_alloc: name=%s\n", name);
 
-	new_bd = kmalloc(sizeof(struct backlight_device), GFP_KERNEL);
-	if (unlikely(!new_bd))
+	new_bd = kzalloc(sizeof(struct backlight_device), GFP_KERNEL);
+	if (!new_bd)
 		return ERR_PTR(-ENOMEM);
 
 	init_MUTEX(&new_bd->sem);
 	new_bd->props = bp;
-	memset(&new_bd->class_dev, 0, sizeof(new_bd->class_dev));
 	new_bd->class_dev.class = &backlight_class;
 	strlcpy(new_bd->class_dev.class_id, name, KOBJ_NAME_LEN);
 	class_set_devdata(&new_bd->class_dev, devdata);
 
-	rc = class_device_register(&new_bd->class_dev);
-	if (unlikely(rc)) {
+	err = class_device_register(&new_bd->class_dev);
+	if (err) {
 		kfree(new_bd);
-		return ERR_PTR(rc);
+		return ERR_PTR(err);
 	}
 
-	memset(&new_bd->fb_notif, 0, sizeof(new_bd->fb_notif));
 	new_bd->fb_notif.notifier_call = fb_notifier_callback;
 
-	rc = fb_register_client(&new_bd->fb_notif);
-	if (unlikely(rc)) {
+	err = fb_register_client(&new_bd->fb_notif);
+	if (err) {
 		class_device_unregister(&new_bd->class_dev);
-		return ERR_PTR(rc);
+		return ERR_PTR(err);
 	}
 
 	return new_bd;

@@ -236,7 +236,7 @@ void backlight_device_unregister(struct 
 	pr_debug("backlight_device_unregister: name=%s\n", bd->class_dev.class_id);
 
 	down(&bd->sem);
-	if (likely(bd->props && bd->props->update_status)) {
+	if (bd->props && bd->props->update_status) {
 		bd->props->brightness = 0;
 		bd->props->power = 0;
 		bd->props->update_status(bd);

@@ -246,7 +246,6 @@ void backlight_device_unregister(struct 
 	up(&bd->sem);
 
 	fb_unregister_client(&bd->fb_notif);
-
 	class_device_unregister(&bd->class_dev);
 }
 EXPORT_SYMBOL(backlight_device_unregister);

drivers/video/backlight/lcd.c +16 ~~-21~~

Index: work/drivers/video/backlight/lcd.c
===================================================================
@@ -16,14 +16,12 @@
 
 static ssize_t lcd_show_power(struct class_device *cdev, char *buf)
 {
-	int rc;
+	int rc = -ENXIO;
 	struct lcd_device *ld = to_lcd_device(cdev);
 
 	down(&ld->sem);
-	if (likely(ld->props && ld->props->get_power))
+	if (ld->props && ld->props->get_power)
 		rc = sprintf(buf, "%d\n", ld->props->get_power(ld));
-	else
-		rc = -ENXIO;
 	up(&ld->sem);
 
 	return rc;

@@ -43,7 +41,7 @@ static ssize_t lcd_store_power(struct cl
 		return -EINVAL;
 
 	down(&ld->sem);
-	if (likely(ld->props && ld->props->set_power)) {
+	if (ld->props && ld->props->set_power) {
 		pr_debug("lcd: set power to %d\n", power);
 		ld->props->set_power(ld, power);
 		rc = count;

@@ -59,7 +57,7 @@ static ssize_t lcd_show_contrast(struct 
 	struct lcd_device *ld = to_lcd_device(cdev);
 
 	down(&ld->sem);
-	if (likely(ld->props && ld->props->get_contrast))
+	if (ld->props && ld->props->get_contrast)
 		rc = sprintf(buf, "%d\n", ld->props->get_contrast(ld));
 	up(&ld->sem);

@@ -80,7 +78,7 @@ static ssize_t lcd_store_contrast(struct
 		return -EINVAL;
 
 	down(&ld->sem);
-	if (likely(ld->props && ld->props->set_contrast)) {
+	if (ld->props && ld->props->set_contrast) {
 		pr_debug("lcd: set contrast to %d\n", contrast);
 		ld->props->set_contrast(ld, contrast);
 		rc = count;

@@ -96,7 +94,7 @@ static ssize_t lcd_show_max_contrast(str
 	struct lcd_device *ld = to_lcd_device(cdev);
 
 	down(&ld->sem);
-	if (likely(ld->props))
+	if (ld->props)
 		rc = sprintf(buf, "%d\n", ld->props->max_contrast);
 	up(&ld->sem);

@@ -130,7 +128,7 @@ static int fb_notifier_callback(struct n
 				 unsigned long event, void *data)
 {
 	struct lcd_device *ld;
-	struct fb_event *evdata =(struct fb_event *)data;
+	struct fb_event *evdata = data;
 
 	/* If we aren't interested in this event, skip it immediately ... */
 	if (event != FB_EVENT_BLANK)

@@ -159,35 +157,33 @@ static int fb_notifier_callback(struct n
 struct lcd_device *lcd_device_register(const char *name, void *devdata,
 				       struct lcd_properties *lp)
 {
-	int rc;
+	int err;
 	struct lcd_device *new_ld;
 
 	pr_debug("lcd_device_register: name=%s\n", name);
 
-	new_ld = kmalloc(sizeof(struct lcd_device), GFP_KERNEL);
-	if (unlikely(!new_ld))
+	new_ld = kzalloc(sizeof(struct lcd_device), GFP_KERNEL);
+	if (!new_ld)
 		return ERR_PTR(-ENOMEM);
 
 	init_MUTEX(&new_ld->sem);
 	new_ld->props = lp;
-	memset(&new_ld->class_dev, 0, sizeof(new_ld->class_dev));
 	new_ld->class_dev.class = &lcd_class;
 	strlcpy(new_ld->class_dev.class_id, name, KOBJ_NAME_LEN);
 	class_set_devdata(&new_ld->class_dev, devdata);
 
-	rc = class_device_register(&new_ld->class_dev);
-	if (unlikely(rc)) {
+	err = class_device_register(&new_ld->class_dev);
+	if (unlikely(err)) {
 		kfree(new_ld);
-		return ERR_PTR(rc);
+		return ERR_PTR(err);
 	}
 
-	memset(&new_ld->fb_notif, 0, sizeof(new_ld->fb_notif));
 	new_ld->fb_notif.notifier_call = fb_notifier_callback;
 
-	rc = fb_register_client(&new_ld->fb_notif);
-	if (unlikely(rc)) {
+	err = fb_register_client(&new_ld->fb_notif);
+	if (err) {
 		class_device_unregister(&new_ld->class_dev);
-		return ERR_PTR(rc);
+		return ERR_PTR(err);
 	}
 
 	return new_ld;

@@ -212,7 +208,6 @@ void lcd_device_unregister(struct lcd_de
 	up(&ld->sem);
 
 	fb_unregister_client(&ld->fb_notif);
-
 	class_device_unregister(&ld->class_dev);
 }
 EXPORT_SYMBOL(lcd_device_unregister);

Dmitry Torokhov

author

2006-08-11 05:09 UTC #4 lore raw

PATCH 4/6 Remove "owner" from backlight_properties structure

In reply to Dmitry Torokhov

Backlight: remove "owner" from backlight_properties structure

Nothing uses it and it is unlikely that it will ever be used - backlight uses other means to ensure that nothing references unloaded code.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/macintosh/via-pmu-backlight.c |    1 -
 drivers/usb/misc/appledisplay.c       |    1 -
 drivers/video/aty/aty128fb.c          |    1 -
 drivers/video/aty/atyfb_base.c        |    1 -
 drivers/video/aty/radeon_backlight.c  |    1 -
 drivers/video/backlight/corgi_bl.c    |    1 -
 drivers/video/backlight/hp680_bl.c    |    1 -
 drivers/video/backlight/locomolcd.c   |    1 -
 drivers/video/nvidia/nv_backlight.c   |    1 -
 drivers/video/riva/fbdev.c            |    1 -
 include/linux/backlight.h             |    3 ---
 include/linux/lcd.h                   |    2 --
 12 files changed, 15 deletions(-)

drivers/macintosh/via-pmu-backlight.c +0 -1

Index: work/drivers/macintosh/via-pmu-backlight.c
===================================================================
@@ -81,7 +81,6 @@ static int pmu_backlight_get_brightness(
 }
 
 static struct backlight_properties pmu_backlight_data = {
-	.owner		= THIS_MODULE,
 	.get_brightness	= pmu_backlight_get_brightness,
 	.update_status	= pmu_backlight_update_status,
 	.max_brightness	= (FB_BACKLIGHT_LEVELS - 1),

drivers/usb/misc/appledisplay.c +0 -1

Index: work/drivers/usb/misc/appledisplay.c
===================================================================
@@ -179,7 +179,6 @@ static int appledisplay_bl_get_brightnes
 }
 
 static struct backlight_properties appledisplay_bl_data = {
-	.owner		= THIS_MODULE,
 	.get_brightness	= appledisplay_bl_get_brightness,
 	.update_status	= appledisplay_bl_update_status,
 	.max_brightness	= 0xFF

drivers/video/aty/aty128fb.c +0 -1

Index: work/drivers/video/aty/aty128fb.c
===================================================================
@@ -1792,7 +1792,6 @@ static int aty128_bl_get_brightness(stru
 }
 
 static struct backlight_properties aty128_bl_data = {
-	.owner		= THIS_MODULE,
 	.get_brightness	= aty128_bl_get_brightness,
 	.update_status	= aty128_bl_update_status,
 	.max_brightness	= (FB_BACKLIGHT_LEVELS - 1),

drivers/video/aty/atyfb_base.c +0 -1

Index: work/drivers/video/aty/atyfb_base.c
===================================================================
@@ -2191,7 +2191,6 @@ static int aty_bl_get_brightness(struct 
 }
 
 static struct backlight_properties aty_bl_data = {
-	.owner	  = THIS_MODULE,
 	.get_brightness = aty_bl_get_brightness,
 	.update_status	= aty_bl_update_status,
 	.max_brightness = (FB_BACKLIGHT_LEVELS - 1),

drivers/video/aty/radeon_backlight.c +0 -1

Index: work/drivers/video/aty/radeon_backlight.c
===================================================================
@@ -134,7 +134,6 @@ static int radeon_bl_get_brightness(stru
 }
 
 static struct backlight_properties radeon_bl_data = {
-	.owner		= THIS_MODULE,
 	.get_brightness = radeon_bl_get_brightness,
 	.update_status	= radeon_bl_update_status,
 	.max_brightness = (FB_BACKLIGHT_LEVELS - 1),

drivers/video/backlight/corgi_bl.c +0 -1

Index: work/drivers/video/backlight/corgi_bl.c
===================================================================
@@ -106,7 +106,6 @@ EXPORT_SYMBOL(corgibl_limit_intensity);
 
 
 static struct backlight_properties corgibl_data = {
-	.owner          = THIS_MODULE,
 	.get_brightness = corgibl_get_intensity,
 	.update_status  = corgibl_set_intensity,
 };

drivers/video/backlight/hp680_bl.c +0 -1

Index: work/drivers/video/backlight/hp680_bl.c
===================================================================
@@ -96,7 +96,6 @@ static int hp680bl_get_intensity(struct 
 }
 
 static struct backlight_properties hp680bl_data = {
-	.owner		= THIS_MODULE,
 	.max_brightness = HP680_MAX_INTENSITY,
 	.get_brightness = hp680bl_get_intensity,
 	.update_status  = hp680bl_set_intensity,

drivers/video/backlight/locomolcd.c +0 -1

Index: work/drivers/video/backlight/locomolcd.c
===================================================================
@@ -142,7 +142,6 @@ static int locomolcd_get_intensity(struc
 }
 
 static struct backlight_properties locomobl_data = {
-	.owner		= THIS_MODULE,
 	.get_brightness = locomolcd_get_intensity,
 	.update_status  = locomolcd_set_intensity,
 	.max_brightness = 4,

drivers/video/nvidia/nv_backlight.c +0 -1

Index: work/drivers/video/nvidia/nv_backlight.c
===================================================================
@@ -104,7 +104,6 @@ static int nvidia_bl_get_brightness(stru
 }
 
 static struct backlight_properties nvidia_bl_data = {
-	.owner		= THIS_MODULE,
 	.get_brightness = nvidia_bl_get_brightness,
 	.update_status	= nvidia_bl_update_status,
 	.max_brightness = (FB_BACKLIGHT_LEVELS - 1),

drivers/video/riva/fbdev.c +0 -1

Index: work/drivers/video/riva/fbdev.c
===================================================================
@@ -346,7 +346,6 @@ static int riva_bl_get_brightness(struct
 }
 
 static struct backlight_properties riva_bl_data = {
-	.owner    = THIS_MODULE,
 	.get_brightness = riva_bl_get_brightness,
 	.update_status	= riva_bl_update_status,
 	.max_brightness = (FB_BACKLIGHT_LEVELS - 1),

include/linux/backlight.h +0 -3

Index: work/include/linux/backlight.h
===================================================================
@@ -17,9 +17,6 @@ struct fb_info;
 /* This structure defines all the properties of a backlight
    (usually attached to a LCD). */
 struct backlight_properties {
-	/* Owner module */
-	struct module *owner;
-
 	/* Notify the backlight driver some property has changed */
 	int (*update_status)(struct backlight_device *);
 	/* Return the current backlight brightness (accounting for power,

include/linux/lcd.h +0 -2

Index: work/include/linux/lcd.h
===================================================================
@@ -16,8 +16,6 @@ struct fb_info;
 
 /* This structure defines all the properties of a LCD flat panel. */
 struct lcd_properties {
-	/* Owner module */
-	struct module *owner;
 	/* Get the LCD panel power status (0: full on, 1..3: controller
 	   power on, flat panel power off, 4: full off), see FB_BLANK_XXX */
 	int (*get_power)(struct lcd_device *);

Dmitry Torokhov

author

2006-08-11 05:09 UTC #5 lore raw

PATCH 5/6 Convert to use mutexes instead of semaphores

In reply to Dmitry Torokhov

Backlight: convert to use mutexes instead of semaphores

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/video/backlight/backlight.c |   89 +++++++++++++++++++++++-------------
 drivers/video/backlight/lcd.c       |   73 +++++++++++++++++++++--------
 include/linux/backlight.h           |    2 
 include/linux/lcd.h                 |    2 
 4 files changed, 112 insertions(+), 54 deletions(-)

drivers/video/backlight/backlight.c +58 ~~-31~~

Index: work/drivers/video/backlight/backlight.c
===================================================================
@@ -16,20 +16,23 @@
 
 static ssize_t backlight_show_power(struct class_device *cdev, char *buf)
 {
-	int rc = -ENXIO;
+	int rc;
 	struct backlight_device *bd = to_backlight_device(cdev);
 
-	down(&bd->sem);
-	if (bd->props)
-		rc = sprintf(buf, "%d\n", bd->props->power);
-	up(&bd->sem);
+	rc = mutex_lock_interruptible(&bd->mutex);
+	if (rc)
+		return rc;
+
+	rc = bd->props ? sprintf(buf, "%d\n", bd->props->power) : -ENXIO;
+
+	mutex_unlock(&bd->mutex);
 
 	return rc;
 }
 
 static ssize_t backlight_store_power(struct class_device *cdev, const char *buf, size_t count)
 {
-	int rc = -ENXIO;
+	int rc;
 	char *endp;
 	struct backlight_device *bd = to_backlight_device(cdev);
 	int power = simple_strtoul(buf, &endp, 0);

@@ -40,35 +43,43 @@ static ssize_t backlight_store_power(str
 	if (size != count)
 		return -EINVAL;
 
-	down(&bd->sem);
+	rc = mutex_lock_interruptible(&bd->mutex);
+	if (rc)
+		return rc;
+
 	if (bd->props) {
 		pr_debug("backlight: set power to %d\n", power);
 		bd->props->power = power;
 		if (bd->props->update_status)
 			bd->props->update_status(bd);
 		rc = count;
-	}
-	up(&bd->sem);
+	} else
+		rc = -ENXIO;
+
+	mutex_unlock(&bd->mutex);
 
 	return rc;
 }
 
 static ssize_t backlight_show_brightness(struct class_device *cdev, char *buf)
 {
-	int rc = -ENXIO;
+	int rc;
 	struct backlight_device *bd = to_backlight_device(cdev);
 
-	down(&bd->sem);
-	if (bd->props)
-		rc = sprintf(buf, "%d\n", bd->props->brightness);
-	up(&bd->sem);
+	rc = mutex_lock_interruptible(&bd->mutex);
+	if (rc)
+		return rc;
+
+	rc = bd->props ? sprintf(buf, "%d\n", bd->props->brightness) : -ENXIO;
+
+	mutex_unlock(&bd->mutex);
 
 	return rc;
 }
 
 static ssize_t backlight_store_brightness(struct class_device *cdev, const char *buf, size_t count)
 {
-	int rc = -ENXIO;
+	int rc;
 	char *endp;
 	struct backlight_device *bd = to_backlight_device(cdev);
 	int brightness = simple_strtoul(buf, &endp, 0);

@@ -79,7 +90,10 @@ static ssize_t backlight_store_brightnes
 	if (size != count)
 		return -EINVAL;
 
-	down(&bd->sem);
+	rc = mutex_lock_interruptible(&bd->mutex);
+	if (rc)
+		return rc;
+
 	if (bd->props) {
 		if (brightness > bd->props->max_brightness)
 			rc = -EINVAL;

@@ -91,21 +105,26 @@ static ssize_t backlight_store_brightnes
 				bd->props->update_status(bd);
 			rc = count;
 		}
-	}
-	up(&bd->sem);
+	} else
+		rc = -ENXIO;
+
+	mutex_unlock(&bd->mutex);
 
 	return rc;
 }
 
 static ssize_t backlight_show_max_brightness(struct class_device *cdev, char *buf)
 {
-	int rc = -ENXIO;
+	int rc;
 	struct backlight_device *bd = to_backlight_device(cdev);
 
-	down(&bd->sem);
-	if (bd->props)
-		rc = sprintf(buf, "%d\n", bd->props->max_brightness);
-	up(&bd->sem);
+	rc = mutex_lock_interruptible(&bd->mutex);
+	if (rc)
+		return rc;
+
+	rc = bd->props ? sprintf(buf, "%d\n", bd->props->max_brightness) : -ENXIO;
+
+	mutex_unlock(&bd->mutex);
 
 	return rc;
 }

@@ -113,13 +132,19 @@ static ssize_t backlight_show_max_bright
 static ssize_t backlight_show_actual_brightness(struct class_device *cdev,
 						char *buf)
 {
-	int rc = -ENXIO;
+	int rc;
 	struct backlight_device *bd = to_backlight_device(cdev);
 
-	down(&bd->sem);
+	rc = mutex_lock_interruptible(&bd->mutex);
+	if (rc)
+		return rc;
+
 	if (bd->props && bd->props->get_brightness)
 		rc = sprintf(buf, "%d\n", bd->props->get_brightness(bd));
-	up(&bd->sem);
+	else
+		rc = -ENXIO;
+
+	mutex_unlock(&bd->mutex);
 
 	return rc;
 }

@@ -161,7 +186,8 @@ static int fb_notifier_callback(struct n
 		return 0;
 
 	bd = container_of(self, struct backlight_device, fb_notif);
-	down(&bd->sem);
+
+	mutex_lock(&bd->mutex);
 	if (bd->props) {
 		if (!bd->props->check_fb ||
 		    bd->props->check_fb(evdata->info)) {

@@ -170,7 +196,8 @@ static int fb_notifier_callback(struct n
 				bd->props->update_status(bd);
 		}
 	}
-	up(&bd->sem);
+	mutex_unlock(&bd->mutex);
+
 	return 0;
 }

@@ -198,7 +225,7 @@ struct backlight_device *backlight_devic
 	if (!new_bd)
 		return ERR_PTR(-ENOMEM);
 
-	init_MUTEX(&new_bd->sem);
+	mutex_init(&new_bd->mutex);
 	new_bd->props = bp;
 	new_bd->class_dev.class = &backlight_class;
 	strlcpy(new_bd->class_dev.class_id, name, KOBJ_NAME_LEN);

@@ -235,7 +262,7 @@ void backlight_device_unregister(struct 
 
 	pr_debug("backlight_device_unregister: name=%s\n", bd->class_dev.class_id);
 
-	down(&bd->sem);
+	mutex_lock(&bd->mutex);
 	if (bd->props && bd->props->update_status) {
 		bd->props->brightness = 0;
 		bd->props->power = 0;

@@ -243,7 +270,7 @@ void backlight_device_unregister(struct 
 	}
 
 	bd->props = NULL;
-	up(&bd->sem);
+	mutex_unlock(&bd->mutex);
 
 	fb_unregister_client(&bd->fb_notif);
 	class_device_unregister(&bd->class_dev);

drivers/video/backlight/lcd.c +52 ~~-21~~

161 lines not on this page · raw message

include/linux/backlight.h +1 -1

Index: work/include/linux/backlight.h
===================================================================
@@ -41,7 +41,7 @@ struct backlight_device {
 	/* This protects the 'props' field. If 'props' is NULL, the driver that
 	   registered this device has been unloaded, and if class_get_devdata()
 	   points to something in the body of that driver, it is also invalid. */
-	struct semaphore sem;
+	struct mutex mutex;
 	/* If this is NULL, the backing module is unloaded */
 	struct backlight_properties *props;
 	/* The framebuffer notifier block */

include/linux/lcd.h +1 -1

Index: work/include/linux/lcd.h
===================================================================
@@ -36,7 +36,7 @@ struct lcd_device {
 	/* This protects the 'props' field. If 'props' is NULL, the driver that
 	   registered this device has been unloaded, and if class_get_devdata()
 	   points to something in the body of that driver, it is also invalid. */
-	struct semaphore sem;
+	struct mutex mutex;
 	/* If this is NULL, the backing module is unloaded */
 	struct lcd_properties *props;
 	/* The framebuffer notifier block */

Dmitry Torokhov

2006-08-11 12:59 UTC #6 lore raw

Re: [patch 5/6] Convert to use mutexes instead of semaphores

Quoted 2 lines · Dmitry Torokhov

Backlight: convert to use mutexes instead of semaphores

Apparently I missed that several drivers also use bd->sem so they need to be converted too... But what is it with the drivers:

static void aty128_bl_set_power(struct fb_info *info, int power)

{
       mutex_lock(&info->bl_mutex);
       up(&info->bl_dev->sem);
       info->bl_dev->props->power = power;
       __aty128_bl_update_status(info->bl_dev);
       down(&info->bl_dev->sem);
       mutex_unlock(&info->bl_mutex);
}

Why we are doing up() before down()??? And it is in almost every driver that uses backlight... Do I need more coffee? [CC-ing bunch of people trying to get an answer...]

Signature · 1 line

Dmitry

Richard Purdie

2006-08-11 13:16 UTC #7 lore raw

Re: [patch 5/6] Convert to use mutexes instead of semaphores

Quoted 17 lines · Dmitry Torokhov

Quoted 2 lines · Dmitry Torokhov
Backlight: convert to use mutexes instead of semaphores
Apparently I missed that several drivers also use bd->sem so they need to be converted too... But what is it with the drivers:
static void aty128_bl_set_power(struct fb_info *info, int power)
{
        mutex_lock(&info->bl_mutex);
        up(&info->bl_dev->sem);
        info->bl_dev->props->power = power;
        __aty128_bl_update_status(info->bl_dev);
        down(&info->bl_dev->sem);
        mutex_unlock(&info->bl_mutex);
}

Quoted 3 lines

Why we are doing up() before down()??? And it is in almost every driver that uses backlight... Do I need more coffee? [CC-ing bunch of people trying to get an answer...]

It looks totally wrong.

In the archives, there are a number of comments from me questioning whether that driver needs to touch bl_dev->sem anyway (esp. given the mutex as well). I never did find out what it was trying to protect against...

Richard

Dmitry Torokhov

2006-08-11 13:34 UTC #8 lore raw

Re: [patch 5/6] Convert to use mutexes instead of semaphores

Quoted 18 lines · Richard Purdie

Quoted 17 lines · Dmitry Torokhov
Quoted 2 lines · Dmitry Torokhov
Backlight: convert to use mutexes instead of semaphores
Apparently I missed that several drivers also use bd->sem so they need to be converted too... But what is it with the drivers:
static void aty128_bl_set_power(struct fb_info *info, int power)
{
        mutex_lock(&info->bl_mutex);
        up(&info->bl_dev->sem);
        info->bl_dev->props->power = power;
        __aty128_bl_update_status(info->bl_dev);
        down(&info->bl_dev->sem);
        mutex_unlock(&info->bl_mutex);
}

Quoted 6 lines

Quoted 3 lines
Why we are doing up() before down()??? And it is in almost every driver that uses backlight... Do I need more coffee? [CC-ing bunch of people trying to get an answer...]
It looks totally wrong.

Ok, so that is not only me seeing things ;)

Quoted 4 lines

In the archives, there are a number of comments from me questioning whether that driver needs to touch bl_dev->sem anyway (esp. given the mutex as well). I never did find out what it was trying to protect against...

I think it is prudent to protect assess to these data structures. For example, it could possibly race with setting power through sysfs attribute. Now for this particular driver the race window is non-existent for all practical purposes, but it looks like atyfb_base might be needig it (of course currentimplementation not only have up/down mixed but also has AB-BA deadlock).

How about we add backlight_set_power(&bd, power) to the backlight core to take care of proper locking for drivers?

Signature · 1 line

Dmitry

Michael Hanselmann

2006-08-11 13:42 UTC #9 lore raw

Re: [patch 5/6] Convert to use mutexes instead of semaphores

Quoted 2 lines · Dmitry Torokhov

How about we add backlight_set_power(&bd, power) to the backlight core to take care of proper locking for drivers?

I've tried to add several functions to the backlight core ({s,g}et_{brightness,power}) and they were rejected. Thus all the locking is spread over the drivers. I agree it's faulty right now. It's still easier to move to backlight core functions than to fix all the drivers.

Because I am responsible/wrote for the broken code, how should I proceed?

Dmitry Torokhov

2006-08-11 14:07 UTC #10 lore raw

Re: [patch 5/6] Convert to use mutexes instead of semaphores

Quoted 4 lines · Michael Hanselmann

Quoted 2 lines · Dmitry Torokhov
How about we add backlight_set_power(&bd, power) to the backlight core to take care of proper locking for drivers?

Quoted 9 lines

I've tried to add several functions to the backlight core ({s,g}et_{brightness,power}) and they were rejected. Thus all the locking is spread over the drivers. I agree it's faulty right now. It's still easier to move to backlight core functions than to fix all the drivers.
Because I am responsible/wrote for the broken code, how should I proceed?

Well, I was reading some more of the drivers and I am also not sure if such methods are needed in backlight core. Let's take atyfb_base.c - it tries to manipulate backlight's power from atyfb_blank. But it is normally called from fb_blank() which is then calls fb_notifier_call_chain(FB_EVENT_BLANK, &event);
So on the end backlight device will get that event and will turn off power anyway. Now, atyfb_blank is also called suring suspend/resume so we probably should just add handling of FB_EVENT_SUSPEND and FB_EVENT_RESUME to the backlight core.

Richard?

Signature · 1 line

Dmitry

Richard Purdie

2006-08-11 16:46 UTC #11 lore raw

Re: [patch 5/6] Convert to use mutexes instead of semaphores

Quoted 4 lines · Dmitry Torokhov

Quoted 3 lines · Michael Hanselmann
Quoted 2 lines · Dmitry Torokhov
How about we add backlight_set_power(&bd, power) to the backlight core to take care of proper locking for drivers?

A couple of patches were attempted for this but they didn't solve the underlying races. The main reason was a lack of understanding of what the existing backlight lock protects and trying to make it do two thinsg at once.

Quoted 5 lines

Quoted 5 lines
I've tried to add several functions to the backlight core ({s,g}et_{brightness,power}) and they were rejected. Thus all the locking is spread over the drivers. I agree it's faulty right now. It's still easier to move to backlight core functions than to fix all the drivers.

If we can find a way to safely do the locking in the backlight core I agree.

Quoted 2 lines

Quoted 2 lines
Because I am responsible/wrote for the broken code, how should I proceed?

First, we need to define the potential problems. Dimitry mentioned: "For example, it could possibly race with setting power through sysfs attribute". This is not what the lock in the backlight core is for though. To quote backlight.h:

/* This protects the 'props' field. If 'props' is NULL, the driver that
registered this device has been unloaded, and if class_get_devdata() points to something in the body of that driver, it is also invalid.
*/

My previous patches have gone a long way to removing race issues. The need for the existing lock comes from backlight_device_unregister() which basically does:

class_device_remove_files()
bd->props->brightness = 0;
bd->props->power = 0;
bd->props->update_status(bd);
bd->props = NULL
fb_unregister_client()
class_device_unregister()

If we could guarantee that after class_device_unregister(), nothing was still executing any of the show/store methods, we'd be fine (the fb_notifier is safe). As I understand the class device and sysfs attributes, we can't guarantee that though. I'd appreciate comments from the device model people as I could be wrong about this. The owner field also can't help us.

Dimitry's "Backlight: convert to use default class device attributes" patch should really mean the class_device_unregister() call is moved to earlier in the function to try and avoid races from the attributes but it still doesn't guarantee anything.

If we could somehow sync class_device_unregister(), we could get rid of that semaphore entirely.

Regardless, if we want to add locking for synchronising the attributes into the core, we need a different lock. I did think the drivers would be able to handle this themselves with locking inside update_status if needed but I can see why certain drivers might not like that.

Quoted 11 lines

Well, I was reading some more of the drivers and I am also not sure if such methods are needed in backlight core. Let's take atyfb_base.c - it tries to manipulate backlight's power from atyfb_blank. But it is normally called from fb_blank() which is then calls fb_notifier_call_chain(FB_EVENT_BLANK, &event);
So on the end backlight device will get that event and will turn off power anyway. Now, atyfb_blank is also called suring suspend/resume so we probably should just add handling of FB_EVENT_SUSPEND and FB_EVENT_RESUME to the backlight core.
Richard?

Think about the case where you have 2 framebuffers. The notification call was left to pass to the driver as only it can work out which framebuffer a given backlight is attached to.

Cheers,

Richard

Dmitry Torokhov

2006-08-11 17:21 UTC #12 lore raw

Re: [patch 5/6] Convert to use mutexes instead of semaphores

Quoted 28 lines · Richard Purdie

Quoted 4 lines · Dmitry Torokhov
Quoted 3 lines · Michael Hanselmann
Quoted 2 lines · Dmitry Torokhov
How about we add backlight_set_power(&bd, power) to the backlight core to take care of proper locking for drivers?
A couple of patches were attempted for this but they didn't solve the underlying races. The main reason was a lack of understanding of what the existing backlight lock protects and trying to make it do two thinsg at once.
Quoted 5 lines
Quoted 5 lines
I've tried to add several functions to the backlight core ({s,g}et_{brightness,power}) and they were rejected. Thus all the locking is spread over the drivers. I agree it's faulty right now. It's still easier to move to backlight core functions than to fix all the drivers.
If we can find a way to safely do the locking in the backlight core I agree.
Quoted 2 lines
Quoted 2 lines
Because I am responsible/wrote for the broken code, how should I proceed?
First, we need to define the potential problems. Dimitry mentioned: "For example, it could possibly race with setting power through sysfs attribute". This is not what the lock in the backlight core is for though. To quote backlight.h:

Quoted 4 lines

/* This protects the 'props' field. If 'props' is NULL, the driver that
registered this device has been unloaded, and if class_get_devdata() points to something in the body of that driver, it is also invalid.
*/

Yes, you are right. As long as ->update_status() method serializes access to the underlying hardware by itself we don't need locking in backlight core. If we have several writes one of them will win but we will never have kernel and hardware disagree about the state they are in. So we can just remove references to baccklight's semaphore from drivers.

Quoted 6 lines

Dimitry's "Backlight: convert to use default class device attributes" patch should really mean the class_device_unregister() call is moved to earlier in the function to try and avoid races from the attributes but it still doesn't guarantee anything.

No, it was not the intent of the patch. I was just trying to remove unneeded code and simplify error handling because driver core can do that for us. The race window is still present and mutex is still needed.

Quoted 11 lines

Quoted 11 lines
Well, I was reading some more of the drivers and I am also not sure if such methods are needed in backlight core. Let's take atyfb_base.c - it tries to manipulate backlight's power from atyfb_blank. But it is normally called from fb_blank() which is then calls fb_notifier_call_chain(FB_EVENT_BLANK, &event);
So on the end backlight device will get that event and will turn off power anyway. Now, atyfb_blank is also called suring suspend/resume so we probably should just add handling of FB_EVENT_SUSPEND and FB_EVENT_RESUME to the backlight core.

Quoted 6 lines

Quoted 1 line
Richard?
Think about the case where you have 2 framebuffers. The notification call was left to pass to the driver as only it can work out which framebuffer a given backlight is attached to.

I am not sure I follow... It would end up in the driver, like FB_EVENT_BLANK ultimately does. RIght now FB_EVENT_SUSPEND and FB_EVENT_RESUME are dropped by the blacklight core.

But it does not matter now - if we do not fiddle with backlight's locks we can continue switching backlight off in drivers.

Signature · 1 line

Dmitry

Michael Hanselmann

2006-08-29 20:54 UTC #13 lore raw

Re: [patch 5/6] Convert to use mutexes instead of semaphores

In reply to Richard Purdie

Quoted 4 lines · Richard Purdie

Quoted 3 lines · Dmitry Torokhov
Quoted 2 lines · Michael Hanselmann
Because I am responsible/wrote for the broken code, how should I proceed?

Somehow this got lost, sorry. The patch below fixes at least the wrongly ordered up()/down() calls. I know there are outstanding with the backlight code in general, but those issues aren't that easy to fix.

Is it okay? If yes, I'm going to send it to akpm.

Signed-off-by Michael Hanselmann <[email protected]>

---

drivers/macintosh/via-pmu-backlight.c +2 -2

diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc5.orig/drivers/macintosh/via-pmu-backlight.c linux-2.6.18-rc5/drivers/macintosh/via-pmu-backlight.c
@@ -168,11 +168,11 @@ void __init pmu_backlight_init()
 		mutex_unlock(&info->bl_mutex);
 	}
 
-	up(&bd->sem);
+	down(&bd->sem);
 	bd->props->brightness = level;
 	bd->props->power = FB_BLANK_UNBLANK;
 	bd->props->update_status(bd);
-	down(&bd->sem);
+	up(&bd->sem);
 
 	mutex_lock(&pmac_backlight_mutex);
 	if (!pmac_backlight)

drivers/video/aty/aty128fb.c +4 -4

diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc5.orig/drivers/video/aty/aty128fb.c linux-2.6.18-rc5/drivers/video/aty/aty128fb.c
@@ -1801,10 +1801,10 @@ static struct backlight_properties aty12
 static void aty128_bl_set_power(struct fb_info *info, int power)
 {
 	mutex_lock(&info->bl_mutex);
-	up(&info->bl_dev->sem);
+	down(&info->bl_dev->sem);
 	info->bl_dev->props->power = power;
 	__aty128_bl_update_status(info->bl_dev);
-	down(&info->bl_dev->sem);
+	up(&info->bl_dev->sem);
 	mutex_unlock(&info->bl_mutex);
 }

@@ -1839,11 +1839,11 @@ static void aty128_bl_init(struct aty128
 		219 * FB_BACKLIGHT_MAX / MAX_LEVEL);
 	mutex_unlock(&info->bl_mutex);
 
-	up(&bd->sem);
+	down(&bd->sem);
 	bd->props->brightness = aty128_bl_data.max_brightness;
 	bd->props->power = FB_BLANK_UNBLANK;
 	bd->props->update_status(bd);
-	down(&bd->sem);
+	up(&bd->sem);
 
 #ifdef CONFIG_PMAC_BACKLIGHT
 	mutex_lock(&pmac_backlight_mutex);

drivers/video/aty/atyfb_base.c +4 -4

diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc5.orig/drivers/video/aty/atyfb_base.c linux-2.6.18-rc5/drivers/video/aty/atyfb_base.c
@@ -2200,10 +2200,10 @@ static struct backlight_properties aty_b
 static void aty_bl_set_power(struct fb_info *info, int power)
 {
 	mutex_lock(&info->bl_mutex);
-	up(&info->bl_dev->sem);
+	down(&info->bl_dev->sem);
 	info->bl_dev->props->power = power;
 	__aty_bl_update_status(info->bl_dev);
-	down(&info->bl_dev->sem);
+	up(&info->bl_dev->sem);
 	mutex_unlock(&info->bl_mutex);
 }

@@ -2234,11 +2234,11 @@ static void aty_bl_init(struct atyfb_par
 		0xFF * FB_BACKLIGHT_MAX / MAX_LEVEL);
 	mutex_unlock(&info->bl_mutex);
 
-	up(&bd->sem);
+	down(&bd->sem);
 	bd->props->brightness = aty_bl_data.max_brightness;
 	bd->props->power = FB_BLANK_UNBLANK;
 	bd->props->update_status(bd);
-	down(&bd->sem);
+	up(&bd->sem);
 
 #ifdef CONFIG_PMAC_BACKLIGHT
 	mutex_lock(&pmac_backlight_mutex);

drivers/video/aty/radeon_backlight.c +2 -2

diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc5.orig/drivers/video/aty/radeon_backlight.c linux-2.6.18-rc5/drivers/video/aty/radeon_backlight.c
@@ -195,11 +195,11 @@ void radeonfb_bl_init(struct radeonfb_in
 		217 * FB_BACKLIGHT_MAX / MAX_RADEON_LEVEL);
 	mutex_unlock(&rinfo->info->bl_mutex);
 
-	up(&bd->sem);
+	down(&bd->sem);
 	bd->props->brightness = radeon_bl_data.max_brightness;
 	bd->props->power = FB_BLANK_UNBLANK;
 	bd->props->update_status(bd);
-	down(&bd->sem);
+	up(&bd->sem);
 
 #ifdef CONFIG_PMAC_BACKLIGHT
 	mutex_lock(&pmac_backlight_mutex);

drivers/video/nvidia/nv_backlight.c +4 -4

diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc5.orig/drivers/video/nvidia/nv_backlight.c linux-2.6.18-rc5/drivers/video/nvidia/nv_backlight.c
@@ -113,10 +113,10 @@ static struct backlight_properties nvidi
 void nvidia_bl_set_power(struct fb_info *info, int power)
 {
 	mutex_lock(&info->bl_mutex);
-	up(&info->bl_dev->sem);
+	down(&info->bl_dev->sem);
 	info->bl_dev->props->power = power;
 	__nvidia_bl_update_status(info->bl_dev);
-	down(&info->bl_dev->sem);
+	up(&info->bl_dev->sem);
 	mutex_unlock(&info->bl_mutex);
 }

@@ -151,11 +151,11 @@ void nvidia_bl_init(struct nvidia_par *p
 		0x534 * FB_BACKLIGHT_MAX / MAX_LEVEL);
 	mutex_unlock(&info->bl_mutex);
 
-	up(&bd->sem);
+	down(&bd->sem);
 	bd->props->brightness = nvidia_bl_data.max_brightness;
 	bd->props->power = FB_BLANK_UNBLANK;
 	bd->props->update_status(bd);
-	down(&bd->sem);
+	up(&bd->sem);
 
 #ifdef CONFIG_PMAC_BACKLIGHT
 	mutex_lock(&pmac_backlight_mutex);

drivers/video/riva/fbdev.c +4 -4

diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc5.orig/drivers/video/riva/fbdev.c linux-2.6.18-rc5/drivers/video/riva/fbdev.c
@@ -355,10 +355,10 @@ static struct backlight_properties riva_
 static void riva_bl_set_power(struct fb_info *info, int power)
 {
 	mutex_lock(&info->bl_mutex);
-	up(&info->bl_dev->sem);
+	down(&info->bl_dev->sem);
 	info->bl_dev->props->power = power;
 	__riva_bl_update_status(info->bl_dev);
-	down(&info->bl_dev->sem);
+	up(&info->bl_dev->sem);
 	mutex_unlock(&info->bl_mutex);
 }

@@ -393,11 +393,11 @@ static void riva_bl_init(struct riva_par
 		0x534 * FB_BACKLIGHT_MAX / MAX_LEVEL);
 	mutex_unlock(&info->bl_mutex);
 
-	up(&bd->sem);
+	down(&bd->sem);
 	bd->props->brightness = riva_bl_data.max_brightness;
 	bd->props->power = FB_BLANK_UNBLANK;
 	bd->props->update_status(bd);
-	down(&bd->sem);
+	up(&bd->sem);
 
 #ifdef CONFIG_PMAC_BACKLIGHT
 	mutex_lock(&pmac_backlight_mutex);

Dmitry Torokhov

author

2006-08-11 05:09 UTC #14 lore raw

PATCH 6/6 Move per-device data out of backlight_properties

In reply to Dmitry Torokhov

Backlight: move per-device data out of backlight_properties

Data such as current brightness belongs to a device and should not be part of a structure shared between several devices.

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/video/backlight/backlight.c |   37 +++++++++---------------------------
 drivers/video/backlight/lcd.c       |    2 -
 include/linux/backlight.h           |   21 +++++++++++---------
 include/linux/lcd.h                 |    4 +--
 4 files changed, 25 insertions(+), 39 deletions(-)

include/linux/backlight.h +12 -9

Index: work/include/linux/backlight.h
===================================================================
@@ -26,15 +26,8 @@ struct backlight_properties {
 	   return 0 if not, !=0 if it is. If NULL, backlight always matches the fb. */
 	int (*check_fb)(struct fb_info *);
 
-	/* Current User requested brightness (0 - max_brightness) */
-	int brightness;
 	/* Maximal value for brightness (read-only) */
 	int max_brightness;
-	/* Current FB Power mode (0: full on, 1..3: power saving
-	   modes; 4: full off), see FB_BLANK_XXX */
-	int power;
-	/* FB Blanking active? (values as for power) */
-	int fb_blank;
 };
 
 struct backlight_device {

@@ -43,15 +36,25 @@ struct backlight_device {
 	   points to something in the body of that driver, it is also invalid. */
 	struct mutex mutex;
 	/* If this is NULL, the backing module is unloaded */
-	struct backlight_properties *props;
+	const struct backlight_properties *props;
+
 	/* The framebuffer notifier block */
 	struct notifier_block fb_notif;
+
+	/* Current User requested brightness (0 - max_brightness) */
+	int brightness;
+	/* Current FB Power mode (0: full on, 1..3: power saving
+	   modes; 4: full off), see FB_BLANK_XXX */
+	int power;
+	/* FB Blanking active? (values as for power) */
+	int fb_blank;
+
 	/* The class device structure */
 	struct class_device class_dev;
 };
 
 extern struct backlight_device *backlight_device_register(const char *name,
-	void *devdata, struct backlight_properties *bp);
+	void *devdata, const struct backlight_properties *bp);
 extern void backlight_device_unregister(struct backlight_device *bd);
 
 #define to_backlight_device(obj) container_of(obj, struct backlight_device, class_dev)

drivers/video/backlight/backlight.c +10 ~~-27~~

Index: work/drivers/video/backlight/backlight.c
===================================================================
@@ -16,18 +16,9 @@
 
 static ssize_t backlight_show_power(struct class_device *cdev, char *buf)
 {
-	int rc;
 	struct backlight_device *bd = to_backlight_device(cdev);
 
-	rc = mutex_lock_interruptible(&bd->mutex);
-	if (rc)
-		return rc;
-
-	rc = bd->props ? sprintf(buf, "%d\n", bd->props->power) : -ENXIO;
-
-	mutex_unlock(&bd->mutex);
-
-	return rc;
+	return sprintf(buf, "%d\n", bd->power);
 }
 
 static ssize_t backlight_store_power(struct class_device *cdev, const char *buf, size_t count)

@@ -49,7 +40,7 @@ static ssize_t backlight_store_power(str
 
 	if (bd->props) {
 		pr_debug("backlight: set power to %d\n", power);
-		bd->props->power = power;
+		bd->power = power;
 		if (bd->props->update_status)
 			bd->props->update_status(bd);
 		rc = count;

@@ -63,18 +54,9 @@ static ssize_t backlight_store_power(str
 
 static ssize_t backlight_show_brightness(struct class_device *cdev, char *buf)
 {
-	int rc;
 	struct backlight_device *bd = to_backlight_device(cdev);
 
-	rc = mutex_lock_interruptible(&bd->mutex);
-	if (rc)
-		return rc;
-
-	rc = bd->props ? sprintf(buf, "%d\n", bd->props->brightness) : -ENXIO;
-
-	mutex_unlock(&bd->mutex);
-
-	return rc;
+	return sprintf(buf, "%d\n", bd->brightness);
 }
 
 static ssize_t backlight_store_brightness(struct class_device *cdev, const char *buf, size_t count)

@@ -100,7 +82,7 @@ static ssize_t backlight_store_brightnes
 		else {
 			pr_debug("backlight: set brightness to %d\n",
 				 brightness);
-			bd->props->brightness = brightness;
+			bd->brightness = brightness;
 			if (bd->props->update_status)
 				bd->props->update_status(bd);
 			rc = count;

@@ -191,7 +173,7 @@ static int fb_notifier_callback(struct n
 	if (bd->props) {
 		if (!bd->props->check_fb ||
 		    bd->props->check_fb(evdata->info)) {
-			bd->props->fb_blank = *(int *)evdata->data;
+			bd->fb_blank = *(int *)evdata->data;
 			if (likely(bd->props && bd->props->update_status))
 				bd->props->update_status(bd);
 		}

@@ -213,8 +195,9 @@ static int fb_notifier_callback(struct n
  * Creates and registers new backlight class_device. Returns either an
  * ERR_PTR() or a pointer to the newly allocated device.
  */
-struct backlight_device *backlight_device_register(const char *name, void *devdata,
-						   struct backlight_properties *bp)
+struct backlight_device *
+backlight_device_register(const char *name, void *devdata,
+			  const struct backlight_properties *bp)
 {
 	int err;
 	struct backlight_device *new_bd;

@@ -264,8 +247,8 @@ void backlight_device_unregister(struct 
 
 	mutex_lock(&bd->mutex);
 	if (bd->props && bd->props->update_status) {
-		bd->props->brightness = 0;
-		bd->props->power = 0;
+		bd->brightness = 0;
+		bd->power = 0;
 		bd->props->update_status(bd);
 	}

drivers/video/backlight/lcd.c +1 -1

Index: work/drivers/video/backlight/lcd.c
===================================================================
@@ -186,7 +186,7 @@ static int fb_notifier_callback(struct n
  * or a pointer to the newly allocated device.
  */
 struct lcd_device *lcd_device_register(const char *name, void *devdata,
-				       struct lcd_properties *lp)
+				       const struct lcd_properties *lp)
 {
 	int err;
 	struct lcd_device *new_ld;

include/linux/lcd.h +2 -2

Index: work/include/linux/lcd.h
===================================================================
@@ -38,7 +38,7 @@ struct lcd_device {
 	   points to something in the body of that driver, it is also invalid. */
 	struct mutex mutex;
 	/* If this is NULL, the backing module is unloaded */
-	struct lcd_properties *props;
+	const struct lcd_properties *props;
 	/* The framebuffer notifier block */
 	struct notifier_block fb_notif;
 	/* The class device structure */

@@ -46,7 +46,7 @@ struct lcd_device {
 };
 
 extern struct lcd_device *lcd_device_register(const char *name,
-	void *devdata, struct lcd_properties *lp);
+	void *devdata, const struct lcd_properties *lp);
 extern void lcd_device_unregister(struct lcd_device *ld);
 
 #define to_lcd_device(obj) container_of(obj, struct lcd_device, class_dev)

Richard Purdie

2006-08-11 08:02 UTC #15 lore raw

Re: [patch 6/6] Move per-device data out of backlight_properties

Quoted 5 lines · Dmitry Torokhov

plain text document attachment (backlight-move-data.patch)
Backlight: move per-device data out of backlight_properties
Data such as current brightness belongs to a device and should not be part of a structure shared between several devices.

I agree there's an issue to address here. Looking at this patch very quickly, it breaks all the existing backlight drivers as they know about the variables in struct backlight_properties and all their references need to be updated e.g.: corgi_bl.c:

if (bd->props->power != FB_BLANK_UNBLANK)
intensity = 0;
if (bd->props->fb_blank != FB_BLANK_UNBLANK)
intensity = 0;

Thinking about this, ideally, struct backlight_properties would be left containing the backlight properties in but become part of struct backlight_device (and allocated with it). The drivers would provide a new struct backlight_ops instead of the properties struct at present and the function pointers would move to that structure.

Your other patches looked ok at a quick glance. I'll aim to test them against the corgi driver over the weekend and I look at reworking this one, unless you want beat me to it :)

Cheers,

Richard

Dmitry Torokhov

2006-08-11 12:27 UTC #16 lore raw

Re: [patch 6/6] Move per-device data out of backlight_properties

Quoted 12 lines · Richard Purdie

Quoted 5 lines · Dmitry Torokhov
plain text document attachment (backlight-move-data.patch)
Backlight: move per-device data out of backlight_properties
Data such as current brightness belongs to a device and should not be part of a structure shared between several devices.
I agree there's an issue to address here. Looking at this patch very quickly, it breaks all the existing backlight drivers as they know about the variables in struct backlight_properties and all their references need to be updated e.g.: corgi_bl.c:

Quoted 5 lines

if (bd->props->power != FB_BLANK_UNBLANK)
intensity = 0;
if (bd->props->fb_blank != FB_BLANK_UNBLANK)
intensity = 0;

Oops, I had them all updated but apparently lost that change. I'll fix it and resend.

Quoted 3 lines

Thinking about this, ideally, struct backlight_properties would be left containing the backlight properties in but become part of struct backlight_device (and allocated with it).

Why would you want to separate properties into a structure? You don't normally pass a set of properties around so I am not sure why would we need this...

Signature · 1 line

Dmitry

Richard Purdie

2006-08-11 12:56 UTC #17 lore raw

Re: [patch 6/6] Move per-device data out of backlight_properties

Quoted 8 lines · Dmitry Torokhov

Quoted 3 lines · Richard Purdie
Thinking about this, ideally, struct backlight_properties would be left containing the backlight properties in but become part of struct backlight_device (and allocated with it).
Why would you want to separate properties into a structure? You don't normally pass a set of properties around so I am not sure why would we need this...

The structure would just end up being optimised away by the compiler so would just serve to keep the properties themselves separate from the device data. I'm not so bothered about that but don't really want a struct backlight_properties around which just contains what would be better called something like struct backlight_ops. The backlight core has changed a fair bit and the names are starting to lose meaning.

Richard

Dmitry Torokhov

2006-08-11 13:10 UTC #18 lore raw

Re: [patch 6/6] Move per-device data out of backlight_properties

Quoted 10 lines · Richard Purdie

Quoted 8 lines · Dmitry Torokhov
Quoted 3 lines · Richard Purdie
Thinking about this, ideally, struct backlight_properties would be left containing the backlight properties in but become part of struct backlight_device (and allocated with it).
Why would you want to separate properties into a structure? You don't normally pass a set of properties around so I am not sure why would we need this...

Quoted 7 lines

The structure would just end up being optimised away by the compiler so would just serve to keep the properties themselves separate from the device data. I'm not so bothered about that but don't really want a struct backlight_properties around which just contains what would be better called something like struct backlight_ops. The backlight core has changed a fair bit and the names are starting to lose meaning.

There is some constant shared data, such as max_brightness, etc, that is still residing in backlist_poperties so _ops would not be quite correct.

Signature · 1 line

Dmitry