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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
