pinctrl: fix and simplify locking

There are many problems with the current pinctrl locking:

struct pinctrl_dev's gpio_ranges_lock isn't effective;
pinctrl_match_gpio_range() only holds this lock while searching for a gpio
range, but the found range is return and manipulated after releading the
lock. This could allow pinctrl_remove_gpio_range() for that range while it
is in use, and the caller may very well delete the range after removing it,
causing pinctrl code to touch the now-free range object.

Solving this requires the introduction of a higher-level lock, at least
a lock per pin controller, which both gpio range registration and
pinctrl_get()/put() will acquire.

There is missing locking on HW programming; pin controllers may pack the
configuration for different pins/groups/config options/... into one
register, and hence have to read-modify-write the register. This needs to
be protected, but currently isn't. Related, a future change will add a
"complete" op to the pin controller drivers, the idea being that each
state's programming will be programmed into the pinctrl driver followed
by the "complete" call, which may e.g. flush a register cache to HW. For
this to work, it must not be possible to interleave the pinctrl driver
calls for different devices.

As above, solving this requires the introduction of a higher-level lock,
at least a lock per pin controller, which will be held for the duration
of any pinctrl_enable()/disable() call.

However, each pinctrl mapping table entry may affect a different pin
controller if necessary. Hence, with a per-pin-controller lock, almost
any pinctrl API may need to acquire multiple locks, one per controller.
To avoid deadlock, these would need to be acquired in the same order in
all cases. This is extremely difficult to implement in the case of
pinctrl_get(), which doesn't know which pin controllers to lock until it
has parsed the entire mapping table, since it contains somewhat arbitrary
data.

The simplest solution here is to introduce a single lock that covers all
pin controllers at once. This will be acquired by all pinctrl APIs.

This then makes struct pinctrl's mutex irrelevant, since that single lock
will always be held whenever this mutex is currently held.
Signed-off-by: Stephen Warren <swarren@nvidia.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

drivers/pinctrl/core.h

@@ -9,6 +9,8 @@
  * License terms: GNU General Public License (GPL) version 2
  */
 
+#include <linux/mutex.h>
+#include <linux/radix-tree.h>
 #include <linux/pinctrl/pinconf.h>
 
 struct pinctrl_gpio_range;
@@ -22,7 +24,6 @@ struct pinctrl_gpio_range;
  *	this radix tree
  * @gpio_ranges: a list of GPIO ranges that is handled by this pin controller,
  *	ranges are added to this list at runtime
- * @gpio_ranges_lock: lock for the GPIO ranges list
  * @dev: the device entry for this pin controller
  * @owner: module providing the pin controller, used for refcounting
  * @driver_data: driver data for drivers registering to the pin controller
