Re: [PATCH v5 1/4] gpiolib: Add "unknown" direction support

[Ryan Mallon <ryan@bluewatersys.com>]

On 03/06/2011 08:25 PM, Grant Likely wrote:

> Back to sysfs:  The sysfs gpio interface is useful for many reasons,
> but it is dangerous much in the same way that /dev/mem is dangerous.
> It gives userspace unfettered access to gpio resources without any
> clues about how it should be used.  I consider it bad practice to
> depend on the gpio sysfs interface for any real system/application.

In an embedded system, where both the kernel and the userspace are
provided by the hardware vendor, it can be completely sensible to rely
on gpio numbers in userspace (though I would also like to see the sysfs
interface able to use named access). There are some existing kernel
interfaces for common gpio functions such as leds and input devices, but
there are also many other valid uses for gpios. There are many reasons
for the access code to be in userspace too: It may be easier to write
and debug, depending on the setup of the device it may be easier to do
firmware upgrades of userspace than it is for the kernel, etc.

> gpio numbers could easily change or become unavailable if a kernel
> driver decides to use them (heck, I'd like to be rid of gpio
> numbers entirely, but that's a separate debate). I'd much rather see
> real device drivers be written for gpio interfaces instead of bodging
> things together from userspace.  Since the addition of an 'unknown'
> direction is solely for benefit of the sysfs interface, I don't (yet)
> see a valid argument for adding it.  *Who cares* if sysfs might report
> direction as 'input' inaccurately?

Because it is incorrect? It may also be useful for a userspace debug
tool to request all available gpios and show the current direction and
value of them.

> It may be mildly inconvenient to a
> human reader, but it doesn't change the usage model one iota because
> the direction still must be explicitly set before using the gpio.

I agree that the usage model should be to request and then explicitly
set the direction before use, but that isn't really an argument for
exporting incorrect information to userspace. The ABI should attempt to
prevent abuse of itself so that crappy userspace apps cannot be written.
Either exporting the direction as "unknown" or making the direction file
unreadable and the value file unreadable/unwritable until the direction
has been explicitly set?

> So, I'm going to say no to this patch.

The patch is small (it adds 9 lines) and fixes an incorrect value being
exported to userspace. By your argument we should actually remove the
ability to read the direction file in sysfs, since userspace must
explicitly set it, and therefore knows what the direction is?

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934