[patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

Provide new implementation infrastructure that platforms may choose to use
when implementing the GPIO programming interface.  Platforms can update their
GPIO support to use this.  In many cases the incremental cost to access a
non-inlined GPIO should be on the order of a dozen instructions, so it won't
normally be a problem.  The upside is:

  * Providing two features which were "want to have (but OK to defer)" when
    GPIO interfaces were first discussed in November 2006:

    -	A "struct gpio_chip" to plug in GPIOs that aren't directly supported
	by SOC platforms, but come from FPGAs or other multifunction devices
	using conventional device registers (like UCB-1x00 or SM501 GPIOs,
	and southbridges in PCs with more open specs than usual).

    -	Full support for message-based GPIO expanders, where registers are
	accessed through sleeping I/O calls.  Previous support for these
	"cansleep" calls was just stubs.  (One example: the widely used
	pcf8574 I2C chips, with 8 GPIOs each.)

  * Including a non-stub implementation of the gpio_{request,free}() calls,
    making those calls much more useful.  The diagnostic labels are also
    recorded given DEBUG_FS, so /sys/kernel/debug/gpio can show a snapshot
    of all GPIOs known to this infrastructure.

The driver programming interfaces introduced in 2.6.21 do not change at all;
this infrastructure is entirely below those covers.

This opens the door to an augmented programming interface, addressing GPIOs
by chip and index.  That could be used as a performance tweak (lookup once
then cache, avoiding locking and lookup overheads) or to support transient
GPIOs not registered in the integer GPIO namespace (maybe a USB-to-GPIO
adapter, or GPIOs coupled to some other type of add-on card).

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 Documentation/gpio.txt     |   15 -
 include/asm-generic/gpio.h |  127 ++++++++++
 lib/Kconfig                |    6 
 lib/Kconfig.debug          |    9 
 lib/Makefile               |    2 
 lib/gpiolib.c              |  538 +++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 693 insertions(+), 4 deletions(-)

--- a/Documentation/gpio.txt	2007-11-05 19:44:55.000000000 -0800
+++ b/Documentation/gpio.txt	2007-11-05 19:44:57.000000000 -0800
@@ -63,7 +63,9 @@ and that can be critical for glue logic.
 Plus, this doesn't define an implementation framework, just an interface.
 One platform might implement it as simple inline functions accessing chip
 registers; another might implement it by delegating through abstractions
-used for several very different kinds of GPIO controller.
+used for several very different kinds of GPIO controller.  (There is some
+library code supporting such an implementation strategy, but drivers acting
+as clients to the GPIO interface should not care how it's implemented.)
 
 That said, if the convention is supported on their platform, drivers should
 use it when possible.  Platforms should declare GENERIC_GPIO support in
@@ -133,6 +135,7 @@ Spinlock-Safe GPIO access
 -------------------------
 Most GPIO controllers can be accessed with memory read/write instructions.
 That doesn't need to sleep, and can safely be done from inside IRQ handlers.
+(That includes hardirq contexts on RT kernels.)
 
 Use these calls to access such GPIOs:
 
@@ -223,6 +226,9 @@ Note that requesting a GPIO does NOT cau
 way; it just marks that GPIO as in use.  Separate code must handle any
 pin setup (e.g. controlling which pin the GPIO uses, pullup/pulldown).
 
+Also note that it's your responsibility to have stopped using a GPIO
+before you free it.
+
 
 GPIOs mapped to IRQs
 --------------------
@@ -238,7 +244,7 @@ map between them using calls like:
 
 Those return either the corresponding number in the other namespace, or
 else a negative errno code if the mapping can't be done.  (For example,
-some GPIOs can't used as IRQs.)  It is an unchecked error to use a GPIO
+some GPIOs can't be used as IRQs.)  It is an unchecked error to use a GPIO
 number that wasn't set up as an input using gpio_direction_input(), or
 to use an IRQ number that didn't originally come from gpio_to_irq().
 
@@ -299,6 +305,7 @@ Related to multiplexing is configuration
 pulldowns integrated on some platforms.  Not all platforms support them,
 or support them in the same way; and any given board might use external
 pullups (or pulldowns) so that the on-chip ones should not be used.
+(When a circuit needs 5 kOhm, on-chip 100 kOhm resistors won't do.)
 
 There are other system-specific mechanisms that are not specified here,
 like the aforementioned options for input de-glitching and wire-OR output.
@@ -308,8 +315,8 @@ commonly grouped in banks of 16 or 32, w
 banks.)  Some systems can trigger IRQs from output GPIOs.  Code relying on
 such mechanisms will necessarily be nonportable.
 
-Dynamic definition of GPIOs is not currently supported; for example, as
+Dynamic definition of GPIOs is not currently standard; for example, as
 a side effect of configuring an add-on board with some GPIO expanders.
 
 These calls are purely for kernel space, but a userspace API could be built
-on top of it.
+on top of them.
--- a/include/asm-generic/gpio.h	2007-11-05 19:44:55.000000000 -0800
+++ b/include/asm-generic/gpio.h	2007-11-05 19:44:57.000000000 -0800
@@ -1,6 +1,131 @@
 #ifndef _ASM_GENERIC_GPIO_H
 #define _ASM_GENERIC_GPIO_H
 
+#ifdef CONFIG_GPIO_LIB
+
+/* Platforms may implement their GPIO interface with library code,
+ * at a small performance cost for non-inlined operations.
+ *
+ * While the GPIO programming interface defines valid GPIO numbers
+ * to be in the range 0..MAX_INT, this library restricts them to the
+ * smaller range 0..ARCH_NR_GPIOS and allocates them in groups of
+ * ARCH_GPIOS_PER_CHIP (which will usually be the word size used for
+ * each bank of a SOC processor's integrated GPIO modules).
+ */
+
+#ifndef ARCH_NR_GPIOS
+#define ARCH_NR_GPIOS		512
+#endif
+
+#ifndef ARCH_GPIOS_PER_CHIP
+#define ARCH_GPIOS_PER_CHIP	BITS_PER_LONG
+#endif
+
+struct seq_file;
+
+/**
+ * struct gpio_chip - abstract a GPIO controller
+ * @label: for diagnostics
+ * @direction_input: configures signal "offset" as input, or returns error
+ * @get: returns value for signal "offset"; for output signals this
+ *	returns either the value actually sensed, or zero
+ * @direction_output: configures signal "offset" as output, or returns error
+ * @set: assigns output value for signal "offset"
+ * @dbg_show: optional routine to show contents in debugfs; default code
+ *	will be used when this is omitted, but custom code can show extra
+ *	state (such as pullup/pulldown configuration).
+ * @base: identifies the first GPIO number handled by this chip; or, if
+ *	negative during registration, requests dynamic ID allocation.
+ * @ngpio: the number of GPIOs handled by this controller; the value must
+ *	be at most ARCH_GPIOS_PER_CHIP, so the last GPIO handled is
+ *	(base + ngpio - 1).
+ * @can_sleep: flag must be set iff get()/set() methods sleep, as they
+ *	must while accessing GPIO expander chips over I2C or SPI
+ * @is_out: bit array where bit N is true iff GPIO with offset N has been
+ *	 called successfully to configure this as an output
+ *
+ * A gpio_chip can help platforms abstract various sources of GPIOs so
+ * they can all be accessed through a common programing interface.
+ * Example sources would be SOC controllers, FPGAs, multifunction
+ * chips, dedicated GPIO expanders, and so on.
+ *
+ * Each chip controls a number of signals, numbered 0..@ngpio, which are
+ * identified in method calls by an "offset" value.  When those signals
+ * are referenced through calls like gpio_get_value(gpio), the offset
+ * is calculated by subtracting @base from the gpio number.
+ */
+struct gpio_chip {
+	char			*label;
+
+	int			(*direction_input)(struct gpio_chip *chip,
+						unsigned offset);
+	int			(*get)(struct gpio_chip *chip,
+						unsigned offset);
+	int			(*direction_output)(struct gpio_chip *chip,
+						unsigned offset, int value);
+	void			(*set)(struct gpio_chip *chip,
+						unsigned offset, int value);
+	void			(*dbg_show)(struct seq_file *s,
+						struct gpio_chip *chip);
+	int			base;
+	u16			ngpio;
+	unsigned		can_sleep:1;
+
+	/* other fields are modified by the gpio library only */
+	DECLARE_BITMAP(is_out, ARCH_GPIOS_PER_CHIP);
+
+#ifdef CONFIG_DEBUG_FS
+	/* fat bits */
+	const char		*requested[ARCH_GPIOS_PER_CHIP];
+#else
+	/* thin bits */
+	DECLARE_BITMAP(requested, ARCH_GPIOS_PER_CHIP);
+#endif
+};
+
+/* returns true iff a given gpio signal has been requested;
+ * primarily for code dumping gpio_chip state.
+ */
+static inline int
+gpiochip_is_requested(struct gpio_chip *chip, unsigned offset)
+{
+#ifdef CONFIG_DEBUG_FS
+	return chip->requested[offset] != NULL;
+#else
+	return test_bit(offset, chip->requested);
+#endif
+}
+
+/* add/remove chips */
+extern int gpiochip_add(struct gpio_chip *chip);
+extern int __must_check gpiochip_remove(struct gpio_chip *chip);
+
+
+/* Always use the library code for GPIO management calls,
+ * or when sleeping may be involved.
+ */
+extern int gpio_request(unsigned gpio, const char *label);
+extern void gpio_free(unsigned gpio);
+
+extern int gpio_direction_input(unsigned gpio);
+extern int gpio_direction_output(unsigned gpio, int value);
+
+extern int gpio_get_value_cansleep(unsigned gpio);
+extern void gpio_set_value_cansleep(unsigned gpio, int value);
+
+
+/* A platform's <asm/gpio.h> code may want to inline the I/O calls when
+ * the GPIO is constant and refers to some always-present controller,
+ * giving direct access to chip registers and tight bitbanging loops.
+ */
+extern int __gpio_get_value(unsigned gpio);
+extern void __gpio_set_value(unsigned gpio, int value);
+
+extern int __gpio_cansleep(unsigned gpio);
+
+
+#else
+
 /* platforms that don't directly support access to GPIOs through I2C, SPI,
  * or other blocking infrastructure can use these wrappers.
  */
@@ -22,4 +147,6 @@ static inline void gpio_set_value_cansle
 	gpio_set_value(gpio, value);
 }
 
+#endif
+
 #endif /* _ASM_GENERIC_GPIO_H */
--- a/lib/Kconfig	2007-11-05 19:44:55.000000000 -0800
+++ b/lib/Kconfig	2007-11-05 19:44:57.000000000 -0800
@@ -141,4 +141,10 @@ config HAS_DMA
 config CHECK_SIGNATURE
 	bool
 
+#
+# gpiolib is selected if needed
+#
+config GPIO_LIB
+	boolean
+
 endmenu
--- a/lib/Kconfig.debug	2007-11-05 19:44:55.000000000 -0800
+++ b/lib/Kconfig.debug	2007-11-05 19:44:57.000000000 -0800
@@ -156,6 +156,15 @@ config DEBUG_SLAB
 	  allocation as well as poisoning memory on free to catch use of freed
 	  memory. This can make kmalloc/kfree-intensive workloads much slower.
 
+config DEBUG_GPIO
+	bool "Debug GPIO calls"
+	depends on DEBUG_KERNEL && GPIO_LIB
+	help
+	  Say Y here to add some extra checks to GPIO calls, ensuring that
+	  GPIOs have been properly initialized before they are used and
+	  that sleeping calls aren't made from nonsleeping contexts.  This
+	  can make bitbanged serial protocols slower.
+
 config DEBUG_SLAB_LEAK
 	bool "Memory leak debugging"
 	depends on DEBUG_SLAB
--- a/lib/Makefile	2007-11-05 19:44:55.000000000 -0800
+++ b/lib/Makefile	2007-11-05 19:44:57.000000000 -0800
@@ -68,6 +68,8 @@ obj-$(CONFIG_FAULT_INJECTION) += fault-i
 
 lib-$(CONFIG_GENERIC_BUG) += bug.o
 
+lib-$(CONFIG_GPIO_LIB) += gpiolib.o
+
 hostprogs-y	:= gen_crc32table
 clean-files	:= crc32table.h
 
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ b/lib/gpiolib.c	2007-11-05 19:44:57.000000000 -0800
@@ -0,0 +1,538 @@
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/irq.h>
+#include <linux/spinlock.h>
+
+#include <asm/gpio.h>
+
+
+/* Optional implementation infrastructure for GPIO interfaces.
+ *
+ * Platforms may want to use this if they tend to use very many GPIOs
+ * that aren't part of a System-On-Chip core; or across I2C/SPI/etc.
+ *
+ * When kernel footprint or instruction count is an issue, simpler
+ * implementations may be preferred.  The GPIO programming interface
+ * allows for inlining speed-critical get/set operations for common
+ * cases, so that access to SOC-integrated GPIOs can sometimes cost
+ * only an instruction or two per bit.
+ */
+
+
+/* When debugging, extend minimal trust to callers and platform code.
+ * Otherwise, minimize overhead in what may be bitbanging codepaths.
+ */
+#ifdef	CONFIG_DEBUG_GPIO
+#define	extra_checks	1
+#else
+#define	extra_checks	0
+#endif
+
+/* gpio_lock protects the table of chips and to gpio_chip->requested.
+ * While any gpio is requested, its gpio_chip is not removable.  It's
+ * a raw spinlock to ensure safe access from hardirq contexts, and to
+ * shrink bitbang overhead:  per-bit preemption would be very wrong.
+ */
+static raw_spinlock_t gpio_lock = __RAW_SPIN_LOCK_UNLOCKED;
+static struct gpio_chip *chips[DIV_ROUND_UP(ARCH_NR_GPIOS,
+					ARCH_GPIOS_PER_CHIP)];
+
+
+/* Warn when drivers omit gpio_request() calls -- legal but
+ * ill-advised when setting direction, and otherwise illegal.
+ */
+static void gpio_ensure_requested(struct gpio_chip *chip, unsigned offset)
+{
+	int		requested;
+
+#ifdef CONFIG_DEBUG_FS
+	requested = (int) chip->requested[offset];
+	if (!requested)
+		chip->requested[offset] = "[auto]";
+#else
+	requested = test_and_set_bit(offset, chip->requested);
+#endif
+
+	if (!requested)
+		printk(KERN_DEBUG "GPIO-%d autorequested\n",
+				chip->base + offset);
+}
+
+/* caller holds gpio_lock */
+static inline struct gpio_chip *gpio_to_chip(unsigned gpio)
+{
+	return chips[gpio / ARCH_GPIOS_PER_CHIP];
+}
+
+/**
+ * gpiochip_add() - register a gpio_chip
+ * @chip: the chip to register, with chip->base initialized
+ * Context: potentially before irqs or kmalloc will work
+ *
+ * Returns a negative errno if the chip can't be registered, such as
+ * because the chip->base is invalid or already associated with a
+ * different chip.  Otherwise it returns zero as a success code.
+ */
+int gpiochip_add(struct gpio_chip *chip)
+{
+	unsigned long	flags;
+	int		status = 0;
+	unsigned	id;
+
+	if (chip->base < 0 || (chip->base % ARCH_GPIOS_PER_CHIP) != 0)
+		return -EINVAL;
+	if ((chip->base + chip->ngpio) >= ARCH_NR_GPIOS)
+		return -EINVAL;
+	if (chip->ngpio > ARCH_GPIOS_PER_CHIP)
+		return -EINVAL;
+
+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
+
+	id = chip->base / ARCH_GPIOS_PER_CHIP;
+	if (chips[id] == NULL)
+		chips[id] = chip;
+	else
+		status = -EBUSY;
+
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+	return status;
+}
+EXPORT_SYMBOL_GPL(gpiochip_add);
+
+/**
+ * gpiochip_remove() - unregister a gpio_chip
+ * @chip: the chip to unregister
+ *
+ * A gpio_chip with any GPIOs still requested may not be removed.
+ */
+int gpiochip_remove(struct gpio_chip *chip)
+{
+	unsigned long	flags;
+	int		status = 0;
+	int		offset;
+	unsigned	id = chip->base / ARCH_GPIOS_PER_CHIP;
+
+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
+
+	for (offset = 0; offset < chip->ngpio; offset++) {
+		if (gpiochip_is_requested(chip, offset)) {
+			status = -EBUSY;
+			break;
+		}
+	}
+
+	if (status == 0) {
+		if (chips[id] == chip)
+			chips[id] = NULL;
+		else
+			status = -EINVAL;
+	}
+
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+	return status;
+}
+EXPORT_SYMBOL_GPL(gpiochip_remove);
+
+
+/* These "optional" allocation calls help prevent drivers from stomping
+ * on each other, and help provide better diagnostics in debugfs.
+ * They're called even less than the "set direction" calls.
+ */
+int gpio_request(unsigned gpio, const char *label)
+{
+	struct gpio_chip	*chip;
+	int			status = -EINVAL;
+	unsigned long		flags;
+
+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
+
+	chip = gpio_to_chip(gpio);
+	if (!chip)
+		goto done;
+	gpio -= chip->base;
+	if (gpio >= chip->ngpio)
+		goto done;
+
+	/* NOTE:  gpio_request() can be called in early boot,
+	 * before IRQs are enabled.
+	 */
+
+	status = 0;
+#ifdef CONFIG_DEBUG_FS
+	if (!label)
+		label = "?";
+	if (chip->requested[gpio])
+		status = -EBUSY;
+	else
+		chip->requested[gpio] = label;
+#else
+	if (test_and_set_bit(gpio, chip->requested))
+		status = -EBUSY;
+#endif
+
+done:
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+	return status;
+}
+EXPORT_SYMBOL_GPL(gpio_request);
+
+void gpio_free(unsigned gpio)
+{
+	unsigned long		flags;
+	struct gpio_chip	*chip;
+
+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
+
+	chip = gpio_to_chip(gpio);
+	if (!chip)
+		goto done;
+	gpio -= chip->base;
+	if (gpio >= chip->ngpio)
+		goto done;
+
+#ifdef CONFIG_DEBUG_FS
+	if (chip->requested[gpio])
+		chip->requested[gpio] = NULL;
+	else
+		chip = NULL;
+#else
+	if (!test_and_clear_bit(gpio, chip->requested))
+		chip = NULL;
+#endif
+	WARN_ON(extra_checks && chip == NULL);
+done:
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+}
+EXPORT_SYMBOL_GPL(gpio_free);
+
+
+
+/* Drivers MUST make configuration calls before get/set calls
+ *
+ * As a rule these aren't called more than once (except for drivers
+ * using the open-drain emulation idiom) so these are natural places
+ * to accumulate extra debugging checks.  Note that we can't rely on
+ * gpio_request() having been called beforehand.
+ */
+
+int gpio_direction_input(unsigned gpio)
+{
+	unsigned long		flags;
+	struct gpio_chip	*chip;
+	int			status = -EINVAL;
+
+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
+
+	chip = gpio_to_chip(gpio);
+	if (!chip || !chip->get || !chip->direction_input)
+		goto fail;
+	gpio -= chip->base;
+	if (gpio >= chip->ngpio)
+		goto fail;
+	gpio_ensure_requested(chip, gpio);
+
+	/* now we know the gpio is valid and chip won't vanish */
+
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+
+	might_sleep_if(extra_checks && chip->can_sleep);
+
+	status = chip->direction_input(chip, gpio);
+	if (status == 0)
+		clear_bit(gpio, chip->is_out);
+	return status;
+fail:
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+	return status;
+}
+EXPORT_SYMBOL_GPL(gpio_direction_input);
+
+int gpio_direction_output(unsigned gpio, int value)
+{
+	unsigned long		flags;
+	struct gpio_chip	*chip;
+	int			status = -EINVAL;
+
+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
+
+	chip = gpio_to_chip(gpio);
+	if (!chip || !chip->set || !chip->direction_output)
+		goto fail;
+	gpio -= chip->base;
+	if (gpio >= chip->ngpio)
+		goto fail;
+	gpio_ensure_requested(chip, gpio);
+
+	/* now we know the gpio is valid and chip won't vanish */
+
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+
+	might_sleep_if(extra_checks && chip->can_sleep);
+
+	status = chip->direction_output(chip, gpio, value);
+	if (status == 0)
+		set_bit(gpio, chip->is_out);
+	return status;
+fail:
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+	return status;
+}
+EXPORT_SYMBOL_GPL(gpio_direction_output);
+
+
+/* I/O calls are only valid after configuration completed; the relevant
+ * "is this a valid GPIO" error checks should already have been done.
+ *
+ * "Get" operations are often inlinable as reading a pin value register,
+ * and masking the relevant bit in that register.
+ *
+ * When "set" operations are inlinable, they involve writing that mask to
+ * one register to set a low value, or a different register to set it high.
+ * Otherwise locking is needed, so there may be little value to inlining.
+ */
+int __gpio_get_value(unsigned gpio)
+{
+	unsigned long		flags;
+	struct gpio_chip	*chip;
+
+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
+
+	chip = gpio_to_chip(gpio);
+	if (extra_checks)
+		gpio_ensure_requested(chip, gpio - chip->base);
+
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+
+	WARN_ON(extra_checks && chip->can_sleep);
+	return chip->get(chip, gpio - chip->base);
+}
+EXPORT_SYMBOL_GPL(__gpio_get_value);
+
+void __gpio_set_value(unsigned gpio, int value)
+{
+	unsigned long		flags;
+	struct gpio_chip	*chip;
+
+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
+
+	chip = gpio_to_chip(gpio);
+	if (extra_checks)
+		gpio_ensure_requested(chip, gpio - chip->base);
+
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+
+	WARN_ON(extra_checks && chip->can_sleep);
+	chip->set(chip, gpio - chip->base, value);
+}
+EXPORT_SYMBOL_GPL(__gpio_set_value);
+
+int __gpio_cansleep(unsigned gpio)
+{
+	unsigned long		flags;
+	struct gpio_chip	*chip;
+
+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
+
+	chip = gpio_to_chip(gpio);
+	if (extra_checks)
+		gpio_ensure_requested(chip, gpio - chip->base);
+
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+
+	return chip->can_sleep;
+}
+EXPORT_SYMBOL_GPL(__gpio_cansleep);
+
+
+
+/* There's no value in inlining GPIO calls that may sleep.
+ * Common examples include ones connected to I2C or SPI chips.
+ */
+
+int gpio_get_value_cansleep(unsigned gpio)
+{
+	unsigned long		flags;
+	struct gpio_chip	*chip;
+
+	might_sleep_if(extra_checks);
+
+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
+
+	chip = gpio_to_chip(gpio);
+	if (extra_checks)
+		gpio_ensure_requested(chip, gpio - chip->base);
+
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+
+	return chip->get(chip, gpio - chip->base);
+}
+EXPORT_SYMBOL_GPL(gpio_get_value_cansleep);
+
+void gpio_set_value_cansleep(unsigned gpio, int value)
+{
+	unsigned long		flags;
+	struct gpio_chip	*chip;
+
+	might_sleep_if(extra_checks);
+
+	local_irq_save(flags);
+	__raw_spin_lock(&gpio_lock);
+
+	chip = gpio_to_chip(gpio);
+	if (extra_checks)
+		gpio_ensure_requested(chip, gpio - chip->base);
+
+	__raw_spin_unlock(&gpio_lock);
+	local_irq_restore(flags);
+
+	chip->set(chip, gpio - chip->base, value);
+}
+EXPORT_SYMBOL_GPL(gpio_set_value_cansleep);
+
+
+#ifdef CONFIG_DEBUG_FS
+
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+
+static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+	unsigned	i;
+
+	for (i = 0; i < chip->ngpio; i++) {
+		unsigned	gpio;
+		int		is_out;
+
+		if (!chip->requested[i])
+			continue;
+
+		gpio = chip->base + i;
+		is_out = test_bit(i, chip->is_out);
+
+		seq_printf(s, " gpio-%-3d (%-12s) %s %s",
+			gpio, chip->requested[i],
+			is_out ? "out" : "in ",
+			chip->get
+				? (chip->get(chip, i) ? "hi" : "lo")
+				: "?  ");
+
+		if (!is_out) {
+			int		irq = gpio_to_irq(gpio);
+			struct irq_desc	*desc = irq_desc + irq;
+
+			/* This races with request_irq(), set_irq_type(),
+			 * and set_irq_wake() ... but those are "rare".
+			 *
+			 * More significantly, trigger type flags aren't
+			 * currently maintained by genirq.
+			 */
+			if (irq >= 0 && desc->action) {
+				char *trigger;
+
+				switch (desc->status & IRQ_TYPE_SENSE_MASK) {
+				case IRQ_TYPE_NONE:
+					trigger = "(default)";
+					break;
+				case IRQ_TYPE_EDGE_FALLING:
+					trigger = "edge-falling";
+					break;
+				case IRQ_TYPE_EDGE_RISING:
+					trigger = "edge-rising";
+					break;
+				case IRQ_TYPE_EDGE_BOTH:
+					trigger = "edge-both";
+					break;
+				case IRQ_TYPE_LEVEL_HIGH:
+					trigger = "level-high";
+					break;
+				case IRQ_TYPE_LEVEL_LOW:
+					trigger = "level-low";
+					break;
+				default:
+					trigger = "?trigger?";
+					break;
+				}
+
+				seq_printf(s, " irq-%d %s%s",
+					irq, trigger,
+					(desc->status & IRQ_WAKEUP)
+						? " wakeup" : "");
+			}
+		}
+
+		seq_printf(s, "\n");
+	}
+}
+
+static int gpiolib_show(struct seq_file *s, void *unused)
+{
+	struct gpio_chip	*chip;
+	unsigned		id;
+	int			started = 0;
+
+	/* REVISIT this isn't locked against gpio_chip removal ... */
+
+	for (id = 0; id < ARRAY_SIZE(chips); id++) {
+		chip = chips[id];
+		if (!chip)
+			continue;
+
+		seq_printf(s, "%sGPIOs %d-%d, %s%s:\n",
+				started ? "\n" : "",
+				chip->base, chip->base + chip->ngpio - 1,
+				chip->label ? : "generic",
+				chip->can_sleep ? ", can sleep" : "");
+		started = 1;
+		if (chip->dbg_show)
+			chip->dbg_show(s, chip);
+		else
+			gpiolib_dbg_show(s, chip);
+	}
+	return 0;
+}
+
+static int gpiolib_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, gpiolib_show, NULL);
+}
+
+static struct file_operations gpiolib_operations = {
+	.open		= gpiolib_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static int __init gpiolib_debugfs_init(void)
+{
+	/* /sys/kernel/debug/gpio */
+	(void) debugfs_create_file("gpio", S_IFREG | S_IRUGO,
+				NULL, NULL, &gpiolib_operations);
+	return 0;
+}
+postcore_initcall(gpiolib_debugfs_init);
+
+#endif	/* DEBUG_FS */

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Andrew Morton]

