[PATCH] pinctrl: samsung: Fixes samsung_gpio_direction_in/output releated with spinlock

[PATCH] pinctrl: samsung: Fixes samsung_gpio_direction_in/output releated with spinlock

[Youngmin Nam]

Previously, samsung_gpio_drection_in/output function were not covered with
one spinlock.

For example, samsung_gpio_direction_output function consists of two functions.
1. samsung_gpio_set
2. samsung_gpio_set_direction

When 2 CPUs try to control the same gpio pin heavily,
(situation like i2c control with gpio emulation)
This situation can cause below problem.

CPU 0                                   | CPU1
                                        |
samsung_gpio_direction_output           |
   samsung_gpio_set(pin A as 1)         | samsung_gpio_direction_output
                                        |    samsung_gpio_set(pin A as 0)
   samsung_gpio_set_direction           |
                                        |    samsung_gpio_set_direction

The initial value of pin A will be set as 0 while we wanted to set pin A as 1.

This patch modifies samsung_gpio_direction_in/output function
to be done in one spinlock to fix race condition.

Additionally, gpio set callback was changed with samsung_gpio_set_value
to implement gpio set callback with spinlock using samsung_gpio_set.

Signed-off-by: Youngmin Nam <ym0914@gmail.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 38 ++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 48294e7..a25f6f6 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -524,20 +524,26 @@ static void samsung_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
 {
 	struct samsung_pin_bank *bank = gc_to_pin_bank(gc);
 	const struct samsung_pin_bank_type *type = bank->type;
-	unsigned long flags;
 	void __iomem *reg;
 	u32 data;
 
 	reg = bank->drvdata->virt_base + bank->pctl_offset;
 
-	spin_lock_irqsave(&bank->slock, flags);
-
 	data = readl(reg + type->reg_offset[PINCFG_TYPE_DAT]);
 	data &= ~(1 << offset);
 	if (value)
 		data |= 1 << offset;
 	writel(data, reg + type->reg_offset[PINCFG_TYPE_DAT]);
+}
+
+static void samsung_gpio_set_value(struct gpio_chip *gc,
+					  unsigned offset, int value)
+{
+	struct samsung_pin_bank *bank = gc_to_pin_bank(gc);
+	unsigned long flags;
 
+	spin_lock_irqsave(&bank->slock, flags);
+	samsung_gpio_set(gc, offset, value);
 	spin_unlock_irqrestore(&bank->slock, flags);
 }
 
@@ -569,7 +575,6 @@ static int samsung_gpio_set_direction(struct gpio_chip *gc,
 	struct samsung_pinctrl_drv_data *drvdata;
 	void __iomem *reg;
 	u32 data, mask, shift;
-	unsigned long flags;
 
 	bank = gc_to_pin_bank(gc);
 	type = bank->type;
@@ -586,31 +591,42 @@ static int samsung_gpio_set_direction(struct gpio_chip *gc,
 		reg += 4;
 	}
 
-	spin_lock_irqsave(&bank->slock, flags);
-
 	data = readl(reg);
 	data &= ~(mask << shift);
 	if (!input)
 		data |= FUNC_OUTPUT << shift;
 	writel(data, reg);
 
-	spin_unlock_irqrestore(&bank->slock, flags);
-
 	return 0;
 }
 
 /* gpiolib gpio_direction_input callback function. */
 static int samsung_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
 {
-	return samsung_gpio_set_direction(gc, offset, true);
+	struct samsung_pin_bank *bank = gc_to_pin_bank(gc);
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&bank->slock, flags);
+	ret = samsung_gpio_set_direction(gc, offset, true);
+	spin_unlock_irqrestore(&bank->slock, flags);
+	return ret;
 }
 
 /* gpiolib gpio_direction_output callback function. */
 static int samsung_gpio_direction_output(struct gpio_chip *gc, unsigned offset,
 							int value)
 {
+	struct samsung_pin_bank *bank = gc_to_pin_bank(gc);
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&bank->slock, flags);
 	samsung_gpio_set(gc, offset, value);
