| Message ID | 1456008716-6236-2-git-send-email-manabian@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sat, Feb 20, 2016 at 11:51 PM, Joachim Eastwood <manabian@gmail.com> wrote: > pinctrl_find_gpio_range_from_pin takes the pctldev->mutex but so > does pinconf_pins_show and this will cause a deadlock if > pinctrl_find_gpio_range_from_pin is used in .pin_config_get > callback. > > Create an unlocked version of pinctrl_find_gpio_range_from_pin to > allow pin to gpio lookup to be used from pinconf_pins_show. > > Signed-off-by: Joachim Eastwood <manabian@gmail.com> I understand that the function is needed and it's semantically OK. > +EXPORT_SYMBOL_GPL(__pinctrl_find_gpio_range_from_pin); (...) > +extern struct pinctrl_gpio_range * > +__pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev, > + unsigned int pin); > + This function name is NOT OK. Rename it pinctrl_fund_gpio_range_from_pin_unlocked(), The arbitrary uses of the __-prefix is one of my biggest confusions when trying to understand code. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 25, 2016 at 10:21 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Sat, Feb 20, 2016 at 11:51 PM, Joachim Eastwood <manabian@gmail.com> wrote: > >> pinctrl_find_gpio_range_from_pin takes the pctldev->mutex but so >> does pinconf_pins_show and this will cause a deadlock if >> pinctrl_find_gpio_range_from_pin is used in .pin_config_get >> callback. >> >> Create an unlocked version of pinctrl_find_gpio_range_from_pin to >> allow pin to gpio lookup to be used from pinconf_pins_show. >> >> Signed-off-by: Joachim Eastwood <manabian@gmail.com> > > I understand that the function is needed and it's semantically > OK. > >> +EXPORT_SYMBOL_GPL(__pinctrl_find_gpio_range_from_pin); > (...) >> +extern struct pinctrl_gpio_range * >> +__pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev, >> + unsigned int pin); >> + > > This function name is NOT OK. > > Rename it pinctrl_fund_gpio_range_from_pin_unlocked(), Or rather, pinctrl_fund_gpio_range_from_pin_locked(), indicating that you're already holding the necessary lock when calling the function. Now I'm even confusing myself, sorry :( Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25 February 2016 at 10:23, Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Feb 25, 2016 at 10:21 AM, Linus Walleij > <linus.walleij@linaro.org> wrote: >> On Sat, Feb 20, 2016 at 11:51 PM, Joachim Eastwood <manabian@gmail.com> wrote: >> >>> pinctrl_find_gpio_range_from_pin takes the pctldev->mutex but so >>> does pinconf_pins_show and this will cause a deadlock if >>> pinctrl_find_gpio_range_from_pin is used in .pin_config_get >>> callback. >>> >>> Create an unlocked version of pinctrl_find_gpio_range_from_pin to >>> allow pin to gpio lookup to be used from pinconf_pins_show. >>> >>> Signed-off-by: Joachim Eastwood <manabian@gmail.com> >> >> I understand that the function is needed and it's semantically >> OK. >> >>> +EXPORT_SYMBOL_GPL(__pinctrl_find_gpio_range_from_pin); >> (...) >>> +extern struct pinctrl_gpio_range * >>> +__pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev, >>> + unsigned int pin); >>> + >> >> This function name is NOT OK. >> >> Rename it pinctrl_fund_gpio_range_from_pin_unlocked(), > > Or rather, pinctrl_fund_gpio_range_from_pin_locked(), > indicating that you're already holding the necessary lock > when calling the function. Now I'm even confusing myself, > sorry :( Shouldn't the function name indicate what the function does with the lock? pinctrl_fund_gpio_range_from_pin_unlocked() would indicate to me that it does not acquire a lock and it is your responsibility as a caller to ensure that the correct lock is held before calling. But I am fine with either pinctrl_fund_gpio_range_from_pin_unlocked() or pinctrl_fund_gpio_range_from_pin_locked(). It's all up to you. regards, Joachim Eastwood -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 25, 2016 at 12:19 PM, Joachim Eastwood <manabian@gmail.com> wrote: >> Or rather, pinctrl_fund_gpio_range_from_pin_locked(), >> indicating that you're already holding the necessary lock >> when calling the function. Now I'm even confusing myself, >> sorry :( > > Shouldn't the function name indicate what the function does with the lock? > > pinctrl_fund_gpio_range_from_pin_unlocked() would indicate to me that > it does not acquire a lock and it is your responsibility as a caller > to ensure that the correct lock is held before calling. OK hm maybe you're right, grep the kernel for precedents. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25 February 2016 at 15:55, Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Feb 25, 2016 at 12:19 PM, Joachim Eastwood <manabian@gmail.com> wrote: > >>> Or rather, pinctrl_fund_gpio_range_from_pin_locked(), >>> indicating that you're already holding the necessary lock >>> when calling the function. Now I'm even confusing myself, >>> sorry :( >> >> Shouldn't the function name indicate what the function does with the lock? >> >> pinctrl_fund_gpio_range_from_pin_unlocked() would indicate to me that >> it does not acquire a lock and it is your responsibility as a caller >> to ensure that the correct lock is held before calling. > > OK hm maybe you're right, grep the kernel for precedents. hmm, I not sure anymore. What do you think about pinctrl_find_gpio_range_from_pin_nolock()? The _nolock() prefix is also used in the kernel and might convey what we want better. Thoughts? regards, Joachim Eastwood -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 25, 2016 at 4:15 PM, Joachim Eastwood <manabian@gmail.com> wrote: > On 25 February 2016 at 15:55, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Thu, Feb 25, 2016 at 12:19 PM, Joachim Eastwood <manabian@gmail.com> wrote: >> >>>> Or rather, pinctrl_fund_gpio_range_from_pin_locked(), >>>> indicating that you're already holding the necessary lock >>>> when calling the function. Now I'm even confusing myself, >>>> sorry :( >>> >>> Shouldn't the function name indicate what the function does with the lock? >>> >>> pinctrl_fund_gpio_range_from_pin_unlocked() would indicate to me that >>> it does not acquire a lock and it is your responsibility as a caller >>> to ensure that the correct lock is held before calling. >> >> OK hm maybe you're right, grep the kernel for precedents. > > hmm, I not sure anymore. > > What do you think about pinctrl_find_gpio_range_from_pin_nolock()? > > The _nolock() prefix is also used in the kernel and might convey what > we want better. Thoughts? Go for it. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" 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/core.c b/drivers/pinctrl/core.c index 2686a4450dfc..0c2cb883b18a 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -487,12 +487,11 @@ EXPORT_SYMBOL_GPL(pinctrl_get_group_pins); * @pin: a controller-local number to find the range for */ struct pinctrl_gpio_range * -pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev, - unsigned int pin) +__pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev, + unsigned int pin) { struct pinctrl_gpio_range *range; - mutex_lock(&pctldev->mutex); /* Loop over the ranges */ list_for_each_entry(range, &pctldev->gpio_ranges, node) { /* Check if we're in the valid range */ @@ -500,15 +499,27 @@ pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev, int a; for (a = 0; a < range->npins; a++) { if (range->pins[a] == pin) - goto out; + return range; } } else if (pin >= range->pin_base && pin < range->pin_base + range->npins) - goto out; + return range; } - range = NULL; -out: + + return NULL; +} +EXPORT_SYMBOL_GPL(__pinctrl_find_gpio_range_from_pin); + +struct pinctrl_gpio_range * +pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev, + unsigned int pin) +{ + struct pinctrl_gpio_range *range; + + mutex_lock(&pctldev->mutex); + range = __pinctrl_find_gpio_range_from_pin(pctldev, pin); mutex_unlock(&pctldev->mutex); + return range; } EXPORT_SYMBOL_GPL(pinctrl_find_gpio_range_from_pin); diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h index b24ea846c867..99bd8428542a 100644 --- a/drivers/pinctrl/core.h +++ b/drivers/pinctrl/core.h @@ -182,6 +182,10 @@ static inline struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev, return radix_tree_lookup(&pctldev->pin_desc_tree, pin); } +extern struct pinctrl_gpio_range * +__pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev, + unsigned int pin); + int pinctrl_register_map(struct pinctrl_map const *maps, unsigned num_maps, bool dup); void pinctrl_unregister_map(struct pinctrl_map const *map);
pinctrl_find_gpio_range_from_pin takes the pctldev->mutex but so does pinconf_pins_show and this will cause a deadlock if pinctrl_find_gpio_range_from_pin is used in .pin_config_get callback. Create an unlocked version of pinctrl_find_gpio_range_from_pin to allow pin to gpio lookup to be used from pinconf_pins_show. Signed-off-by: Joachim Eastwood <manabian@gmail.com> --- Hi Linus, Here is traceback I got before I added this: [ 161.370000] cat D 281b5275 0 23 1 0x00000000 [ 161.370000] [<281b5275>] (__schedule) from [<281b5479>] (schedule+0x29/0x5c) [ 161.370000] [<281b5479>] (schedule) from [<281b54bb>] (schedule_preempt_disabled+0xf/0x18) [ 161.370000] [<281b54bb>] (schedule_preempt_disabled) from [<281b6401>] (__mutex_lock_slowpath+0x45/0xf8) [ 161.370000] [<281b6401>] (__mutex_lock_slowpath) from [<280b5ddb>] (pinctrl_find_gpio_range_from_pin+0x13/0x68) [ 161.370000] [<280b5ddb>] (pinctrl_find_gpio_range_from_pin) from [<280b8da9>] (lpc18xx_pconf_get+0x95/0x204) [ 161.370000] [<280b8da9>] (lpc18xx_pconf_get) from [<280b827b>] (pinconf_generic_dump_one+0xbb/0xcc) [ 161.370000] [<280b827b>] (pinconf_generic_dump_one) from [<280b8337>] (pinconf_generic_dump_pins+0x4f/0x54) [ 161.370000] [<280b8337>] (pinconf_generic_dump_pins) from [<280b7b01>] (pinconf_pins_show+0x8d/0xc0) [ 161.370000] [<280b7b01>] (pinconf_pins_show) from [<28068fb7>] (seq_read+0x7b/0x2bc) [ 161.370000] [<28068fb7>] (seq_read) from [<280547f3>] (SyS_read+0x2b/0x68) [ 161.370000] [<280547f3>] (SyS_read) from [<28008c61>] (ret_fast_syscall+0x1/0x4a) drivers/pinctrl/core.c | 25 ++++++++++++++++++------- drivers/pinctrl/core.h | 4 ++++ 2 files changed, 22 insertions(+), 7 deletions(-)