On Fri, 9 Nov 2007 11:36:19 -0800 David Brownell <david-b@pacbell.net> wrote:

> Provide new implementation infrastructure that platforms may choose to use
> when implementing the GPIO programming interface.  Platforms can update their
> GPIO support to use this.  In many cases the incremental cost to access a
> non-inlined GPIO should be on the order of a dozen instructions, so it won't
> normally be a problem.  The upside is:
> 
>   * Providing two features which were "want to have (but OK to defer)" when
>     GPIO interfaces were first discussed in November 2006:
> 
>     -	A "struct gpio_chip" to plug in GPIOs that aren't directly supported
> 	by SOC platforms, but come from FPGAs or other multifunction devices
> 	using conventional device registers (like UCB-1x00 or SM501 GPIOs,
> 	and southbridges in PCs with more open specs than usual).
> 
>     -	Full support for message-based GPIO expanders, where registers are
> 	accessed through sleeping I/O calls.  Previous support for these
> 	"cansleep" calls was just stubs.  (One example: the widely used
> 	pcf8574 I2C chips, with 8 GPIOs each.)
> 
>   * Including a non-stub implementation of the gpio_{request,free}() calls,
>     making those calls much more useful.  The diagnostic labels are also
>     recorded given DEBUG_FS, so /sys/kernel/debug/gpio can show a snapshot
>     of all GPIOs known to this infrastructure.
> 
> The driver programming interfaces introduced in 2.6.21 do not change at all;
> this infrastructure is entirely below those covers.
> 
> This opens the door to an augmented programming interface, addressing GPIOs
> by chip and index.  That could be used as a performance tweak (lookup once
> then cache, avoiding locking and lookup overheads) or to support transient
> GPIOs not registered in the integer GPIO namespace (maybe a USB-to-GPIO
> adapter, or GPIOs coupled to some other type of add-on card).
> 
> ...
>
> +
> +/* gpio_lock protects the table of chips and to gpio_chip->requested.
> + * While any gpio is requested, its gpio_chip is not removable.  It's
> + * a raw spinlock to ensure safe access from hardirq contexts, and to
> + * shrink bitbang overhead:  per-bit preemption would be very wrong.
> + */
> +static raw_spinlock_t gpio_lock = __RAW_SPIN_LOCK_UNLOCKED;

