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

[Richard Purdie <rpurdie@rpsys.net>]

On Fri, 2006-08-11 at 10:07 -0400, Dmitry Torokhov wrote: 
> On 8/11/06, Michael Hanselmann <linux-kernel@hansmi.ch> 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