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

["Dmitry Torokhov" <dmitry.torokhov@gmail.com>]

On 8/11/06, Richard Purdie <rpurdie@rpsys.net> wrote:
> On Fri, 2006-08-11 at 08:58 -0400, Dmitry Torokhov wrote:
> > On 8/11/06, Dmitry Torokhov <dtor@insightbb.com> 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