Well that's weird.

For starters, this initialisation will confound lockdep: it should use
DEFINE_SPINLOCK.

And the rationale seems dubious.  All you're saving here is a couple of
accesses to task_struct at spin_unlock()-time.  If the current task has a
preemption pending then yes, we'll schedule away but that's a very rare
thing and that's just what we're supposed to do.

So please tell us more about this.  Perhaps there are performance problems
with the current core preemption machinery.

> +	local_irq_save(flags);
> +	__raw_spin_lock(&gpio_lock);
>
> ...
> +	__raw_spin_unlock(&gpio_lock);
> +	local_irq_restore(flags);
> +	return status;
> +}

And of course if this code is converted to conventional locking, the above
becomes spin_lock_irqsave()/spin_lock_irqrestore() in many places.

> +/* There's no value in inlining GPIO calls that may sleep.

There's no value in inlining anything, hardly ;)

> +postcore_initcall(gpiolib_debugfs_init);

postcore_initcall() is unusual, hence a comment describing why it was
employed would be a good thing to have.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

On Monday 12 November 2007, Andrew Morton wrote:
> On Fri, 9 Nov 2007 11:36:19 -0800 David Brownell <david-b@pacbell.net> wrote:
> 
> > Provide new implementation infrastructure that platforms may choose to use
> > when implementing the GPIO programming interface.  ...
> > 
> > ...
> >
> > +/* gpio_lock protects the table of chips and to gpio_chip->requested.
> > + * While any gpio is requested, its gpio_chip is not removable.  It's
> > + * a raw spinlock to ensure safe access from hardirq contexts, and to
> > + * shrink bitbang overhead:  per-bit preemption would be very wrong.
> > + */
> > +static raw_spinlock_t gpio_lock = __RAW_SPIN_LOCK_UNLOCKED;
> 
> Well that's weird.
> 
> For starters, this initialisation will confound lockdep: it should use
> DEFINE_SPINLOCK.

Then it wouldn't be "raw"!  It seems info on raw spinlocks is out
of sync with current code.  Allegedly it should be possible to just
pass a raw_spinlock_t pointer to spin_lock_irqsave() and friends
and have GCC sort out the right stuff ... but that didn't work.

I speculate that either the design has changed (without fanfare),
or else that stuff is in RT kernels and has not yet gone upstream.


> And the rationale seems dubious.  All you're saving here is a couple of
> accesses to task_struct at spin_unlock()-time.  If the current task has a
> preemption pending then yes, we'll schedule away but that's a very rare
> thing and that's just what we're supposed to do.

Unfortunately, that's not what I observed/measured.

  http://marc.info/?l=linux-kernel&m=119429680220361&w=2

Plus, note the comment about hardirq context and RT environments.
When that spinlock is really a mutex, and the non-hardirq contexts
are tasks, non-raw spinlocks would wrongly prevent access to GPIOs
from true hardirq contexts.


> So please tell us more about this.  Perhaps there are performance problems
> with the current core preemption machinery.

The above email summarizes significant slowdown I observed using
just i2c-gpio, which happened to be easy to measure down to the
microsecond level.


> > +	local_irq_save(flags);
> > +	__raw_spin_lock(&gpio_lock);
> >
> > ...
> > +	__raw_spin_unlock(&gpio_lock);
> > +	local_irq_restore(flags);
> > +	return status;
> > +}
> 
> And of course if this code is converted to conventional locking, the above
> becomes spin_lock_irqsave()/spin_lock_irqrestore() in many places.

See above.  Allegedly we ought to be able to use those calls even
with raw spinlocks ... but that didn't work when I tried it. I'd
certainly prefer if it did work.


> > +/* There's no value in inlining GPIO calls that may sleep.
> 
> There's no value in inlining anything, hardly ;)

Remember that GPIOs do get used for bitbanging, and there's lots
of kernel code to make bit ops stay fast!

The canonical reason to inline GPIO operations is so that when
you bitbang some serial protocol, you're closer to one instruction
per bit toggle than fifty ... at one point I measured a factor of
nearly three speedup for byte-level bitbanging for SPI (ca. 70 Kbit
with quick function calls, vs about 2 Mbits inlined, ISTR) ... with
most of the overhead being to pass the next byte down to the bitbang
loop.  Using a bigger wordsize saw even more improvement.


> > +postcore_initcall(gpiolib_debugfs_init);
> 
> postcore_initcall() is unusual, hence a comment describing why it was
> employed would be a good thing to have.

Actually that one could easily be later, fs_initcall or whatnot.
I think that's debris from other versions.

- Dave

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Andrew Morton]

On Mon, 12 Nov 2007 14:32:53 -0800 David Brownell <david-b@pacbell.net> wrote:

> On Monday 12 November 2007, Andrew Morton wrote:
> > On Fri, 9 Nov 2007 11:36:19 -0800 David Brownell <david-b@pacbell.net> wrote:
> > 
> > > Provide new implementation infrastructure that platforms may choose to use
> > > when implementing the GPIO programming interface.  ...
> > > 
> > > ...
> > >
> > > +/* gpio_lock protects the table of chips and to gpio_chip->requested.
> > > + * While any gpio is requested, its gpio_chip is not removable.  It's
> > > + * a raw spinlock to ensure safe access from hardirq contexts, and to
> > > + * shrink bitbang overhead:  per-bit preemption would be very wrong.
> > > + */
> > > +static raw_spinlock_t gpio_lock = __RAW_SPIN_LOCK_UNLOCKED;
> > 
> > Well that's weird.
> > 
> > For starters, this initialisation will confound lockdep: it should use
> > DEFINE_SPINLOCK.
> 
> Then it wouldn't be "raw"!  It seems info on raw spinlocks is out
> of sync with current code.  Allegedly it should be possible to just
> pass a raw_spinlock_t pointer to spin_lock_irqsave() and friends
> and have GCC sort out the right stuff ... but that didn't work.
> 
> I speculate that either the design has changed (without fanfare),
> or else that stuff is in RT kernels and has not yet gone upstream.

Well whatever.  We shouldn't have to resort to caller-side party tricks
like this to get acceptable performance.

> 
> > And the rationale seems dubious.  All you're saving here is a couple of
> > accesses to task_struct at spin_unlock()-time.  If the current task has a
> > preemption pending then yes, we'll schedule away but that's a very rare
> > thing and that's just what we're supposed to do.
> 
> Unfortunately, that's not what I observed/measured.
> 
>   http://marc.info/?l=linux-kernel&m=119429680220361&w=2

There's something missing here.  It went from 6.4 usec/bit up to 11.2
usec/bit.  You seem to be saying that enabling preemption accounts for 1
usec.  What caused the rest of the slowdown?

> Plus, note the comment about hardirq context and RT environments.
> When that spinlock is really a mutex, and the non-hardirq contexts
> are tasks, non-raw spinlocks would wrongly prevent access to GPIOs
> from true hardirq contexts.

I don't understand all this about raw spinlocks and hardirq context. 
We use plain old spin_lock()/spin_unlock() in hardirq context all the time?

I'm still trying to understand what you've observed here.  Is it the case
that a single gpio operation went from 6.4 up to 11.2 usecs?  And that this
operation does a single spin_lock/spin_unlock?  If so, something would have
had to have gone seriously wrong for a spin_lock/unlock time to increase by
4.8 microseconds.

I assume these timings are from a reasonably fast machine?

> 
> > So please tell us more about this.  Perhaps there are performance problems
> > with the current core preemption machinery.
> 
> The above email summarizes significant slowdown I observed using
> just i2c-gpio, which happened to be easy to measure down to the
> microsecond level.
> 
> 
> > > +	local_irq_save(flags);
> > > +	__raw_spin_lock(&gpio_lock);
> > >
> > > ...
> > > +	__raw_spin_unlock(&gpio_lock);
> > > +	local_irq_restore(flags);
> > > +	return status;
> > > +}
> > 
> > And of course if this code is converted to conventional locking, the above
> > becomes spin_lock_irqsave()/spin_lock_irqrestore() in many places.
> 
> See above.  Allegedly we ought to be able to use those calls even
> with raw spinlocks ... but that didn't work when I tried it. I'd
> certainly prefer if it did work.

It would of course be better to not use raw spinlocks at all.  It's an
internal implementation detail and drivers shouldn't go poking at it:

box:/usr/src/linux-2.6.24-rc2> grep -rl raw_spinlock_t .
./arch/x86/kernel/traps_32.c
./arch/x86/kernel/traps_64.c
./arch/x86/kernel/tsc_sync.c
./arch/mips/kernel/gdb-stub.c
./arch/parisc/lib/bitops.c
./arch/powerpc/lib/locks.c
./arch/s390/lib/spinlock.c
./include/asm-alpha/spinlock.h
./include/asm-alpha/spinlock_types.h
./include/asm-arm/spinlock.h
./include/asm-arm/spinlock_types.h
./include/asm-generic/bitops/atomic.h
./include/asm-x86/spinlock_32.h
./include/asm-x86/spinlock_64.h
./include/asm-x86/spinlock_types.h
./include/asm-ia64/spinlock.h
./include/asm-ia64/spinlock_types.h
./include/asm-m32r/spinlock.h
./include/asm-m32r/spinlock_types.h
./include/asm-mips/spinlock.h
./include/asm-mips/spinlock_types.h
./include/asm-parisc/atomic.h
./include/asm-parisc/spinlock.h
./include/asm-parisc/spinlock_types.h
./include/asm-powerpc/spinlock.h
./include/asm-powerpc/spinlock_types.h
./include/asm-ppc/spinlock.h
./include/asm-s390/spinlock.h
./include/asm-s390/spinlock_types.h
./include/asm-sh/spinlock.h
./include/asm-sh/spinlock_types.h
./include/asm-sparc/spinlock.h
./include/asm-sparc/spinlock_types.h
./include/asm-sparc64/spinlock.h
./include/asm-sparc64/spinlock_types.h
./include/linux/spinlock.h
./include/linux/spinlock_types.h
./include/linux/spinlock_types_up.h
./include/linux/spinlock_up.h
./kernel/lockdep.c
./lib/spinlock_debug.c


So can we please drill down and work out what went wrong and see if we can
fix it all up properly?

