| Message ID | 1350551224-12857-7-git-send-email-haojian.zhuang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 18, 2012 at 11:07 AM, Haojian Zhuang <haojian.zhuang@gmail.com> wrote: > Mutex is locked duplicatly by pinconf_groups_show() and > pin_config_group_get(). It results dead lock. So avoid to lock mutex > in pinconf_groups_show(). > > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> Thanks! Applied to my fixes branch with a Cc: stable tag. Yours, Linus Walleij
On 10/18/2012 03:07 AM, Haojian Zhuang wrote: > Mutex is locked duplicatly by pinconf_groups_show() and > pin_config_group_get(). It results dead lock. So avoid to lock mutex > in pinconf_groups_show(). With this outer lock removed, how do we ensure that the pinctrl driver that is being called into remains loaded? Does the existence of the debugfs file ensure this, such that if it's open, the pinctrl driver can't be removed? Related, I wonder if much of the variable setup at the start of the function shouldn't happen inside the lock instead of outside: static int pinconf_groups_show(struct seq_file *s, void *what) { struct pinctrl_dev *pctldev = s->private; const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; const struct pinconf_ops *ops = pctldev->desc->confops; unsigned ngroups = pctlops->get_groups_count(pctldev); since what if s->private is unregistered/destroyed while this function is running?
On Fri, Oct 19, 2012 at 12:26 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 10/18/2012 03:07 AM, Haojian Zhuang wrote: >> Mutex is locked duplicatly by pinconf_groups_show() and >> pin_config_group_get(). It results dead lock. So avoid to lock mutex >> in pinconf_groups_show(). > > With this outer lock removed, how do we ensure that the pinctrl driver > that is being called into remains loaded? Does the existence of the > debugfs file ensure this, such that if it's open, the pinctrl driver > can't be removed? No, don't think so, dangling debugfs files is a common problem. > Related, I wonder if much of the variable setup at the start of the > function shouldn't happen inside the lock instead of outside: > > static int pinconf_groups_show(struct seq_file *s, void *what) > { > struct pinctrl_dev *pctldev = s->private; > const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; > const struct pinconf_ops *ops = pctldev->desc->confops; > unsigned ngroups = pctlops->get_groups_count(pctldev); > > since what if s->private is unregistered/destroyed while this function > is running? The debugfs code is fragile by nature I think, that's why we are usually a bit relaxed here and it's also why it should be disabled on production systems. But any hardening patches are welcome, however we need to get around the deadlock Haojian was seeing, maybe we should introduce a separate debugfs mutex? /me is slightly confused though... Yours, Linus Walleij
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c index 43f474c..baee2cc 100644 --- a/drivers/pinctrl/pinconf.c +++ b/drivers/pinctrl/pinconf.c @@ -537,8 +537,6 @@ static int pinconf_groups_show(struct seq_file *s, void *what) seq_puts(s, "Pin config settings per pin group\n"); seq_puts(s, "Format: group (name): configs\n"); - mutex_lock(&pinctrl_mutex); - while (selector < ngroups) { const char *gname = pctlops->get_group_name(pctldev, selector); @@ -549,8 +547,6 @@ static int pinconf_groups_show(struct seq_file *s, void *what) selector++; } - mutex_unlock(&pinctrl_mutex); - return 0; }
Mutex is locked duplicatly by pinconf_groups_show() and pin_config_group_get(). It results dead lock. So avoid to lock mutex in pinconf_groups_show(). Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> --- drivers/pinctrl/pinconf.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)