Re: [PATCH 2/2] pinctrl: core: Add proper mutex lock in pinctrl_request_gpio
On Thu, Aug 22, 2013 at 5:41 AM, Axel Lin wrote: > 2013/8/22 Linus Walleij : >>> + mutex_lock(&pctldev->mutex); >>> + >>> /* Convert to the pin controllers number space */ >>> pin = gpio_to_pin(range, gpio); >>> >>> ret = pinmux_request_gpio(pctldev, range, pin, gpio); >>> >>> + mutex_unlock(&pctldev->mutex); >>> + >>> return ret; >> >> What is this protecting against? >> >> I'm not sure I follow this so better ask. > > I think this fixes the race between pin_free() and pin_request() calls. > (Well, I don't have a h/w to test at this moment.) > It protects accessing the members of pctldev->desc. > (e.g. update desc->mux_usecount, desc->gpio_owner, desc->mux_owner, etc) > Current code grabs pctldev->mutex before calling pinmux_free_gpio(), > but did not grab the mutex while calling pinmux_request_gpio(). OK you won me over, patch applied! I also edited in some of the above text into the commit. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] pinctrl: core: Add proper mutex lock in pinctrl_request_gpio
2013/8/22 Linus Walleij : > On Mon, Aug 19, 2013 at 4:07 AM, Axel Lin wrote: > >> This one is missed in commit 42fed7ba "pinctrl: move subsystem mutex to >> pinctrl_dev struct". > > I think this was never there. I think it was protected by the global pinctrl_mutex in v3.9, you can check the source code here: https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/pinctrl/core.c?id=refs/tags/v3.9.11 However, commit 42fed7ba44 "pinctrl: move subsystem mutex to pinctrl_dev struct" then adds mutex_lock(&pinctrldev_list_mutex); but forgot to add mutex_lock(&pctldev->mutex); here although it adds mutex_lock(&pctldev->mutex) in pinctrl_free_gpio(). The mutex_lock(&pinctrldev_list_mutex) is then moved into pinctrl_get_device_gpio_range() and pinctrl_ready_for_gpio_range() by commit 44d5f7b "pinctrl: sink pinctrldev_list_mutex". > >> Signed-off-by: Axel Lin >> --- >> drivers/pinctrl/core.c | 4 >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c >> index 53c40d9..92f86ab 100644 >> --- a/drivers/pinctrl/core.c >> +++ b/drivers/pinctrl/core.c >> @@ -562,11 +562,15 @@ int pinctrl_request_gpio(unsigned gpio) >> return ret; >> } >> >> + mutex_lock(&pctldev->mutex); >> + >> /* Convert to the pin controllers number space */ >> pin = gpio_to_pin(range, gpio); >> >> ret = pinmux_request_gpio(pctldev, range, pin, gpio); >> >> + mutex_unlock(&pctldev->mutex); >> + >> return ret; > > What is this protecting against? > > I'm not sure I follow this so better ask. I think this fixes the race between pin_free() and pin_request() calls. (Well, I don't have a h/w to test at this moment.) It protects accessing the members of pctldev->desc. (e.g. update desc->mux_usecount, desc->gpio_owner, desc->mux_owner, etc) Current code grabs pctldev->mutex before calling pinmux_free_gpio(), but did not grab the mutex while calling pinmux_request_gpio(). Regards, Axel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] pinctrl: core: Add proper mutex lock in pinctrl_request_gpio
On Mon, Aug 19, 2013 at 4:07 AM, Axel Lin wrote: > This one is missed in commit 42fed7ba "pinctrl: move subsystem mutex to > pinctrl_dev struct". I think this was never there. > Signed-off-by: Axel Lin > --- > drivers/pinctrl/core.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > index 53c40d9..92f86ab 100644 > --- a/drivers/pinctrl/core.c > +++ b/drivers/pinctrl/core.c > @@ -562,11 +562,15 @@ int pinctrl_request_gpio(unsigned gpio) > return ret; > } > > + mutex_lock(&pctldev->mutex); > + > /* Convert to the pin controllers number space */ > pin = gpio_to_pin(range, gpio); > > ret = pinmux_request_gpio(pctldev, range, pin, gpio); > > + mutex_unlock(&pctldev->mutex); > + > return ret; What is this protecting against? I'm not sure I follow this so better ask. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
