Messages in this thread | | | Subject | Re: [patch 5/6] Convert to use mutexes instead of semaphores | From | Richard Purdie <> | Date | Fri, 11 Aug 2006 17:45:51 +0100 |
| |
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
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |