[ibm-acpi-devel] [ltp] Re: thinkpad-acpi release 0.17-20071002 uploaded to ibm-acpi.sf.net

[Henrique de Moraes Holschuh]

On Thu, 04 Oct 2007, Henrique de Moraes Holschuh wrote:
> On Thu, 04 Oct 2007, Chris Hanson wrote:
> > OK, I have a lot more information now.
> > 
> > Contrary to what I previously (thought? I) saw, the procfs interface now
> > works OK after writing a bad value to sysfs.  So this is a sysfs-only
> > problem, I think.
> > 
> > The bug is definitely in the backlight implementation, and not in
> > thinkpad-acpi.  What is happening is that after I write "8\n" to the
> > brightness attribute, those characters are never removed from the input
> > buffer, and subsequent input is just appended, e.g. "8\n7\n".  So the
> > string never parses after that.
> 
> Thanks.  That really narrows it down.  I will look into it.

Here's the offending code, from backlight.c:

static ssize_t backlight_store_power(struct class_device *cdev, const char
*buf, size_t count)
{
        int rc = -ENXIO;
        char *endp;
        struct backlight_device *bd = to_backlight_device(cdev);
        int power = simple_strtoul(buf, &endp, 0);
        size_t size = endp - buf;

        if (*endp && isspace(*endp))
                size++;
        if (size != count)
                return -EINVAL;

        mutex_lock(&bd->ops_lock);
        if (bd->ops) {
                pr_debug("backlight: set power to %d\n", power);
                bd->props.power = power;
                backlight_update_status(bd);
                rc = count;
        }
        mutex_unlock(&bd->ops_lock);

        return rc;
}

Note the way it handles spaces, and the count variable. It will skip at best
one space or EOL char.  Icky.  It should loop, not skip just one char.

But:

echo "6 " > /sys/class/backlight/thinkpad_screen/brightness and
echo " 6" > /sys/class/backlight/thinkpad_screen/brightness

EINVALs.  And it shoudln't, IMHO.  That's not what you are observing,
though, and a minor bug at best.  However, it *is* skipping the "\n" here,
so echo "6" > /sys/class/backlight/thinkpad_screen/brightness works just
fine here.

I just noticed the thinkpad-acpi sysfs handlers are not skipping leading
spaces, either.  Drat.  I will fix that.  It is an unrelated annoyance,
though.

If you use echo -n "6" > /sys/class/backlight/thinkpad_screen/brightness,
does it EINVALs?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh