linux-kernel - Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

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: <201412050303.49206@pali>
Date:	Fri, 5 Dec 2014 03:03:49 +0100
From:	Pali Rohár <pali.rohar@...il.com>
To:	Darren Hart <dvhart@...radead.org>
Cc:	Matthew Garrett <mjg59@...f.ucam.org>,
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
	libsmbios-devel@...ts.us.dell.com, Srinivas_G_Gowda@...l.com,
	Michael_E_Brown@...l.com,
	Gabriele Mazzotta <gabriele.mzt@...il.com>
Subject: Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

On Wednesday 03 December 2014 09:43:21 Darren Hart wrote:
> On Sun, Nov 23, 2014 at 03:50:45PM +0100, Pali Rohár wrote:
> > This patch adds support for configuring keyboard backlight
> > settings on supported Dell laptops. It exports kernel leds
> > interface and uses Dell SMBIOS tokens or keyboard class
> > interface.
> > 
> > With this patch it is possible to set:
> > * keyboard backlight level
> > * timeout after which will be backlight automatically turned
> > off * input activity triggers (keyboard, touchpad, mouse)
> > which enable backlight * ambient light settings
> > 
> > Settings are exported via sysfs:
> > /sys/class/leds/dell::kbd_backlight/
> > 
> > Code is based on newly released documentation by Dell in
> > libsmbios project.
> 
> Hi Pali,
> 
> I would really like to see this broken up. Possibly adding
> levels and timeouts as separate patches for example. It is
> quite difficult to review this all at once in a reasonable
> amount of time.
> 

It is really hard to split this patch into more parts which every 
one will compile and ideally also work. In my opinion every 
single git commit should be possible to compile and should also 
work (it helps also other developers to use git bisect).

And because we need to pass all (previous unchanged) values in 
smbios call we need to parse all of them.

I understand that it could be hard to review long patch, but 
there are more parts which interacts and do not work without 
other. Also some of settings (e.g keyboard backlight level) could 
be changed via different ways (and on some machines only one is 
working) and also that smbios keyboard interface has complicated 
logic...

> During this review I caught a few more CodingStyle violations,
> and raised some questions about the timeout and levels
> mechanisms.
> 

I will fix style problems in next v3 patch.

What to do with Dan Carpenter patch which fixing kbd_timeout 
handling? Should I integrate it into v3? It does not apply on top 
of next v3 patch, because there will be new comment about quirk 
plus style fixes...

-- 
Pali Rohár
pali.rohar@...il.com

Download attachment "signature.asc " of type "application/pgp-signature" (199 bytes)