Date:	Sun, 6 Mar 2011 23:50:02 -0700
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Ryan Mallon <ryan@...ewatersys.com>
Cc:	Peter Tyser <ptyser@...-inc.com>, linux-kernel@...r.kernel.org,
	Alek Du <alek.du@...el.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	David Brownell <dbrownell@...rs.sourceforge.net>,
	Eric Miao <eric.y.miao@...il.com>,
	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Joe Perches <joe@...ches.com>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Alexander Stein <alexander.stein@...tec-electronic.com>
Subject: Re: [PATCH v5 1/4] gpiolib: Add "unknown" direction support

On Mon, Mar 07, 2011 at 09:19:21AM +1300, Ryan Mallon wrote:
> 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.

I don't dispute any of the above points.  It is the sysfs interface
itself that I see as problematic.  It provides no discoverability and
it does not change the fact that once gpios are actually attached to
something they become specific purpose and users *absolutely must*
understand exactly what they are doing.

> > 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.

If it was a simple & correct fix, then I'd have no problem with it.
The current proposed solution doesn't meet that criteria for me (more
below)

> 
> > 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?

I'm far more interested in a solution that puts the onus on the GPIO
driver to populate the starting state at registration time, and
possibly to update it if it changes due to unrelated events.

Another consideration is that adding an 'unknown' value changes the
user space ABI, which must always be approached carefully.

> 
> > 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.

It solves the problem at the wrong level.  Rather than having an
'unknown' state, I'm far more interested in a solution that pushed
back on the drivers to correctly initialize the gpio starting state
at gpiochip_add() time.  This would be both simpler and more correct
from my perspective.

And, yes, I do realize that some controllers cannot be probed for the
starting state, but I strongly suspect them to be a minority and
starting state can be handled with pdata in those cases.

> 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?

No, I'm arguing that the driver should provide good data from the
outset.  If it does not, then defaulting to 'input' is both a
reasonable assumption, and is safe.

g.
--
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/

Powered by blists - more mailing lists