Re: powermac: More powermac backlight fixes

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Mon, 2006-07-24 at 20:03 -0700, Andrew Morton wrote:
> On Sat, 15 Jul 2006 15:09:00 +0200
> Michael Hanselmann <linux-kernel@hansmi.ch> wrote:
> 
> > This patch fixes several problems:
> > - The legacy backlight value might be set at interrupt time. Introduced
> >   a worker to prevent it from directly calling the backlight code.
> > - via-pmu allows the backlight to be grabbed, in which case we need to
> >   prevent other kernel code from changing the brightness.
> > - Don't send PMU requests in via-pmu-backlight when the machine is about
> >   to sleep or waking up.
> > - More Kconfig fixes.
> > 
> > ...
> >
> >  static void pmac_backlight_key_worker(void *data);
> > +static void pmac_backlight_set_legacy_worker(void *data);
> > +
> >  static DECLARE_WORK(pmac_backlight_key_work, pmac_backlight_key_worker, NULL);
> > +static DECLARE_WORK(pmac_backlight_set_legacy_work, pmac_backlight_set_legacy_worker, NULL);
> 
> I see schedule_work()s in there, but no flush_scheduled_work()s or anything
> like that.  Generally, this means there are races against rmmod, close(),
> etc.

Can this be rmmod'ed ? No code at hand right now, but the old version
certainly couldn't and the PMU driver who calls that can't neither. It's
not a file descriptor neither.

> >  
> > +void pmac_backlight_disable()
> > +{
> > +	atomic_inc(&kernel_backlight_disabled);
> > +}
> > +
> > +void pmac_backlight_enable()
> > +{
> > +	atomic_dec(&kernel_backlight_disabled);
> > +}
> > +
> 
> So if userspace calls ioctl(PMU_IOC_GRAB_BACKLIGHT) eleven times, eleven
> enables are needed?  (Actually, eleven open()/close() sequences, I think).

It makes sense to do it his way... You do that to stop kernel from
handling the backlight keys as long as at least one userspace client has
set the "grab" and thus basically says it handles them there. When they
all are gone and/or their fd closed, the backlight control returns to
the kernel default version 

> Methinks you wanted just
> 
> 	kernel_backlight_disabled = 1;

Nope, I think he's right there. There are cases where both gnome and
pbbuttons for example say to the kernel they handle the backlight keys.
A bit dodgy but it actually works and you want the kernel to properly
refcount before re-enabling it's own implementation. 

Ben.