| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <507B9D05.1000204@gmail.com>
Date: Mon, 15 Oct 2012 16:20:05 +1100
From: Ryan Mallon <rmallon@...il.com>
To: Roland Stigge <stigge@...com.de>
CC: grant.likely@...retlab.ca, linus.walleij@...aro.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
w.sang@...gutronix.de, jbe@...gutronix.de, plagnioj@...osoft.com,
highguy@...il.com, broonie@...nsource.wolfsonmicro.com
Subject: Re: [PATCH RFC 1/6 v3] gpio: Add a block GPIO API to gpiolib
On 13/10/12 06:11, Roland Stigge wrote:
> The recurring task of providing simultaneous access to GPIO lines (especially
> for bit banging protocols) needs an appropriate API.
>
> This patch adds a kernel internal "Block GPIO" API that enables simultaneous
> access to several GPIOs. This is done by abstracting GPIOs to an n-bit word:
> Once requested, it provides access to a group of GPIOs which can range over
> multiple GPIO chips.
>
> Signed-off-by: Roland Stigge <stigge@...com.de>
Hi Roland,
Some comments below.
~Ryan
> ---
>
> Documentation/gpio.txt | 45 +++++++++
> drivers/gpio/gpiolib.c | 223 +++++++++++++++++++++++++++++++++++++++++++++
> include/asm-generic/gpio.h | 14 ++
> include/linux/gpio.h | 61 ++++++++++++
> 4 files changed, 343 insertions(+)
>
> --- linux-2.6.orig/Documentation/gpio.txt
> +++ linux-2.6/Documentation/gpio.txt
> @@ -439,6 +439,51 @@ slower clock delays the rising edge of S
> signaling rate accordingly.
>
>
> +Block GPIO
> +----------
> +
> +The above described interface concentrates on handling single GPIOs. However,
> +in applications where it is critical to set several GPIOs at once, this
> +interface doesn't work well, e.g. bit-banging protocols via grouped GPIO lines.
> +Consider a GPIO controller that is connected via a slow I2C line. When
> +switching two or more GPIOs one after another, there can be considerable time
> +between those events. This is solved by an interface called Block GPIO:
The emulate behaviour of gpio block switches gpios one after the other.
Is the problem only solved if the block_get/block_set callbacks can be
implemented?
> +struct gpio_block *gpio_block_create(unsigned int *gpios, size_t size);
> +
> +This creates a new block of GPIOs as a list of GPIO numbers with the specified
> +size which are accessible via the returned struct gpio_block and the accessor
> +functions described below. Please note that you need to request the GPIOs
> +separately via gpio_request(). An arbitrary list of globally valid GPIOs can be
> +specified, even ranging over several gpio_chips. Actual handling of I/O
> +operations will be done on a best effort base, i.e. simultaneous I/O only where
> +possible by hardware and implemented in the respective GPIO driver. The number
> +of GPIOs in one block is limited to 32 on a 32 bit system, and 64 on a 64 bit
> +system. However, several blocks can be defined at once.
> +
> +unsigned gpio_block_get(struct gpio_block *block);
> +void gpio_block_set(struct gpio_block *block, unsigned value);
> +
> +With those accessor functions, setting and getting the GPIO values is possible,
> +analogous to gpio_get_value() and gpio_set_value(). Each bit in the return
> +value of gpio_block_get() and in the value argument of gpio_block_set()
> +corresponds to a bit specified on gpio_block_create(). Block operations in
> +hardware are only possible where the respective GPIO driver implements it,
> +falling back to using single GPIO operations where the driver only implements
> +single GPIO access.
> +
> +void gpio_block_free(struct gpio_block *block);
> +
> +After the GPIO block isn't used anymore, it should be free'd via
> +gpio_block_free().
> +
> +int gpio_block_register(struct gpio_block *block);
> +void gpio_block_unregister(struct gpio_block *block);
> +
> +These functions can be used to register a GPIO block. Blocks registered this
> +way will be available via sysfs.
> +
> +
> What do these conventions omit?
> ===============================
> One of the biggest things these conventions omit is pin multiplexing, since
> --- linux-2.6.orig/drivers/gpio/gpiolib.c
> +++ linux-2.6/drivers/gpio/gpiolib.c
> @@ -83,6 +83,10 @@ static inline void desc_set_label(struct
> #endif
> }
>
> +#define NR_GPIO_BLOCKS 16
> +
> +static struct gpio_block *gpio_block[NR_GPIO_BLOCKS];
> +
> /* Warn when drivers omit gpio_request() calls -- legal but ill-advised
> * when setting direction, and otherwise illegal. Until board setup code
> * and drivers use explicit requests everywhere (which won't happen when
> @@ -1676,6 +1680,225 @@ void __gpio_set_value(unsigned gpio, int
> }
> EXPORT_SYMBOL_GPL(__gpio_set_value);
>
> +static inline
Nitpick - don't need the inline, the compiler will do so for you.
> +int gpio_block_chip_index(struct gpio_block *block, struct gpio_chip *gc)
Should be static?
> +{
> + int i;
> +
> + for (i = 0; i < block->nchip; i++) {
> + if (block->gbc[i].gc == gc)
> + return i;
> + }
> + return -1;
> +}
> +
> +struct gpio_block *gpio_block_create(unsigned *gpios, size_t size,
> + const char *name)
> +{
> + struct gpio_block *block;
> + struct gpio_block_chip *gbc;
> + struct gpio_remap *remap;
> + int i;
> +
> + if (size < 1 || size > sizeof(unsigned) * 8)
> + return ERR_PTR(-EINVAL);
> +
> + block = kzalloc(sizeof(struct gpio_block), GFP_KERNEL);
> + if (!block)
> + return ERR_PTR(-ENOMEM);
> +
> + block->name = name;
> + block->ngpio = size;
> + block->gpio = kzalloc(sizeof(*block->gpio) * size, GFP_KERNEL);
> + if (!block->gpio)
> + goto err1;
> +
> + memcpy(block->gpio, gpios, sizeof(*block->gpio) * size);
> +
> + for (i = 0; i < size; i++)
> + if (!gpio_is_valid(gpios[i]))
> + goto err2;
This loop should probably go at the start of the function, so you can
avoid doing the kzalloc/memcpy if it fails.
This function doesn't call gpio_request. Either it should, or it should
rely on the caller to have already done so, and call
gpio_ensure_requested here. There is also an implicit rule that any
gpios inside a block must not be freed as long as the block exists. The
code can't easily prevent this since gpios aren't refcounted. The rule
should be documented.
> +
> + for (i = 0; i < size; i++) {
> + struct gpio_chip *gc = gpio_to_chip(gpios[i]);
> + int bit = gpios[i] - gc->base;
> + int index = gpio_block_chip_index(block, gc);
> +
> + if (index < 0) {
> + block->nchip++;
> + block->gbc = krealloc(block->gbc,
> + sizeof(struct gpio_block_chip) *
> + block->nchip, GFP_KERNEL);
krealloc is a bit nasty. Can't you add a struct list_head to struct
gpio_block_chip or something?
This also leaks memory if the krealloc fails, since the original pointer
is lost. You need to use a temporary for the re-allocation.
> + if (!block->gbc)
> + goto err2;
> + gbc = &block->gbc[block->nchip - 1];
> + gbc->gc = gc;
> + gbc->remap = NULL;
> + gbc->nremap = 0;
> + gbc->mask = 0;
> + } else {
> + gbc = &block->gbc[index];
> + }
> + /* represents the mask necessary on calls to the driver's
> + * .get_block() and .set_block()
> + */
/*
* Nitpick - multi-line comment style looks like this.
* However, other comments in this file use this style
* so maybe keep for consistency.
*/
> + gbc->mask |= BIT(bit);
> +
> + /* collect gpios that are specified together, represented by
> + * neighboring bits
> + */
> + remap = &gbc->remap[gbc->nremap - 1];
This looks broken. If gbc was re-alloced above (index < 0) then
gbc->remap == NULL and this will oops?
> + if (!gbc->nremap || (bit - i != remap->offset)) {
gbc->nremap would have to be non-zero here, otherwise you have:
gbc->remap[0 - 1]
above.
> + gbc->nremap++;
> + gbc->remap = krealloc(gbc->remap,
> + sizeof(struct gpio_remap) *
> + gbc->nremap, GFP_KERNEL);
Memory leak on failure. Also, is an alternative to krealloc possible.
Maybe a list?
> + if (!gbc->remap)
> + goto err3;
> + remap = &gbc->remap[gbc->nremap - 1];
> + remap->offset = bit - i;
> + remap->mask = 0;
> + }
> +
> + /* represents the mask necessary for bit reordering between
> + * gpio_block (i.e. as specified on gpio_block_get() and
> + * gpio_block_set()) and gpio_chip domain (i.e. as specified on
> + * the driver's .set_block() and .get_block())
> + */
> + remap->mask |= BIT(i);
> + }
The remap functionality isn't very well explained (and looks like it
doesn't work properly anyway). Some comments explaining what the remap
does and how it works would be useful.
> + return block;
> +err3:
> + for (i = 0; i < block->nchip - 1; i++)
> + kfree(block->gbc[i].remap);
> + kfree(block->gbc);
> +err2:
> + kfree(block->gpio);
> +err1:
> + kfree(block);
> + return ERR_PTR(-ENOMEM);
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_create);
> +
> +void gpio_block_free(struct gpio_block *block)
> +{
> + int i;
> +
> + for (i = 0; i < block->nchip; i++)
> + kfree(block->gbc[i].remap);
> + kfree(block->gpio);
> + kfree(block->gbc);
> + kfree(block);
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_free);
> +
> +unsigned gpio_block_get(const struct gpio_block *block)
> +{
> + struct gpio_block_chip *gbc;
> + int i, j;
> + unsigned values = 0;
> +
> + for (i = 0; i < block->nchip; i++) {
> + unsigned remapped = 0;
> +
> + gbc = &block->gbc[i];
> +
> + if (gbc->gc->get_block) {
> + remapped = gbc->gc->get_block(gbc->gc, gbc->mask);
> + } else { /* emulate */
> + unsigned bit = 1;
> +
> + for (j = 0; j < sizeof(unsigned) * 8; j++, bit <<= 1) {
> + if (gbc->mask & bit)
A proper bitmask might be more ideal for this. It would remove the
sizeof(unsigned) restriction and allow you to use for_each_set_bit for
these loops.
> + remapped |= gbc->gc->get(gbc->gc,
> + gbc->gc->base + j) << j;
> + }
> + }
> +
> + for (j = 0; j < gbc->nremap; j++) {
> + struct gpio_remap *gr = &gbc->remap[j];
> +
> + values |= (remapped >> gr->offset) & gr->mask;
> + }
> + }
> +
> + return values;
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_get);
> +
> +void gpio_block_set(struct gpio_block *block, unsigned values)
> +{
> + struct gpio_block_chip *gbc;
> + int i, j;
> +
> + for (i = 0; i < block->nchip; i++) {
> + unsigned remapped = 0;
> +
> + gbc = &block->gbc[i];
> +
> + for (j = 0; j < gbc->nremap; j++) {
> + struct gpio_remap *gr = &gbc->remap[j];
> +
> + remapped |= (values & gr->mask) << gr->offset;
> + }
> + if (gbc->gc->set_block) {
> + gbc->gc->set_block(gbc->gc, gbc->mask, remapped);
> + } else { /* emulate */
Nitpick - Put the comment on a line by itself.
> + unsigned bit = 1;
> +
> + for (j = 0; j < sizeof(unsigned) * 8; j++, bit <<= 1) {
> + if (gbc->mask & bit)
> + gbc->gc->set(gbc->gc, gbc->gc->base + j,
> + (remapped >> j) & 1);
> + }
This doesn't clear pins which are set to zero? If you are using
gpio_block to bit-bang a bus then you probably want that to happen.
Probably you want three functions, gpio_block_set (set only),
gpio_block_clear (clear only) and gpio_block_drive (set/clear).
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_set);
> +
> +struct gpio_block *gpio_block_find_by_name(const char *name)
> +{
> + int i;
> +
> + for (i = 0; i < NR_GPIO_BLOCKS; i++) {
A static limit is lame. Make it a list.
> + if (gpio_block[i] && !strcmp(gpio_block[i]->name, name))
> + return gpio_block[i];
> + }
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_find_by_name);
> +
> +int gpio_block_register(struct gpio_block *block)
> +{
> + int i;
> +
> + if (gpio_block_find_by_name(block->name))
> + return -EBUSY;
> +
> + for (i = 0; i < NR_GPIO_BLOCKS; i++) {
> + if (!gpio_block[i]) {
> + gpio_block[i] = block;
> + break;
> + }
> + }
> + return i == NR_GPIO_BLOCKS ? -EBUSY : 0;
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_register);
> +
> +void gpio_block_unregister(struct gpio_block *block)
> +{
> + int i;
> +
> + for (i = 0; i < NR_GPIO_BLOCKS; i++) {
> + if (gpio_block[i] == block) {
> + gpio_block[i] = NULL;
> + break;
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_unregister);
> +
> /**
> * __gpio_cansleep() - report whether gpio value access will sleep
> * @gpio: gpio in question
> --- linux-2.6.orig/include/asm-generic/gpio.h
> +++ linux-2.6/include/asm-generic/gpio.h
> @@ -43,6 +43,7 @@ static inline bool gpio_is_valid(int num
>
> struct device;
> struct gpio;
> +struct gpio_block;
> struct seq_file;
> struct module;
> struct device_node;
> @@ -105,6 +106,8 @@ struct gpio_chip {
> unsigned offset);
> int (*get)(struct gpio_chip *chip,
> unsigned offset);
> + unsigned (*get_block)(struct gpio_chip *chip,
> + unsigned mask);
> int (*direction_output)(struct gpio_chip *chip,
> unsigned offset, int value);
> int (*set_debounce)(struct gpio_chip *chip,
> @@ -112,6 +115,8 @@ struct gpio_chip {
>
> void (*set)(struct gpio_chip *chip,
> unsigned offset, int value);
> + void (*set_block)(struct gpio_chip *chip,
> + unsigned mask, unsigned values);
>
> int (*to_irq)(struct gpio_chip *chip,
> unsigned offset);
> @@ -171,6 +176,15 @@ extern void gpio_set_value_cansleep(unsi
> extern int __gpio_get_value(unsigned gpio);
> extern void __gpio_set_value(unsigned gpio, int value);
>
> +extern struct gpio_block *gpio_block_create(unsigned *gpio, size_t size,
> + const char *name);
> +extern void gpio_block_free(struct gpio_block *block);
> +extern unsigned gpio_block_get(const struct gpio_block *block);
> +extern void gpio_block_set(struct gpio_block *block, unsigned values);
> +extern struct gpio_block *gpio_block_find_by_name(const char *name);
> +extern int gpio_block_register(struct gpio_block *block);
> +extern void gpio_block_unregister(struct gpio_block *block);
> +
> extern int __gpio_cansleep(unsigned gpio);
>
> extern int __gpio_to_irq(unsigned gpio);
> --- linux-2.6.orig/include/linux/gpio.h
> +++ linux-2.6/include/linux/gpio.h
> @@ -2,6 +2,7 @@
> #define __LINUX_GPIO_H
>
> #include <linux/errno.h>
> +#include <linux/types.h>
>
> /* see Documentation/gpio.txt */
>
> @@ -39,6 +40,31 @@ struct gpio {
> const char *label;
> };
>
> +struct gpio_remap {
> + int mask;
> + int offset;
> +};
> +
> +struct gpio_block_chip {
> + struct gpio_chip *gc;
> + struct gpio_remap *remap;
> + int nremap;
> + unsigned mask;
> +};
> +
> +/**
> + * struct gpio_block - a structure describing a list of GPIOs for simultaneous
> + * operations
> + */
> +struct gpio_block {
> + struct gpio_block_chip *gbc;
> + size_t nchip;
> + const char *name;
> +
> + int ngpio;
> + unsigned *gpio;
> +};
> +
> #ifdef CONFIG_GENERIC_GPIO
>
> #ifdef CONFIG_ARCH_HAVE_CUSTOM_GPIO_H
> @@ -169,6 +195,41 @@ static inline void gpio_set_value(unsign
> WARN_ON(1);
> }
>
> +static inline
> +struct gpio_block *gpio_block_create(unsigned int *gpios, size_t size,
> + const char *name)
> +{
> + WARN_ON(1);
> + return NULL;
> +}
> +
> +static inline void gpio_block_free(struct gpio_block *block)
> +{
> + WARN_ON(1);
> +}
> +
> +static inline unsigned gpio_block_get(const struct gpio_block *block)
> +{
> + WARN_ON(1);
> + return 0;
> +}
> +
> +static inline void gpio_block_set(struct gpio_block *block, unsigned value)
> +{
> + WARN_ON(1);
> +}
> +
> +static inline int gpio_block_register(struct gpio_block *block)
> +{
> + WARN_ON(1);
> + return 0;
> +}
> +
> +static inline void gpio_block_unregister(struct gpio_block *block)
> +{
> + WARN_ON(1);
> +}
> +
> static inline int gpio_cansleep(unsigned gpio)
> {
> /* GPIO can never have been requested or set as {in,out}put */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists