linux-kernel - Re: [PATCH 2/2] pinctrl: abx500: fix abx500_pin_config

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]

Date:	Thu, 20 Jun 2013 10:18:50 +0200
From:	Fabio Baltieri <fabio.baltieri@...aro.org>
To:	patrice.chotard.st@...il.com
Cc:	linux-kernel@...r.kernel.org,
	Linus Walleij <linus.walleij@...aro.org>,
	Olivier Clergeaud <olivier.clergeaud@...com>,
	Lee Jones <lee.jones@...aro.org>,
	Patrice Chotard <patrice.chotard@...com>
Subject: Re: [PATCH 2/2] pinctrl: abx500: fix abx500_pin_config_set()

On Thu, Jun 20, 2013 at 09:23:22AM +0200, patrice.chotard.st@...il.com wrote:
> From: Patrice Chotard <patrice.chotard@...com>
> 
> _ Update abx500_pin_config_set() in order to take in
> account PIN_CONFIG_BIAS_DISABLE state to disable
> pull up or pull down.
> 
> _ Rework error path.
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@...com>
> ---
>  drivers/pinctrl/pinctrl-abx500.c |   31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-abx500.c b/drivers/pinctrl/pinctrl-abx500.c
> index b5b5460..14dc078 100644
> --- a/drivers/pinctrl/pinctrl-abx500.c
> +++ b/drivers/pinctrl/pinctrl-abx500.c
> @@ -33,6 +33,7 @@
>  #include <linux/pinctrl/machine.h>
>  
>  #include "pinctrl-abx500.h"
> +#include "core.h"
>  #include "pinconf.h"
>  
>  /*
> @@ -963,7 +964,7 @@ static int abx500_pin_config_set(struct pinctrl_dev *pctldev,
>  	struct pullud *pullud = pct->soc->pullud;
>  	struct gpio_chip *chip = &pct->chip;
>  	unsigned offset;
> -	int ret = 0;
> +	int ret = -EINVAL;
>  	enum pin_config_param param = pinconf_to_config_param(config);
>  	enum pin_config_param argument = pinconf_to_config_argument(config);
>  
> @@ -976,13 +977,32 @@ static int abx500_pin_config_set(struct pinctrl_dev *pctldev,
>  	offset = pin - 1;
>  
>  	switch (param) {
> -	case PIN_CONFIG_BIAS_PULL_DOWN:
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		ret = abx500_gpio_direction_input(chip, offset);
>  		/*
> -		 * if argument = 1 set the pull down
> -		 * else clear the pull down
> +		 * Some chips only support pull down, while some actually
> +		 * support both pull up and pull down. Such chips have
> +		 * a "pullud" range specified for the pins that support
> +		 * both features. If the pin is not within that range, we
> +		 * fall back to the old bit set that only support pull down.
>  		 */
> +		if (pullud &&
> +		    pin >= pullud->first_pin &&
> +		    pin <= pullud->last_pin)

This multi-line check is replicated in all conditions, would it make
sense to move it on a dedicated function to improve readability?

> +			ret = abx500_set_pull_updown(pct,
> +				pin,
> +				ABX500_GPIO_PULL_NONE);
> +		else
> +			/* Chip only supports pull down */
> +			ret = abx500_gpio_set_bits(chip, AB8500_GPIO_PUD1_REG,
> +				offset, ABX500_GPIO_PULL_NONE);
> +		break;
> +
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
>  		ret = abx500_gpio_direction_input(chip, offset);
>  		/*
> +		 * if argument = 1 set the pull down
> +		 * else clear the pull down
>  		 * Some chips only support pull down, while some actually
>  		 * support both pull up and pull down. Such chips have
>  		 * a "pullud" range specified for the pins that support
> @@ -1002,6 +1022,7 @@ static int abx500_pin_config_set(struct pinctrl_dev *pctldev,
>  		break;
>  
>  	case PIN_CONFIG_BIAS_PULL_UP:
> +		ret = abx500_gpio_direction_input(chip, offset);

Here the return value of abx500_gpio_direction_input is set but never
checked, and will be always overwritten by the next abx500_gpio_ call...
Would it make sense to add a pr_err for it?  On the other side, if it
never fails, you can just drop the return field altogether.

That's also done in other conditions in the same 'switch', it may make
sense to have a patch just for that.

Thanks,
Fabio

>  		/*
>  		 * if argument = 1 set the pull up
>  		 * else clear the pull up
> @@ -1030,8 +1051,6 @@ static int abx500_pin_config_set(struct pinctrl_dev *pctldev,
>  
>  	default:
>  		dev_err(chip->dev, "illegal configuration requested\n");
> -
> -		return -EINVAL;
>  	}
>  
>  	return ret;
> -- 
> 1.7.10
> 

-- 
Fabio Baltieri
--
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/