linux-kernel - Re: [patch 2.6.20-rc1 1/6] GPIO core

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:	Mon, 1 Jan 2007 21:55:21 +0100
From:	Pavel Machek <pavel@....cz>
To:	David Brownell <david-b@...bell.net>
Cc:	Linux Kernel list <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...l.org>,
	Andrew Victor <andrew@...people.com>,
	Bill Gatliff <bgat@...lgatliff.com>,
	Haavard Skinnemoen <hskinnemoen@...el.com>, jamey.hicks@...com,
	Kevin Hilman <khilman@...sta.com>,
	Nicolas Pitre <nico@....org>,
	Russell King <rmk@....linux.org.uk>,
	Tony Lindgren <tony@...mide.com>,
	pHilipp Zabel <philipp.zabel@...il.com>
Subject: Re: [patch 2.6.20-rc1 1/6] GPIO core

Hi!

> > Well. when you see (something) = gpio_number + 5 ... you most likely
> > have an error.
> 
> One could surely apply that argument to hundreds of places throughout
> the kernel ... that doesn't make it a good one.  One of the downfalls
> of many "object oriented programming" efforts was this same desire to
> encapsulate things that don't need it; it's lose, not a don't-care.
> 
> Think of it as "cookies represented by integers" if you like.

typedef int gpio_t would hurt, and would serve as a useful
documentation hint. Furthermore, you could switch it to enum on
platform where it makes sense.

> > No, that's a wrong way. I want you to admit that gpio numbers are
> > opaque cookies noone should look at, and use (something like)
> > gpio_t... so that we can teach sparse to check them.
> 
> You're welcome to dream on.  :)
> 
> The goal here is not to create new complexity, it's to wrap the

...it adds exactly one line.

> > > > > +The get/set calls have no error returns because "invalid GPIO" should have
> > > > > +been reported earlier in gpio_set_direction().  However, note that not all
> > > > > +platforms can read the value of output pins; those that can't should always
> > > > > +return zero.  Also, these calls will be ignored for GPIOs that can't safely
> > > > > +be accessed without sleeping (see below).
> > > > 
> > > > 'Silently ignored' is ugly. BUG() would be okay there.
> > > 
> > > The reason for "silently ignored" is that we really don't want to be
> > > cluttering up the code (source or object) with logic to test for this
> > > kind of "can't happen" failure, especially since there's not going to
> > > be any way to _resolve_ such failures cleanly.
> > 
> > You may not want to clutter up code for one arch, but for some of them
> > maybe it is okay and welcome. Please do not document "silently
> > ignored" into API.
> 
> Those words were yours; so you can consider that already done.
> Should it instead say that's an (obviously unchecked) error?

Saying it is an error would be okay by me. (Or "Behaviour of these calls for
GPIOs that can't be safely accessed without sleeping is undefined.").

> You are perverting what _I_ said.  (As you've done before; stop
>that.)

Sorry.

> In terms of API specs, emitting any warning is traditionally
> out-of-scope.

Ok, I mis-read your specs as trying to push warnings into the scope.

> > > > > +... It is an unchecked error to use a GPIO
> > > > > +number that hasn't been marked as an input using gpio_set_direction(), or
> > > > 
> > > > It should be valid to do irqs on outputs,
> > > 
> > > Good point -- it _might_ be valid to do that, on some platforms.
> > > Such things have been used as a software IRQ trigger, which can
> > > make bootstrapping some things easier.
> > > 
> > > That's not incompatible with it being an error for portable code to
> > > try that, or with refusing to check it so that those platforms don't
> > > needlessly cause trouble!
> > 
> > I believe your text suggests it _is_ incompatible. Plus that seems to
> > mean that  architecture must not check for that error...
> 
> Which -- that portable code mustn't try such things?  That seems clearly
> wrong; that's what the "is an error" phrase means.  Or that code

Ok, "debug behaviour is out of scope, again".

What about "Behavour of reading a GPIO that hasn't been marked as an
input using gpio_set_direction() is implementation-defined"?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-
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/