Re: powermac: More powermac backlight fixes

On Mon, 2006-07-24 at 20:03 -0700, Andrew Morton wrote:

On Sat, 15 Jul 2006 15:09:00 +0200
Michael Hanselmann [off-list ref] wrote:

quoted

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.

quoted

 
+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.