[v2] pinctrl: samsung: Fixes samsung_gpio_direction_in/output releated with spinlock
diff mbox

Message ID 1453615352-9748-1-git-send-email-ym0914@gmail.com
State New, archived
Headers show

Commit Message

Youngmin Nam Jan. 24, 2016, 6:02 a.m. UTC
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, the new samsung_gpio_set_value was added to implement
gpio set callback(samsung_gpio_set) with spinlock using this function.

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

Comments

Linus Walleij Jan. 27, 2016, 12:36 p.m. UTC | #1
On Sun, Jan 24, 2016 at 7:02 AM, Youngmin Nam <ym0914@gmail.com> 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, the new samsung_gpio_set_value was added to implement
> gpio set callback(samsung_gpio_set) with spinlock using this function.
>
> Signed-off-by: Youngmin Nam <ym0914@gmail.com>

This patch does not apply to v4.5-rc1. Please rebase this
and resend. (With Thomas ACK if it's valid.)

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Jan. 27, 2016, 1:51 p.m. UTC | #2
Hi Youngmin,

2016-01-24 15:02 GMT+09:00 Youngmin Nam <ym0914@gmail.com>:
> 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, the new samsung_gpio_set_value was added to implement
> gpio set callback(samsung_gpio_set) with spinlock using this function.
>
> Signed-off-by: Youngmin Nam <ym0914@gmail.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 48 ++++++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 13 deletions(-)
>

Thanks for addressing my comments.

Acked-by: Tomasz Figa <tomasz.figa@gmail.com>

However please rebase as Linus suggested.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Youngmin Nam Feb. 8, 2016, 4:02 a.m. UTC | #3
Thanks Linus and Tomasz.

Sorry for late. I will rebase on v4.5-rc3 and resend.

Thanks.

On 2016? 01? 27? 21:36, Linus Walleij wrote:
> On Sun, Jan 24, 2016 at 7:02 AM, Youngmin Nam <ym0914@gmail.com> 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, the new samsung_gpio_set_value was added to implement
>> gpio set callback(samsung_gpio_set) with spinlock using this function.
>>
>> Signed-off-by: Youngmin Nam <ym0914@gmail.com>
> 
> This patch does not apply to v4.5-rc1. Please rebase this
> and resend. (With Thomas ACK if it's valid.)
> 
> Yours,
> Linus Walleij
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 48294e7..c5828b8 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -519,25 +519,35 @@  static const struct pinconf_ops samsung_pinconf_ops = {
 	.pin_config_group_set	= samsung_pinconf_group_set,
 };
 
-/* gpiolib gpio_set callback function */
-static void samsung_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+/*
+ * The samsung_gpio_set_vlaue() should be called with "bank->slock" held
+ * to avoid race condition.
+ */
+static void samsung_gpio_set_value(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]);
+}
+
+/* gpiolib gpio_set callback function */
+static void samsung_gpio_set(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_value(gc, offset, value);
 	spin_unlock_irqrestore(&bank->slock, flags);
 }
 
@@ -558,6 +568,8 @@  static int samsung_gpio_get(struct gpio_chip *gc, unsigned offset)
 }
 
 /*
+ * The samsung_gpio_set_direction() should be called with "bank->slock" held
+ * to avoid race condition.
  * The calls to gpio_direction_output() and gpio_direction_input()
  * leads to this function call.
  */
@@ -569,7 +581,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 +597,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)
 {
-	samsung_gpio_set(gc, offset, value);
-	return samsung_gpio_set_direction(gc, offset, false);
+	struct samsung_pin_bank *bank = gc_to_pin_bank(gc);
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&bank->slock, flags);
+	samsung_gpio_set_value(gc, offset, value);
+	ret = samsung_gpio_set_direction(gc, offset, false);
+	spin_unlock_irqrestore(&bank->slock, flags);
+
+	return ret;
 }
 
 /*