2006-08-11 05:09:31

by Dmitry Torokhov

[permalink] [raw]
Subject: [patch 5/6] Convert to use mutexes instead of semaphores

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(-)

Index: work/drivers/video/backlight/backlight.c
===================================================================
--- work.orig/drivers/video/backlight/backlight.c
+++ 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);
Index: work/drivers/video/backlight/lcd.c
===================================================================
--- work.orig/drivers/video/backlight/lcd.c
+++ work/drivers/video/backlight/lcd.c
@@ -16,20 +16,26 @@

static ssize_t lcd_show_power(struct class_device *cdev, char *buf)
{
- int rc = -ENXIO;
+ int rc;
struct lcd_device *ld = to_lcd_device(cdev);

- down(&ld->sem);
+ rc = mutex_lock_interruptible(&ld->mutex);
+ if (rc)
+ return rc;
+
if (ld->props && ld->props->get_power)
rc = sprintf(buf, "%d\n", ld->props->get_power(ld));
- up(&ld->sem);
+ else
+ rc = -ENXIO;
+
+ mutex_unlock(&ld->mutex);

return rc;
}

static ssize_t lcd_store_power(struct class_device *cdev, const char *buf, size_t count)
{
- int rc = -ENXIO;
+ int rc;
char *endp;
struct lcd_device *ld = to_lcd_device(cdev);
int power = simple_strtoul(buf, &endp, 0);
@@ -40,33 +46,44 @@ static ssize_t lcd_store_power(struct cl
if (size != count)
return -EINVAL;

- down(&ld->sem);
+ rc = mutex_lock_interruptible(&ld->mutex);
+ if (rc)
+ return rc;
+
if (ld->props && ld->props->set_power) {
pr_debug("lcd: set power to %d\n", power);
ld->props->set_power(ld, power);
rc = count;
- }
- up(&ld->sem);
+ } else
+ rc = -ENXIO;
+
+ mutex_unlock(&ld->mutex);

return rc;
}

static ssize_t lcd_show_contrast(struct class_device *cdev, char *buf)
{
- int rc = -ENXIO;
+ int rc;
struct lcd_device *ld = to_lcd_device(cdev);

- down(&ld->sem);
+ rc = mutex_lock_interruptible(&ld->mutex);
+ if (rc)
+ return rc;
+
if (ld->props && ld->props->get_contrast)
rc = sprintf(buf, "%d\n", ld->props->get_contrast(ld));
- up(&ld->sem);
+ else
+ rc = -ENXIO;
+
+ mutex_unlock(&ld->mutex);

return rc;
}

static ssize_t lcd_store_contrast(struct class_device *cdev, const char *buf, size_t count)
{
- int rc = -ENXIO;
+ int rc;
char *endp;
struct lcd_device *ld = to_lcd_device(cdev);
int contrast = simple_strtoul(buf, &endp, 0);
@@ -77,13 +94,18 @@ static ssize_t lcd_store_contrast(struct
if (size != count)
return -EINVAL;

- down(&ld->sem);
+ rc = mutex_lock_interruptible(&ld->mutex);
+ if (rc)
+ return rc;
+
if (ld->props && ld->props->set_contrast) {
pr_debug("lcd: set contrast to %d\n", contrast);
ld->props->set_contrast(ld, contrast);
rc = count;
- }
- up(&ld->sem);
+ } else
+ rc = -ENXIO;
+
+ mutex_unlock(&ld->mutex);

return rc;
}
@@ -93,10 +115,16 @@ static ssize_t lcd_show_max_contrast(str
int rc = -ENXIO;
struct lcd_device *ld = to_lcd_device(cdev);

- down(&ld->sem);
+ rc = mutex_lock_interruptible(&ld->mutex);
+ if (rc)
+ return rc;
+
if (ld->props)
rc = sprintf(buf, "%d\n", ld->props->max_contrast);
- up(&ld->sem);
+ else
+ rc = -ENXIO;
+
+ mutex_unlock(&ld->mutex);

return rc;
}
@@ -104,6 +132,7 @@ static ssize_t lcd_show_max_contrast(str
static void lcd_class_release(struct class_device *dev)
{
struct lcd_device *ld = to_lcd_device(dev);
+
kfree(ld);
}

@@ -135,11 +164,13 @@ static int fb_notifier_callback(struct n
return 0;

ld = container_of(self, struct lcd_device, fb_notif);
- down(&ld->sem);
+
+ mutex_lock(&ld->mutex);
if (ld->props)
if (!ld->props->check_fb || ld->props->check_fb(evdata->info))
ld->props->set_power(ld, *(int *)evdata->data);
- up(&ld->sem);
+ mutex_unlock(&ld->mutex);
+
return 0;
}

@@ -166,7 +197,7 @@ struct lcd_device *lcd_device_register(c
if (!new_ld)
return ERR_PTR(-ENOMEM);

- init_MUTEX(&new_ld->sem);
+ mutex_init(&new_ld->mutex);
new_ld->props = lp;
new_ld->class_dev.class = &lcd_class;
strlcpy(new_ld->class_dev.class_id, name, KOBJ_NAME_LEN);
@@ -203,9 +234,9 @@ void lcd_device_unregister(struct lcd_de

pr_debug("lcd_device_unregister: name=%s\n", ld->class_dev.class_id);

- down(&ld->sem);
+ mutex_lock(&ld->mutex);
ld->props = NULL;
- up(&ld->sem);
+ mutex_unlock(&ld->mutex);

fb_unregister_client(&ld->fb_notif);
class_device_unregister(&ld->class_dev);
Index: work/include/linux/backlight.h
===================================================================
--- work.orig/include/linux/backlight.h
+++ 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 */
Index: work/include/linux/lcd.h
===================================================================
--- work.orig/include/linux/lcd.h
+++ 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 */


2006-08-11 12:59:06

by Dmitry Torokhov

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

On 8/11/06, Dmitry Torokhov <[email protected]> wrote:
> 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...]

--
Dmitry

2006-08-11 13:16:43

by Richard Purdie

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

On Fri, 2006-08-11 at 08:58 -0400, Dmitry Torokhov wrote:
> On 8/11/06, Dmitry Torokhov <[email protected]> wrote:
> > 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...]

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

2006-08-11 13:34:51

by Dmitry Torokhov

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

On 8/11/06, Richard Purdie <[email protected]> wrote:
> On Fri, 2006-08-11 at 08:58 -0400, Dmitry Torokhov wrote:
> > On 8/11/06, Dmitry Torokhov <[email protected]> wrote:
> > > 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...]
>
> It looks totally wrong.
>

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

