| Message ID | 1453130462-3621-1-git-send-email-ym0914@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 > -- 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
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,
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(-)