-	return samsung_gpio_set_direction(gc, offset, false);
+	ret = samsung_gpio_set_direction(gc, offset, false);
+	spin_unlock_irqrestore(&bank->slock, flags);
+
+	return ret;
 }
 
 /*
@@ -891,7 +907,7 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
 static const struct gpio_chip samsung_gpiolib_chip = {
 	.request = gpiochip_generic_request,
 	.free = gpiochip_generic_free,
-	.set = samsung_gpio_set,
+	.set = samsung_gpio_set_value,
 	.get = samsung_gpio_get,
 	.direction_input = samsung_gpio_direction_input,
 	.direction_output = samsung_gpio_direction_output,
-- 
2.7.0.25.gfc10eb5

Re: [PATCH] pinctrl: samsung: Fixes samsung_gpio_direction_in/output releated with spinlock

[Krzysztof Kozlowski]

On 17.01.2016 16:41, Youngmin Nam wrote:
> Previously, samsung_gpio_drection_in/output function were not covered with
> one spinlock.
> 
> For example, samsung_gpio_direction_output function consists of two functions.
> 1. samsung_gpio_set
> 2. samsung_gpio_set_direction
> 
> When 2 CPUs try to control the same gpio pin heavily,
> (situation like i2c control with gpio emulation)
> This situation can cause below problem.
> 
> CPU 0                                   | CPU1
>                                         |
> samsung_gpio_direction_output           |
>    samsung_gpio_set(pin A as 1)         | samsung_gpio_direction_output
>                                         |    samsung_gpio_set(pin A as 0)
>    samsung_gpio_set_direction           |
>                                         |    samsung_gpio_set_direction
> 
> The initial value of pin A will be set as 0 while we wanted to set pin A as 1.
> 
> This patch modifies samsung_gpio_direction_in/output function
> to be done in one spinlock to fix race condition.
> 
> Additionally, gpio set callback was changed with samsung_gpio_set_value
> to implement gpio set callback with spinlock using samsung_gpio_set.
> 
> Signed-off-by: Youngmin Nam <ym0914@gmail.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 38 ++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 11 deletions(-)


Hi,

Thank you for the patch.

Unfortunately you sent it to insufficient number of folks. Actually you
skipped all of necessary people.

Please use the scripts/get_maintainer.pl script to obtain a list of
maintainers which you should put on CC (Tomasz, Linus and more mailing
lists, including the gpio list).

Can you resend the patch CC-ing all necessary guys and lists?

Best regards,
Krzysztof

> 
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 48294e7..a25f6f6 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -524,20 +524,26 @@ static void samsung_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
>  {
>  	struct samsung_pin_bank *bank = gc_to_pin_bank(gc);
>  	const struct samsung_pin_bank_type *type = bank->type;
> -	unsigned long flags;
>  	void __iomem *reg;
>  	u32 data;
>  
>  	reg = bank->drvdata->virt_base + bank->pctl_offset;
>  
> -	spin_lock_irqsave(&bank->slock, flags);
> -
>  	data = readl(reg + type->reg_offset[PINCFG_TYPE_DAT]);
>  	data &= ~(1 << offset);
>  	if (value)
>  		data |= 1 << offset;
>  	writel(data, reg + type->reg_offset[PINCFG_TYPE_DAT]);
> +}
> +
> +static void samsung_gpio_set_value(struct gpio_chip *gc,
> +					  unsigned offset, int value)
> +{
> +	struct samsung_pin_bank *bank = gc_to_pin_bank(gc);
> +	unsigned long flags;
>  
> +	spin_lock_irqsave(&bank->slock, flags);
> +	samsung_gpio_set(gc, offset, value);
>  	spin_unlock_irqrestore(&bank->slock, flags);
>  }
>  
> @@ -569,7 +575,6 @@ static int samsung_gpio_set_direction(struct gpio_chip *gc,
>  	struct samsung_pinctrl_drv_data *drvdata;
>  	void __iomem *reg;
>  	u32 data, mask, shift;
> -	unsigned long flags;
>  
>  	bank = gc_to_pin_bank(gc);
>  	type = bank->type;
> @@ -586,31 +591,42 @@ static int samsung_gpio_set_direction(struct gpio_chip *gc,
>  		reg += 4;
>  	}
>  
> -	spin_lock_irqsave(&bank->slock, flags);
> -
>  	data = readl(reg);
>  	data &= ~(mask << shift);
>  	if (!input)
>  		data |= FUNC_OUTPUT << shift;
>  	writel(data, reg);
>  
> -	spin_unlock_irqrestore(&bank->slock, flags);
> -
>  	return 0;
>  }
>  
>  /* gpiolib gpio_direction_input callback function. */
>  static int samsung_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
>  {
> -	return samsung_gpio_set_direction(gc, offset, true);
> +	struct samsung_pin_bank *bank = gc_to_pin_bank(gc);
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&bank->slock, flags);
> +	ret = samsung_gpio_set_direction(gc, offset, true);
> +	spin_unlock_irqrestore(&bank->slock, flags);
> +	return ret;
>  }
>  
>  /* gpiolib gpio_direction_output callback function. */
>  static int samsung_gpio_direction_output(struct gpio_chip *gc, unsigned offset,
>  							int value)
>  {
> +	struct samsung_pin_bank *bank = gc_to_pin_bank(gc);
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&bank->slock, flags);
>  	samsung_gpio_set(gc, offset, value);
> -	return samsung_gpio_set_direction(gc, offset, false);
> +	ret = samsung_gpio_set_direction(gc, offset, false);
> +	spin_unlock_irqrestore(&bank->slock, flags);
> +
> +	return ret;
>  }
>  
>  /*
> @@ -891,7 +907,7 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
>  static const struct gpio_chip samsung_gpiolib_chip = {
>  	.request = gpiochip_generic_request,
>  	.free = gpiochip_generic_free,
> -	.set = samsung_gpio_set,
> +	.set = samsung_gpio_set_value,
>  	.get = samsung_gpio_get,
>  	.direction_input = samsung_gpio_direction_input,
>  	.direction_output = samsung_gpio_direction_output,
> 

[PATCH] pinctrl: samsung: Fixes samsung_gpio_direction_in/output releated with spinlock

[Youngmin Nam]

Previously, samsung_gpio_drection_in/output function were not covered with
one spinlock.

For example, samsung_gpio_direction_output function consists of two functions.
1. samsung_gpio_set
2. samsung_gpio_set_direction

When 2 CPUs try to control the same gpio pin heavily,
(situation like i2c control with gpio emulation)
This situation can cause below problem.

CPU 0                                   | CPU1
                                        |
samsung_gpio_direction_output           |
   samsung_gpio_set(pin A as 1)         | samsung_gpio_direction_output
                                        |    samsung_gpio_set(pin A as 0)
   samsung_gpio_set_direction           |
                                        |    samsung_gpio_set_direction

The initial value of pin A will be set as 0 while we wanted to set pin A as 1.

This patch modifies samsung_gpio_direction_in/output function
to be done in one spinlock to fix race condition.

Additionally, gpio set callback was changed with samsung_gpio_set_value
to implement gpio set callback with spinlock using samsung_gpio_set.

Signed-off-by: Youngmin Nam <ym0914@gmail.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 38 ++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 48294e7..a25f6f6 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -524,20 +524,26 @@ static void samsung_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
 {
 	struct samsung_pin_bank *bank = gc_to_pin_bank(gc);
 	const struct samsung_pin_bank_type *type = bank->type;
-	unsigned long flags;
 	void __iomem *reg;
 	u32 data;
 
 	reg = bank->drvdata->virt_base + bank->pctl_offset;
 
-	spin_lock_irqsave(&bank->slock, flags);
-
 	data = readl(reg + type->reg_offset[PINCFG_TYPE_DAT]);
 	data &= ~(1 << offset);
 	if (value)
 		data |= 1 << offset;
 	writel(data, reg + type->reg_offset[PINCFG_TYPE_DAT]);
+}
+
+static void samsung_gpio_set_value(struct gpio_chip *gc,
+					  unsigned offset, int value)
+{
+	struct samsung_pin_bank *bank = gc_to_pin_bank(gc);
+	unsigned long flags;
 
+	spin_lock_irqsave(&bank->slock, flags);
+	samsung_gpio_set(gc, offset, value);
 	spin_unlock_irqrestore(&bank->slock, flags);
 }
 
@@ -569,7 +575,6 @@ static int samsung_gpio_set_direction(struct gpio_chip *gc,
 	struct samsung_pinctrl_drv_data *drvdata;
 	void __iomem *reg;
 	u32 data, mask, shift;
-	unsigned long flags;
 
 	bank = gc_to_pin_bank(gc);
 	type = bank->type;
@@ -586,31 +591,42 @@ static int samsung_gpio_set_direction(struct gpio_chip *gc,
 		reg += 4;
 	}
 
-	spin_lock_irqsave(&bank->slock, flags);
-
 	data = readl(reg);
 	data &= ~(mask << shift);
 	if (!input)
 		data |= FUNC_OUTPUT << shift;
 	writel(data, reg);
 
-	spin_unlock_irqrestore(&bank->slock, flags);
-
 	return 0;
 }
 
 /* gpiolib gpio_direction_input callback function. */
 static int samsung_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
 {
-	return samsung_gpio_set_direction(gc, offset, true);
+	struct samsung_pin_bank *bank = gc_to_pin_bank(gc);
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&bank->slock, flags);
+	ret = samsung_gpio_set_direction(gc, offset, true);
+	spin_unlock_irqrestore(&bank->slock, flags);
+	return ret;
 }
 
 /* gpiolib gpio_direction_output callback function. */
 static int samsung_gpio_direction_output(struct gpio_chip *gc, unsigned offset,
 							int value)
 {
+	struct samsung_pin_bank *bank = gc_to_pin_bank(gc);
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&bank->slock, flags);
 	samsung_gpio_set(gc, offset, value);
