Re: [PATCH 07/10] pinctrl: remove mutex lock in groups show

On Fri, Oct 19, 2012 at 12:26 AM, Stephen Warren [off-list ref] wrote:

On 10/18/2012 03:07 AM, Haojian Zhuang wrote:

quoted

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