linux-kernel - Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

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: <200804281745.59442.david-b@pacbell.net>
Date:	Mon, 28 Apr 2008 17:45:58 -0700
From:	David Brownell <david-b@...bell.net>
To:	Trent Piepho <tpiepho@...escale.com>
Cc:	lkml <linux-kernel@...r.kernel.org>,
	hartleys <hartleys@...ionengravers.com>,
	Ben Nizette <bn@...sdigital.com>,
	Mike Frysinger <vapier.adi@...il.com>,
	Bryan Wu <cooloney@...nel.org>
Subject: Re: [patch/rfc 2.6.25-git] gpio: sysfs interface

On Monday 28 April 2008, Trent Piepho wrote:
> On Mon, 28 Apr 2008, David Brownell wrote:
> > Simple sysfs interface for GPIOs.
> >
> >    /sys/class/gpio
> >        /gpio-N ... for each exported GPIO #N
> 
> I liked it better they way I had it, "label:N".

Those labels may not be available though; or valid in pathnames.

 
> When you have multiple GPIO sources, it's a lot easier to see where they are
> comming from if they use the chip label.  Especially if support for dynamic
> allocation of gpio numbers is written.

I'd rather see such stuff in a "chip_label" attribute.  Easy enough to
add such a thing.  (The dynamic allocation code is now kernel GIT...)

 
> > 	    /value ... always readable, writes fail except for output GPIOs
> > 	    /direction ... writable as: in, out (default low), high, low
> 
> You took away the code for the label field?  That was one of the features of
> my code that Ben Nizette mentioned as an advantage over a char-device
> interface.

Since it's not always available, I removed it.  Note that you're
talking about the label passed to gpio_request() here, not the one
applied to the gpio_chip.  I could restore this as a "gpio_label"
attribute, that's not always present ... but I'd rather not have
such "optional" stuff.

 
> >    	/control ... to request a GPIO be exported or unexported
> >
> > GPIOs may be exported by kernel code using gpio_export(), which should
> > be most useful for driver debugging.  Userspace may also ask that they
> > be exported by writing to the sysfs control file, helping to cope with
> > incomplete board support:
> 
> Why can't all gpios appear read-only in sysfs by default?

Typical SOC based systems have up to a few hundred pins that could
be used as GPIOs.  The number actually used as GPIOs is rarely more
than a dozen or two, with other pins generally muxed to a peripheral
function ... or not even connected.

There's no point in having a few hundred useless nodes in sysfs!


> > This adds a device pointer to "struct gpio_chip".  When GPIO providers
> > initialize that, sysfs gpio class devices become children of that device
> > instead of being "virtual" devices.  The (few) gpio_chip providers which
> > have such a device node have been updated.  (Some also needed to update
> > their module "owner" field ... for which missing kerneldoc was added.)
> 
> I don't see what's wrong with having devices add to gpiolib create a device

It can't know where in the device tree to put such device nodes,
or how to implement operations like suspend() and resume() for
such nodes.  Creating such nodes, and their drivers, is NOT a role
for generic code like gpiolib.


> for the gpio's to be the children of.  You said that some devices can't do
> this, but I don't see the difficulty.
> 
> platform_device_register_simple("my-gpio", 0, NULL, 0);
> 
> How hard is that?

Most GPIOs come from platform code that doesn't create such a device
today.  In cases like OMAP, GPIOs are usable even before memory
allocations can work, so creating and using device nodes for GPIO
controllers would involve updating code running VERY early in boot...

You're free to write patches creating such device nodes, and work
with the platform maintainers to convert their GPIO support to use
standard driver model devices and drivers, then merge the results.

 
> > Based on a patch from Trent Piepho <tpiepho@...escale.com>, and comments
> > from various folk including Hartley Sweeten.
> 
> I don't recall seeing those comments.  Where were they posted?

Some were on-list, some were off-list.  The comments have been
coming off and on for a few years now, so I'm certain you would
not have seen all of them.  (I wouldn't have, either!)


> > +		status = gpio_export(gpio);
> > +		if (status < 0)
> > +			gpio_free(gpio);
> > +		else
> > +			set_bit(FLAG_SYSFS, &gpio_desc[gpio].flags);
> > +
> > +	} else if (strcmp(cmd, "unexport") == 0) {
> > +		/* reject bogus commands (gpio_unexport ignores them) */
> > +		if (!gpio_is_valid(gpio))
> > +			goto fail;
> > +		if (!test_and_clear_bit(FLAG_SYSFS, &gpio_desc[gpio].flags))
> > +			goto fail;
> > +
> > +		gpio_free(gpio);
> 
> Maybe you could simplify the text parsing by having positive gpio numbers
> export the gpio and negative numbers un-export the gpio?  Then there would not
> be any need to parse a command with arguments.

I had that thought too.  Even though I think that solution
is kind o fugly.  ;)

If need be, that can be changed.

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