> 
> > > +/* There's no value in inlining GPIO calls that may sleep.
> > 
> > There's no value in inlining anything, hardly ;)
> 
> Remember that GPIOs do get used for bitbanging, and there's lots
> of kernel code to make bit ops stay fast!
> 
> The canonical reason to inline GPIO operations is so that when
> you bitbang some serial protocol, you're closer to one instruction
> per bit toggle than fifty ... at one point I measured a factor of
> nearly three speedup for byte-level bitbanging for SPI (ca. 70 Kbit
> with quick function calls, vs about 2 Mbits inlined, ISTR) ... with
> most of the overhead being to pass the next byte down to the bitbang
> loop.  Using a bigger wordsize saw even more improvement.
> 
> 
> > > +postcore_initcall(gpiolib_debugfs_init);
> > 
> > postcore_initcall() is unusual, hence a comment describing why it was
> > employed would be a good thing to have.
> 
> Actually that one could easily be later, fs_initcall or whatnot.
> I think that's debris from other versions.
> 
> - Dave

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

> >	 It seems info on raw spinlocks is out
> > of sync with current code.  Allegedly it should be possible to just
> > pass a raw_spinlock_t pointer to spin_lock_irqsave() and friends
> > and have GCC sort out the right stuff ... but that didn't work.
> > 
> > I speculate that either the design has changed (without fanfare),
> > or else that stuff is in RT kernels and has not yet gone upstream.
> 
> Well whatever.  We shouldn't have to resort to caller-side party tricks
> like this to get acceptable performance.

I'd be happy if, as originally presented, it were possible to just
pass a raw_spinlock_t to spin_lock_irqsave() and friends.


> > > And the rationale seems dubious.  All you're saving here is a couple of
> > > accesses to task_struct at spin_unlock()-time.  If the current task has a
> > > preemption pending then yes, we'll schedule away but that's a very rare
> > > thing and that's just what we're supposed to do.

I don't think we really want to be scheduling on bit ops;
that seems a bit more fundamental.


> > 
> > Unfortunately, that's not what I observed/measured.
> > 
> >   http://marc.info/?l=linux-kernel&m=119429680220361&w=2
> 
> There's something missing here.  It went from 6.4 usec/bit up to 11.2
> usec/bit.  You seem to be saying that enabling preemption accounts for 1
> usec.  What caused the rest of the slowdown?

It was caused by some debug options I run with all the time,
which don't otherwise have easily observed performance effects.

DEBUG_PREEMPT, DEBUG_SPINLOCK, and DEBUG_SPINLOCK_SLEEP, but not
LOCKDEP (which I run with all the time, on platforms where it's
available).  There were other options too, which didn't matter.
This was with a basically idle system, note ... there was no
preemption happening.


> > Plus, note the comment about hardirq context and RT environments.
> > When that spinlock is really a mutex, and the non-hardirq contexts
> > are tasks, non-raw spinlocks would wrongly prevent access to GPIOs
> > from true hardirq contexts.
> 
> I don't understand all this about raw spinlocks and hardirq context. 
> We use plain old spin_lock()/spin_unlock() in hardirq context all the time?

Think RT kernels, with threaded IRQs and spinlocks morphed to mutexes.

Only irq handlers marked IRQF_NODELAY|IRQF_DISABLED will always be
"hard irqs" [1] which run the way you describe ... i.e. the way the
current mainstream kernel always works for IRQ handlers.


> I'm still trying to understand what you've observed here.  Is it the case
> that a single gpio operation went from 6.4 up to 11.2 usecs?

That was a single bitbanged I2C bit transfer, with embedded udelay()s.
I believe that was four gpio operations, as summarized at the end of
that email above.  Enabling preempt + debug increased the cost of
each GPIO call from whatever it was (reasonable) by 1.2 usecs.


> And that this 
> operation does a single spin_lock/spin_unlock?  If so, something would have
> had to have gone seriously wrong for a spin_lock/unlock time to increase by
> 4.8 microseconds.

See above; that was probably four lock/unlocks; 1.2 usec a pair.

But per-bit preemption for GPIOs would in any case be Just Wrong;
much like making test_bit(), set_bit(), and clear_bit() become
preemption points would be wrong.


> I assume these timings are from a reasonably fast machine?

They were from a relatively normal speed embedded processor; exactly
the kind which tends to both use these operations, and not uncommonly
also involve bitbanging.  In this case it was an AVR32 devel board,
BogoMips about 250 (comparable to ARM5 ones with BogoMips around 90).


> > > > +	local_irq_save(flags);
> > > > +	__raw_spin_lock(&gpio_lock);
> > > >
> > > > ...
> > > > +	__raw_spin_unlock(&gpio_lock);
> > > > +	local_irq_restore(flags);
> > > > +	return status;
> > > > +}
> > > 
> > > And of course if this code is converted to conventional locking, the above
> > > becomes spin_lock_irqsave()/spin_lock_irqrestore() in many places.
> > 
> > See above.  Allegedly we ought to be able to use those calls even
> > with raw spinlocks ... but that didn't work when I tried it. I'd
> > certainly prefer if it did work.
> 
> It would of course be better to not use raw spinlocks at all.  It's an
> internal implementation detail and drivers shouldn't go poking at it:

They weren't originally supposed to be like that.  They were to be
reserved for cases where their behavior was necessary ... like, to
synch with IRQF_NODELAY ("hard irq") handlers, and in other cases
where preemption would be wrong.


> So can we please drill down and work out what went wrong and see if we can
> fix it all up properly?

That's what I thought I did.  As I summarized it in the message
with the URL given above:

>>> Fortuntely it turns out those problems all go away if the gpiolib
>>> code uses a *raw* spinlock to guard its table lookups...
>>>
>>> That's as it should be, since the only substantive changes were to
>>> grab and release a lock, do one table lookup a bit differently, and
>>> add one indirect function call ... changes which should not have
>>> any visible performance impact on per-bit codepaths, and one might
>>> expect to cost on the order of one dozen instructions.

At most, I think one counter-argument would be that the costs of that
preempt (and debug) code aren't "enough" to merit using raw spinlocks;
a judgement call.

But that would have ignored the fact that such bit operations shouldn't
really be preemption points.  Only the "cansleep" versions should be;
they need to work through e.g. an I2C or SPI gpio expanders.

- Dave 

[1] http://rt.wiki.kernel.org/index.php/RT_PREEMPT_HOWTO

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Ingo Molnar]

* David Brownell <david-b@pacbell.net> wrote:

> > > I speculate that either the design has changed (without fanfare), 
> > > or else that stuff is in RT kernels and has not yet gone upstream.
> > 
> > Well whatever.  We shouldn't have to resort to caller-side party 
> > tricks like this to get acceptable performance.
> 
> I'd be happy if, as originally presented, it were possible to just 
> pass a raw_spinlock_t to spin_lock_irqsave() and friends.

that's a spinlock type abstraction of PREEMPT_RT, not of mainline. In 
mainline there's basically almost never any valid reason to use a raw 
spinlock - please use spinlock_t instead. Why do you want to use 
raw_spinlock_t?

	Ingo

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Nick Piggin]

On Wednesday 14 November 2007 06:22, David Brownell wrote:
> On Tuesday 13 November 2007, Ingo Molnar wrote:
> > * David Brownell <david-b@pacbell.net> wrote:
> > > > > I speculate that either the design has changed (without fanfare),
> > > > > or else that stuff is in RT kernels and has not yet gone upstream.
> > > >
> > > > Well whatever.  We shouldn't have to resort to caller-side party
> > > > tricks like this to get acceptable performance.
> > >
> > > I'd be happy if, as originally presented, it were possible to just
> > > pass a raw_spinlock_t to spin_lock_irqsave() and friends.
> >
> > that's a spinlock type abstraction of PREEMPT_RT, not of mainline.

Even when you're talking about the -rt tree, I suspect you really
shouldn't be using raw spinlocks, right? I mean, if you have a
timing critical operation, then you should ensure you have priorities
set correctly so that you simply don't get preempted.

By using a raw_spinlock_t, you're saying that you're more important
than anyone else (for the period of the critical section) including
processes which the user has explicitly set to a higher priority.

If you get preempted, then you should be happy, because <standard
RTOS nuclear power plant scenario> didn't occur.


> Any reason that stuff shouldn't move into mainline?

This sort of raw_spinlock_t arms race throughout drivers/ would be
a huge reason not to move it into mainline.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

On Tuesday 13 November 2007, Ingo Molnar wrote:
> 
> * David Brownell <david-b@pacbell.net> wrote:
> 
> > > > I speculate that either the design has changed (without fanfare), 
> > > > or else that stuff is in RT kernels and has not yet gone upstream.
> > > 
> > > Well whatever.  We shouldn't have to resort to caller-side party 
> > > tricks like this to get acceptable performance.
> > 
> > I'd be happy if, as originally presented, it were possible to just 
> > pass a raw_spinlock_t to spin_lock_irqsave() and friends.
> 
> that's a spinlock type abstraction of PREEMPT_RT, not of mainline.

Any reason that stuff shouldn't move into mainline?


> 	 Why do you want to use raw_spinlock_t?

Already answered elsewhere in this thread ... I'll highlight the
point that such bitops shouldn't be preemption points.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Nick Piggin]

On Wednesday 14 November 2007 17:52, David Brownell wrote:
> On Tuesday 13 November 2007, Andrew Morton wrote:

> > > I'll highlight the
> > > point that such bitops shouldn't be preemption points.
> >
> > Disagree.  *everything* should be a preemption point.
>
> So it's wrong that <asm-generic/bitops/atomic.h> uses the
> same calls to prevent *those* bitops from being prempted?

Upstream, all spinlocks prevent preemption. But these ones
are raw locks rather than normal locks probably because that
they are trivially an innermost and correct lock. It's not
going to be very helpful to bloat it and slow it down when
debugging is turned on (atomic operations are _very_ frequent).

For -rt, who knows. They probably don't even run on parisc
or sparc so don't really care at this point.


> Thus, that code should switch over to normal spinlocks...

For upstream, there is little reason to switch them over, and
some reasons not to.


> I believe that if I submitted patches to do that, there'd
> be a not-so-small riot.  And the arguments would all boil
> down to much the same ones applying to *these* bitops...

I don't think you have valid reasons. Also, core/arch code has
some different considerations to driver code.


> > For internal-implementation
> > details we do need to disable preemtion sometimes
> > (to prevent deadlocks and to protect per-cpu resources).
>
> You're certainly talking about "internal-implementation
> details" in this case.  It's not like the lock is used
> outside of those routines.  Or as if other implementations
> would even *need* such a lock.  (Just like the IRQ table,
> if the entries can't be removed and are all set up very
> early, locking would be pointless.)

Internal implementation details, as in: spin lock code has to
disable preemption otherwise they deadlock; get_cpu_var() has
to disable preemption to give coherent access to the variable
etc.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Andrew Morton]

On Tue, 13 Nov 2007 11:22:45 -0800 David Brownell <david-b@pacbell.net> wrote:

> On Tuesday 13 November 2007, Ingo Molnar wrote:
> > 
> > * David Brownell <david-b@pacbell.net> wrote:
> > 
> > > > > I speculate that either the design has changed (without fanfare), 
> > > > > or else that stuff is in RT kernels and has not yet gone upstream.
> > > > 
> > > > Well whatever.  We shouldn't have to resort to caller-side party 
> > > > tricks like this to get acceptable performance.
> > > 
> > > I'd be happy if, as originally presented, it were possible to just 
> > > pass a raw_spinlock_t to spin_lock_irqsave() and friends.
> > 
> > that's a spinlock type abstraction of PREEMPT_RT, not of mainline.
> 
> Any reason that stuff shouldn't move into mainline?
> 
> 
> > 	 Why do you want to use raw_spinlock_t?
> 
> Already answered elsewhere in this thread ...

Can't say I really understood the answer.  I don't think we actually know
where all of this extra time is being spent?

> I'll highlight the
> point that such bitops shouldn't be preemption points.

Disagree.  *everything* should be a preemption point.  For
internal-implementation details we do need to disable preemtion sometimes
(to prevent deadlocks and to protect per-cpu resources).  But those
preemption-off periods should be minimised.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Nick Piggin]

On Wednesday 14 November 2007 19:37, David Brownell wrote:
> On Tuesday 13 November 2007, Nick Piggin wrote:

> > Upstream, all spinlocks prevent preemption.
>
> I chose my wording carefully though.  A preemption point is
> more than just a small region where preemption isn't allowed.
>
> It's one of those where preemption is *INVITED* ...

With CONFIG_PREEMPT upstream, that's exactly the same (unless
you're considering preempt breaking points, which you don't
seem t obe).


> Now, in the RT case, I believe the rationale for inviting
> preemption when dropping a lock is largely related to the
> way priority inversion is handled.  When lock contention can
> block higher priority activities, dropping the lock must
> be able to trigger the relevant activity switch.

There is no specific inviting of preemption. The locks are
preemptible -- they can be preempted even while being *held*


> ... and the raw spinlocks don't support that machinery,
> while "normal" spinlocks become inversion-aware mutexes.
>
> > But these ones
> > are raw locks rather than normal locks probably because that
> > they are trivially an innermost and correct lock.
>
> As in the $SUBJECT case, I'd say.
>
> Although another point is related to "trivial":  the data
> is being protected through an operation too trivial to be
> worth paying for any of that priority logic.

A driver shouldn't get to decide that, IMO. And if there is
some policy in the -rt tree allowing these decisions, then
it's exactly the kind of thing we don't want upsream.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Nick Piggin]

On Wednesday 14 November 2007 19:20, David Brownell wrote:
> On Tuesday 13 November 2007, Nick Piggin wrote:

> > I mean, if you have a
> > timing critical operation, then you should ensure you have priorities
> > set correctly so that you simply don't get preempted.
>
> Which is why bitops like <asm-generic/bitops/atomic.h> use
> normal spinlocks.  Oh, wait, no they don't ...

No it isn't. It's nothing to do with that because upstream raw
spinlocks disable preemption as well; the reason for using raw
spinlocks in atomic.h is completely different.

Anyway, this whole line of argument is flawed. Even if the atomic.h
code is crap, that doesn't give any license to introduce more bad
code.


> > By using a raw_spinlock_t, you're saying that you're more important
> > than anyone else (for the period of the critical section) including
> > processes which the user has explicitly set to a higher priority.
>
> Nope.  Just saying that the relevant instructions (three, in the
> hot path I looked at, and not a case where priority inversion
> scenarios should be a concern) shouldn't be forcibly morphed into
> preemption points.  Any more than other bitops were (not).

Don't raw_spinlock_t's quite explictly disallow preemption in the
critical section? Eg. as opposed to spinlock_t, which does not, in 
-rt.


> If a higher priority task needs that CPU, nothing prevents it
> from being immediately descheduled.  Ditto handling a hardirq.
>
> All this does is prevent constant and needless checking for
> "do you want to preempt me now?" "now?" "now?" in "now?" the
> middle "now?" of "now?" i/o "now?" loops.

Actually that's wrong. By disabling preemption, you have to
explicitly check whether you should be preempted when you enable
it again. If you don't disable preemption then you don't have to
check anything -- you'll simply be preempted by asynchronous
events.

> > > Any reason that stuff shouldn't move into mainline?
> >
> > This sort of raw_spinlock_t arms race throughout drivers/ would be
> > a huge reason not to move it into mainline.
>
> This isn't driver code...

Semantics. It's not something like the scheduler or interrupt handler
or something that might have real reasons to use raw locks.


> I think you've just presented an argument why that stuff
> shouldn't really exist in -rt either... :)

Anyway, I'm not going to argue about -rt specific stuff any longer.
Whatever. It shouldn't go upstream like this though.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

On Tuesday 13 November 2007, Andrew Morton wrote:
> 
> > >      Why do you want to use raw_spinlock_t?
> > 
> > Already answered elsewhere in this thread ...
> 
> Can't say I really understood the answer.  I don't think we
> actually know where all of this extra time is being spent?

Reading that, one could think performance was the only factor.
But it isn't, and wasn't the one which really persuaded me.

And yes we do where that time went.  Although it seems that
repeating that info once again won't help...


> > I'll highlight the
> > point that such bitops shouldn't be preemption points.
> 
> Disagree.  *everything* should be a preemption point.

So it's wrong that <asm-generic/bitops/atomic.h> uses the
same calls to prevent *those* bitops from being prempted?

Thus, that code should switch over to normal spinlocks...

I believe that if I submitted patches to do that, there'd
be a not-so-small riot.  And the arguments would all boil
down to much the same ones applying to *these* bitops...


> For internal-implementation
> details we do need to disable preemtion sometimes 
> (to prevent deadlocks and to protect per-cpu resources).

You're certainly talking about "internal-implementation
details" in this case.  It's not like the lock is used
outside of those routines.  Or as if other implementations
would even *need* such a lock.  (Just like the IRQ table,
if the entries can't be removed and are all set up very
early, locking would be pointless.)


> But those 
> preemption-off periods should be minimised.

Like the three instructions in the "hot paths" for getting
and setting GPIO values, when using raw spinlocks ... check.

- Dave

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

On Tuesday 13 November 2007, Nick Piggin wrote:
> > > > I'd be happy if, as originally presented, it were possible to just
> > > > pass a raw_spinlock_t to spin_lock_irqsave() and friends.
> > >
> > > that's a spinlock type abstraction of PREEMPT_RT, not of mainline.
> 
> Even when you're talking about the -rt tree, I suspect you really
> shouldn't be using raw spinlocks, right?

That's one subject of discussion here.  I got the expected
knee-jerk responses (saying just that) -- right, ignore those
and aim for ones that look at the actual issues.


> I mean, if you have a 
> timing critical operation, then you should ensure you have priorities
> set correctly so that you simply don't get preempted.

Which is why bitops like <asm-generic/bitops/atomic.h> use
normal spinlocks.  Oh, wait, no they don't ...


Today, platform GPIO logic has no preemption points.

There are a few dozen implementations in the tree, not all
using the generic calls (yet!), none radically different.

Certainly when gpio_set_value() is inlined, it works very
much like set_bit()/clear_bit() on hardware with atomic ops.
Not a preemption point

But it often turns into a function call that needs to map
from GPIO number to GPIO bank, and then to a bitslice through
that bank's registers.  I know of only one platform which
even has any locking on those code paths(*) ... so again
those calls aren't preemption points.


> By using a raw_spinlock_t, you're saying that you're more important
> than anyone else (for the period of the critical section) including
> processes which the user has explicitly set to a higher priority.

Nope.  Just saying that the relevant instructions (three, in the
hot path I looked at, and not a case where priority inversion
scenarios should be a concern) shouldn't be forcibly morphed into
preemption points.  Any more than other bitops were (not).

If a higher priority task needs that CPU, nothing prevents it
from being immediately descheduled.  Ditto handling a hardirq.

All this does is prevent constant and needless checking for
"do you want to preempt me now?" "now?" "now?" in "now?" the
middle "now?" of "now?" i/o "now?" loops.


> > Any reason that stuff shouldn't move into mainline?
> 
> This sort of raw_spinlock_t arms race throughout drivers/ would be
> a huge reason not to move it into mainline.

This isn't driver code...

I think you've just presented an argument why that stuff
shouldn't really exist in -rt either... :)

- Dave

(*)  Exception, arch/arm/plat-omap/gpio.c uses spinlocks.
     Maybe I should say "misuses"; ISTR getting various
     lockdep complaints about the same spinlock being used
     both from IRQ and non-IRQ contexts.  In some common
     cases that lock could probably be eliminated, but
     if you look at the code you'll know why nobody's much
     wanted to clean it up.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

On Tuesday 13 November 2007, Nick Piggin wrote:
> On Wednesday 14 November 2007 17:52, David Brownell wrote:
> > On Tuesday 13 November 2007, Andrew Morton wrote:
> 
> > > > I'll highlight the
> > > > point that such bitops shouldn't be preemption points.
> > >
> > > Disagree.  *everything* should be a preemption point.
> >
> > So it's wrong that <asm-generic/bitops/atomic.h> uses the
> > same calls to prevent *those* bitops from being prempted?
> 
> Upstream, all spinlocks prevent preemption.

I chose my wording carefully though.  A preemption point is
more than just a small region where preemption isn't allowed.

It's one of those where preemption is *INVITED* ...

Now, in the RT case, I believe the rationale for inviting
preemption when dropping a lock is largely related to the
way priority inversion is handled.  When lock contention can
block higher priority activities, dropping the lock must
be able to trigger the relevant activity switch.

... and the raw spinlocks don't support that machinery,
while "normal" spinlocks become inversion-aware mutexes.


> But these ones 
> are raw locks rather than normal locks probably because that
> they are trivially an innermost and correct lock.

As in the $SUBJECT case, I'd say.

Although another point is related to "trivial":  the data
is being protected through an operation too trivial to be
worth paying for any of that priority logic.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Haavard Skinnemoen]

On Wed, 14 Nov 2007 00:37:57 -0800
David Brownell <david-b@pacbell.net> wrote:

> Although another point is related to "trivial":  the data
> is being protected through an operation too trivial to be
> worth paying for any of that priority logic.

But isn't there any way we can remove the lock from the fast path
altogether? What is it really protecting?

Since this is the code that runs under the lock (excluding the "extra
checks" case):

+static inline struct gpio_chip *gpio_to_chip(unsigned gpio)
+{
+	return chips[gpio / ARCH_GPIOS_PER_CHIP];
+}

I'd say it protects against chips being removed in the middle of the
operation. However, this comment says that chips cannot be removed
while any gpio on it is requested:

+/* gpio_lock protects the table of chips and to gpio_chip->requested.
+ * While any gpio is requested, its gpio_chip is not removable.  It's
+ * a raw spinlock to ensure safe access from hardirq contexts, and to
+ * shrink bitbang overhead:  per-bit preemption would be very wrong.
+ */

