| 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: <CACRpkdbpxR8hQ13R43T1X9be95rJNMdgNPXfT3bQOO_dZU4Fkg@mail.gmail.com>
Date: Wed, 31 Oct 2012 15:06:50 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Roland Stigge <stigge@...com.de>
Cc: gregkh@...uxfoundation.org, grant.likely@...retlab.ca,
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,
daniel-gl@....net, rmallon@...il.com
Subject: Re: [PATCH RESEND 1/5 v6] gpio: Add a block GPIO API to gpiolib
On Sun, Oct 28, 2012 at 9:46 PM, Roland Stigge <stigge@...com.de> wrote:
Just a quick few things I spotted:
> +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;
> + void *tmp;
> + int i;
> +
> + if (size < 1 || size > sizeof(unsigned long) * 8)
> + return ERR_PTR(-EINVAL);
If the most GPIOs in a block is BITS_PER_LONG, why is the
latter clause not size > BITS_PER_LONG?
Maybe there was some discussion I missed, anyway it deserves
a comment because this looks completely arbitrary.
> + for (i = 0; i < size; i++)
> + if (!gpio_is_valid(gpios[i]))
> + return ERR_PTR(-EINVAL);
> +
> + block = kzalloc(sizeof(struct gpio_block), GFP_KERNEL);
> + if (!block)
> + return ERR_PTR(-ENOMEM);
If these were instead glued to a struct device you could do
devm_kzalloc() here and have it garbage collected.
This is why
https://blueprints.launchpad.net/linux-linaro/+spec/gpiochip-to-dev
needs to happen. (Sorry for hyperlinking.)
> + 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++) {
> + 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++;
> + tmp = krealloc(block->gbc,
> + sizeof(struct gpio_block_chip) *
> + block->nchip, GFP_KERNEL);
Don't do this dynamic array business. Use a linked list instead.
> + if (!tmp) {
> + kfree(block->gbc);
> + goto err2;
> + }
> + block->gbc = tmp;
> + gbc = &block->gbc[block->nchip - 1];
So this becomes a list traversal and lookup instead.
> + gbc->gc = gc;
> + gbc->remap = NULL;
> + gbc->nremap = 0;
> + gbc->mask = 0;
> + } else {
> + gbc = &block->gbc[index];
So find it in the list instead.
> + }
> + /* represents the mask necessary on calls to the driver's
> + * .get_block() and .set_block()
> + */
> + gbc->mask |= BIT(bit);
> +
> + /* collect gpios that are specified together, represented by
> + * neighboring bits
> + *
> + * Note that even though in setting remap is given a negative
> + * index, the next lines guard that the potential out-of-bounds
> + * pointer is not dereferenced when out of bounds.
> + */
/*
* Maybe I'm a bit picky on comment style but I prefer
* that the first line of a multi-line comment is blank.
* Applies everywhere.
*/
> + remap = &gbc->remap[gbc->nremap - 1];
> + if (!gbc->nremap || (bit - i != remap->offset)) {
> + gbc->nremap++;
> + tmp = krealloc(gbc->remap,
> + sizeof(struct gpio_remap) *
> + gbc->nremap, GFP_KERNEL);
I don't like this dynamic array either.
Basic pattern: wherever there is a krealloc, use a list.
If lists aren't efficient enough, use some other datastructure, rbtree or
whatever.
> + if (!tmp) {
> + kfree(gbc->remap);
> + goto err3;
> + }
> + gbc->remap = tmp;
> + 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);
> + }
> +
> + 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);
So if we only had a real struct device inside the gpiochip all
this boilerplate would go away, as devm_* take care of releasing
the resources.
> +unsigned long gpio_block_get(const struct gpio_block *block)
> +{
> + struct gpio_block_chip *gbc;
> + int i, j;
> + unsigned long values = 0;
> +
> + for (i = 0; i < block->nchip; i++) {
> + unsigned long remapped = 0;
> +
> + gbc = &block->gbc[i];
> +
> + if (gbc->gc->get_block) {
> + remapped = gbc->gc->get_block(gbc->gc, gbc->mask);
> + } else {
> + /* emulate */
> + for_each_set_bit(j, &gbc->mask, BITS_PER_LONG)
> + 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 long values)
> +{
> + struct gpio_block_chip *gbc;
> + int i, j;
> +
> + for (i = 0; i < block->nchip; i++) {
> + unsigned long 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 */
> + for_each_set_bit(j, &gbc->mask, BITS_PER_LONG)
> + gbc->gc->set(gbc->gc, gbc->gc->base + j,
> + (remapped >> j) & 1);
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_set);
> +
> +struct gpio_block *gpio_block_find_by_name(const char *name)
> +{
> + struct gpio_block *i;
> +
> + list_for_each_entry(i, &gpio_block_list, list)
> + if (!strcmp(i->name, name))
> + return i;
And here is a list anyway, I'm getting confused of this partitioning between
using dynamic arrays and lists. Just use a list please. This part looks
good.
Yours,
Linus Walleij
--
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