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: <a26b13f1-84e2-44fa-9801-690f7edf9345@email.android.com>
Date:	Wed, 19 Dec 2012 21:53:23 +0100
From:	Michael Trimarchi <michael@...rulasolutions.com>
To:	Grant Likely <grant.likely@...retlab.ca>,
	Peter Ujfalusi <peter.ujfalusi@...com>,
	Linus Walleij <linus.walleij@...aro.org>
CC:	linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org
Subject: Re: [PATCH v2 2/3] gpio: twl4030: Cache the direction and output states in private data

Hi

Grant Likely <grant.likely@...retlab.ca> wrote:

>On Thu, 6 Dec 2012 11:52:06 +0100, Peter Ujfalusi
><peter.ujfalusi@...com> wrote:
>> Use more coherent locking in the driver. Use bitfield to store the
>GPIO
>> direction and if the pin is configured as output store the status
>also in a
>> bitfiled.
>> In this way we can just look at these bitfields when we need
>information
>> about the pin status and only reach out to the chip when it is
>needed.
>> 
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@...com>
>
>Applied, thanks
>
>g.
>
>> ---
>>  drivers/gpio/gpio-twl4030.c | 99
>++++++++++++++++++++++++++++++---------------
>>  1 file changed, 66 insertions(+), 33 deletions(-)
>> 
>> diff --git a/drivers/gpio/gpio-twl4030.c
>b/drivers/gpio/gpio-twl4030.c
>> index c092739..a38e6e9c 100644
>> --- a/drivers/gpio/gpio-twl4030.c
>> +++ b/drivers/gpio/gpio-twl4030.c
>> @@ -64,14 +64,15 @@
>>  /* Mask for GPIO registers when aggregated into a 32-bit integer */
>>  #define GPIO_32_MASK			0x0003ffff
>>  
>> -/* Data structures */
>> -static DEFINE_MUTEX(gpio_lock);
>> -
>>  struct gpio_twl4030_priv {
>>  	struct gpio_chip gpio_chip;
>> +	struct mutex mutex;
>>  	int irq_base;
>>  
>> +	/* Bitfields for state caching */
>>  	unsigned int usage_count;
>> +	unsigned int direction;
>> +	unsigned int out_state;
>>  };
>>  
>> 
>/*----------------------------------------------------------------------*/
>> @@ -130,7 +131,7 @@ static inline int gpio_twl4030_read(u8 address)
>>  
>> 
>/*----------------------------------------------------------------------*/
>>  
>> -static u8 cached_leden;		/* protected by gpio_lock */
>> +static u8 cached_leden;
>>  
>>  /* The LED lines are open drain outputs ... a FET pulls to GND, so
>an
>>   * external pullup is needed.  We could also expose the integrated
>PWM
>> @@ -144,14 +145,12 @@ static void twl4030_led_set_value(int led, int
>value)
>>  	if (led)
>>  		mask <<= 1;
>>  
>> -	mutex_lock(&gpio_lock);
>>  	if (value)
>>  		cached_leden &= ~mask;
>>  	else
>>  		cached_leden |= mask;
>>  	status = twl_i2c_write_u8(TWL4030_MODULE_LED, cached_leden,
>>  				  TWL4030_LED_LEDEN_REG);
>> -	mutex_unlock(&gpio_lock);
>>  }
>>  
>>  static int twl4030_set_gpio_direction(int gpio, int is_input)
>> @@ -162,7 +161,6 @@ static int twl4030_set_gpio_direction(int gpio,
>int is_input)
>>  	u8 base = REG_GPIODATADIR1 + d_bnk;
>>  	int ret = 0;
>>  
>> -	mutex_lock(&gpio_lock);
>>  	ret = gpio_twl4030_read(base);
>>  	if (ret >= 0) {
>>  		if (is_input)
>> @@ -172,7 +170,6 @@ static int twl4030_set_gpio_direction(int gpio,
>int is_input)
>>  
>>  		ret = gpio_twl4030_write(base, reg);
>>  	}
>> -	mutex_unlock(&gpio_lock);
>>  	return ret;
>>  }
>>  
>> @@ -212,7 +209,7 @@ static int twl_request(struct gpio_chip *chip,
>unsigned offset)
>>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>>  	int status = 0;
>>  
>> -	mutex_lock(&gpio_lock);
>> +	mutex_lock(&priv->mutex);
>>  
>>  	/* Support the two LED outputs as output-only GPIOs. */
>>  	if (offset >= TWL4030_GPIO_MAX) {
>> @@ -271,7 +268,7 @@ done:
>>  	if (!status)
>>  		priv->usage_count |= BIT(offset);
>>  
>> -	mutex_unlock(&gpio_lock);
>> +	mutex_unlock(&priv->mutex);
>>  	return status;
>>  }
>>  
>> @@ -279,64 +276,98 @@ static void twl_free(struct gpio_chip *chip,
>unsigned offset)
>>  {
>>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>>  
>> +	mutex_lock(&priv->mutex);
>>  	if (offset >= TWL4030_GPIO_MAX) {
>>  		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, 1);

I have the mobile but where is the unlock here?


>>  		return;
>>  	}
>>  
>> -	mutex_lock(&gpio_lock);
>> -
>>  	priv->usage_count &= ~BIT(offset);
>>  
>>  	/* on last use, switch off GPIO module */
>>  	if (!priv->usage_count)
>>  		gpio_twl4030_write(REG_GPIO_CTRL, 0x0);
>>  
>> -	mutex_unlock(&gpio_lock);
>> +	mutex_unlock(&priv->mutex);
>>  }
>>  
>>  static int twl_direction_in(struct gpio_chip *chip, unsigned offset)
>>  {
>> -	return (offset < TWL4030_GPIO_MAX)
>> -		? twl4030_set_gpio_direction(offset, 1)
>> -		: -EINVAL;
>> +	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>> +	int ret;
>> +
>> +	mutex_lock(&priv->mutex);
>> +	if (offset < TWL4030_GPIO_MAX)
>> +		ret = twl4030_set_gpio_direction(offset, 1);
>> +	else
>> +		ret = -EINVAL;
>> +
>> +	if (!ret)
>> +		priv->direction &= ~BIT(offset);
>> +
>> +	mutex_unlock(&priv->mutex);
>> +
>> +	return ret;
>>  }
>>  
>>  static int twl_get(struct gpio_chip *chip, unsigned offset)
>>  {
>>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>> +	int ret;
>>  	int status = 0;
>>  
>> -	if (!(priv->usage_count & BIT(offset)))
>> -		return -EPERM;
>> +	mutex_lock(&priv->mutex);
>> +	if (!(priv->usage_count & BIT(offset))) {
>> +		ret = -EPERM;
>> +		goto out;
>> +	}
>>  
>> -	if (offset < TWL4030_GPIO_MAX)
>> -		status = twl4030_get_gpio_datain(offset);
>> -	else if (offset == TWL4030_GPIO_MAX)
>> -		status = cached_leden & LEDEN_LEDAON;
>> +	if (priv->direction & BIT(offset))
>> +		status = priv->out_state & BIT(offset);
>>  	else
>> -		status = cached_leden & LEDEN_LEDBON;
>> +		status = twl4030_get_gpio_datain(offset);
>>  
>> -	return (status < 0) ? 0 : status;
>> +	ret = (status <= 0) ? 0 : 1;
>> +out:
>> +	mutex_unlock(&priv->mutex);
>> +	dev_err(chip->dev, "%s: offset %d value %d\n", __func__, offset,
>ret);
>> +	return ret;
>>  }
>>  
>> -static int twl_direction_out(struct gpio_chip *chip, unsigned
>offset, int value)
>> +static void twl_set(struct gpio_chip *chip, unsigned offset, int
>value)
>>  {
>> -	if (offset < TWL4030_GPIO_MAX) {
>> +	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>> +
>> +	dev_err(chip->dev, "%s: offset %d value %d\n", __func__, offset,
>value);
>> +	mutex_lock(&priv->mutex);
>> +	if (offset < TWL4030_GPIO_MAX)
>>  		twl4030_set_gpio_dataout(offset, value);
>> -		return twl4030_set_gpio_direction(offset, 0);
>> -	} else {
>> +	else
>>  		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, value);
>> -		return 0;
>> -	}
>> +
>> +	if (value)
>> +		priv->out_state |= BIT(offset);
>> +	else
>> +		priv->out_state &= ~BIT(offset);
>> +
>> +	mutex_unlock(&priv->mutex);
>>  }
>>  
>> -static void twl_set(struct gpio_chip *chip, unsigned offset, int
>value)
>> +static int twl_direction_out(struct gpio_chip *chip, unsigned
>offset, int value)
>>  {
>> +	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>> +
>> +	dev_err(chip->dev, "%s: offset %d value %d\n", __func__, offset,
>value);
>> +	mutex_lock(&priv->mutex);
>>  	if (offset < TWL4030_GPIO_MAX)
>>  		twl4030_set_gpio_dataout(offset, value);
>> -	else
>> -		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, value);
>> +
>> +	priv->direction |= BIT(offset);
>> +	mutex_unlock(&priv->mutex);
>> +
>> +	twl_set(chip, offset, value);
>> +
>> +	return 0;
>>  }
>>  
>>  static int twl_to_irq(struct gpio_chip *chip, unsigned offset)
>> @@ -469,6 +500,8 @@ no_irqs:
>>  	priv->gpio_chip.ngpio = TWL4030_GPIO_MAX;
>>  	priv->gpio_chip.dev = &pdev->dev;
>>  
>> +	mutex_init(&priv->mutex);
>> +
>>  	if (node)
>>  		pdata = of_gpio_twl4030(&pdev->dev);
>>  
>> -- 
>> 1.8.0
>> 

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
--
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