> 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?

--
Dmitry

2006-08-11 13:42:21

by Michael Hanselmann

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

On Fri, Aug 11, 2006 at 09:34:44AM -0400, Dmitry Torokhov wrote:
> 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?


Attachments:
(No filename) (534.00 B)
(No filename) (189.00 B)
Download all attachments

2006-08-11 14:07:34

by Dmitry Torokhov

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

On 8/11/06, Michael Hanselmann <[email protected]> wrote:
> On Fri, Aug 11, 2006 at 09:34:44AM -0400, Dmitry Torokhov wrote:
> > 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?
>

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?

--
Dmitry

2006-08-11 16:46:26

by Richard Purdie

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

On Fri, 2006-08-11 at 10:07 -0400, Dmitry Torokhov wrote:
> On 8/11/06, Michael Hanselmann <[email protected]> wrote:
> > On Fri, Aug 11, 2006 at 09:34:44AM -0400, Dmitry Torokhov wrote:
> > > 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.

> > 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.

> > 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.

> 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



2006-08-11 17:21:05

by Dmitry Torokhov

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

On 8/11/06, Richard Purdie <[email protected]> wrote:
> On Fri, 2006-08-11 at 10:07 -0400, Dmitry Torokhov wrote:
> > On 8/11/06, Michael Hanselmann <[email protected]> wrote:
> > > On Fri, Aug 11, 2006 at 09:34:44AM -0400, Dmitry Torokhov wrote:
> > > > 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.
>
> > > 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.
>
> > > 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.
> */

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.

>
> 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.

>
> > 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.
>

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.

--
Dmitry

2006-08-29 20:54:37

by Michael Hanselmann

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

On Fri, Aug 11, 2006 at 05:45:51PM +0100, Richard Purdie wrote:
> On Fri, 2006-08-11 at 10:07 -0400, Dmitry Torokhov wrote:
> > On 8/11/06, Michael Hanselmann <[email protected]> wrote:
> > > 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]>

---
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
--- linux-2.6.18-rc5.orig/drivers/macintosh/via-pmu-backlight.c 2006-08-29 22:27:01.000000000 +0200
+++ linux-2.6.18-rc5/drivers/macintosh/via-pmu-backlight.c 2006-08-29 22:40:58.000000000 +0200
@@ -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)
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
--- linux-2.6.18-rc5.orig/drivers/video/aty/aty128fb.c 2006-08-29 22:27:01.000000000 +0200
+++ linux-2.6.18-rc5/drivers/video/aty/aty128fb.c 2006-08-29 22:41:24.000000000 +0200
@@ -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);
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
--- linux-2.6.18-rc5.orig/drivers/video/aty/atyfb_base.c 2006-08-29 22:27:01.000000000 +0200
+++ linux-2.6.18-rc5/drivers/video/aty/atyfb_base.c 2006-08-29 22:41:47.000000000 +0200
@@ -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);
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
--- linux-2.6.18-rc5.orig/drivers/video/aty/radeon_backlight.c 2006-08-29 22:27:01.000000000 +0200
+++ linux-2.6.18-rc5/drivers/video/aty/radeon_backlight.c 2006-08-29 22:39:23.000000000 +0200
@@ -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);
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
--- linux-2.6.18-rc5.orig/drivers/video/nvidia/nv_backlight.c 2006-08-29 22:27:01.000000000 +0200
+++ linux-2.6.18-rc5/drivers/video/nvidia/nv_backlight.c 2006-08-29 22:43:03.000000000 +0200
@@ -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);
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
--- linux-2.6.18-rc5.orig/drivers/video/riva/fbdev.c 2006-08-29 22:27:01.000000000 +0200
+++ linux-2.6.18-rc5/drivers/video/riva/fbdev.c 2006-08-29 22:43:26.000000000 +0200
@@ -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);