lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20121217155153.GD16561@e106331-lin.cambridge.arm.com>
Date:	Mon, 17 Dec 2012 15:51:53 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	Roland Stigge <stigge@...com.de>
Cc:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"grant.likely@...retlab.ca" <grant.likely@...retlab.ca>,
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"w.sang@...gutronix.de" <w.sang@...gutronix.de>,
	"jbe@...gutronix.de" <jbe@...gutronix.de>,
	"plagnioj@...osoft.com" <plagnioj@...osoft.com>,
	"highguy@...il.com" <highguy@...il.com>,
	"broonie@...nsource.wolfsonmicro.com" 
	<broonie@...nsource.wolfsonmicro.com>,
	"daniel-gl@....net" <daniel-gl@....net>,
	"rmallon@...il.com" <rmallon@...il.com>, "sr@...x.de" <sr@...x.de>,
	"wg@...ndegger.com" <wg@...ndegger.com>
Subject: Re: [PATCH RESEND 5/6 v10] gpio: Add device tree support to block
 GPIO API

Hi,

I have a few comments on the parsing code.

On Fri, Dec 14, 2012 at 02:26:24PM +0000, Roland Stigge wrote:
> This patch adds device tree support to the block GPIO API.
> 
> Signed-off-by: Roland Stigge <stigge@...com.de>
> 
> ---
>  Documentation/devicetree/bindings/gpio/gpio-block.txt |   36 +++++++
>  drivers/gpio/Makefile                                 |    1 
>  drivers/gpio/gpioblock-of.c                           |   84 ++++++++++++++++++
>  3 files changed, 121 insertions(+)
> 
> --- /dev/null
> +++ linux-2.6/Documentation/devicetree/bindings/gpio/gpio-block.txt
> @@ -0,0 +1,36 @@
> +Block GPIO definition
> +=====================
> +
> +This binding specifies arbitrary blocks of gpios, combining gpios from one or
> +more GPIO controllers together, to form a word for r/w access.
> +
> +Required property:
> +- compatible: must be "linux,gpio-block"
> +
> +Required subnodes:
> +- the name will be the registered name of the block
> +- property "gpios" is a list of gpios for the respective block
> +
> +Example:
> +
> +        blockgpio {
> +                compatible = "linux,gpio-block";
> +
> +                block0 {
> +                        gpios = <&gpio 3 0 0>,
> +                                <&gpio 3 1 0>;
> +                };
> +                block1 {
> +                        gpios = <&gpio 4 1 0>,
> +                                <&gpio 4 3 0>,
> +                                <&gpio 4 2 0>,
> +                                <&gpio 4 4 0>,
> +                                <&gpio 4 5 0>,
> +                                <&gpio 4 6 0>,
> +                                <&gpio 4 7 0>,
> +                                <&gpio 4 8 0>,
> +                                <&gpio 4 9 0>,
> +                                <&gpio 4 10 0>,
> +                                <&gpio 4 19 0>;
> +                };
> +        };
> --- linux-2.6.orig/drivers/gpio/Makefile
> +++ linux-2.6/drivers/gpio/Makefile
> @@ -4,6 +4,7 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
>  
>  obj-$(CONFIG_GPIOLIB)		+= gpiolib.o devres.o
>  obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
> +obj-$(CONFIG_OF_GPIO)		+= gpioblock-of.o
>  
>  # Device drivers. Generally keep list sorted alphabetically
>  obj-$(CONFIG_GPIO_GENERIC)	+= gpio-generic.o
> --- /dev/null
> +++ linux-2.6/drivers/gpio/gpioblock-of.c
> @@ -0,0 +1,84 @@
> +/*
> + * OF implementation for Block GPIO
> + *
> + * Copyright (C) 2012 Roland Stigge <stigge@...com.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +
> +static int __devinit gpioblock_of_probe(struct platform_device *pdev)
> +{
> +	struct device_node *block;
> +	unsigned *gpios;
> +	int ngpio;
> +	int ret;
> +	struct gpio_block *gb;
> +
> +	for_each_available_child_of_node(pdev->dev.of_node, block) {
> +		int i;
> +
> +		ngpio = of_gpio_count(block);
> +		gpios = kzalloc(ngpio * sizeof(*gpios), GFP_KERNEL);

What if the block node is malformed? ngpio might be -ENOENT or -EINVAL.

> +		if (!gpios)
> +			return -ENOMEM;
> +		for (i = 0; i < ngpio; i++) {
> +			ret = of_get_gpio(block, i);
> +			if (ret < 0)
> +				return ret; /* expect -EPROBE_DEFER */
> +			gpios[i] = ret;
> +		}
> +		gb = gpio_block_create(gpios, ngpio, block->name);
> +		if (IS_ERR(gb)) {
> +			dev_err(&pdev->dev,
> +				"Error creating GPIO block from device tree\n");
> +			return PTR_ERR(gb);

Won't this leak the memory for the gpios object we kzalloc'd earlier?

> +		}
> +		ret = gpio_block_register(gb);
> +		if (ret < 0) {
> +			gpio_block_free(gb);
> +			return ret;

Same here.

> +		}
> +		kfree(gpios);
> +		dev_info(&pdev->dev, "Registered gpio block %s: %d gpios\n",
> +			 block->name, ngpio);

Any of the returns in this block will leave the block node's refcount
incremented.

> +	}
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static struct of_device_id gpioblock_of_match[] __devinitdata = {
> +	{ .compatible = "linux,gpio-block", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, gpioblock_of_match);
> +#endif
> +
> +static struct platform_driver gpioblock_of_driver = {
> +	.driver	= {
> +		.name = "gpio-block",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(gpioblock_of_match),
> +
> +	},
> +	.probe	= gpioblock_of_probe,
> +};
> +
> +module_platform_driver(gpioblock_of_driver);
> +
> +MODULE_DESCRIPTION("GPIO Block driver");
> +MODULE_AUTHOR("Roland Stigge <stigge@...com.de>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:gpioblock-of");
> 

Thanks,
Mark.

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

Powered by Openwall GNU/*/Linux Powered by OpenVZ