-	return samsung_gpio_set_direction(gc, offset, false);
+	ret = samsung_gpio_set_direction(gc, offset, false);
+	spin_unlock_irqrestore(&bank->slock, flags);
+
+	return ret;
 }
 
 /*
@@ -891,7 +907,7 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
 static const struct gpio_chip samsung_gpiolib_chip = {
 	.request = gpiochip_generic_request,
 	.free = gpiochip_generic_free,
-	.set = samsung_gpio_set,
+	.set = samsung_gpio_set_value,
 	.get = samsung_gpio_get,
 	.direction_input = samsung_gpio_direction_input,
 	.direction_output = samsung_gpio_direction_output,
-- 
2.7.0.25.gfc10eb5

Re: [PATCH] pinctrl: samsung: Fixes samsung_gpio_direction_in/output releated with spinlock

[Tomasz Figa]

Hi Youngmin,

2016-01-19 0:21 GMT+09:00 Youngmin Nam <ym0914@gmail.com>:
> Previously, samsung_gpio_drection_in/output function were not covered with
> one spinlock.
>

Thanks for the patch, nice catch. One nitpick inline, though.

> For example, samsung_gpio_direction_output function consists of two functions.
> 1. samsung_gpio_set
> 2. samsung_gpio_set_direction
[snip]
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -524,20 +524,26 @@ static void samsung_gpio_set(struct gpio_chip *gc, unsigned offset, int value)

Can we, for consistency purposes, rename this function to
samsung_gpio_set_value() and keep the one that does locking as
samsung_gpio_set()?

This way we would match the gpio_chip op name (.set) and also have
both functions with the same name template (set_value and
set_direction) follow the same semantics of not doing any locking.

Also adding a comment above the new samsung_gpio_set_value() and
existing samsung_gpio_set_direction() that they have to be called with
bank->slock held would be helpful to avoid similar bugs in the future.

Best regards,
Tomasz

Re: [PATCH] pinctrl: samsung: Fixes samsung_gpio_direction_in/output releated with spinlock

[Youngmin Nam]

Hi Tomasz,

Thank you for your review.
I will update my patch applying your review and will resend.

Best regards,
Youngmin

On 01/22/2016 01:27 PM, Tomasz Figa wrote:
> Hi Youngmin,
> 
> 2016-01-19 0:21 GMT+09:00 Youngmin Nam <ym0914@gmail.com>:
>> Previously, samsung_gpio_drection_in/output function were not covered with
>> one spinlock.
>>
> 
> Thanks for the patch, nice catch. One nitpick inline, though.
> 
>> For example, samsung_gpio_direction_output function consists of two functions.
>> 1. samsung_gpio_set
>> 2. samsung_gpio_set_direction
> [snip]
>> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
>> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
>> @@ -524,20 +524,26 @@ static void samsung_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
> 
> Can we, for consistency purposes, rename this function to
> samsung_gpio_set_value() and keep the one that does locking as
> samsung_gpio_set()?
> 
> This way we would match the gpio_chip op name (.set) and also have
> both functions with the same name template (set_value and
> set_direction) follow the same semantics of not doing any locking.
> 
> Also adding a comment above the new samsung_gpio_set_value() and
> existing samsung_gpio_set_direction() that they have to be called with
> bank->slock held would be helpful to avoid similar bugs in the future.
> 
> Best regards,
> Tomasz
>