linux-kernel - Re: [PATCH] misc: Add oqo-wmi driver for model 2 OQO backlight and rfkill control

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]

Date:	Wed, 3 Dec 2008 20:28:39 -0500 (EST)
From:	"Brian S. Julin" <bri@...ij.org>
To:	akpm@...ux-foundation.org
cc:	lenb@...nel.org, linux-kernel@...r.kernel.org, greg@...ah.com,
	linux-acpi@...r.kernel.org, mjg59@...f.ucam.org
Subject: Re: [PATCH] misc: Add oqo-wmi driver for model 2 OQO backlight and
 rfkill control

Yeah Matthew picked up the code before I did a final cleanup, at that
point I was still just waiting for someone(anyone) to test it.  So the edges are
rough and there's still some painter's tape stuck to the walls.  I guess I 
can afford some time to shave the fuzzies off, but Matthew and I should 
coordinate timewise if this all needs to be fixed before it's in a repository -- 
my opportunities to code are sparse, and his attention is probably a highly 
contested resource.

Thanks for the review (and Sven in other reply, too) -- a few items I 
probably would have missed cleaning.  Some stuff is the wmi core 
and sample drivers at least as they were when I coded, and those aren't 
from me.

On Wed, 3 Dec 2008, Andrew Morton wrote:
>> +static int smread_s16(u8 hi_addr, u8 lo_addr)
>> +{
>> +	s16 ret = -1;
>> +	acpi_status status;
>> +	u8 r;
>> +
>> +	/* Keep some ACPI routines off the SMBUS */
>> +	status = oqo_lock_smbus(1);
>> +	if (ACPI_FAILURE(status))
>> +		goto skip;
>
> Does this mean that we didn't take the lock?  If so, will running
> oqo_lock_smbus(0) be correct?

That does this (in DSL) after unpacking arguments and running a switch statement:

             Store (BUF1, VFLG)
                     Return (0x00)

...VFLG being a preallocated RAM value in the ACPI VM.  That's all.  Famous last 
words but I really don't see it failing ever, or if it does the ACPI VM's in
deep doodoo anyways.  At any rate it will not spin.

Really an ACPI doctor is needed to even explain why it's there at all -- 
there's a real mutex on the SMBUS, and this here is not it.

It seems to just be some sort of badly coded (in DSDT) advisory that keeps the 
machine awake.  I only even bothered to call it because it seemed like safer to 
do so than not.  We should really be the only writer, as ACPI routines merely 
read it (and only a few of them.)  The only access to it is through the WMI 
interface.  Can we just claim exclusivity on using that given this is an 
omnibus driver?

> This driver does quite a lot of casting of things which the compiler
> will happily do for us.  This could be viewed as having documentation
> benefit, but not much, and it is atypical.

Yep I know, I'm cast-and-paren trigger happy.  Keeps me sane, and has saved
my butt a few times, but can be cleaned to match style.

P.S. I think it was Sven that pointed out a

label:
  return

IIRC that was due to a cranky GCC warning FWIW.

--
Brian
--
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/