Re: [PATCH] applesmc - fix crash when activating a led trigger on the keyboard backlight

[Richard Purdie <rpurdie@rpsys.net>]

Hi,

On Sat, 2007-04-14 at 16:05 +0800, Nicolas Boichat wrote:
> Bradley Hook wrote:
> > Slightly off-topic, but I've been experiencing a minor bug in the
> > keyboard backlight feature.
> >
> > I say it is "minor" only because the feature serves no real functional
> > purpose. You can activate a trigger called "heartbeat" that will cause
> > the keyboard light to pulse at a speed based on the CPU usage. On my
> > MBP17, after activating this trigger the machine will either lock-up
> > or core dump within about a minute (timing is not consistent).
> >   
> 
> This is caused by the fact applesmc_backlight_set locks a mutex (or more
> precisely sleeps while trying to lock a mutex) while being in a softirq
> context.
> 
> This might be obvious for others, but it was not for me, and there is
> absolutely no mention in the documentation of the fact it is not always
> safe to sleep in the brightness_set handler of a led_class device (it is
> safe when it is called because someone wrote to the brightness sysfs file).

Its never safe for a brightness_set handler to sleep. They're designed
to be called from interrupt context and as you've noted, several
triggers do that.

The solution if you have locks like your case is to offload the work to
a workqueue, there is simply no other way to do it.

I'll have a look to see if I can improve the documentation (patches
welcome).

> Also, the led-trigger code seems buggy when it comes to locking. Setting
> CONFIG_DEBUG_SPINLOCK_SLEEP causes a lot a warnings. The problem is that
> the list of triggers is locked using a rw spinlock, but the rest of the
> code seems to ignore that, and calls a lot of functions which can sleep
> (kzalloc with GFP_KERNEL, sysfs_add_file, mutex_lock, etc...). I think
> the list lock should be converted to a mutex (or maybe modified to use
> RCU). I'm not very experienced in that domain, but if you want I can
> provide a patch for this.

Someone else has mentioned this within the past few days and there is a
problem with the trigger->activate and ->deactivate calls occurring
within a spinlock. Its not a simple problem to solve unfortunately and
you can't just convert to a mutex but I'm looking at it. Again, patches
welcome but its going to need careful thought.

Cheers,

Richard