And since we drop the lock before calling the actual get/set bit
operation, we would be screwed anyway if the chip was removed during
the call to __gpio_set_value(). So what does the lock really buy us?

Håvard

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Ingo Molnar]

* David Brownell <david-b@pacbell.net> wrote:

> > 	 Why do you want to use raw_spinlock_t?
> 
> Already answered elsewhere in this thread ... I'll highlight the point 
> that such bitops shouldn't be preemption points.

raw_spinlock_t is a spinlock-internal implementation detail in the 
upstream kernel. You should _not_ be using it. The PREEMPT_RT 
raw_spinlock_t markings will move upstream in the future together with 
that feature, not piecemail wise with other patches. In other words: 
raw_spinlock_t in PREEMPT_RT != raw_spinlock_t in the upstream kernel. 
(and dont be worried, any necessary raw_spinlock_t annotations _will_ 
move upstream together with PREEMPT_RT, once that feature is being 
merged, so your code is not missing out on anything.)

The only code that should use raw_spinlock_t in the upstream kernel is 
the spinlock code and occasionally some debugging code. (If you still 
see any other strong reasons for it then please state it in simple, 
standalone terms without depending on context - your reasons were not 
clear to me so far so either me or you are confused about something and 
this is probably just a matter of communication.)

	Ingo

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Thomas Gleixner]

On Mon, 12 Nov 2007, David Brownell wrote:
> > I'm still trying to understand what you've observed here.  Is it the case
> > that a single gpio operation went from 6.4 up to 11.2 usecs?
> 
> That was a single bitbanged I2C bit transfer, with embedded udelay()s.
> I believe that was four gpio operations, as summarized at the end of
> that email above.  Enabling preempt + debug increased the cost of
> each GPIO call from whatever it was (reasonable) by 1.2 usecs.

This raw lock change is just pampering over the design problem of the
gpio lib:

There is no need to check for every single access to a GPIO pin,
whether the pin has a valid number and the chip, which provides access
to the pin, is still registered.

Each driver, which wants to access a pin, needs to make sure that

- the pin is available
- the pin is associated to this driver
- the chip reference count is incremented

_before_ it starts to do anything with the pin. Once this is done the
access to the pin is completely lock free except for the protection of
the chip hardware itself.

What you really need is a different API to request and free the access
to a particular pin like:

struct gpio_desc {
       struct gpio_chip *chip;
       int pinoffset;
};

int gpio_request_pin(int nr, struct gpio_desc *desc);
void gpio_release_pin(struct gpio_desc *desc);

where the description structure is caller provided to avoid static
allocation overhead for a large number of unused gpios.

The modification functions just use the description to access the pin
instead of the number and the per access lookup/locking.

The protection of the chip list can be converted to a mutex and
does not need to be a spinlock at all.

Thanks,

	tglx

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Nick Piggin]

On Thursday 15 November 2007 17:28, David Brownell wrote:
> On Tuesday 13 November 2007, Nick Piggin wrote:
> > > All this does is prevent constant and needless checking for
> > > "do you want to preempt me now?" "now?" "now?" in "now?" the
> > > middle "now?" of "now?" i/o "now?" loops.
> >
> > Actually that's wrong.
>
> Certainly it's right for the mainstream kernel.  Dropping a
> lock (other than a raw spinlock) does that checking; when a
> loop needs to acquire then drop such a lock, that's exactly
> what's going on.

Obviously a raw spinlock is no different from a regular
spinlock upstream.


> And in the RT kernel, it's got to do the same thing ...
> because dropping that lock may mean that a higher priority
> task should immediately run and grab the lock.

No, it wakes up the highest priority waiter. If that wakeup
causes a preemption, then that's fine, but it does not have
to explicitly check preemption when dropping the lock.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Nick Piggin]

On Thursday 15 November 2007 19:17, David Brownell wrote:
> On Wednesday 14 November 2007, Nick Piggin wrote:
> > > > > All this does is prevent constant and needless checking for
> > > > > "do you want to preempt me now?" "now?" "now?" in "now?" the
> > > > > middle "now?" of "now?" i/o "now?" loops.
> > > >
> > > > Actually that's wrong.
> > >
> > > Certainly it's right for the mainstream kernel.  Dropping a
> > > lock (other than a raw spinlock) does that checking; when a
> > > loop needs to acquire then drop such a lock, that's exactly
> > > what's going on.
> >
> > Obviously a raw spinlock is no different from a regular
> > spinlock upstream.
>
> Erm, no.  The raw ones don't have the extra logic when
> the lock gets dropped.

If you don't have preemption disabled already, then it is a
bug to use raw spinlocks. If you do have preemption disabled,
then a regular spinlock isn't going to check preemption after
the unlock either.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Nick Piggin]

On Thursday 15 November 2007 06:19, Nick Piggin wrote:
> On Thursday 15 November 2007 19:17, David Brownell wrote:
> > On Wednesday 14 November 2007, Nick Piggin wrote:
> > > > > > All this does is prevent constant and needless checking for
> > > > > > "do you want to preempt me now?" "now?" "now?" in "now?" the
> > > > > > middle "now?" of "now?" i/o "now?" loops.
> > > > >
> > > > > Actually that's wrong.
> > > >
> > > > Certainly it's right for the mainstream kernel.  Dropping a
> > > > lock (other than a raw spinlock) does that checking; when a
> > > > loop needs to acquire then drop such a lock, that's exactly
> > > > what's going on.
> > >
> > > Obviously a raw spinlock is no different from a regular
> > > spinlock upstream.
> >
> > Erm, no.  The raw ones don't have the extra logic when
> > the lock gets dropped.
>
> If you don't have preemption disabled already, then it is a
> bug to use raw spinlocks. If you do have preemption disabled,
> then a regular spinlock isn't going to check preemption after
> the unlock either.

And I might add that this is just trying to nitpick at a weak link
in the argument rather than prove anything important.

Even if you are avoiding preemption checks in upstream kernels,
this is probably like several instructions to do. So if you're
avoiding this for preformance reasons, then something's not right.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

On Tuesday 13 November 2007, Nick Piggin wrote:
> > > But these ones
> > > are raw locks rather than normal locks probably because that
> > > they are trivially an innermost and correct lock.
> >
> > As in the $SUBJECT case, I'd say.
> >
> > Although another point is related to "trivial":  the data
> > is being protected through an operation too trivial to be
> > worth paying for any of that priority logic.
> 
> A driver shouldn't get to decide that, IMO.

Not that I was talking about driver code...


> And if there is 
> some policy in the -rt tree allowing these decisions, then
> it's exactly the kind of thing we don't want upsream.

Making raw spinlocks available allows those decisions...

On the other hand, I can't see things working sanely
without them being available.  The problem seems to be
the usual one that crops up whenever anyone tries to
create a "bright line" decision algorithm in areas that
need flexibility.  Any "bright line" rule will lead to
wrong results.

- Dave

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

On Tuesday 13 November 2007, Nick Piggin wrote:
> > All this does is prevent constant and needless checking for
> > "do you want to preempt me now?" "now?" "now?" in "now?" the
> > middle "now?" of "now?" i/o "now?" loops.
> 
> Actually that's wrong.

Certainly it's right for the mainstream kernel.  Dropping a
lock (other than a raw spinlock) does that checking; when a
loop needs to acquire then drop such a lock, that's exactly
what's going on.

And in the RT kernel, it's got to do the same thing ...
because dropping that lock may mean that a higher priority
task should immediately run and grab the lock.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

On Wednesday 14 November 2007, Haavard Skinnemoen wrote:
> On Wed, 14 Nov 2007 00:37:57 -0800
> David Brownell <david-b@pacbell.net> wrote:
> 
> > Although another point is related to "trivial":  the data
> > is being protected through an operation too trivial to be
> > worth paying for any of that priority logic.
> 
> But isn't there any way we can remove the lock from the fast path
> altogether? What is it really protecting?

The integrity of the table.  Entries can be added and removed
(both operations being *RARE* which is good!) at any time.


> Since this is the code that runs under the lock

No, there's more than that.  This is what runs under it in
the hot paths, yes, but the gpio request/free paths do
more work than this.  (That includes direction setting,
since that can be an implicit request.)


> (excluding the "extra checks" case):
> 
> +static inline struct gpio_chip *gpio_to_chip(unsigned gpio)
> +{
> +	return chips[gpio / ARCH_GPIOS_PER_CHIP];
> +}
> 
> I'd say it protects against chips being removed in the middle of the
> operation. However, this comment says that chips cannot be removed
> while any gpio on it is requested:
> 
> +/* gpio_lock protects the table of chips and to gpio_chip->requested.
> + * While any gpio is requested, its gpio_chip is not removable.  It's
> + * a raw spinlock to ensure safe access from hardirq contexts, and to
> + * shrink bitbang overhead:  per-bit preemption would be very wrong.
> + */
>
> And since we drop the lock before calling the actual get/set bit
> operation, we would be screwed anyway if the chip was removed during
> the call to __gpio_set_value(). So what does the lock really buy us?

The get/set bit calls are the hot paths.  Locking on those paths
buys us a consistent locking policy, which is obviously correct.
It's consistent with the request/free paths.

But I think what you're suggesting is that the "requested" flag
is effectively a long-term lock, so grabbing the spinlock on
those paths is not necessary.  Right?

Hmm ... that makes some sense.  I hadn't started out thinking of
that "requested" flag as a lock bit, but in fact that's what it
ended up becoming.

Removing the spinlock from those paths -- at least in the "no
extra checks case" -- would let us avoid all this flamage about
whether raw spinlocks are ever OK.

I think I forsee a patch coming...

- Dave

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

