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 <linux-kernel@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: [patch 5/6] Convert to use mutexes instead of semaphores
    ... underlying races. ... >> locking is spread over the drivers. ... >> It's still easier to move to backlight core functions than to fix all ...
    (Linux-Kernel)
  • [FAQ] Frequently asked questions to rec.autos.sport.f1.moderated - Part 2 of 2
    ... The drivers ... Panasonic Toyota Racing ... How many races has y won? ... Who raced car y in z? ...
    (rec.autos.sport.f1.moderated)
  • Re: Weekend Mechanic: Anyone?
    ... Drivers who took the longest to get their first podium, ... epic races - refers to races where a car / driver ... Almost all stats you could have ever imagined on Team Williams. ... You are a fucking idiot. ...
    (rec.autos.sport.f1)
  • Re: JG Not To Be Forgotten
    ... If you're one of the mediocre drivers ... What I hate about the Chase is that it focuses too much attention on the ... for their preseason accomplishments so those races don't lose importance. ... They're just part of the equation that every Chaser has to deal with, ...
    (rec.autos.sport.nascar)
  • Re: What happened to Ryan Newman?
    ... about every other pole and about eight races just a couple of seasons ago? ... Over the years I have seen drivers go from also rans to media darlings, ... winning a lot of poles only without winning races. ... change to the Dodge nose since all of the Dodge drivers seem to have ...
    (rec.autos.sport.nascar)