linux-kernel - Re: [PATCH] video: backlight: Remove backlight sysfs uevent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPXgP11oHZXbzCt72yutmeF-P=GudzPb9whTpWe5n9LDqOMM+g@mail.gmail.com>
Date:	Tue, 12 Nov 2013 02:19:32 +0100
From:	Kay Sievers <kay@...y.org>
To:	Henrique de Moraes Holschuh <hmh@....eng.br>
Cc:	Jingoo Han <jg1.han@...sung.com>,
	Kyungmin Park <kmpark@...radead.org>,
	Henrique de Moraes Holschuh <ibm-acpi@....eng.br>,
	linux-fbdev@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
	Richard Purdie <rpurdie@...ys.net>,
	ibm-acpi-devel@...ts.sourceforge.net,
	platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH] video: backlight: Remove backlight sysfs uevent

On Tue, Nov 12, 2013 at 1:56 AM, Henrique de Moraes Holschuh
<hmh@....eng.br> wrote:
> On Tue, 12 Nov 2013, Jingoo Han wrote:
>> On Tuesday, November 12, 2013 8:57 AM, Kyungmin Park wrote:
>> > From: Kyungmin Park <kyungmin.park@...sung.com>
>> >
>> > The most mobile phones have Ambient Light Sensors and it changes brightness according lux.
>> > It means it changes backlight brightness frequently by just writing sysfs node, so it generates uevent.
>> >
>> > Usually there's no user to use this backlight changes. But it forks udev worker threads and it takes
>> > about 5ms. The main problem is that it hurts other process activities. so remove it.
>> >
>> > Kay said
>> > "Uevents are for the major, low-frequent, global device state-changes,
>> >  not for carrying-out any sort of measurement data. Subsystems which
>> >  need that should use other facilities like poll()-able sysfs file or
>> >  any other subscription-based, client-tracking interface which does not
>> >  cause overhead if it isn't used. Uevents are not the right thing to
>> >  use here, and upstream udev should not paper-over broken kernel
>> >  subsystems."
>
> True.
>
> Now, let's take a look at reality: should you poll()/select() on a sysfs
> node that doesn't suport it, it will wait until the poll/select timeout
> happens (or EINTR happens), and userspace has absolutely NO way to detect
> whether a sysfs node has poll/select support.
>
> What happens if the sysfs interface did not provide poll/select support
> since day one, but rather added it later?  Nobody will use it for a *long*
> time, if ever... unless you actually took pains to version the sysfs
> interface, and people actually care.

If that's an issue, we can add a new "event" file, just for that.

>> 'thinkpad_acpi.c' uses the 'BACKLIGHT_UPDATE_SYSFS'.
>> Henrique, can we remove it?
>
> Can't you fix this by rate-limiting, or otherwise adding an attribute that
> backlight devices should set when they need to supress change events?

Yeah, great idea, fix a bad hack with another bad one on top. :)
Passing measurement data through uevents is just an utterly broken
idea which cannot be fixed.

> Is there a proper on-screen-display support path for the backlight class
> nowadays?  Otherwise, you'd be removing the only way userspace ever had to
> do proper OSD of backlight changes...

OSD drawing and event sounds usually happen as a fedback for
keypresses of brightness control, it would be weird to show up when
something else, like a light-sensor, adjusts the brightness in the
background.

Anyway, there might be the need for coordination and a new interface,
but uevents for measurement data need to die entirely; they make no
sense, never made any; and the sooner they are gone the better.

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/