@@ -35,7 +36,6 @@ struct pinctrl_dev {
 	struct pinctrl_desc *desc;
 	struct radix_tree_root pin_desc_tree;
 	struct list_head gpio_ranges;
-	struct mutex gpio_ranges_lock;
 	struct device *dev;
 	struct module *owner;
 	void *driver_data;
@@ -52,7 +52,6 @@ struct pinctrl_dev {
  * @usecount: the number of active users of this pin controller setting, used
  *	to keep track of nested use cases
  * @pctldev: pin control device handling this pin control handle
- * @mutex: a lock for the pin control state holder
  * @groups: the group selectors for the pinmux device and
  *	selector combination handling this pinmux, this is a list that
  *	will be traversed on all pinmux operations such as
@@ -63,7 +62,6 @@ struct pinctrl {
 	struct device *dev;
 	unsigned usecount;
 	struct pinctrl_dev *pctldev;
-	struct mutex mutex;
 #ifdef CONFIG_PINMUX
 	struct list_head groups;
 #endif
@@ -75,14 +73,12 @@ struct pinctrl {
  * @name: a name for the pin, e.g. the name of the pin/pad/finger on a
  *	datasheet or such
  * @dynamic_name: if the name of this pin was dynamically allocated
- * @lock: a lock to protect the descriptor structure
  * @owner: the device holding this pin or NULL of no device has claimed it
  */
 struct pin_desc {
 	struct pinctrl_dev *pctldev;
 	const char *name;
 	bool dynamic_name;
-	spinlock_t lock;
 	/* These fields only added when supporting pinmux drivers */
 #ifdef CONFIG_PINMUX
 	const char *owner;
@@ -99,3 +95,5 @@ static inline struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev,
 {
 	return radix_tree_lookup(&pctldev->pin_desc_tree, pin);
 }
+
+extern struct mutex pinctrl_mutex;

drivers/pinctrl/pinconf.c

@@ -64,15 +64,23 @@ int pin_config_get(const char *dev_name, const char *name,
 	struct pinctrl_dev *pctldev;
 	int pin;
 
+	mutex_lock(&pinctrl_mutex);
+
 	pctldev = get_pinctrl_dev_from_devname(dev_name);
-	if (!pctldev)
-		return -EINVAL;
+	if (!pctldev) {
+		pin = -EINVAL;
+		goto unlock;
+	}
 
 	pin = pin_get_from_name(pctldev, name);
 	if (pin < 0)
-		return pin;
+		goto unlock;
 
-	return pin_config_get_for_pin(pctldev, pin, config);
+	pin = pin_config_get_for_pin(pctldev, pin, config);
+
+unlock:
+	mutex_unlock(&pinctrl_mutex);
+	return pin;
 }
 EXPORT_SYMBOL(pin_config_get);
 
@@ -110,17 +118,27 @@ int pin_config_set(const char *dev_name, const char *name,
 		   unsigned long config)
 {
 	struct pinctrl_dev *pctldev;
-	int pin;
+	int pin, ret;
+
+	mutex_lock(&pinctrl_mutex);
 
 	pctldev = get_pinctrl_dev_from_devname(dev_name);
-	if (!pctldev)
-		return -EINVAL;
+	if (!pctldev) {
+		ret = -EINVAL;
+		goto unlock;
+	}
 
 	pin = pin_get_from_name(pctldev, name);
-	if (pin < 0)
-		return pin;
+	if (pin < 0) {
+		ret = pin;
+		goto unlock;
+	}
+
+	ret = pin_config_set_for_pin(pctldev, pin, config);
 
-	return pin_config_set_for_pin(pctldev, pin, config);
+unlock:
+	mutex_unlock(&pinctrl_mutex);
+	return ret;
 }
 EXPORT_SYMBOL(pin_config_set);
 
@@ -129,25 +147,36 @@ int pin_config_group_get(const char *dev_name, const char *pin_group,
 {
 	struct pinctrl_dev *pctldev;
 	const struct pinconf_ops *ops;
-	int selector;
+	int selector, ret;
+
+	mutex_lock(&pinctrl_mutex);
 
 	pctldev = get_pinctrl_dev_from_devname(dev_name);
-	if (!pctldev)
-		return -EINVAL;
+	if (!pctldev) {
+		ret = -EINVAL;
+		goto unlock;
+	}
 	ops = pctldev->desc->confops;
 
 	if (!ops || !ops->pin_config_group_get) {
 		dev_err(pctldev->dev, "cannot get configuration for pin "
 			"group, missing group config get function in "
 			"driver\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto unlock;
 	}
 
 	selector = pinctrl_get_group_selector(pctldev, pin_group);
-	if (selector < 0)
-		return selector;
+	if (selector < 0) {
+		ret = selector;
+		goto unlock;
+	}
 
-	return ops->pin_config_group_get(pctldev, selector, config);
+	ret = ops->pin_config_group_get(pctldev, selector, config);
+
+unlock:
+	mutex_unlock(&pinctrl_mutex);
+	return ret;
 }
 EXPORT_SYMBOL(pin_config_group_get);
 
@@ -163,27 +192,34 @@ int pin_config_group_set(const char *dev_name, const char *pin_group,
 	int ret;
 	int i;
 
+	mutex_lock(&pinctrl_mutex);
+
 	pctldev = get_pinctrl_dev_from_devname(dev_name);
-	if (!pctldev)
-		return -EINVAL;
+	if (!pctldev) {
+		ret = -EINVAL;
+		goto unlock;
+	}
 	ops = pctldev->desc->confops;
 	pctlops = pctldev->desc->pctlops;
 
 	if (!ops || (!ops->pin_config_group_set && !ops->pin_config_set)) {
 		dev_err(pctldev->dev, "cannot configure pin group, missing "
 			"config function in driver\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto unlock;
 	}
 
 	selector = pinctrl_get_group_selector(pctldev, pin_group);
-	if (selector < 0)
-		return selector;
+	if (selector < 0) {
+		ret = selector;
+		goto unlock;
+	}
 
 	ret = pctlops->get_group_pins(pctldev, selector, &pins, &num_pins);
 	if (ret) {
 		dev_err(pctldev->dev, "cannot configure pin group, error "
 			"getting pins\n");
-		return ret;
+		goto unlock;
 	}
 
 	/*
@@ -197,23 +233,30 @@ int pin_config_group_set(const char *dev_name, const char *pin_group,
 		 * pin-by-pin as well, it returns -EAGAIN.
 		 */
 		if (ret != -EAGAIN)
-			return ret;
+			goto unlock;
 	}
 
 	/*
 	 * If the controller cannot handle entire groups, we configure each pin
 	 * individually.
 	 */
-	if (!ops->pin_config_set)
-		return 0;
+	if (!ops->pin_config_set) {
+		ret = 0;
+		goto unlock;
+	}
 
 	for (i = 0; i < num_pins; i++) {
 		ret = ops->pin_config_set(pctldev, pins[i], config);
 		if (ret < 0)
-			return ret;
+			goto unlock;
 	}
 
-	return 0;
+	ret = 0;
+
+unlock:
+	mutex_unlock(&pinctrl_mutex);
+
+	return ret;
 }
 EXPORT_SYMBOL(pin_config_group_set);
 
@@ -236,6 +279,8 @@ static int pinconf_pins_show(struct seq_file *s, void *what)
 	seq_puts(s, "Pin config settings per pin\n");
 	seq_puts(s, "Format: pin (name): pinmux setting array\n");
 
+	mutex_lock(&pinctrl_mutex);
+
 	/* The pin number can be retrived from the pin controller descriptor */
 	for (i = 0; i < pctldev->desc->npins; i++) {
 		struct pin_desc *desc;
@@ -254,6 +299,8 @@ static int pinconf_pins_show(struct seq_file *s, void *what)
 		seq_printf(s, "\n");
 	}
 
+	mutex_unlock(&pinctrl_mutex);
+
 	return 0;
 }
 
@@ -280,6 +327,8 @@ 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): pinmux setting array\n");
 
+	mutex_lock(&pinctrl_mutex);
+
 	while (pctlops->list_groups(pctldev, selector) >= 0) {
 		const char *gname = pctlops->get_group_name(pctldev, selector);
 
@@ -290,6 +339,8 @@ static int pinconf_groups_show(struct seq_file *s, void *what)
 		selector++;
 	}
 
+	mutex_unlock(&pinctrl_mutex);
+
 	return 0;
 }
 

drivers/pinctrl/pinmux.c

@@ -19,8 +19,6 @@
 #include <linux/radix-tree.h>
 #include <linux/err.h>
 #include <linux/list.h>
-#include <linux/mutex.h>
-#include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/sysfs.h>
 #include <linux/debugfs.h>
@@ -96,15 +94,12 @@ static int pin_request(struct pinctrl_dev *pctldev,
 		goto out;
 	}
 
-	spin_lock(&desc->lock);
 	if (desc->owner && strcmp(desc->owner, owner)) {
-		spin_unlock(&desc->lock);
 		dev_err(pctldev->dev,
 			"pin already requested\n");
 		goto out;
 	}
 	desc->owner = owner;
-	spin_unlock(&desc->lock);
 
 	/* Let each pin increase references to this module */
 	if (!try_module_get(pctldev->owner)) {
@@ -131,11 +126,8 @@ static int pin_request(struct pinctrl_dev *pctldev,
 		dev_err(pctldev->dev, "->request on device %s failed for pin %d\n",
 		       pctldev->desc->name, pin);
 out_free_pin:
-	if (status) {
-		spin_lock(&desc->lock);
+	if (status)
 		desc->owner = NULL;
-		spin_unlock(&desc->lock);
-	}
 out:
 	if (status)
 		dev_err(pctldev->dev, "pin-%d (%s) status %d\n",
@@ -178,10 +170,8 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
 	else if (ops->free)
 		ops->free(pctldev, pin);
 
-	spin_lock(&desc->lock);
 	owner = desc->owner;
 	desc->owner = NULL;
-	spin_unlock(&desc->lock);
 	module_put(pctldev->owner);
 
 	return owner;
@@ -580,6 +570,8 @@ static int pinmux_functions_show(struct seq_file *s, void *what)
 	const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
 	unsigned func_selector = 0;
 
+	mutex_lock(&pinctrl_mutex);
+
 	while (pmxops->list_functions(pctldev, func_selector) >= 0) {
 		const char *func = pmxops->get_function_name(pctldev,
 							  func_selector);
@@ -600,9 +592,10 @@ static int pinmux_functions_show(struct seq_file *s, void *what)
 		seq_puts(s, "]\n");
 
 		func_selector++;
-
 	}
 
+	mutex_unlock(&pinctrl_mutex);
+
 	return 0;
 }
 
@@ -614,6 +607,8 @@ static int pinmux_pins_show(struct seq_file *s, void *what)
 	seq_puts(s, "Pinmux settings per pin\n");
 	seq_puts(s, "Format: pin (name): owner\n");
 
+	mutex_lock(&pinctrl_mutex);
+
 	/* The pin number can be retrived from the pin controller descriptor */
 	for (i = 0; i < pctldev->desc->npins; i++) {
 		struct pin_desc *desc;
@@ -635,6 +630,8 @@ static int pinmux_pins_show(struct seq_file *s, void *what)
 			   is_hog ? " (HOG)" : "");
 	}
 
+	mutex_unlock(&pinctrl_mutex);
+
 	return 0;
 }
 

include/linux/pinctrl/pinctrl.h

@@ -15,7 +15,6 @@
 #ifdef CONFIG_PINCTRL
 
 #include <linux/radix-tree.h>
-#include <linux/spinlock.h>
 #include <linux/list.h>
 #include <linux/seq_file.h>