On Wednesday 14 November 2007, Thomas Gleixner wrote:
> On Mon, 12 Nov 2007, David Brownell wrote:
> > > I'm still trying to understand what you've observed here.  Is it the case
> > > that a single gpio operation went from 6.4 up to 11.2 usecs?
> > 
> > That was a single bitbanged I2C bit transfer, with embedded udelay()s.
> > I believe that was four gpio operations, as summarized at the end of
> > that email above.  Enabling preempt + debug increased the cost of
> > each GPIO call from whatever it was (reasonable) by 1.2 usecs.
> 
> This raw lock change is just pampering over the design problem of the
> gpio lib:
> 
> There is no need to check for every single access to a GPIO pin,
> whether the pin has a valid number and the chip, which provides access
> to the pin, is still registered.

As Haavard had noted.  The "requested" flag is actually serving
as a longterm bit-level lock, which -- assuming well-behaved
callers, and no debug instrumentation -- obviates any need to
grab a spinlock in hot paths.


> Each driver, which wants to access a pin, needs to make sure that
> 
> - the pin is available
> - the pin is associated to this driver
> - the chip reference count is incremented
> 
> _before_ it starts to do anything with the pin. Once this is done the
> access to the pin is completely lock free except for the protection of
> the chip hardware itself.

That's what the gpio_request() call does, although it's using
something isomorphic to a refcount, not an actual refcount.

The key observation here is that we already *have* a bit which
is serving as a per-gpio lock.  It's just never been viewed as
a lock before.  :)


> The protection of the chip list can be converted to a mutex and
> does not need to be a spinlock at all.

No, we still need to use a spinlock to protect table changes.
The reason for that is briefly:

  - gpio_request()/gpio_free() have so far been optional.  Most
    platforms implement them as NOPs, not all drivers use them.
    (Having gpiolib in place should help change that ...)

  - gpio_direction_input()/gpio_direction_output() implicitly
    request the pins, if they weren't already requested.

  - Those input/output direction-setting calls may be called
    in IRQ contexts, which means (on non-RT kernels) no mutex.


So we're actually in good shape; just take out a bit of code
(or turn it into debugging instrumentation) and I don't think
anyone will complain about the locking any more.

- Dave

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

On Wednesday 14 November 2007, Thomas Gleixner wrote:
> struct gpio_desc {
>        struct gpio_chip *chip;
>        int pinoffset;
> };

Eric Miao has a different notion of "gpio_desc" more analagous to
the "irq_desc", so I'll not use that name for such a structure.

Let me call yours a "struct gpio_pin" instead -- purely for some
discussion here.


There's been some desire for a "gpio_pin" struct for entirely
different reasons ... as the identifiers passed into a slightly
different type of GPIO programming interface.  So for example

  static inline void gpiopin_set_value(struct gpio_pin *p, int v)
  {
	p->chip->set(p->chip, p->pinoffset, v);
  }

One reason to be interested in gpio_chip is that it it would
easily support interoperation between traditional GPIO ID schemes
(numbered to match the SOC at the heart of a given board) and
more dynamic ones that might be needed to use curiousities like
a set of USB-to-GPIO adapters.

- Dave

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Thomas Gleixner]

On Wed, 14 Nov 2007, David Brownell wrote:
> > The protection of the chip list can be converted to a mutex and
> > does not need to be a spinlock at all.
> 
> No, we still need to use a spinlock to protect table changes.
> The reason for that is briefly:
> 
>   - gpio_request()/gpio_free() have so far been optional.  Most
>     platforms implement them as NOPs, not all drivers use them.
>     (Having gpiolib in place should help change that ...)

By magically doing the request of the pin ? See below.

>   - gpio_direction_input()/gpio_direction_output() implicitly
>     request the pins, if they weren't already requested.

Eek, that's completely wrong. Allowing to access a resource _before_
it is assigned and then doing the assignment implicit is a really bad
idea.

>   - Those input/output direction-setting calls may be called
>     in IRQ contexts, which means (on non-RT kernels) no mutex.

There is no reason to do that if you actually have a useful reference
to the chip _before_ accessing the pin.
 
> So we're actually in good shape; just take out a bit of code
> (or turn it into debugging instrumentation) and I don't think
> anyone will complain about the locking any more.

This still does not solve the lookup, which is done for each operation
on a pin (direction setting, read, write).

     	 tglx

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Thomas Gleixner]

On Wed, 14 Nov 2007, David Brownell wrote:

> On Wednesday 14 November 2007, Thomas Gleixner wrote:
> > struct gpio_desc {
> > 	   struct gpio_chip *chip;
> > 	   int pinoffset;
> > };
> 
> Eric Miao has a different notion of "gpio_desc" more analagous to
> the "irq_desc", so I'll not use that name for such a structure.
> 
> Let me call yours a "struct gpio_pin" instead -- purely for some
> discussion here.
>
> 
> There's been some desire for a "gpio_pin" struct for entirely
> different reasons ... as the identifiers passed into a slightly
> different type of GPIO programming interface.  So for example
> 
>   static inline void gpiopin_set_value(struct gpio_pin *p, int v)
>   {
> 	p->chip->set(p->chip, p->pinoffset, v);
>   }
> 
> One reason to be interested in gpio_chip is that it it would
> easily support interoperation between traditional GPIO ID schemes
> (numbered to match the SOC at the heart of a given board) and
> more dynamic ones that might be needed to use curiousities like
> a set of USB-to-GPIO adapters.

So can we please do this right from the beginning instead of adding a
new library first which suffers from bad design compromises.

    tglx

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

On Wednesday 14 November 2007, Nick Piggin wrote:
> > > > All this does is prevent constant and needless checking for
> > > > "do you want to preempt me now?" "now?" "now?" in "now?" the
> > > > middle "now?" of "now?" i/o "now?" loops.
> > >
> > > Actually that's wrong.
> >
> > Certainly it's right for the mainstream kernel.  Dropping a
> > lock (other than a raw spinlock) does that checking; when a
> > loop needs to acquire then drop such a lock, that's exactly
> > what's going on.
> 
> Obviously a raw spinlock is no different from a regular
> spinlock upstream.

Erm, no.  The raw ones don't have the extra logic when
the lock gets dropped.

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

On Wednesday 14 November 2007, Thomas Gleixner wrote:
> On Wed, 14 Nov 2007, David Brownell wrote:
> > > The protection of the chip list can be converted to a mutex and
> > > does not need to be a spinlock at all.
> > 
> > No, we still need to use a spinlock to protect table changes.
> > The reason for that is briefly:
> > 
> >   - gpio_request()/gpio_free() have so far been optional.  Most
> >     platforms implement them as NOPs, not all drivers use them.
> >     (Having gpiolib in place should help change that ...)
> 
> By magically doing the request of the pin ? See below.
> 
> >   - gpio_direction_input()/gpio_direction_output() implicitly
> >     request the pins, if they weren't already requested.
> 
> Eek, that's completely wrong. Allowing to access a resource _before_
> it is assigned and then doing the assignment implicit is a really bad
> idea.

This is an artifact of making the GPIO interface easy to adopt,
by letting all the initial adopters wrap "legacy" SOC-specific
GPIO interfaces instead of creating a bunch of new platform code.

Only one of those legacy interfaces actually had a request/free
model.  (That was OMAP, which really needed it at one point to
help sort out driver conflicts ... current drivers don't seem
to trip over such conflicts, they've been fixed.)

You'll observe that a GPIO interface providing a standardized
interface to the capabilities of SOC registers doesn't *need*
to add a per-GPIO "is it allocated" flag ... most didn't.


However, now that we actually *have* such an interface, the
merit of such flags is becoming more apparent.  (As one person
observed:  "Eek"!)  And now gpiolib makes the costt to implement
that mechanism a lot lower.


> >   - Those input/output direction-setting calls may be called
> >     in IRQ contexts, which means (on non-RT kernels) no mutex.
> 
> There is no reason to do that if you actually have a useful reference
> to the chip _before_ accessing the pin.

See above ... until all platforms change and implement the
gpio_request() as more than a NOP, we *do* have a reason.

Fortunately drivers are tending to do the right thing (making
that call), but when they don't there are no warnings/errors.


> > So we're actually in good shape; just take out a bit of code
> > (or turn it into debugging instrumentation) and I don't think
> > anyone will complain about the locking any more.
> 
> This still does not solve the lookup, which is done for each operation
> on a pin (direction setting, read, write).

Are you saying that an array index is a cost to worry about here??

FYI, see the appended patch ... it's been sanity tested.

- Dave


=============
Simplify the hot-path (gpio value get/set) locking by taking advantage
of the fact that gpio_request() effectively locks that GPIO in memory.
This helps us avoid the belated complaints about whether this locking
was one of the places raw spinlocks are OK to use.

This is a net code shrink, most of it by removing explicit locking
calls from those hot paths.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 Documentation/gpio.txt |    3 -
 lib/gpiolib.c          |  118 ++++++++++++++-----------------------------------
 2 files changed, 36 insertions(+), 85 deletions(-)

--- a/Documentation/gpio.txt	2007-11-14 21:36:34.000000000 -0800
+++ b/Documentation/gpio.txt	2007-11-15 00:04:56.000000000 -0800
@@ -173,7 +173,8 @@ get to the head of a queue to transmit a
 This requires sleeping, which can't be done from inside IRQ handlers.
 
 Platforms that support this type of GPIO distinguish them from other GPIOs
-by returning nonzero from this call:
+by returning nonzero from this call (which may be issued even for GPIOs
+that have not been requested):
 
 	int gpio_cansleep(unsigned gpio);
 
--- a/lib/gpiolib.c	2007-11-15 00:04:50.000000000 -0800
+++ b/lib/gpiolib.c	2007-11-15 00:04:56.000000000 -0800
@@ -29,11 +29,10 @@
 #endif
 
 /* gpio_lock protects the table of chips and gpio_chip->requested.
- * While any gpio is requested, its gpio_chip is not removable.  It's
- * a raw spinlock to ensure safe access from hardirq contexts, and to
- * shrink bitbang overhead:  per-bit preemption would be very wrong.
+ * While any GPIO is requested, its gpio_chip is not removable;
+ * each GPIO's "requested" flag serves as a lock and refcount.
  */
-static raw_spinlock_t gpio_lock = __RAW_SPIN_LOCK_UNLOCKED;
+static DEFINE_SPINLOCK(gpio_lock);
 static struct gpio_chip *chips[DIV_ROUND_UP(ARCH_NR_GPIOS,
 					ARCH_GPIOS_PER_CHIP)];
 
@@ -86,8 +85,7 @@ int gpiochip_add(struct gpio_chip *chip)
 	if (chip->ngpio > ARCH_GPIOS_PER_CHIP)
 		return -EINVAL;
 
-	local_irq_save(flags);
-	__raw_spin_lock(&gpio_lock);
+	spin_lock_irqsave(&gpio_lock, flags);
 
 	id = chip->base / ARCH_GPIOS_PER_CHIP;
 	if (chips[id] == NULL)
@@ -95,8 +93,7 @@ int gpiochip_add(struct gpio_chip *chip)
 	else
 		status = -EBUSY;
 
-	__raw_spin_unlock(&gpio_lock);
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&gpio_lock, flags);
 	return status;
 }
 EXPORT_SYMBOL_GPL(gpiochip_add);
@@ -114,8 +111,7 @@ int gpiochip_remove(struct gpio_chip *ch
 	int		offset;
 	unsigned	id = chip->base / ARCH_GPIOS_PER_CHIP;
 
-	local_irq_save(flags);
-	__raw_spin_lock(&gpio_lock);
+	spin_lock_irqsave(&gpio_lock, flags);
 
 	for (offset = 0; offset < chip->ngpio; offset++) {
 		if (gpiochip_is_requested(chip, offset)) {
@@ -131,8 +127,7 @@ int gpiochip_remove(struct gpio_chip *ch
 			status = -EINVAL;
 	}
 
-	__raw_spin_unlock(&gpio_lock);
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&gpio_lock, flags);
 	return status;
 }
 EXPORT_SYMBOL_GPL(gpiochip_remove);
@@ -148,8 +143,7 @@ int gpio_request(unsigned gpio, const ch
 	int			status = -EINVAL;
 	unsigned long		flags;
 
-	local_irq_save(flags);
-	__raw_spin_lock(&gpio_lock);
+	spin_lock_irqsave(&gpio_lock, flags);
 
 	chip = gpio_to_chip(gpio);
 	if (!chip)
@@ -176,8 +170,7 @@ int gpio_request(unsigned gpio, const ch
 #endif
 
 done:
-	__raw_spin_unlock(&gpio_lock);
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&gpio_lock, flags);
 	return status;
 }
 EXPORT_SYMBOL_GPL(gpio_request);
@@ -187,8 +180,7 @@ void gpio_free(unsigned gpio)
 	unsigned long		flags;
 	struct gpio_chip	*chip;
 
-	local_irq_save(flags);
-	__raw_spin_lock(&gpio_lock);
+	spin_lock_irqsave(&gpio_lock, flags);
 
 	chip = gpio_to_chip(gpio);
 	if (!chip)
@@ -208,8 +200,7 @@ void gpio_free(unsigned gpio)
 #endif
 	WARN_ON(extra_checks && chip == NULL);
 done:
-	__raw_spin_unlock(&gpio_lock);
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&gpio_lock, flags);
 }
 EXPORT_SYMBOL_GPL(gpio_free);
 
@@ -229,8 +220,7 @@ int gpio_direction_input(unsigned gpio)
 	struct gpio_chip	*chip;
 	int			status = -EINVAL;
 
-	local_irq_save(flags);
-	__raw_spin_lock(&gpio_lock);
+	spin_lock_irqsave(&gpio_lock, flags);
 
 	chip = gpio_to_chip(gpio);
 	if (!chip || !chip->get || !chip->direction_input)
@@ -242,8 +232,7 @@ int gpio_direction_input(unsigned gpio)
 
 	/* now we know the gpio is valid and chip won't vanish */
 
-	__raw_spin_unlock(&gpio_lock);
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&gpio_lock, flags);
 
 	might_sleep_if(extra_checks && chip->can_sleep);
 
@@ -252,8 +241,7 @@ int gpio_direction_input(unsigned gpio)
 		clear_bit(gpio, chip->is_out);
 	return status;
 fail:
-	__raw_spin_unlock(&gpio_lock);
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&gpio_lock, flags);
 	return status;
 }
 EXPORT_SYMBOL_GPL(gpio_direction_input);
@@ -264,8 +252,7 @@ int gpio_direction_output(unsigned gpio,
 	struct gpio_chip	*chip;
 	int			status = -EINVAL;
 
-	local_irq_save(flags);
-	__raw_spin_lock(&gpio_lock);
+	spin_lock_irqsave(&gpio_lock, flags);
 
 	chip = gpio_to_chip(gpio);
 	if (!chip || !chip->set || !chip->direction_output)
@@ -277,8 +264,7 @@ int gpio_direction_output(unsigned gpio,
 
 	/* now we know the gpio is valid and chip won't vanish */
 
-	__raw_spin_unlock(&gpio_lock);
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&gpio_lock, flags);
 
 	might_sleep_if(extra_checks && chip->can_sleep);
 
@@ -287,8 +273,7 @@ int gpio_direction_output(unsigned gpio,
 		set_bit(gpio, chip->is_out);
 	return status;
 fail:
-	__raw_spin_unlock(&gpio_lock);
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&gpio_lock, flags);
 	return status;
 }
 EXPORT_SYMBOL_GPL(gpio_direction_output);
@@ -303,22 +288,22 @@ EXPORT_SYMBOL_GPL(gpio_direction_output)
  * When "set" operations are inlinable, they involve writing that mask to
  * one register to set a low value, or a different register to set it high.
  * Otherwise locking is needed, so there may be little value to inlining.
+ *
+ *------------------------------------------------------------------------
+ *
+ * IMPORTANT!!!  These hot paths -- get/set value -- assume that callers
+ * have correctly requested the GPIO.  That can include implicit requesting
+ * through direction setting.  Marking a gpio as requested locks its chip
+ * in memory, guaranteeing that these table lookups need no more locking.
+ *
+ * REVISIT when debugging, cosider adding some instrumentation to ensure
+ * that the GPIO was actually requested.
  */
 int __gpio_get_value(unsigned gpio)
 {
-	unsigned long		flags;
 	struct gpio_chip	*chip;
 
-	local_irq_save(flags);
-	__raw_spin_lock(&gpio_lock);
-
 	chip = gpio_to_chip(gpio);
-	if (extra_checks)
-		gpio_ensure_requested(chip, gpio - chip->base);
-
-	__raw_spin_unlock(&gpio_lock);
-	local_irq_restore(flags);
-
 	WARN_ON(extra_checks && chip->can_sleep);
 	return chip->get(chip, gpio - chip->base);
 }
@@ -326,19 +311,9 @@ EXPORT_SYMBOL_GPL(__gpio_get_value);
 
 void __gpio_set_value(unsigned gpio, int value)
 {
-	unsigned long		flags;
 	struct gpio_chip	*chip;
 
-	local_irq_save(flags);
-	__raw_spin_lock(&gpio_lock);
-
 	chip = gpio_to_chip(gpio);
-	if (extra_checks)
-		gpio_ensure_requested(chip, gpio - chip->base);
-
-	__raw_spin_unlock(&gpio_lock);
-	local_irq_restore(flags);
-
 	WARN_ON(extra_checks && chip->can_sleep);
 	chip->set(chip, gpio - chip->base, value);
 }
@@ -348,65 +323,40 @@ int __gpio_cansleep(unsigned gpio)
 {
 	unsigned long		flags;
 	struct gpio_chip	*chip;
+	int			retval;
 
-	local_irq_save(flags);
-	__raw_spin_lock(&gpio_lock);
-
+	/* it's OK to call this on a GPIO you've not requested */
+	spin_lock_irqsave(&gpio_lock, flags);
 	chip = gpio_to_chip(gpio);
-	if (extra_checks)
-		gpio_ensure_requested(chip, gpio - chip->base);
-
-	__raw_spin_unlock(&gpio_lock);
-	local_irq_restore(flags);
+	retval = chip->can_sleep;
+	spin_unlock_irqrestore(&gpio_lock, flags);
 
-	return chip->can_sleep;
+	return retval;
 }
 EXPORT_SYMBOL_GPL(__gpio_cansleep);
 
 
 
-/* There's no value in inlining GPIO calls that may sleep.
+/* There's no value in making it easy to inline GPIO calls that may sleep.
  * Common examples include ones connected to I2C or SPI chips.
  */
 
 int gpio_get_value_cansleep(unsigned gpio)
 {
-	unsigned long		flags;
 	struct gpio_chip	*chip;
 
 	might_sleep_if(extra_checks);
-
-	local_irq_save(flags);
-	__raw_spin_lock(&gpio_lock);
-
 	chip = gpio_to_chip(gpio);
-	if (extra_checks)
-		gpio_ensure_requested(chip, gpio - chip->base);
-
-	__raw_spin_unlock(&gpio_lock);
-	local_irq_restore(flags);
-
 	return chip->get(chip, gpio - chip->base);
 }
 EXPORT_SYMBOL_GPL(gpio_get_value_cansleep);
 
 void gpio_set_value_cansleep(unsigned gpio, int value)
 {
-	unsigned long		flags;
 	struct gpio_chip	*chip;
 
 	might_sleep_if(extra_checks);
-
-	local_irq_save(flags);
-	__raw_spin_lock(&gpio_lock);
-
 	chip = gpio_to_chip(gpio);
-	if (extra_checks)
-		gpio_ensure_requested(chip, gpio - chip->base);
-
-	__raw_spin_unlock(&gpio_lock);
-	local_irq_restore(flags);
-
 	chip->set(chip, gpio - chip->base, value);
 }
 EXPORT_SYMBOL_GPL(gpio_set_value_cansleep);

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Haavard Skinnemoen]

On Wed, 14 Nov 2007 22:50:17 -0800
David Brownell <david-b@pacbell.net> wrote:

> > Since this is the code that runs under the lock  
> 
> No, there's more than that.  This is what runs under it in
> the hot paths, yes, but the gpio request/free paths do
> more work than this.  (That includes direction setting,
> since that can be an implicit request.)

Yeah, I was talking about the hot paths. That's the only place where
raw vs. non-raw performance matters.

> The get/set bit calls are the hot paths.  Locking on those paths
> buys us a consistent locking policy, which is obviously correct.
> It's consistent with the request/free paths.
> 
> But I think what you're suggesting is that the "requested" flag
> is effectively a long-term lock, so grabbing the spinlock on
> those paths is not necessary.  Right?

Exactly. If we add two (quite reasonable) restrictions:
  * The GPIO framework must ensure that GPIO chips cannot be removed
    when one or more pins have been assigned to a client.
  * The client must ensure that it never calls gpio_free()
    simultaneously with gpio_[sg]et_value(), adding locking of its own if
    necessary.

this should be safe.

Håvard

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[Haavard Skinnemoen]

On Thu, 15 Nov 2007 00:20:33 -0800
David Brownell <david-b@pacbell.net> wrote:

> > >   - gpio_direction_input()/gpio_direction_output() implicitly
> > >     request the pins, if they weren't already requested.  
> > 
> > Eek, that's completely wrong. Allowing to access a resource _before_
> > it is assigned and then doing the assignment implicit is a really bad
> > idea.  
> 
> This is an artifact of making the GPIO interface easy to adopt,
> by letting all the initial adopters wrap "legacy" SOC-specific
> GPIO interfaces instead of creating a bunch of new platform code.

It's still ok for platforms that do not use gpiolib to provide NOP
gpio_request() and gpio_free() functions if they don't care about
tracking gpio usage. That doesn't mean we shouldn't require all drivers
to use those calls -- if they are implemented as empty inlines, it
won't cost anything to call them.

I'd rather speed up the gpio_direction_* functions a bit by removing
the implicit gpio_request() since they may be called a lot more
frequently and, as you pointed out, possibly from atomic context.

Håvard

Re: [patch 2.6.24-rc2 1/3] generic gpio -- gpio_chip support

[David Brownell]

On Thursday 15 November 2007, Haavard Skinnemoen wrote:
> On Thu, 15 Nov 2007 00:20:33 -0800
> David Brownell <david-b@pacbell.net> wrote:
> 
> > > >   - gpio_direction_input()/gpio_direction_output() implicitly
> > > >     request the pins, if they weren't already requested.  
> > > 
> > > Eek, that's completely wrong. Allowing to access a resource _before_
> > > it is assigned and then doing the assignment implicit is a really bad
> > > idea.  
> > 
> > This is an artifact of making the GPIO interface easy to adopt,
> > by letting all the initial adopters wrap "legacy" SOC-specific
> > GPIO interfaces instead of creating a bunch of new platform code.
> 
> It's still ok for platforms that do not use gpiolib to provide NOP
> gpio_request() and gpio_free() functions if they don't care about
> tracking gpio usage.

In fact they *must* provide some implementation of those calls,
and several platforms do exactly that.  (Since the legacy GPIO
code didn't use such primitives ... nothing to wrap.)


> That doesn't mean we shouldn't require all drivers 
> to use those calls -- if they are implemented as empty inlines, it
> won't cost anything to call them.

Thing is, issuing those calls is not currently mandatory ... so
requiring use of them would be an incompatible API change, which
would require auditing (and fixing, and testing) much platform code.


> I'd rather speed up the gpio_direction_* functions a bit by removing
> the implicit gpio_request() since they may be called a lot more
> frequently and, as you pointed out, possibly from atomic context.

In fact, possibly before IRQs get set up ... some platforms start
using GPIOs very early in system setup.

- Dave