[PATCH v3 00/24] gpio: rework locking and object life-time control

[PATCH v3 00/24] gpio: rework locking and object life-time control

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This is a big rework of locking in GPIOLIB. The current serialization is
pretty much useless. There is one big spinlock (gpio_lock) that "protects"
both the GPIO device list, GPIO descriptor access and who knows what else.

I'm putting "protects" in quotes as in several places the lock is
taken, released whenever a sleeping function is called and re-taken
without regards for the "protected" state that may have changed.

First a little background on what we're dealing with in GPIOLIB. We have
consumer API functions that can be called from any context explicitly
(get/set value, set direction) as well as many others which will get
called in atomic context implicitly (e.g. set config called in certain
situations from gpiod_direction_output()).

On the other side: we have GPIO provider drivers whose callbacks may or
may not sleep depending on the underlying protocol.

This makes any attempts at serialization quite complex. We typically
cannot use sleeping locks - we may be called from atomic - but we also
often cannot use spinlocks - provider callbacks may sleep. Moreover: we
have close ties with the interrupt and pinctrl subsystems, often either
calling into them or getting called from them. They use their own locking
schemes which are at odds with ours (pinctrl uses mutexes, the interrupt
subsystem can call GPIO helpers with spinlock taken).

There is also another significant issue: the GPIO device object contains
a pointer to gpio_chip which is the implementation of the GPIO provider.
This object can be removed at any point - as GPIOLIB officially supports
hotplugging with all the dynamic expanders that we provide drivers for -
and leave the GPIO API callbacks with a suddenly NULL pointer. This is
a problem that allowed user-space processes to easily crash the kernel
until we patched it with a read-write semaphore in the user-space facing
code (but the problem still exists for in-kernel users). This was
recognized before as evidenced by the implementation of validate_desc()
but without proper serialization, simple checking for a NULL pointer is
pointless and we do need a generic solution for that issue as well.

If we want to get it right - the more lockless we go, the better. This is
why SRCU seems to be the right candidate for the mechanism to use. In fact
it's the only mechanism we can use our read-only critical sections to be
called from atomic and protecc contexts as well as call driver callbacks
that may sleep (for the latter case).

We're going to use it in three places: to protect the global list of GPIO
devices, to ensure consistency when dereferencing the chip pointer in GPIO
device struct and finally to ensure that users can access GPIO descriptors
and always see a consistent state.

We do NOT serialize all API callbacks. This means that provider callbacks
may be called simultaneously and GPIO drivers need to provide their own
locking if needed. This is on purpose. First: we only support exclusive
GPIO usage* so there's no risk of two drivers getting in each other's way
over the same GPIO. Second: with this series, we ensure enough consistency
to limit the chance of drivers or user-space users crashing the kernel.
With additional improvements in handling the flags field in GPIO
descriptors there's very little to gain, while bitbanging drivers may care
about the increased performance of going lockless.

This series brings in one somewhat significant functional change for
in-kernel users, namely: GPIO API calls, for which the underlying GPIO
chip is gone, will no longer return 0 and emit a log message but instead
will return -ENODEV.

I know this is a lot of code to go through but the more eyes we get on it
the better.

Thanks,
Bartosz

* - This is not technically true. We do provide the
GPIOD_FLAGS_BIT_NONEXCLUSIVE flag. However this is just another piece of
technical debt. This is a hack provided for a single use-case in the
regulator framework which got out of control and is now used in many
places that should have never touched it. It's utterly broken and doesn't
even provide any contract as to what a "shared GPIO" is. I would argue
that it's the next thing we should address by providing "reference counted
GPIO enable", not just a flag allowing to request the same GPIO twice
and then allow two drivers to fight over who toggles it as is the case
now. For now, let's just treat users of GPIOD_FLAGS_BIT_NONEXCLUSIVE like
they're consciously and deliberately choosing to risk undefined behavior.

v2 -> v3:
- fix SRCU cleanup in error path
- add a comment explaining the use of gpio_device_find() in sysfs code
- don't needlessly dereference gdev->chip in sysfs code
- don't return 1 (INPUT) for NULL descriptors from gpiod_get_direction(),
  rather: return -EINVAL
- fix debugfs code: take the SRCU read lock in .start() and release it in
  .stop() callbacks for seq file instead of taking it locally which
  doesn't protect the entire seq printout
- move the removal of the GPIO device from the list before setting the
  chip pointer to NULL

v1 -> v2:
- fix jumping over variable initialization in sysfs code
- fix RCU-related sparse warnings
- fix a smatch complaint about uninitialized variables (even though it's
  a false positive coming from the fact that scoped_guard() is implemented
  as a for loop
- fix a potential NULL-pointer dereference in debugfs callbacks
- improve commit messages

Bartosz Golaszewski (24):
  gpio: protect the list of GPIO devices with SRCU
  gpio: of: assign and read the hog pointer atomically
  gpio: remove unused logging helpers
  gpio: provide and use gpiod_get_label()
  gpio: don't set label from irq helpers
  gpio: add SRCU infrastructure to struct gpio_desc
  gpio: protect the descriptor label with SRCU
  gpio: sysfs: use gpio_device_find() to iterate over existing devices
  gpio: remove gpio_lock
  gpio: reinforce desc->flags handling
  gpio: remove unneeded code from gpio_device_get_desc()
  gpio: sysfs: extend the critical section for unregistering sysfs
    devices
  gpio: sysfs: pass the GPIO device - not chip - to sysfs callbacks
  gpio: cdev: replace gpiochip_get_desc() with gpio_device_get_desc()
  gpio: cdev: don't access gdev->chip if it's not needed
  gpio: sysfs: don't access gdev->chip if it's not needed
  gpio: don't dereference gdev->chip in gpiochip_setup_dev()
  gpio: reduce the functionality of validate_desc()
  gpio: remove unnecessary checks from gpiod_to_chip()
  gpio: add the can_sleep flag to struct gpio_device
  gpio: add SRCU infrastructure to struct gpio_device
  gpio: protect the pointer to gpio_chip in gpio_device with SRCU
  gpio: remove the RW semaphore from the GPIO device
  gpio: mark unsafe gpio_chip manipulators as deprecated

 drivers/gpio/gpiolib-cdev.c  |  92 ++--
 drivers/gpio/gpiolib-of.c    |   4 +-
 drivers/gpio/gpiolib-sysfs.c | 151 ++++---
 drivers/gpio/gpiolib.c       | 791 +++++++++++++++++++----------------
 drivers/gpio/gpiolib.h       |  86 ++--
 5 files changed, 635 insertions(+), 489 deletions(-)

-- 
2.40.1

[PATCH v3 01/24] gpio: protect the list of GPIO devices with SRCU

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We're working towards removing the "multi-function" GPIO spinlock that's
implemented terribly wrong. We tried using an RW-semaphore to protect
the list of GPIO devices but it turned out that we still have old code
using legacy GPIO calls that need to translate the global GPIO number to
the address of the associated descriptor and - to that end - traverse
the list while holding the lock. If we change the spinlock to a sleeping
lock then we'll end up with "scheduling while atomic" bugs.

Let's allow lockless traversal of the list using SRCU and only use the
mutex when modyfing the list.

While at it: let's protect the period between when we start the lookup
and when we finally request the descriptor (increasing the reference
count of the GPIO device) with the SRCU read lock.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 251 ++++++++++++++++++++++-------------------
 1 file changed, 137 insertions(+), 114 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 3cad49363a72..181faeeceb3f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2,6 +2,7 @@
 
 #include <linux/acpi.h>
 #include <linux/bitmap.h>
+#include <linux/cleanup.h>
 #include <linux/compat.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
@@ -14,12 +15,14 @@
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
+#include <linux/lockdep.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/srcu.h>
 #include <linux/string.h>
 
 #include <linux/gpio.h>
@@ -81,7 +84,12 @@ DEFINE_SPINLOCK(gpio_lock);
 
 static DEFINE_MUTEX(gpio_lookup_lock);
 static LIST_HEAD(gpio_lookup_list);
+
 LIST_HEAD(gpio_devices);
+/* Protects the GPIO device list against concurrent modifications. */
+static DEFINE_MUTEX(gpio_devices_lock);
+/* Ensures coherence during read-only accesses to the list of GPIO devices. */
+DEFINE_STATIC_SRCU(gpio_devices_srcu);
 
 static DEFINE_MUTEX(gpio_machine_hogs_mutex);
 static LIST_HEAD(gpio_machine_hogs);
@@ -113,20 +121,16 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label)
 struct gpio_desc *gpio_to_desc(unsigned gpio)
 {
 	struct gpio_device *gdev;
-	unsigned long flags;
 
-	spin_lock_irqsave(&gpio_lock, flags);
-
-	list_for_each_entry(gdev, &gpio_devices, list) {
-		if (gdev->base <= gpio &&
-		    gdev->base + gdev->ngpio > gpio) {
-			spin_unlock_irqrestore(&gpio_lock, flags);
-			return &gdev->descs[gpio - gdev->base];
+	scoped_guard(srcu, &gpio_devices_srcu) {
+		list_for_each_entry_srcu(gdev, &gpio_devices, list,
+				srcu_read_lock_held(&gpio_devices_srcu)) {
+			if (gdev->base <= gpio &&
+			    gdev->base + gdev->ngpio > gpio)
+				return &gdev->descs[gpio - gdev->base];
 		}
 	}
 
-	spin_unlock_irqrestore(&gpio_lock, flags);
-
 	if (!gpio_is_valid(gpio))
 		pr_warn("invalid GPIO %d\n", gpio);
 
@@ -282,7 +286,8 @@ static int gpiochip_find_base_unlocked(int ngpio)
 	struct gpio_device *gdev;
 	int base = GPIO_DYNAMIC_BASE;
 
-	list_for_each_entry(gdev, &gpio_devices, list) {
+	list_for_each_entry_srcu(gdev, &gpio_devices, list,
+				 lockdep_is_held(&gpio_devices_lock)) {
 		/* found a free space? */
 		if (gdev->base >= base + ngpio)
 			break;
@@ -354,23 +359,25 @@ static int gpiodev_add_to_list_unlocked(struct gpio_device *gdev)
 {
 	struct gpio_device *prev, *next;
 
+	lockdep_assert_held(&gpio_devices_lock);
+
 	if (list_empty(&gpio_devices)) {
 		/* initial entry in list */
-		list_add_tail(&gdev->list, &gpio_devices);
+		list_add_tail_rcu(&gdev->list, &gpio_devices);
 		return 0;
 	}
 
 	next = list_first_entry(&gpio_devices, struct gpio_device, list);
 	if (gdev->base + gdev->ngpio <= next->base) {
 		/* add before first entry */
-		list_add(&gdev->list, &gpio_devices);
+		list_add_rcu(&gdev->list, &gpio_devices);
 		return 0;
 	}
 
 	prev = list_last_entry(&gpio_devices, struct gpio_device, list);
 	if (prev->base + prev->ngpio <= gdev->base) {
 		/* add behind last entry */
-		list_add_tail(&gdev->list, &gpio_devices);
+		list_add_tail_rcu(&gdev->list, &gpio_devices);
 		return 0;
 	}
 
@@ -382,11 +389,13 @@ static int gpiodev_add_to_list_unlocked(struct gpio_device *gdev)
 		/* add between prev and next */
 		if (prev->base + prev->ngpio <= gdev->base
 				&& gdev->base + gdev->ngpio <= next->base) {
-			list_add(&gdev->list, &prev->list);
+			list_add_rcu(&gdev->list, &prev->list);
 			return 0;
 		}
 	}
 
+	synchronize_srcu(&gpio_devices_srcu);
+
 	return -EBUSY;
 }
 
@@ -399,26 +408,21 @@ static int gpiodev_add_to_list_unlocked(struct gpio_device *gdev)
 static struct gpio_desc *gpio_name_to_desc(const char * const name)
 {
 	struct gpio_device *gdev;
-	unsigned long flags;
+	struct gpio_desc *desc;
 
 	if (!name)
 		return NULL;
 
-	spin_lock_irqsave(&gpio_lock, flags);
-
-	list_for_each_entry(gdev, &gpio_devices, list) {
-		struct gpio_desc *desc;
+	guard(srcu)(&gpio_devices_srcu);
 
+	list_for_each_entry_srcu(gdev, &gpio_devices, list,
+				 srcu_read_lock_held(&gpio_devices_srcu)) {
 		for_each_gpio_desc(gdev->chip, desc) {
-			if (desc->name && !strcmp(desc->name, name)) {
-				spin_unlock_irqrestore(&gpio_lock, flags);
+			if (desc->name && !strcmp(desc->name, name))
 				return desc;
-			}
 		}
 	}
 
-	spin_unlock_irqrestore(&gpio_lock, flags);
-
 	return NULL;
 }
 
@@ -752,7 +756,10 @@ static void gpiochip_setup_devs(void)
 	struct gpio_device *gdev;
 	int ret;
 
-	list_for_each_entry(gdev, &gpio_devices, list) {
+	guard(srcu)(&gpio_devices_srcu);
+
+	list_for_each_entry_srcu(gdev, &gpio_devices, list,
+				 srcu_read_lock_held(&gpio_devices_srcu)) {
 		ret = gpiochip_setup_dev(gdev);
 		if (ret)
 			dev_err(&gdev->dev,
@@ -817,7 +824,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 			       struct lock_class_key *request_key)
 {
 	struct gpio_device *gdev;
-	unsigned long flags;
 	unsigned int i;
 	int base = 0;
 	int ret = 0;
@@ -883,49 +889,47 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 
 	gdev->ngpio = gc->ngpio;
 
-	spin_lock_irqsave(&gpio_lock, flags);
-
-	/*
-	 * TODO: this allocates a Linux GPIO number base in the global
-	 * GPIO numberspace for this chip. In the long run we want to
-	 * get *rid* of this numberspace and use only descriptors, but
-	 * it may be a pipe dream. It will not happen before we get rid
-	 * of the sysfs interface anyways.
-	 */
-	base = gc->base;
-	if (base < 0) {
-		base = gpiochip_find_base_unlocked(gc->ngpio);
+	scoped_guard(mutex, &gpio_devices_lock) {
+		/*
+		 * TODO: this allocates a Linux GPIO number base in the global
+		 * GPIO numberspace for this chip. In the long run we want to
+		 * get *rid* of this numberspace and use only descriptors, but
+		 * it may be a pipe dream. It will not happen before we get rid
+		 * of the sysfs interface anyways.
+		 */
+		base = gc->base;
 		if (base < 0) {
-			spin_unlock_irqrestore(&gpio_lock, flags);
-			ret = base;
-			base = 0;
+			base = gpiochip_find_base_unlocked(gc->ngpio);
+			if (base < 0) {
+				ret = base;
+				base = 0;
+				goto err_free_label;
+			}
+
+			/*
+			 * TODO: it should not be necessary to reflect the
+			 * assigned base outside of the GPIO subsystem. Go over
+			 * drivers and see if anyone makes use of this, else
+			 * drop this and assign a poison instead.
+			 */
+			gc->base = base;
+		} else {
+			dev_warn(&gdev->dev,
+				 "Static allocation of GPIO base is deprecated, use dynamic allocation.\n");
+		}
+
+		gdev->base = base;
+
+		ret = gpiodev_add_to_list_unlocked(gdev);
+		if (ret) {
+			chip_err(gc, "GPIO integer space overlap, cannot add chip\n");
 			goto err_free_label;
 		}
-		/*
-		 * TODO: it should not be necessary to reflect the assigned
-		 * base outside of the GPIO subsystem. Go over drivers and
-		 * see if anyone makes use of this, else drop this and assign
-		 * a poison instead.
-		 */
-		gc->base = base;
-	} else {
-		dev_warn(&gdev->dev,
-			 "Static allocation of GPIO base is deprecated, use dynamic allocation.\n");
-	}
-	gdev->base = base;
-
-	ret = gpiodev_add_to_list_unlocked(gdev);
-	if (ret) {
-		spin_unlock_irqrestore(&gpio_lock, flags);
-		chip_err(gc, "GPIO integer space overlap, cannot add chip\n");
-		goto err_free_label;
 	}
 
 	for (i = 0; i < gc->ngpio; i++)
 		gdev->descs[i].gdev = gdev;
 
-	spin_unlock_irqrestore(&gpio_lock, flags);
-
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
 	init_rwsem(&gdev->sem);
@@ -1016,9 +1020,9 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		goto err_print_message;
 	}
 err_remove_from_list:
-	spin_lock_irqsave(&gpio_lock, flags);
-	list_del(&gdev->list);
-	spin_unlock_irqrestore(&gpio_lock, flags);
+	scoped_guard(mutex, &gpio_devices_lock)
+		list_del_rcu(&gdev->list);
+	synchronize_srcu(&gpio_devices_srcu);
 err_free_label:
 	kfree_const(gdev->label);
 err_free_descs:
@@ -1057,6 +1061,11 @@ void gpiochip_remove(struct gpio_chip *gc)
 	/* FIXME: should the legacy sysfs handling be moved to gpio_device? */
 	gpiochip_sysfs_unregister(gdev);
 	gpiochip_free_hogs(gc);
+
+	scoped_guard(mutex, &gpio_devices_lock)
+		list_del_rcu(&gdev->list);
+	synchronize_srcu(&gpio_devices_srcu);
+
 	/* Numb the device, cancelling all outstanding operations */
 	gdev->chip = NULL;
 	gpiochip_irqchip_remove(gc);
@@ -1081,9 +1090,6 @@ void gpiochip_remove(struct gpio_chip *gc)
 		dev_crit(&gdev->dev,
 			 "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
 
-	scoped_guard(spinlock_irqsave, &gpio_lock)
-		list_del(&gdev->list);
-
 	/*
 	 * The gpiochip side puts its use of the device to rest here:
 	 * if there are no userspace clients, the chardev and device will
@@ -1130,7 +1136,7 @@ struct gpio_device *gpio_device_find(void *data,
 	 */
 	might_sleep();
 
-	guard(spinlock_irqsave)(&gpio_lock);
+	guard(srcu)(&gpio_devices_srcu);
 
 	list_for_each_entry(gdev, &gpio_devices, list) {
 		if (gdev->chip && match(gdev->chip, data))
@@ -4138,30 +4144,39 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
 						bool platform_lookup_allowed)
 {
 	unsigned long lookupflags = GPIO_LOOKUP_FLAGS_DEFAULT;
-	struct gpio_desc *desc;
-	int ret;
-
-	desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, &flags, &lookupflags);
-	if (gpiod_not_found(desc) && platform_lookup_allowed) {
-		/*
-		 * Either we are not using DT or ACPI, or their lookup did not
-		 * return a result. In that case, use platform lookup as a
-		 * fallback.
-		 */
-		dev_dbg(consumer, "using lookup tables for GPIO lookup\n");
-		desc = gpiod_find(consumer, con_id, idx, &lookupflags);
-	}
-
-	if (IS_ERR(desc)) {
-		dev_dbg(consumer, "No GPIO consumer %s found\n", con_id);
-		return desc;
-	}
-
 	/*
-	 * If a connection label was passed use that, else attempt to use
-	 * the device name as label
+	 * scoped_guard() is implemented as a for loop, meaning static
+	 * analyzers will complain about these two not being initialized.
 	 */
-	ret = gpiod_request(desc, label);
+	struct gpio_desc *desc = NULL;
+	int ret = 0;
+
+	scoped_guard(srcu, &gpio_devices_srcu) {
+		desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
+					    &flags, &lookupflags);
+		if (gpiod_not_found(desc) && platform_lookup_allowed) {
+			/*
+			 * Either we are not using DT or ACPI, or their lookup
+			 * did not return a result. In that case, use platform
+			 * lookup as a fallback.
+			 */
+			dev_dbg(consumer,
+				"using lookup tables for GPIO lookup\n");
+			desc = gpiod_find(consumer, con_id, idx, &lookupflags);
+		}
+
+		if (IS_ERR(desc)) {
+			dev_dbg(consumer, "No GPIO consumer %s found\n",
+				con_id);
+			return desc;
+		}
+
+		/*
+		 * If a connection label was passed use that, else attempt to use
+		 * the device name as label
+		 */
+		ret = gpiod_request(desc, label);
+	}
 	if (ret) {
 		if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
 			return ERR_PTR(ret);
@@ -4730,61 +4745,69 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
 	}
 }
 
+struct gpiolib_seq_priv {
+	bool newline;
+	int idx;
+};
+
 static void *gpiolib_seq_start(struct seq_file *s, loff_t *pos)
 {
-	unsigned long flags;
-	struct gpio_device *gdev = NULL;
+	struct gpiolib_seq_priv *priv;
+	struct gpio_device *gdev;
 	loff_t index = *pos;
 
-	s->private = "";
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return NULL;
 
-	spin_lock_irqsave(&gpio_lock, flags);
-	list_for_each_entry(gdev, &gpio_devices, list)
-		if (index-- == 0) {
-			spin_unlock_irqrestore(&gpio_lock, flags);
+	s->private = priv;
+	priv->idx = srcu_read_lock(&gpio_devices_srcu);
+
+	list_for_each_entry_srcu(gdev, &gpio_devices, list,
+				 srcu_read_lock_held(&gpio_devices_srcu)) {
+		if (index-- == 0)
 			return gdev;
-		}
-	spin_unlock_irqrestore(&gpio_lock, flags);
+	}
 
 	return NULL;
 }
 
 static void *gpiolib_seq_next(struct seq_file *s, void *v, loff_t *pos)
 {
-	unsigned long flags;
-	struct gpio_device *gdev = v;
-	void *ret = NULL;
+	struct gpiolib_seq_priv *priv = s->private;
+	struct gpio_device *gdev = v, *next;
 
-	spin_lock_irqsave(&gpio_lock, flags);
-	if (list_is_last(&gdev->list, &gpio_devices))
-		ret = NULL;
-	else
-		ret = list_first_entry(&gdev->list, struct gpio_device, list);
-	spin_unlock_irqrestore(&gpio_lock, flags);
-
-	s->private = "\n";
+	next = list_entry_rcu(gdev->list.next, struct gpio_device, list);
+	gdev = &next->list == &gpio_devices ? NULL : next;
+	priv->newline = true;
 	++*pos;
 
-	return ret;
+	return gdev;
 }
 
 static void gpiolib_seq_stop(struct seq_file *s, void *v)
 {
+	struct gpiolib_seq_priv *priv = s->private;
+
+	srcu_read_unlock(&gpio_devices_srcu, priv->idx);
+	kfree(priv);
 }
 
 static int gpiolib_seq_show(struct seq_file *s, void *v)
 {
+	struct gpiolib_seq_priv *priv = s->private;
 	struct gpio_device *gdev = v;
 	struct gpio_chip *gc = gdev->chip;
 	struct device *parent;
 
 	if (!gc) {
-		seq_printf(s, "%s%s: (dangling chip)", (char *)s->private,
+		seq_printf(s, "%s%s: (dangling chip)",
+			   priv->newline ? "\n" : "",
 			   dev_name(&gdev->dev));
 		return 0;
 	}
 
-	seq_printf(s, "%s%s: GPIOs %d-%d", (char *)s->private,
+	seq_printf(s, "%s%s: GPIOs %d-%d", priv->newline ? "\n" : "",
 		   dev_name(&gdev->dev),
 		   gdev->base, gdev->base + gdev->ngpio - 1);
 	parent = gc->parent;
-- 
2.40.1

Re: [PATCH v3 01/24] gpio: protect the list of GPIO devices with SRCU

[Hillf Danton]

On Thu,  8 Feb 2024 10:58:57 +0100 Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> @@ -382,11 +389,13 @@ static int gpiodev_add_to_list_unlocked(struct gpio_device *gdev)
>  		/* add between prev and next */
>  		if (prev->base + prev->ngpio <= gdev->base
>  				&& gdev->base + gdev->ngpio <= next->base) {
> -			list_add(&gdev->list, &prev->list);
> +			list_add_rcu(&gdev->list, &prev->list);
>  			return 0;
>  		}
>  	}
>  
> +	synchronize_srcu(&gpio_devices_srcu);
> +
If not typo, could you shed light on why this sync is needed?

>  	return -EBUSY;
>  }

Re: [PATCH v3 01/24] gpio: protect the list of GPIO devices with SRCU

[Bartosz Golaszewski]

On Sat, Feb 10, 2024 at 12:01 PM Hillf Danton <hdanton@sina.com> wrote:
>
> On Thu,  8 Feb 2024 10:58:57 +0100 Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > @@ -382,11 +389,13 @@ static int gpiodev_add_to_list_unlocked(struct gpio_device *gdev)
> >               /* add between prev and next */
> >               if (prev->base + prev->ngpio <= gdev->base
> >                               && gdev->base + gdev->ngpio <= next->base) {
> > -                     list_add(&gdev->list, &prev->list);
> > +                     list_add_rcu(&gdev->list, &prev->list);
> >                       return 0;
> >               }
> >       }
> >
> > +     synchronize_srcu(&gpio_devices_srcu);
> > +
> If not typo, could you shed light on why this sync is needed?
>

No it's not a typo. I'm not an expert on RCU but I figured that right
after we add the new element to the SRCU-protected list, we need to
wait for all read-only critical sections to complete before
continuing? Is that not correct?

Bart

> >       return -EBUSY;
> >  }
>

[PATCH v3 02/24] gpio: of: assign and read the hog pointer atomically

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The device nodes representing GPIO hogs cannot be deleted without
unregistering the GPIO chip so there's no need to serialize their access.
However we must ensure that users can get the right address so write and
read it atomically.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-of.c | 4 ++--
 drivers/gpio/gpiolib.c    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 77509aa19900..bd6b8702c790 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -800,7 +800,7 @@ static int of_gpiochip_add_hog(struct gpio_chip *chip, struct device_node *hog)
 			return ret;
 
 #ifdef CONFIG_OF_DYNAMIC
-		desc->hog = hog;
+		WRITE_ONCE(desc->hog, hog);
 #endif
 	}
 
@@ -848,7 +848,7 @@ static void of_gpiochip_remove_hog(struct gpio_chip *chip,
 	struct gpio_desc *desc;
 
 	for_each_gpio_desc_with_flag(chip, desc, FLAG_IS_HOGGED)
-		if (desc->hog == hog)
+		if (READ_ONCE(desc->hog) == hog)
 			gpiochip_free_own_desc(desc);
 }
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 181faeeceb3f..8ca9aebfa81f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2330,7 +2330,7 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 		clear_bit(FLAG_EDGE_FALLING, &desc->flags);
 		clear_bit(FLAG_IS_HOGGED, &desc->flags);
 #ifdef CONFIG_OF_DYNAMIC
-		desc->hog = NULL;
+		WRITE_ONCE(desc->hog, NULL);
 #endif
 		ret = true;
 	}
-- 
2.40.1

[PATCH v3 03/24] gpio: remove unused logging helpers

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The general rule of the kernel is to not provide symbols that have no
users upstream. Let's remove logging helpers that are not used anywhere.

This will save us work later when we'll be modifying them to use the
upcoming SRCU infrastructure.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.h | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index a4a2520b5f31..c3ae5bfa3f2e 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -219,31 +219,18 @@ static inline int gpio_chip_hwgpio(const struct gpio_desc *desc)
 
 /* With descriptor prefix */
 
-#define gpiod_emerg(desc, fmt, ...)					       \
-	pr_emerg("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\
-		 ##__VA_ARGS__)
-#define gpiod_crit(desc, fmt, ...)					       \
-	pr_crit("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \
-		 ##__VA_ARGS__)
 #define gpiod_err(desc, fmt, ...)					       \
 	pr_err("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",  \
 		 ##__VA_ARGS__)
 #define gpiod_warn(desc, fmt, ...)					       \
 	pr_warn("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \
 		 ##__VA_ARGS__)
-#define gpiod_info(desc, fmt, ...)					       \
-	pr_info("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \
-		 ##__VA_ARGS__)
 #define gpiod_dbg(desc, fmt, ...)					       \
 	pr_debug("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\
 		 ##__VA_ARGS__)
 
 /* With chip prefix */
 
-#define chip_emerg(gc, fmt, ...)					\
-	dev_emerg(&gc->gpiodev->dev, "(%s): " fmt, gc->label, ##__VA_ARGS__)
-#define chip_crit(gc, fmt, ...)					\
-	dev_crit(&gc->gpiodev->dev, "(%s): " fmt, gc->label, ##__VA_ARGS__)
 #define chip_err(gc, fmt, ...)					\
 	dev_err(&gc->gpiodev->dev, "(%s): " fmt, gc->label, ##__VA_ARGS__)
 #define chip_warn(gc, fmt, ...)					\
-- 
2.40.1

[PATCH v3 04/24] gpio: provide and use gpiod_get_label()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We will soon serialize access to the descriptor label using SRCU. The
write-side of the protection will require calling synchronize_srcu()
which must not be called from atomic context. We have two irq helpers:
gpiochip_lock_as_irq() and gpiochip_unlock_as_irq() that set the label
if the GPIO is not requested but is being used as interrupt. They are
called with a spinlock held from the interrupt subsystem.

They must not do it if we are to use SRCU so instead let's move the
special corner case to a dedicated getter.

First: let's implement and use the getter where it's applicable.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 4 ++--
 drivers/gpio/gpiolib.c      | 9 +++++++--
 drivers/gpio/gpiolib.h      | 1 +
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 34d6712fa07c..2c0a0700762d 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2305,8 +2305,8 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 		if (desc->name)
 			strscpy(info->name, desc->name, sizeof(info->name));
 
-		if (desc->label)
-			strscpy(info->consumer, desc->label,
+		if (gpiod_get_label(desc))
+			strscpy(info->consumer, gpiod_get_label(desc),
 				sizeof(info->consumer));
 
 		dflags = READ_ONCE(desc->flags);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8ca9aebfa81f..8331fdbb6faa 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -105,6 +105,11 @@ static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gc);
 
 static bool gpiolib_initialized;
 
+const char *gpiod_get_label(struct gpio_desc *desc)
+{
+	return desc->label;
+}
+
 static inline void desc_set_label(struct gpio_desc *d, const char *label)
 {
 	d->label = label;
@@ -2391,7 +2396,7 @@ char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset)
 	 *
 	 * Until this happens, this allocation needs to be atomic.
 	 */
-	label = kstrdup(desc->label, GFP_ATOMIC);
+	label = kstrdup(gpiod_get_label(desc), GFP_ATOMIC);
 	if (!label)
 		return ERR_PTR(-ENOMEM);
 
@@ -4732,7 +4737,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
 			is_irq = test_bit(FLAG_USED_AS_IRQ, &desc->flags);
 			active_low = test_bit(FLAG_ACTIVE_LOW, &desc->flags);
 			seq_printf(s, " gpio-%-3d (%-20.20s|%-20.20s) %s %s %s%s\n",
-				   gpio, desc->name ?: "", desc->label,
+				   gpio, desc->name ?: "", gpiod_get_label(desc),
 				   is_out ? "out" : "in ",
 				   value >= 0 ? (value ? "hi" : "lo") : "?  ",
 				   is_irq ? "IRQ " : "",
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index c3ae5bfa3f2e..1058f326fe2b 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -208,6 +208,7 @@ int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
 		unsigned long lflags, enum gpiod_flags dflags);
 int gpiochip_get_ngpios(struct gpio_chip *gc, struct device *dev);
+const char *gpiod_get_label(struct gpio_desc *desc);
 
 /*
  * Return the GPIO number of the passed descriptor relative to its chip
-- 
2.40.1

[PATCH v3 05/24] gpio: don't set label from irq helpers

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We will soon serialize access to the descriptor label using SRCU. The
write-side of the protection will require calling synchronize_srcu()
which must not be called from atomic context. We have two irq helpers:
gpiochip_lock_as_irq() and gpiochip_unlock_as_irq() that set the label
if the GPIO is not requested but is being used as interrupt. They are
called with a spinlock held from the interrupt subsystem.

They must not do it if we are to use SRCU so instead let's move the
special corner case to a dedicated getter.

Don't actually set the label to "interrupt" in the above case but rather
use the newly added gpiod_get_label() helper to hide the logic that
atomically checks the descriptor flags and returns the address of a
static "interrupt" string.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8331fdbb6faa..fbfddc9860fd 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -107,7 +107,14 @@ static bool gpiolib_initialized;
 
 const char *gpiod_get_label(struct gpio_desc *desc)
 {
-	return desc->label;
+	unsigned long flags;
+
+	flags = READ_ONCE(desc->flags);
+	if (test_bit(FLAG_USED_AS_IRQ, &flags) &&
+	    !test_bit(FLAG_REQUESTED, &flags))
+		return "interrupt";
+
+	return test_bit(FLAG_REQUESTED, &flags) ? desc->label : NULL;
 }
 
 static inline void desc_set_label(struct gpio_desc *d, const char *label)
@@ -3599,14 +3606,6 @@ int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset)
 	set_bit(FLAG_USED_AS_IRQ, &desc->flags);
 	set_bit(FLAG_IRQ_IS_ENABLED, &desc->flags);
 
-	/*
-	 * If the consumer has not set up a label (such as when the
-	 * IRQ is referenced from .to_irq()) we set up a label here
-	 * so it is clear this is used as an interrupt.
-	 */
-	if (!desc->label)
-		desc_set_label(desc, "interrupt");
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(gpiochip_lock_as_irq);
@@ -3629,10 +3628,6 @@ void gpiochip_unlock_as_irq(struct gpio_chip *gc, unsigned int offset)
 
 	clear_bit(FLAG_USED_AS_IRQ, &desc->flags);
 	clear_bit(FLAG_IRQ_IS_ENABLED, &desc->flags);
-
-	/* If we only had this marking, erase it */
-	if (desc->label && !strcmp(desc->label, "interrupt"))
-		desc_set_label(desc, NULL);
 }
 EXPORT_SYMBOL_GPL(gpiochip_unlock_as_irq);
 
-- 
2.40.1

[PATCH v3 06/24] gpio: add SRCU infrastructure to struct gpio_desc

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Extend the GPIO descriptor with an SRCU structure in order to serialize
the access to the label. Initialize and clean it up where applicable.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 18 ++++++++++++++++--
 drivers/gpio/gpiolib.h |  3 +++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index fbfddc9860fd..b26d39bc3389 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -672,6 +672,10 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
 static void gpiodev_release(struct device *dev)
 {
 	struct gpio_device *gdev = to_gpio_device(dev);
+	unsigned int i;
+
+	for (i = 0; i < gdev->ngpio; i++)
+		cleanup_srcu_struct(&gdev->descs[i].srcu);
 
 	ida_free(&gpio_ida, gdev->id);
 	kfree_const(gdev->label);
@@ -836,7 +840,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 			       struct lock_class_key *request_key)
 {
 	struct gpio_device *gdev;
-	unsigned int i;
+	unsigned int i, j;
 	int base = 0;
 	int ret = 0;
 
@@ -970,6 +974,13 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	for (i = 0; i < gc->ngpio; i++) {
 		struct gpio_desc *desc = &gdev->descs[i];
 
+		ret = init_srcu_struct(&desc->srcu);
+		if (ret) {
+			for (j = 0; j < i; j++)
+				cleanup_srcu_struct(&gdev->descs[j].srcu);
+			goto err_remove_of_chip;
+		}
+
 		if (gc->get_direction && gpiochip_line_is_valid(gc, i)) {
 			assign_bit(FLAG_IS_OUT,
 				   &desc->flags, !gc->get_direction(gc, i));
@@ -981,7 +992,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 
 	ret = gpiochip_add_pin_ranges(gc);
 	if (ret)
-		goto err_remove_of_chip;
+		goto err_cleanup_desc_srcu;
 
 	acpi_gpiochip_add(gc);
 
@@ -1020,6 +1031,9 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	gpiochip_irqchip_free_valid_mask(gc);
 err_remove_acpi_chip:
 	acpi_gpiochip_remove(gc);
+err_cleanup_desc_srcu:
+	for (i = 0; i < gdev->ngpio; i++)
+		cleanup_srcu_struct(&gdev->descs[i].srcu);
 err_remove_of_chip:
 	gpiochip_free_hogs(gc);
 	of_gpiochip_remove(gc);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 1058f326fe2b..6e14b629c48b 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/notifier.h>
 #include <linux/rwsem.h>
+#include <linux/srcu.h>
 
 #define GPIOCHIP_NAME	"gpiochip"
 
@@ -147,6 +148,7 @@ void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);
  * @label:		Name of the consumer
  * @name:		Line name
  * @hog:		Pointer to the device node that hogs this line (if any)
+ * @srcu:		SRCU struct protecting the label pointer.
  *
  * These are obtained using gpiod_get() and are preferable to the old
  * integer-based handles.
@@ -184,6 +186,7 @@ struct gpio_desc {
 #ifdef CONFIG_OF_DYNAMIC
 	struct device_node	*hog;
 #endif
+	struct srcu_struct	srcu;
 };
 
 #define gpiod_not_found(desc)		(IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
-- 
2.40.1

[PATCH v3 07/24] gpio: protect the descriptor label with SRCU

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

In order to ensure that the label is not freed while it's being
accessed, let's protect it with SRCU and synchronize it everytime it's
changed.

Let's modify desc_set_label() to manage the memory used for the label as
it can only be freed once synchronize_srcu() returns.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 10 +++++---
 drivers/gpio/gpiolib.c      | 47 +++++++++++++++++++++++--------------
 drivers/gpio/gpiolib.h      | 34 +++++++++++++++++++--------
 3 files changed, 61 insertions(+), 30 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 2c0a0700762d..75f4912339a6 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2297,6 +2297,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 {
 	struct gpio_chip *gc = desc->gdev->chip;
 	unsigned long dflags;
+	const char *label;
 
 	memset(info, 0, sizeof(*info));
 	info->offset = gpio_chip_hwgpio(desc);
@@ -2305,9 +2306,12 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 		if (desc->name)
 			strscpy(info->name, desc->name, sizeof(info->name));
 
-		if (gpiod_get_label(desc))
-			strscpy(info->consumer, gpiod_get_label(desc),
-				sizeof(info->consumer));
+		scoped_guard(srcu, &desc->srcu) {
+			label = gpiod_get_label(desc);
+			if (label)
+				strscpy(info->consumer, label,
+					sizeof(info->consumer));
+		}
 
 		dflags = READ_ONCE(desc->flags);
 	}
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b26d39bc3389..a87ab8f013c4 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -114,12 +114,26 @@ const char *gpiod_get_label(struct gpio_desc *desc)
 	    !test_bit(FLAG_REQUESTED, &flags))
 		return "interrupt";
 
-	return test_bit(FLAG_REQUESTED, &flags) ? desc->label : NULL;
+	return test_bit(FLAG_REQUESTED, &flags) ?
+			rcu_dereference(desc->label) : NULL;
 }
 
-static inline void desc_set_label(struct gpio_desc *d, const char *label)
+static int desc_set_label(struct gpio_desc *desc, const char *label)
 {
-	d->label = label;
+	const char *new = NULL, *old;
+
+	if (label) {
+		/* FIXME: make this GFP_KERNEL once the spinlock is out. */
+		new = kstrdup_const(label, GFP_ATOMIC);
+		if (!new)
+			return -ENOMEM;
+	}
+
+	old = rcu_replace_pointer(desc->label, new, 1);
+	synchronize_srcu(&desc->srcu);
+	kfree_const(old);
+
+	return 0;
 }
 
 /**
@@ -2229,9 +2243,7 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
 	 */
 
-	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
-		desc_set_label(desc, label ? : "?");
-	} else {
+	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags)) {
 		ret = -EBUSY;
 		goto out_free_unlock;
 	}
@@ -2259,6 +2271,13 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 		spin_lock_irqsave(&gpio_lock, flags);
 	}
 	spin_unlock_irqrestore(&gpio_lock, flags);
+
+	ret = desc_set_label(desc, label ? : "?");
+	if (ret) {
+		clear_bit(FLAG_REQUESTED, &desc->flags);
+		return ret;
+	}
+
 	return 0;
 
 out_free_unlock:
@@ -2343,8 +2362,6 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 			gc->free(gc, gpio_chip_hwgpio(desc));
 			spin_lock_irqsave(&gpio_lock, flags);
 		}
-		kfree_const(desc->label);
-		desc_set_label(desc, NULL);
 		clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
 		clear_bit(FLAG_REQUESTED, &desc->flags);
 		clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
@@ -2362,6 +2379,7 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 	}
 
 	spin_unlock_irqrestore(&gpio_lock, flags);
+	desc_set_label(desc, NULL);
 	gpiod_line_state_notify(desc, GPIOLINE_CHANGED_RELEASED);
 
 	return ret;
@@ -2409,6 +2427,8 @@ char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset)
 	if (!test_bit(FLAG_REQUESTED, &desc->flags))
 		return NULL;
 
+	guard(srcu)(&desc->srcu);
+
 	/*
 	 * FIXME: Once we mark gpiod_direction_input/output() and
 	 * gpiod_get_direction() with might_sleep(), we'll be able to protect
@@ -3520,16 +3540,8 @@ EXPORT_SYMBOL_GPL(gpiod_cansleep);
 int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name)
 {
 	VALIDATE_DESC(desc);
-	if (name) {
-		name = kstrdup_const(name, GFP_KERNEL);
-		if (!name)
-			return -ENOMEM;
-	}
 
-	kfree_const(desc->label);
-	desc_set_label(desc, name);
-
-	return 0;
+	return desc_set_label(desc, name);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_consumer_name);
 
@@ -4739,6 +4751,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
 	int value;
 
 	for_each_gpio_desc(gc, desc) {
+		guard(srcu)(&desc->srcu);
 		if (test_bit(FLAG_REQUESTED, &desc->flags)) {
 			gpiod_get_direction(desc);
 			is_out = test_bit(FLAG_IS_OUT, &desc->flags);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 6e14b629c48b..d2e73eea9e92 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -180,7 +180,7 @@ struct gpio_desc {
 #define FLAG_EVENT_CLOCK_HTE		19 /* GPIO CDEV reports hardware timestamps in events */
 
 	/* Connection label */
-	const char		*label;
+	const char __rcu	*label;
 	/* Name of the GPIO */
 	const char		*name;
 #ifdef CONFIG_OF_DYNAMIC
@@ -223,15 +223,29 @@ static inline int gpio_chip_hwgpio(const struct gpio_desc *desc)
 
 /* With descriptor prefix */
 
-#define gpiod_err(desc, fmt, ...)					       \
-	pr_err("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",  \
-		 ##__VA_ARGS__)
-#define gpiod_warn(desc, fmt, ...)					       \
-	pr_warn("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \
-		 ##__VA_ARGS__)
-#define gpiod_dbg(desc, fmt, ...)					       \
-	pr_debug("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\
-		 ##__VA_ARGS__)
+#define gpiod_err(desc, fmt, ...) \
+do { \
+	scoped_guard(srcu, &desc->srcu) { \
+		pr_err("gpio-%d (%s): " fmt, desc_to_gpio(desc), \
+		       gpiod_get_label(desc) ? : "?", ##__VA_ARGS__); \
+	} \
+} while (0)
+
+#define gpiod_warn(desc, fmt, ...) \
+do { \
+	scoped_guard(srcu, &desc->srcu) { \
+		pr_warn("gpio-%d (%s): " fmt, desc_to_gpio(desc), \
+			gpiod_get_label(desc) ? : "?", ##__VA_ARGS__); \
+	} \
+} while (0)
+
+#define gpiod_dbg(desc, fmt, ...) \
+do { \
+	scoped_guard(srcu, &desc->srcu) { \
+		pr_debug("gpio-%d (%s): " fmt, desc_to_gpio(desc), \
+			 gpiod_get_label(desc) ? : "?", ##__VA_ARGS__); \
+	} \
+} while (0)
 
 /* With chip prefix */
 
-- 
2.40.1

Re: [PATCH v3 07/24] gpio: protect the descriptor label with SRCU

[kernel test robot]

Hello,

kernel test robot noticed "canonical_address#:#[##]" on:

commit: 04cb69cd30bb05c127e2b86b31ee778100439d14 ("[PATCH v3 07/24] gpio: protect the descriptor label with SRCU")
url: https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/gpio-protect-the-list-of-GPIO-devices-with-SRCU/20240208-180822
base: https://git.kernel.org/cgit/linux/kernel/git/brgl/linux.git gpio/for-next
patch link: https://lore.kernel.org/all/20240208095920.8035-8-brgl@bgdev.pl/
patch subject: [PATCH v3 07/24] gpio: protect the descriptor label with SRCU

in testcase: boot

compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G

(please refer to attached dmesg/kmsg for entire log/backtrace)


+----------------------------------------------------------------------------------+------------+------------+
|                                                                                  | 47b87115af | 04cb69cd30 |
+----------------------------------------------------------------------------------+------------+------------+
| canonical_address#:#[##]                                                         | 0          | 9          |
| RIP:check_init_srcu_struct                                                       | 0          | 9          |
| Kernel_panic-not_syncing:Fatal_exception                                         | 0          | 9          |
+----------------------------------------------------------------------------------+------------+------------+


If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202402122228.e607a080-lkp@intel.com



[   53.381777][    T1] gpiochip_find_base_unlocked: found new base at 512
[   53.383799][    T1] general protection fault, probably for non-canonical address 0xdffffc000000002f: 0000 [#1] PREEMPT KASAN PTI
[   53.384902][    T1] KASAN: null-ptr-deref in range [0x0000000000000178-0x000000000000017f]
[   53.384902][    T1] CPU: 0 PID: 1 Comm: swapper Tainted: G                 N 6.8.0-rc1-00035-g04cb69cd30bb #1
[ 53.384902][ T1] RIP: 0010:check_init_srcu_struct (kernel/rcu/srcutree.c:408) 
[ 53.384902][ T1] Code: 53 48 89 fb 80 3c 02 00 0f 85 fe 00 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 38 48 8d bd 78 01 00 00 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 ce 00 00 00 48 8b 85 78 01 00 00 a8 03 75 0b 5b
All code
========
   0:	53                   	push   %rbx
   1:	48 89 fb             	mov    %rdi,%rbx
   4:	80 3c 02 00          	cmpb   $0x0,(%rdx,%rax,1)
   8:	0f 85 fe 00 00 00    	jne    0x10c
   e:	48 b8 00 00 00 00 00 	movabs $0xdffffc0000000000,%rax
  15:	fc ff df 
  18:	48 8b 6b 38          	mov    0x38(%rbx),%rbp
  1c:	48 8d bd 78 01 00 00 	lea    0x178(%rbp),%rdi
  23:	48 89 fa             	mov    %rdi,%rdx
  26:	48 c1 ea 03          	shr    $0x3,%rdx
  2a:*	80 3c 02 00          	cmpb   $0x0,(%rdx,%rax,1)		<-- trapping instruction
  2e:	0f 85 ce 00 00 00    	jne    0x102
  34:	48 8b 85 78 01 00 00 	mov    0x178(%rbp),%rax
  3b:	a8 03                	test   $0x3,%al
  3d:	75 0b                	jne    0x4a
  3f:	5b                   	pop    %rbx

Code starting with the faulting instruction
===========================================
   0:	80 3c 02 00          	cmpb   $0x0,(%rdx,%rax,1)
   4:	0f 85 ce 00 00 00    	jne    0xd8
   a:	48 8b 85 78 01 00 00 	mov    0x178(%rbp),%rax
  11:	a8 03                	test   $0x3,%al
  13:	75 0b                	jne    0x20
  15:	5b                   	pop    %rbx
[   53.384902][    T1] RSP: 0018:ffff888103e67730 EFLAGS: 00010202
[   53.384902][    T1] RAX: dffffc0000000000 RBX: ffff88810a65f8f8 RCX: 0000000000000000
[   53.384902][    T1] RDX: 000000000000002f RSI: ffff888168ad7b40 RDI: 0000000000000178
[   53.384902][    T1] RBP: 0000000000000000 R08: 692d422d656e696c R09: 007475706e692d42
[   53.384902][    T1] R10: ffff888103e67768 R11: ffffffff8190f8d4 R12: ffff88810a65f930
[   53.384902][    T1] R13: 0000000000000000 R14: ffff88810a65f8d8 R15: 0000000000000000
[   53.384902][    T1] FS:  0000000000000000(0000) GS:ffffffff84cd1000(0000) knlGS:0000000000000000
[   53.384902][    T1] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   53.384902][    T1] CR2: 00007fb8169d96f4 CR3: 0000000004cac000 CR4: 00000000000406b0
[   53.384902][    T1] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   53.384902][    T1] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   53.384902][    T1] Call Trace:
[   53.384902][    T1]  <TASK>
[ 53.384902][ T1] ? die_addr (arch/x86/kernel/dumpstack.c:421 arch/x86/kernel/dumpstack.c:460) 
[ 53.384902][ T1] ? exc_general_protection (arch/x86/kernel/traps.c:701 arch/x86/kernel/traps.c:643) 
[ 53.384902][ T1] ? asm_exc_general_protection (arch/x86/include/asm/idtentry.h:564) 
[ 53.384902][ T1] ? kasan_save_track (arch/x86/include/asm/current.h:42 mm/kasan/common.c:60 mm/kasan/common.c:70) 
[ 53.384902][ T1] ? check_init_srcu_struct (kernel/rcu/srcutree.c:408) 
[ 53.384902][ T1] synchronize_srcu (kernel/rcu/srcutree.c:1167 kernel/rcu/srcutree.c:1458) 
[ 53.384902][ T1] ? kstrdup (mm/util.c:62) 
[ 53.384902][ T1] gpiod_request_commit (drivers/gpio/gpiolib.c:134 drivers/gpio/gpiolib.c:2275) 
[ 53.384902][ T1] gpiochip_request_own_desc (drivers/gpio/gpiolib.c:2484) 
[ 53.384902][ T1] gpiod_hog (drivers/gpio/gpiolib.c:4474) 
[ 53.384902][ T1] ? of_find_property (drivers/of/base.c:223) 
[ 53.384902][ T1] of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:799) 
[ 53.384902][ T1] ? __pfx_of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:785) 
[ 53.384902][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360) 
[ 53.384902][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423) 
[ 53.384902][ T1] ? _raw_spin_unlock_irqrestore (arch/x86/include/asm/preempt.h:103 include/linux/spinlock_api_smp.h:152 kernel/locking/spinlock.c:194) 
[ 53.384902][ T1] of_gpiochip_add (drivers/gpio/gpiolib-of.c:828 drivers/gpio/gpiolib-of.c:1143) 
[ 53.384902][ T1] ? fwnode_property_read_int_array (drivers/base/property.c:268 (discriminator 5)) 
[ 53.384902][ T1] gpiochip_add_data_with_key (drivers/gpio/gpiolib.c:985) 
[ 53.384902][ T1] ? kasan_save_track (arch/x86/include/asm/current.h:42 mm/kasan/common.c:60 mm/kasan/common.c:70) 
[ 53.384902][ T1] unittest_gpio_probe (drivers/of/unittest.c:1886) 
[ 53.384902][ T1] platform_probe (drivers/base/platform.c:1404) 
[ 53.384902][ T1] really_probe (drivers/base/dd.c:579 drivers/base/dd.c:658) 
[ 53.384902][ T1] __driver_probe_device (drivers/base/dd.c:800) 
[ 53.384902][ T1] driver_probe_device (drivers/base/dd.c:830) 
[ 53.384902][ T1] __driver_attach (drivers/base/dd.c:1217) 
[ 53.384902][ T1] ? __pfx___driver_attach (drivers/base/dd.c:1157) 
[ 53.384902][ T1] bus_for_each_dev (drivers/base/bus.c:367) 
[ 53.384902][ T1] ? lockdep_init_map_type (kernel/locking/lockdep.c:4892) 
[ 53.384902][ T1] ? __pfx_bus_for_each_dev (drivers/base/bus.c:356) 
[ 53.384902][ T1] ? bus_add_driver (drivers/base/bus.c:672) 
[ 53.384902][ T1] bus_add_driver (drivers/base/bus.c:674) 
[ 53.384902][ T1] driver_register (drivers/base/driver.c:246) 
[ 53.384902][ T1] of_unittest_overlay_gpio (drivers/of/unittest.c:1969 (discriminator 4)) 
[ 53.384902][ T1] of_unittest_overlay (drivers/of/unittest.c:2189 drivers/of/unittest.c:3217) 
[ 53.384902][ T1] ? __pfx_of_unittest_overlay (drivers/of/unittest.c:3155) 
[ 53.384902][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360) 
[ 53.384902][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423) 
[ 53.384902][ T1] of_unittest (drivers/of/unittest.c:4129) 
[ 53.384902][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080) 
[ 53.384902][ T1] ? add_device_randomness (drivers/char/random.c:918) 
[ 53.384902][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080) 
[ 53.384902][ T1] do_one_initcall (init/main.c:1236) 
[ 53.384902][ T1] ? __pfx_do_one_initcall (init/main.c:1227) 
[ 53.384902][ T1] do_initcalls (init/main.c:1297 init/main.c:1314) 
[ 53.384902][ T1] kernel_init_freeable (init/main.c:1555) 
[ 53.384902][ T1] ? __pfx_kernel_init (init/main.c:1433) 
[ 53.384902][ T1] kernel_init (init/main.c:1443) 
[ 53.384902][ T1] ? _raw_spin_unlock_irq (arch/x86/include/asm/preempt.h:103 include/linux/spinlock_api_smp.h:160 kernel/locking/spinlock.c:202) 
[ 53.384902][ T1] ret_from_fork (arch/x86/kernel/process.c:153) 
[ 53.384902][ T1] ? __pfx_kernel_init (init/main.c:1433) 
[ 53.384902][ T1] ret_from_fork_asm (arch/x86/entry/entry_64.S:250) 
[   53.384902][    T1]  </TASK>
[   53.384902][    T1] Modules linked in:
[   53.485664][    T1] ---[ end trace 0000000000000000 ]---
[ 53.486974][ T1] RIP: 0010:check_init_srcu_struct (kernel/rcu/srcutree.c:408) 
[ 53.488507][ T1] Code: 53 48 89 fb 80 3c 02 00 0f 85 fe 00 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 38 48 8d bd 78 01 00 00 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 ce 00 00 00 48 8b 85 78 01 00 00 a8 03 75 0b 5b
All code
========
   0:	53                   	push   %rbx
   1:	48 89 fb             	mov    %rdi,%rbx
   4:	80 3c 02 00          	cmpb   $0x0,(%rdx,%rax,1)
   8:	0f 85 fe 00 00 00    	jne    0x10c
   e:	48 b8 00 00 00 00 00 	movabs $0xdffffc0000000000,%rax
  15:	fc ff df 
  18:	48 8b 6b 38          	mov    0x38(%rbx),%rbp
  1c:	48 8d bd 78 01 00 00 	lea    0x178(%rbp),%rdi
  23:	48 89 fa             	mov    %rdi,%rdx
  26:	48 c1 ea 03          	shr    $0x3,%rdx
  2a:*	80 3c 02 00          	cmpb   $0x0,(%rdx,%rax,1)		<-- trapping instruction
  2e:	0f 85 ce 00 00 00    	jne    0x102
  34:	48 8b 85 78 01 00 00 	mov    0x178(%rbp),%rax
  3b:	a8 03                	test   $0x3,%al
  3d:	75 0b                	jne    0x4a
  3f:	5b                   	pop    %rbx

Code starting with the faulting instruction
===========================================
   0:	80 3c 02 00          	cmpb   $0x0,(%rdx,%rax,1)
   4:	0f 85 ce 00 00 00    	jne    0xd8
   a:	48 8b 85 78 01 00 00 	mov    0x178(%rbp),%rax
  11:	a8 03                	test   $0x3,%al
  13:	75 0b                	jne    0x20
  15:	5b                   	pop    %rbx


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240212/202402122228.e607a080-lkp@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 07/24] gpio: protect the descriptor label with SRCU

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4326 bytes --]

On Thu, Feb 08, 2024 at 10:59:03AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> In order to ensure that the label is not freed while it's being
> accessed, let's protect it with SRCU and synchronize it everytime it's
> changed.

This patch, which is now in -next as 1f2bcb8c8ccd, appears to cause a
boot regression on imx8mp-verdin-nonwifi-dahlia with arm64 defconfig.
We die with an invalid pointer dereference after registering the GPIOs:

[    1.973513] gpio gpiochip3: Static allocation of GPIO base is deprecated, use dynamic allocation.
[    1.982467] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000078

...

[    2.161467] Call trace:
[    2.163915]  check_init_srcu_struct+0x1c/0xa0
[    2.168284]  synchronize_srcu+0x1c/0x100
[    2.172216]  gpiod_request_commit+0xec/0x1e0
[    2.176496]  gpiochip_request_own_desc+0x58/0x124
[    2.181205]  gpiod_hog+0x74/0x140
[    2.184529]  of_gpiochip_add+0x208/0x370
[    2.188456]  gpiochip_add_data_with_key+0x720/0xf14

and a bisect appears to converge smoothly onto this commit.  None of my
other platforms (including the i.MX8MP EVK with the same SoC in it) are
showing similar issues, I've not checked the CI systems and haven't done
any investigation beyond checking that the commit does look like it
could plausibly be related to the symptom.

You can see a full boot log at:

   https://lava.sirena.org.uk/scheduler/job/579038

bisect log:

git bisect start
# good: [7b17b1384cd6454c4ea2744c8e8a06de0d27b5b3] Merge branch 'for-linux-next-fixes' of git://anongit.freedesktop.org/drm/drm-misc
git bisect good 7b17b1384cd6454c4ea2744c8e8a06de0d27b5b3
# bad: [46d4e2eb58e14c8935fa0e27d16d4c62ef82849a] Add linux-next specific files for 20240213
git bisect bad 46d4e2eb58e14c8935fa0e27d16d4c62ef82849a
# good: [f85363faaa040a9b9ac6502464a8b1ed7f711eab] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git
git bisect good f85363faaa040a9b9ac6502464a8b1ed7f711eab
# good: [0ca88723ff14aa0a28d31772ef330f3eef97cba1] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git
git bisect good 0ca88723ff14aa0a28d31772ef330f3eef97cba1
# good: [c9545b54561efbedfe184a97dd07b4cdd8176146] Merge branch 'usb-next' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
git bisect good c9545b54561efbedfe184a97dd07b4cdd8176146
# good: [4e22a2de97fb3b37e241058a4f9b91f3245590ea] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git
git bisect good 4e22a2de97fb3b37e241058a4f9b91f3245590ea
# bad: [903a65bcdcda676e86b1504f909c6565b1bd9df2] Merge branch 'pwm/for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git
git bisect bad 903a65bcdcda676e86b1504f909c6565b1bd9df2
# good: [a3468cca30fe896b58f9f7b3bb5484f079010a12] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git
git bisect good a3468cca30fe896b58f9f7b3bb5484f079010a12
# bad: [7fe595b3c3cf3f9b8f21fce72f1f48a2cb41522e] gpio: don't dereference gdev->chip in gpiochip_setup_dev()
git bisect bad 7fe595b3c3cf3f9b8f21fce72f1f48a2cb41522e
# good: [f57595788244a838deec2d3be375291327cbc035] gpio: vf610: allow disabling the vf610 driver
git bisect good f57595788244a838deec2d3be375291327cbc035
# good: [ccfb6ff4f6c0574e01fb16934fb60a46285c5f3f] gpio: don't set label from irq helpers
git bisect good ccfb6ff4f6c0574e01fb16934fb60a46285c5f3f
# bad: [b6f87adbacfab9001d08e56ac869e1c75734633d] gpio: remove unneeded code from gpio_device_get_desc()
git bisect bad b6f87adbacfab9001d08e56ac869e1c75734633d
# bad: [2a9101e875bc3aa6423b559e0ea43b2077f3be87] gpio: sysfs: use gpio_device_find() to iterate over existing devices
git bisect bad 2a9101e875bc3aa6423b559e0ea43b2077f3be87
# bad: [1f2bcb8c8ccdf9dc2e46f7986e1e22408506a6d6] gpio: protect the descriptor label with SRCU
git bisect bad 1f2bcb8c8ccdf9dc2e46f7986e1e22408506a6d6
# good: [be711caa87c5c81d5dc00b244cac3a0b775adb18] gpio: add SRCU infrastructure to struct gpio_desc
git bisect good be711caa87c5c81d5dc00b244cac3a0b775adb18
# first bad commit: [1f2bcb8c8ccdf9dc2e46f7986e1e22408506a6d6] gpio: protect the descriptor label with SRCU

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 07/24] gpio: protect the descriptor label with SRCU

[Bartosz Golaszewski]

On Tue, Feb 13, 2024 at 10:16 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Feb 08, 2024 at 10:59:03AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > In order to ensure that the label is not freed while it's being
> > accessed, let's protect it with SRCU and synchronize it everytime it's
> > changed.
>
> This patch, which is now in -next as 1f2bcb8c8ccd, appears to cause a
> boot regression on imx8mp-verdin-nonwifi-dahlia with arm64 defconfig.
> We die with an invalid pointer dereference after registering the GPIOs:
>
> [    1.973513] gpio gpiochip3: Static allocation of GPIO base is deprecated, use dynamic allocation.
> [    1.982467] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000078
>
> ...
>
> [    2.161467] Call trace:
> [    2.163915]  check_init_srcu_struct+0x1c/0xa0
> [    2.168284]  synchronize_srcu+0x1c/0x100
> [    2.172216]  gpiod_request_commit+0xec/0x1e0
> [    2.176496]  gpiochip_request_own_desc+0x58/0x124
> [    2.181205]  gpiod_hog+0x74/0x140
> [    2.184529]  of_gpiochip_add+0x208/0x370
> [    2.188456]  gpiochip_add_data_with_key+0x720/0xf14
>
> and a bisect appears to converge smoothly onto this commit.  None of my
> other platforms (including the i.MX8MP EVK with the same SoC in it) are
> showing similar issues, I've not checked the CI systems and haven't done
> any investigation beyond checking that the commit does look like it
> could plausibly be related to the symptom.
>
> You can see a full boot log at:
>
>    https://lava.sirena.org.uk/scheduler/job/579038
>
> bisect log:
>
> git bisect start
> # good: [7b17b1384cd6454c4ea2744c8e8a06de0d27b5b3] Merge branch 'for-linux-next-fixes' of git://anongit.freedesktop.org/drm/drm-misc
> git bisect good 7b17b1384cd6454c4ea2744c8e8a06de0d27b5b3
> # bad: [46d4e2eb58e14c8935fa0e27d16d4c62ef82849a] Add linux-next specific files for 20240213
> git bisect bad 46d4e2eb58e14c8935fa0e27d16d4c62ef82849a
> # good: [f85363faaa040a9b9ac6502464a8b1ed7f711eab] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git
> git bisect good f85363faaa040a9b9ac6502464a8b1ed7f711eab
> # good: [0ca88723ff14aa0a28d31772ef330f3eef97cba1] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git
> git bisect good 0ca88723ff14aa0a28d31772ef330f3eef97cba1
> # good: [c9545b54561efbedfe184a97dd07b4cdd8176146] Merge branch 'usb-next' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> git bisect good c9545b54561efbedfe184a97dd07b4cdd8176146
> # good: [4e22a2de97fb3b37e241058a4f9b91f3245590ea] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git
> git bisect good 4e22a2de97fb3b37e241058a4f9b91f3245590ea
> # bad: [903a65bcdcda676e86b1504f909c6565b1bd9df2] Merge branch 'pwm/for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git
> git bisect bad 903a65bcdcda676e86b1504f909c6565b1bd9df2
> # good: [a3468cca30fe896b58f9f7b3bb5484f079010a12] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git
> git bisect good a3468cca30fe896b58f9f7b3bb5484f079010a12
> # bad: [7fe595b3c3cf3f9b8f21fce72f1f48a2cb41522e] gpio: don't dereference gdev->chip in gpiochip_setup_dev()
> git bisect bad 7fe595b3c3cf3f9b8f21fce72f1f48a2cb41522e
> # good: [f57595788244a838deec2d3be375291327cbc035] gpio: vf610: allow disabling the vf610 driver
> git bisect good f57595788244a838deec2d3be375291327cbc035
> # good: [ccfb6ff4f6c0574e01fb16934fb60a46285c5f3f] gpio: don't set label from irq helpers
> git bisect good ccfb6ff4f6c0574e01fb16934fb60a46285c5f3f
> # bad: [b6f87adbacfab9001d08e56ac869e1c75734633d] gpio: remove unneeded code from gpio_device_get_desc()
> git bisect bad b6f87adbacfab9001d08e56ac869e1c75734633d
> # bad: [2a9101e875bc3aa6423b559e0ea43b2077f3be87] gpio: sysfs: use gpio_device_find() to iterate over existing devices
> git bisect bad 2a9101e875bc3aa6423b559e0ea43b2077f3be87
> # bad: [1f2bcb8c8ccdf9dc2e46f7986e1e22408506a6d6] gpio: protect the descriptor label with SRCU
> git bisect bad 1f2bcb8c8ccdf9dc2e46f7986e1e22408506a6d6
> # good: [be711caa87c5c81d5dc00b244cac3a0b775adb18] gpio: add SRCU infrastructure to struct gpio_desc
> git bisect good be711caa87c5c81d5dc00b244cac3a0b775adb18
> # first bad commit: [1f2bcb8c8ccdf9dc2e46f7986e1e22408506a6d6] gpio: protect the descriptor label with SRCU

Hi Mark,

Thanks for the report. Patch fixing this crash is already on the list
as of this morning. I'll queue it tomorrow and next should be fixed on
Thursday.

Bartosz

[PATCH v3 08/24] gpio: sysfs: use gpio_device_find() to iterate over existing devices

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

With the list of GPIO devices now protected with SRCU we can use
gpio_device_find() to traverse it from sysfs.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-sysfs.c | 46 ++++++++++++++++++------------------
 drivers/gpio/gpiolib.c       |  2 +-
 drivers/gpio/gpiolib.h       |  1 -
 3 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 6bf5332136e5..d6ff044ce887 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -790,11 +790,29 @@ void gpiochip_sysfs_unregister(struct gpio_device *gdev)
 	}
 }
 
+/*
+ * We're not really looking for a device - we just want to iterate over the
+ * list and call this callback for each GPIO device. This is why this function
+ * always returns 0.
+ */
+static int gpiofind_sysfs_register(struct gpio_chip *gc, void *data)
+{
+	struct gpio_device *gdev = gc->gpiodev;
+	int ret;
+
+	if (gdev->mockdev)
+		return 0;
+
+	ret = gpiochip_sysfs_register(gdev);
+	if (ret)
+		chip_err(gc, "failed to register the sysfs entry: %d\n", ret);
+
+	return 0;
+}
+
 static int __init gpiolib_sysfs_init(void)
 {
-	int		status;
-	unsigned long	flags;
-	struct gpio_device *gdev;
+	int status;
 
 	status = class_register(&gpio_class);
 	if (status < 0)
@@ -806,26 +824,8 @@ static int __init gpiolib_sysfs_init(void)
 	 * We run before arch_initcall() so chip->dev nodes can have
 	 * registered, and so arch_initcall() can always gpiod_export().
 	 */
-	spin_lock_irqsave(&gpio_lock, flags);
-	list_for_each_entry(gdev, &gpio_devices, list) {
-		if (gdev->mockdev)
-			continue;
+	(void)gpio_device_find(NULL, gpiofind_sysfs_register);
 
-		/*
-		 * TODO we yield gpio_lock here because
-		 * gpiochip_sysfs_register() acquires a mutex. This is unsafe
-		 * and needs to be fixed.
-		 *
-		 * Also it would be nice to use gpio_device_find() here so we
-		 * can keep gpio_chips local to gpiolib.c, but the yield of
-		 * gpio_lock prevents us from doing this.
-		 */
-		spin_unlock_irqrestore(&gpio_lock, flags);
-		status = gpiochip_sysfs_register(gdev);
-		spin_lock_irqsave(&gpio_lock, flags);
-	}
-	spin_unlock_irqrestore(&gpio_lock, flags);
-
-	return status;
+	return 0;
 }
 postcore_initcall(gpiolib_sysfs_init);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a87ab8f013c4..68543ab34fb6 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -85,7 +85,7 @@ DEFINE_SPINLOCK(gpio_lock);
 static DEFINE_MUTEX(gpio_lookup_lock);
 static LIST_HEAD(gpio_lookup_list);
 
-LIST_HEAD(gpio_devices);
+static LIST_HEAD(gpio_devices);
 /* Protects the GPIO device list against concurrent modifications. */
 static DEFINE_MUTEX(gpio_devices_lock);
 /* Ensures coherence during read-only accesses to the list of GPIO devices. */
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index d2e73eea9e92..2bf3f9e13ae4 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -136,7 +136,6 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
 
 extern spinlock_t gpio_lock;
-extern struct list_head gpio_devices;
 
 void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);
 
-- 
2.40.1

[PATCH v3 09/24] gpio: remove gpio_lock

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The "multi-function" gpio_lock is pretty much useless with how it's used
in GPIOLIB currently. Because many GPIO API calls can be called from all
contexts but may also call into sleeping driver callbacks, there are
many places with utterly broken workarounds like yielding the lock to
call a possibly sleeping function and then re-acquiring it again without
taking into account that the protected state may have changed.

It was also used to protect several unrelated things: like individual
descriptors AND the GPIO device list. We now serialize access to these
two with SRCU and so can finally remove the spinlock.

There is of course the question of consistency of lockless access to
GPIO descriptors. Because we only support exclusive access to GPIOs
(officially anyway, I'm looking at you broken
GPIOD_FLAGS_BIT_NONEXCLUSIVE bit...) and the API contract with providers
does not guarantee serialization, it's enough to ensure we cannot
accidentally dereference an invalid pointer and that the state we present
to both users and providers remains consistent. To achieve that: read the
flags field atomically except for a few special cases. Read their current
value before executing callback code and use this value for any subsequent
logic. Modifying the flags depends on the particular use-case and can
differ. For instance: when requesting a GPIO, we need to set the
REQUESTED bit immediately so that the next user trying to request the
same line sees -EBUSY.

While at it: the allocations that used GFP_ATOMIC until this point can
now switch to GFP_KERNEL.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c  |  18 +++---
 drivers/gpio/gpiolib-sysfs.c |  17 ++----
 drivers/gpio/gpiolib.c       | 106 +++++++++++------------------------
 drivers/gpio/gpiolib.h       |   2 -
 4 files changed, 46 insertions(+), 97 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 75f4912339a6..3588aaf90e45 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2302,18 +2302,16 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	memset(info, 0, sizeof(*info));
 	info->offset = gpio_chip_hwgpio(desc);
 
-	scoped_guard(spinlock_irqsave, &gpio_lock) {
-		if (desc->name)
-			strscpy(info->name, desc->name, sizeof(info->name));
+	if (desc->name)
+		strscpy(info->name, desc->name, sizeof(info->name));
 
-		scoped_guard(srcu, &desc->srcu) {
-			label = gpiod_get_label(desc);
-			if (label)
-				strscpy(info->consumer, label,
-					sizeof(info->consumer));
-		}
+	dflags = READ_ONCE(desc->flags);
 
-		dflags = READ_ONCE(desc->flags);
+	scoped_guard(srcu, &desc->srcu) {
+		label = gpiod_get_label(desc);
+		if (label && test_bit(FLAG_REQUESTED, &dflags))
+			strscpy(info->consumer, label,
+				sizeof(info->consumer));
 	}
 
 	/*
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index d6ff044ce887..3fe740b7d84d 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -563,7 +563,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	struct gpio_device *gdev;
 	struct gpiod_data *data;
 	struct gpio_chip *chip;
-	unsigned long flags;
 	struct device *dev;
 	int status, offset;
 
@@ -578,6 +577,9 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 		return -EINVAL;
 	}
 
+	if (!test_and_set_bit(FLAG_EXPORT, &desc->flags))
+		return -EPERM;
+
 	gdev = desc->gdev;
 	chip = gdev->chip;
 
@@ -589,18 +591,11 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 		goto err_unlock;
 	}
 
-	spin_lock_irqsave(&gpio_lock, flags);
-	if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
-	     test_bit(FLAG_EXPORT, &desc->flags)) {
-		spin_unlock_irqrestore(&gpio_lock, flags);
-		gpiod_dbg(desc, "%s: unavailable (requested=%d, exported=%d)\n",
-				__func__,
-				test_bit(FLAG_REQUESTED, &desc->flags),
-				test_bit(FLAG_EXPORT, &desc->flags));
+	if (!test_bit(FLAG_REQUESTED, &desc->flags)) {
+		gpiod_dbg(desc, "%s: unavailable (not requested)\n", __func__);
 		status = -EPERM;
 		goto err_unlock;
 	}
-	spin_unlock_irqrestore(&gpio_lock, flags);
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data) {
@@ -628,7 +623,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 		goto err_free_data;
 	}
 
-	set_bit(FLAG_EXPORT, &desc->flags);
 	mutex_unlock(&sysfs_lock);
 	return 0;
 
@@ -636,6 +630,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	kfree(data);
 err_unlock:
 	mutex_unlock(&sysfs_lock);
+	clear_bit(FLAG_EXPORT, &desc->flags);
 	gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 	return status;
 }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 68543ab34fb6..d0de492acd4c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -76,12 +76,6 @@ static const struct bus_type gpio_bus_type = {
  */
 #define FASTPATH_NGPIO CONFIG_GPIOLIB_FASTPATH_LIMIT
 
-/* gpio_lock prevents conflicts during gpio_desc[] table updates.
- * While any GPIO is requested, its gpio_chip is not removable;
- * each GPIO's "requested" flag serves as a lock and refcount.
- */
-DEFINE_SPINLOCK(gpio_lock);
-
 static DEFINE_MUTEX(gpio_lookup_lock);
 static LIST_HEAD(gpio_lookup_list);
 
@@ -123,8 +117,7 @@ static int desc_set_label(struct gpio_desc *desc, const char *label)
 	const char *new = NULL, *old;
 
 	if (label) {
-		/* FIXME: make this GFP_KERNEL once the spinlock is out. */
-		new = kstrdup_const(label, GFP_ATOMIC);
+		new = kstrdup_const(label, GFP_KERNEL);
 		if (!new)
 			return -ENOMEM;
 	}
@@ -1093,7 +1086,6 @@ EXPORT_SYMBOL_GPL(gpiochip_add_data_with_key);
 void gpiochip_remove(struct gpio_chip *gc)
 {
 	struct gpio_device *gdev = gc->gpiodev;
-	unsigned long flags;
 	unsigned int i;
 
 	down_write(&gdev->sem);
@@ -1119,12 +1111,10 @@ void gpiochip_remove(struct gpio_chip *gc)
 	 */
 	gpiochip_set_data(gc, NULL);
 
-	spin_lock_irqsave(&gpio_lock, flags);
 	for (i = 0; i < gdev->ngpio; i++) {
 		if (test_bit(FLAG_REQUESTED, &gdev->descs[i].flags))
 			break;
 	}
-	spin_unlock_irqrestore(&gpio_lock, flags);
 
 	if (i != gdev->ngpio)
 		dev_crit(&gdev->dev,
@@ -2227,62 +2217,43 @@ EXPORT_SYMBOL_GPL(gpiochip_remove_pin_ranges);
 static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 {
 	struct gpio_chip *gc = desc->gdev->chip;
-	unsigned long flags;
 	unsigned int offset;
 	int ret;
 
+	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags))
+		return -EBUSY;
+
 	if (label) {
 		label = kstrdup_const(label, GFP_KERNEL);
 		if (!label)
 			return -ENOMEM;
 	}
 
-	spin_lock_irqsave(&gpio_lock, flags);
-
 	/* NOTE:  gpio_request() can be called in early boot,
 	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
 	 */
 
-	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags)) {
-		ret = -EBUSY;
-		goto out_free_unlock;
-	}
-
 	if (gc->request) {
-		/* gc->request may sleep */
-		spin_unlock_irqrestore(&gpio_lock, flags);
 		offset = gpio_chip_hwgpio(desc);
 		if (gpiochip_line_is_valid(gc, offset))
 			ret = gc->request(gc, offset);
 		else
 			ret = -EINVAL;
-		spin_lock_irqsave(&gpio_lock, flags);
+		if (ret)
+			goto out_clear_bit;
+	}
 
-		if (ret) {
-			desc_set_label(desc, NULL);
-			clear_bit(FLAG_REQUESTED, &desc->flags);
-			goto out_free_unlock;
-		}
-	}
-	if (gc->get_direction) {
-		/* gc->get_direction may sleep */
-		spin_unlock_irqrestore(&gpio_lock, flags);
+	if (gc->get_direction)
 		gpiod_get_direction(desc);
-		spin_lock_irqsave(&gpio_lock, flags);
-	}
-	spin_unlock_irqrestore(&gpio_lock, flags);
 
 	ret = desc_set_label(desc, label ? : "?");
-	if (ret) {
-		clear_bit(FLAG_REQUESTED, &desc->flags);
-		return ret;
-	}
+	if (ret)
+		goto out_clear_bit;
 
 	return 0;
 
-out_free_unlock:
-	spin_unlock_irqrestore(&gpio_lock, flags);
-	kfree_const(label);
+out_clear_bit:
+	clear_bit(FLAG_REQUESTED, &desc->flags);
 	return ret;
 }
 
@@ -2352,35 +2323,32 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 
 	might_sleep();
 
-	spin_lock_irqsave(&gpio_lock, flags);
-
 	gc = desc->gdev->chip;
-	if (gc && test_bit(FLAG_REQUESTED, &desc->flags)) {
-		if (gc->free) {
-			spin_unlock_irqrestore(&gpio_lock, flags);
-			might_sleep_if(gc->can_sleep);
+	flags = READ_ONCE(desc->flags);
+
+	if (gc && test_bit(FLAG_REQUESTED, &flags)) {
+		if (gc->free)
 			gc->free(gc, gpio_chip_hwgpio(desc));
-			spin_lock_irqsave(&gpio_lock, flags);
-		}
-		clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
-		clear_bit(FLAG_REQUESTED, &desc->flags);
-		clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
-		clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
-		clear_bit(FLAG_PULL_UP, &desc->flags);
-		clear_bit(FLAG_PULL_DOWN, &desc->flags);
-		clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
-		clear_bit(FLAG_EDGE_RISING, &desc->flags);
-		clear_bit(FLAG_EDGE_FALLING, &desc->flags);
-		clear_bit(FLAG_IS_HOGGED, &desc->flags);
+
+		clear_bit(FLAG_ACTIVE_LOW, &flags);
+		clear_bit(FLAG_REQUESTED, &flags);
+		clear_bit(FLAG_OPEN_DRAIN, &flags);
+		clear_bit(FLAG_OPEN_SOURCE, &flags);
+		clear_bit(FLAG_PULL_UP, &flags);
+		clear_bit(FLAG_PULL_DOWN, &flags);
+		clear_bit(FLAG_BIAS_DISABLE, &flags);
+		clear_bit(FLAG_EDGE_RISING, &flags);
+		clear_bit(FLAG_EDGE_FALLING, &flags);
+		clear_bit(FLAG_IS_HOGGED, &flags);
 #ifdef CONFIG_OF_DYNAMIC
 		WRITE_ONCE(desc->hog, NULL);
 #endif
 		ret = true;
-	}
+		desc_set_label(desc, NULL);
+		WRITE_ONCE(desc->flags, flags);
 
-	spin_unlock_irqrestore(&gpio_lock, flags);
-	desc_set_label(desc, NULL);
-	gpiod_line_state_notify(desc, GPIOLINE_CHANGED_RELEASED);
+		gpiod_line_state_notify(desc, GPIOLINE_CHANGED_RELEASED);
+	}
 
 	return ret;
 }
@@ -2422,22 +2390,12 @@ char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset)
 	if (IS_ERR(desc))
 		return NULL;
 
-	guard(spinlock_irqsave)(&gpio_lock);
-
 	if (!test_bit(FLAG_REQUESTED, &desc->flags))
 		return NULL;
 
 	guard(srcu)(&desc->srcu);
 
-	/*
-	 * FIXME: Once we mark gpiod_direction_input/output() and
-	 * gpiod_get_direction() with might_sleep(), we'll be able to protect
-	 * the GPIO descriptors with mutex (while value setting operations will
-	 * become lockless).
-	 *
-	 * Until this happens, this allocation needs to be atomic.
-	 */
-	label = kstrdup(gpiod_get_label(desc), GFP_ATOMIC);
+	label = kstrdup(gpiod_get_label(desc), GFP_KERNEL);
 	if (!label)
 		return ERR_PTR(-ENOMEM);
 
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 2bf3f9e13ae4..9b7afe87f1bd 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -135,8 +135,6 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 
 int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
 
-extern spinlock_t gpio_lock;
-
 void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);
 
 /**
-- 
2.40.1

[PATCH v3 10/24] gpio: reinforce desc->flags handling

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We now removed the gpio_lock spinlock and modified the places
previously protected by it to handle desc->flags access in a consistent
way. Let's improve other places that were previously unprotected by
reading the flags field of gpio_desc once and using the stored value for
logic consistency. If we need to modify the field, let's also write it
back once with a consistent value resulting from the function's logic.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d0de492acd4c..5e098a7816bd 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -336,18 +336,20 @@ static int gpiochip_find_base_unlocked(int ngpio)
 int gpiod_get_direction(struct gpio_desc *desc)
 {
 	struct gpio_chip *gc;
+	unsigned long flags;
 	unsigned int offset;
 	int ret;
 
 	gc = gpiod_to_chip(desc);
 	offset = gpio_chip_hwgpio(desc);
+	flags = READ_ONCE(desc->flags);
 
 	/*
 	 * Open drain emulation using input mode may incorrectly report
 	 * input here, fix that up.
 	 */
-	if (test_bit(FLAG_OPEN_DRAIN, &desc->flags) &&
-	    test_bit(FLAG_IS_OUT, &desc->flags))
+	if (test_bit(FLAG_OPEN_DRAIN, &flags) &&
+	    test_bit(FLAG_IS_OUT, &flags))
 		return 0;
 
 	if (!gc->get_direction)
@@ -361,7 +363,8 @@ int gpiod_get_direction(struct gpio_desc *desc)
 	if (ret > 0)
 		ret = 1;
 
-	assign_bit(FLAG_IS_OUT, &desc->flags, !ret);
+	assign_bit(FLAG_IS_OUT, &flags, !ret);
+	WRITE_ONCE(desc->flags, flags);
 
 	return ret;
 }
@@ -751,9 +754,6 @@ static void gpiochip_machine_hog(struct gpio_chip *gc, struct gpiod_hog *hog)
 		return;
 	}
 
-	if (test_bit(FLAG_IS_HOGGED, &desc->flags))
-		return;
-
 	rv = gpiod_hog(desc, hog->line_name, hog->lflags, hog->dflags);
 	if (rv)
 		gpiod_err(desc, "%s: unable to hog GPIO line (%s:%u): %d\n",
@@ -2528,13 +2528,16 @@ static int gpio_set_config(struct gpio_desc *desc, enum pin_config_param mode)
 static int gpio_set_bias(struct gpio_desc *desc)
 {
 	enum pin_config_param bias;
+	unsigned long flags;
 	unsigned int arg;
 
-	if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
+	flags = READ_ONCE(desc->flags);
+
+	if (test_bit(FLAG_BIAS_DISABLE, &flags))
 		bias = PIN_CONFIG_BIAS_DISABLE;
-	else if (test_bit(FLAG_PULL_UP, &desc->flags))
+	else if (test_bit(FLAG_PULL_UP, &flags))
 		bias = PIN_CONFIG_BIAS_PULL_UP;
-	else if (test_bit(FLAG_PULL_DOWN, &desc->flags))
+	else if (test_bit(FLAG_PULL_DOWN, &flags))
 		bias = PIN_CONFIG_BIAS_PULL_DOWN;
 	else
 		return 0;
@@ -2700,24 +2703,28 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
  */
 int gpiod_direction_output(struct gpio_desc *desc, int value)
 {
+	unsigned long flags;
 	int ret;
 
 	VALIDATE_DESC(desc);
-	if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
+
+	flags = READ_ONCE(desc->flags);
+
+	if (test_bit(FLAG_ACTIVE_LOW, &flags))
 		value = !value;
 	else
 		value = !!value;
 
 	/* GPIOs used for enabled IRQs shall not be set as output */
-	if (test_bit(FLAG_USED_AS_IRQ, &desc->flags) &&
-	    test_bit(FLAG_IRQ_IS_ENABLED, &desc->flags)) {
+	if (test_bit(FLAG_USED_AS_IRQ, &flags) &&
+	    test_bit(FLAG_IRQ_IS_ENABLED, &flags)) {
 		gpiod_err(desc,
 			  "%s: tried to set a GPIO tied to an IRQ as output\n",
 			  __func__);
 		return -EIO;
 	}
 
-	if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
+	if (test_bit(FLAG_OPEN_DRAIN, &flags)) {
 		/* First see if we can enable open drain in hardware */
 		ret = gpio_set_config(desc, PIN_CONFIG_DRIVE_OPEN_DRAIN);
 		if (!ret)
@@ -2727,7 +2734,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 			ret = gpiod_direction_input(desc);
 			goto set_output_flag;
 		}
-	} else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) {
+	} else if (test_bit(FLAG_OPEN_SOURCE, &flags)) {
 		ret = gpio_set_config(desc, PIN_CONFIG_DRIVE_OPEN_SOURCE);
 		if (!ret)
 			goto set_output_value;
@@ -4424,21 +4431,22 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
 	int hwnum;
 	int ret;
 
+	if (test_and_set_bit(FLAG_IS_HOGGED, &desc->flags))
+		return 0;
+
 	gc = gpiod_to_chip(desc);
 	hwnum = gpio_chip_hwgpio(desc);
 
 	local_desc = gpiochip_request_own_desc(gc, hwnum, name,
 					       lflags, dflags);
 	if (IS_ERR(local_desc)) {
+		clear_bit(FLAG_IS_HOGGED, &desc->flags);
 		ret = PTR_ERR(local_desc);
 		pr_err("requesting hog GPIO %s (chip %s, offset %d) failed, %d\n",
 		       name, gc->label, hwnum, ret);
 		return ret;
 	}
 
-	/* Mark GPIO as hogged so it can be identified and removed later */
-	set_bit(FLAG_IS_HOGGED, &desc->flags);
-
 	gpiod_dbg(desc, "hogged as %s%s\n",
 		(dflags & GPIOD_FLAGS_BIT_DIR_OUT) ? "output" : "input",
 		(dflags & GPIOD_FLAGS_BIT_DIR_OUT) ?
-- 
2.40.1

[PATCH v3 11/24] gpio: remove unneeded code from gpio_device_get_desc()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The GPIO chip pointer is unused. Let's remove it.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5e098a7816bd..939ae167244c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -184,16 +184,6 @@ EXPORT_SYMBOL_GPL(gpiochip_get_desc);
 struct gpio_desc *
 gpio_device_get_desc(struct gpio_device *gdev, unsigned int hwnum)
 {
-	struct gpio_chip *gc;
-
-	/*
-	 * FIXME: This will be locked once we protect gdev->chip everywhere
-	 * with SRCU.
-	 */
-	gc = gdev->chip;
-	if (!gc)
-		return ERR_PTR(-ENODEV);
-
 	if (hwnum >= gdev->ngpio)
 		return ERR_PTR(-EINVAL);
 
-- 
2.40.1

[PATCH v3 12/24] gpio: sysfs: extend the critical section for unregistering sysfs devices

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Checking the gdev->mockdev pointer for NULL must be part of the critical
section.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-sysfs.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 3fe740b7d84d..fce4ef9754af 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/idr.h>
 #include <linux/init.h>
@@ -768,15 +769,15 @@ void gpiochip_sysfs_unregister(struct gpio_device *gdev)
 	struct gpio_desc *desc;
 	struct gpio_chip *chip = gdev->chip;
 
-	if (!gdev->mockdev)
-		return;
+	scoped_guard(mutex, &sysfs_lock) {
+		if (!gdev->mockdev)
+			return;
 
-	device_unregister(gdev->mockdev);
+		device_unregister(gdev->mockdev);
 
-	/* prevent further gpiod exports */
-	mutex_lock(&sysfs_lock);
-	gdev->mockdev = NULL;
-	mutex_unlock(&sysfs_lock);
+		/* prevent further gpiod exports */
+		gdev->mockdev = NULL;
+	}
 
 	/* unregister gpiod class devices owned by sysfs */
 	for_each_gpio_desc_with_flag(chip, desc, FLAG_SYSFS) {
-- 
2.40.1

[PATCH v3 13/24] gpio: sysfs: pass the GPIO device - not chip - to sysfs callbacks

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We're working towards protecting the chip pointer in struct gpio_device
with SRCU. In order to use it in sysfs callbacks we must pass the pointer
to the GPIO device that wraps the chip instead of the address of the
chip itself as the user data.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-sysfs.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index fce4ef9754af..b46d50ac2452 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -401,27 +401,27 @@ static const struct attribute_group *gpio_groups[] = {
 static ssize_t base_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
-	const struct gpio_chip *chip = dev_get_drvdata(dev);
+	const struct gpio_device *gdev = dev_get_drvdata(dev);
 
-	return sysfs_emit(buf, "%d\n", chip->base);
+	return sysfs_emit(buf, "%d\n", gdev->chip->base);
 }
 static DEVICE_ATTR_RO(base);
 
 static ssize_t label_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
-	const struct gpio_chip *chip = dev_get_drvdata(dev);
+	const struct gpio_device *gdev = dev_get_drvdata(dev);
 
-	return sysfs_emit(buf, "%s\n", chip->label ?: "");
+	return sysfs_emit(buf, "%s\n", gdev->chip->label ?: "");
 }
 static DEVICE_ATTR_RO(label);
 
 static ssize_t ngpio_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
-	const struct gpio_chip *chip = dev_get_drvdata(dev);
+	const struct gpio_device *gdev = dev_get_drvdata(dev);
 
-	return sysfs_emit(buf, "%u\n", chip->ngpio);
+	return sysfs_emit(buf, "%u\n", gdev->chip->ngpio);
 }
 static DEVICE_ATTR_RO(ngpio);
 
@@ -751,7 +751,7 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
 		parent = &gdev->dev;
 
 	/* use chip->base for the ID; it's already known to be unique */
-	dev = device_create_with_groups(&gpio_class, parent, MKDEV(0, 0), chip,
+	dev = device_create_with_groups(&gpio_class, parent, MKDEV(0, 0), gdev,
 					gpiochip_groups, GPIOCHIP_NAME "%d",
 					chip->base);
 	if (IS_ERR(dev))
-- 
2.40.1

[PATCH v3 14/24] gpio: cdev: replace gpiochip_get_desc() with gpio_device_get_desc()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

gpio_device_get_desc() is the safer alternative to gpiochip_get_desc().
As we don't really need to dereference the chip pointer to retrieve the
descriptors in character device code, let's use it.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 3588aaf90e45..8e37e3befa08 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -332,7 +332,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	/* Request each GPIO */
 	for (i = 0; i < handlereq.lines; i++) {
 		u32 offset = handlereq.lineoffsets[i];
-		struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);
+		struct gpio_desc *desc = gpio_device_get_desc(gdev, offset);
 
 		if (IS_ERR(desc)) {
 			ret = PTR_ERR(desc);
@@ -1739,7 +1739,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
 	/* Request each GPIO */
 	for (i = 0; i < ulr.num_lines; i++) {
 		u32 offset = ulr.offsets[i];
-		struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);
+		struct gpio_desc *desc = gpio_device_get_desc(gdev, offset);
 
 		if (IS_ERR(desc)) {
 			ret = PTR_ERR(desc);
@@ -2123,7 +2123,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	lflags = eventreq.handleflags;
 	eflags = eventreq.eventflags;
 
-	desc = gpiochip_get_desc(gdev->chip, offset);
+	desc = gpio_device_get_desc(gdev, offset);
 	if (IS_ERR(desc))
 		return PTR_ERR(desc);
 
@@ -2419,7 +2419,7 @@ static int lineinfo_get_v1(struct gpio_chardev_data *cdev, void __user *ip,
 		return -EFAULT;
 
 	/* this doubles as a range check on line_offset */
-	desc = gpiochip_get_desc(cdev->gdev->chip, lineinfo.line_offset);
+	desc = gpio_device_get_desc(cdev->gdev, lineinfo.line_offset);
 	if (IS_ERR(desc))
 		return PTR_ERR(desc);
 
@@ -2456,7 +2456,7 @@ static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
 	if (memchr_inv(lineinfo.padding, 0, sizeof(lineinfo.padding)))
 		return -EINVAL;
 
-	desc = gpiochip_get_desc(cdev->gdev->chip, lineinfo.offset);
+	desc = gpio_device_get_desc(cdev->gdev, lineinfo.offset);
 	if (IS_ERR(desc))
 		return PTR_ERR(desc);
 
-- 
2.40.1

[PATCH v3 15/24] gpio: cdev: don't access gdev->chip if it's not needed

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The variable holding the number of GPIO lines is duplicated in GPIO
device so read it instead of unnecessarily dereferencing the chip
pointer.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 8e37e3befa08..e993c6a7215a 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2701,7 +2701,7 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
 	if (!cdev)
 		return -ENODEV;
 
-	cdev->watched_lines = bitmap_zalloc(gdev->chip->ngpio, GFP_KERNEL);
+	cdev->watched_lines = bitmap_zalloc(gdev->ngpio, GFP_KERNEL);
 	if (!cdev->watched_lines)
 		goto out_free_cdev;
 
-- 
2.40.1

[PATCH v3 16/24] gpio: sysfs: don't access gdev->chip if it's not needed

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Don't dereference gdev->chip if the same information can be obtained
from struct gpio_device.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-sysfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index b46d50ac2452..2e3edf41e853 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -403,7 +403,7 @@ static ssize_t base_show(struct device *dev,
 {
 	const struct gpio_device *gdev = dev_get_drvdata(dev);
 
-	return sysfs_emit(buf, "%d\n", gdev->chip->base);
+	return sysfs_emit(buf, "%d\n", gdev->base);
 }
 static DEVICE_ATTR_RO(base);
 
@@ -412,7 +412,7 @@ static ssize_t label_show(struct device *dev,
 {
 	const struct gpio_device *gdev = dev_get_drvdata(dev);
 
-	return sysfs_emit(buf, "%s\n", gdev->chip->label ?: "");
+	return sysfs_emit(buf, "%s\n", gdev->label);
 }
 static DEVICE_ATTR_RO(label);
 
@@ -421,7 +421,7 @@ static ssize_t ngpio_show(struct device *dev,
 {
 	const struct gpio_device *gdev = dev_get_drvdata(dev);
 
-	return sysfs_emit(buf, "%u\n", gdev->chip->ngpio);
+	return sysfs_emit(buf, "%u\n", gdev->ngpio);
 }
 static DEVICE_ATTR_RO(ngpio);
 
-- 
2.40.1

Re: [PATCH v3 16/24] gpio: sysfs: don't access gdev->chip if it's not needed

[Linus Walleij]

On Thu, Feb 8, 2024 at 10:59 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Don't dereference gdev->chip if the same information can be obtained
> from struct gpio_device.
>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v3 17/24] gpio: don't dereference gdev->chip in gpiochip_setup_dev()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We don't need to dereference gdev->chip in gpiochip_setup_dev() as at
the time it's called, the label in the associated struct gpio_device is
already set.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 939ae167244c..6598960b29de 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -723,7 +723,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
 		goto err_remove_device;
 
 	dev_dbg(&gdev->dev, "registered GPIOs %d to %d on %s\n", gdev->base,
-		gdev->base + gdev->ngpio - 1, gdev->chip->label ? : "generic");
+		gdev->base + gdev->ngpio - 1, gdev->label);
 
 	return 0;
 
-- 
2.40.1

Re: [PATCH v3 17/24] gpio: don't dereference gdev->chip in gpiochip_setup_dev()

[Linus Walleij]

On Thu, Feb 8, 2024 at 10:59 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We don't need to dereference gdev->chip in gpiochip_setup_dev() as at
> the time it's called, the label in the associated struct gpio_device is
> already set.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v3 18/24] gpio: reduce the functionality of validate_desc()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Checking desc->gdev->chip for NULL without holding it in place with some
serializing mechanism is pointless. Remove this check. Also don't check
desc->gdev for NULL as it can never happen. We'll be protecting
gdev->chip with SRCU soon but we will provide a dedicated, automatic
class for that.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6598960b29de..9be7ec470cc0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2257,19 +2257,12 @@ static int validate_desc(const struct gpio_desc *desc, const char *func)
 {
 	if (!desc)
 		return 0;
+
 	if (IS_ERR(desc)) {
 		pr_warn("%s: invalid GPIO (errorpointer)\n", func);
 		return PTR_ERR(desc);
 	}
-	if (!desc->gdev) {
-		pr_warn("%s: invalid GPIO (no device)\n", func);
-		return -EINVAL;
-	}
-	if (!desc->gdev->chip) {
-		dev_warn(&desc->gdev->dev,
-			 "%s: backing chip is gone\n", func);
-		return 0;
-	}
+
 	return 1;
 }
 
@@ -2345,12 +2338,7 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 
 void gpiod_free(struct gpio_desc *desc)
 {
-	/*
-	 * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip
-	 * may already be NULL but we still want to put the references.
-	 */
-	if (!desc)
-		return;
+	VALIDATE_DESC_VOID(desc);
 
 	if (!gpiod_free_commit(desc))
 		WARN_ON(1);
-- 
2.40.1

[PATCH v3 19/24] gpio: remove unnecessary checks from gpiod_to_chip()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We don't need to check the gdev pointer in struct gpio_desc - it's
always assigned and never cleared. It's also pointless to check
gdev->chip before we actually serialize access to it.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9be7ec470cc0..140a44dec0be 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -214,7 +214,7 @@ EXPORT_SYMBOL_GPL(desc_to_gpio);
  */
 struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
 {
-	if (!desc || !desc->gdev)
+	if (!desc)
 		return NULL;
 	return desc->gdev->chip;
 }
@@ -3505,7 +3505,7 @@ int gpiod_to_irq(const struct gpio_desc *desc)
 	 * requires this function to not return zero on an invalid descriptor
 	 * but rather a negative error number.
 	 */
-	if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
+	if (!desc || IS_ERR(desc))
 		return -EINVAL;
 
 	gc = desc->gdev->chip;
-- 
2.40.1

Re: [PATCH v3 19/24] gpio: remove unnecessary checks from gpiod_to_chip()

[Andy Shevchenko]

On Thu, Feb 08, 2024 at 10:59:15AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We don't need to check the gdev pointer in struct gpio_desc - it's
> always assigned and never cleared. It's also pointless to check
> gdev->chip before we actually serialize access to it.

...

>  struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
>  {
> -	if (!desc || !desc->gdev)
> +	if (!desc)

Wondering if it makes sense to align with the below and use IS_ERR_OR_NULL() check.

>  		return NULL;
>  	return desc->gdev->chip;

...

> -	if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
> +	if (!desc || IS_ERR(desc))

IS_ERR_OR_NULL()

>  		return -EINVAL;
>  
>  	gc = desc->gdev->chip;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 19/24] gpio: remove unnecessary checks from gpiod_to_chip()

[Bartosz Golaszewski]

On Thu, Feb 8, 2024 at 6:39 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Feb 08, 2024 at 10:59:15AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We don't need to check the gdev pointer in struct gpio_desc - it's
> > always assigned and never cleared. It's also pointless to check
> > gdev->chip before we actually serialize access to it.
>
> ...
>
> >  struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
> >  {
> > -     if (!desc || !desc->gdev)
> > +     if (!desc)
>
> Wondering if it makes sense to align with the below and use IS_ERR_OR_NULL() check.
>

Nah, it's not supposed to be used with optional GPIOs anyway as it's
not a consumer facing API.

> >               return NULL;
> >       return desc->gdev->chip;
>
> ...
>
> > -     if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
> > +     if (!desc || IS_ERR(desc))
>
> IS_ERR_OR_NULL()
>

Ah, good point. It's a small nit though so I'll fix it when applying
barring some major objections for the rest.

Bart

> >               return -EINVAL;
> >
> >       gc = desc->gdev->chip;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3 19/24] gpio: remove unnecessary checks from gpiod_to_chip()

[Andy Shevchenko]

On Thu, Feb 08, 2024 at 08:17:14PM +0100, Bartosz Golaszewski wrote:
> On Thu, Feb 8, 2024 at 6:39 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Feb 08, 2024 at 10:59:15AM +0100, Bartosz Golaszewski wrote:

...

> > > -     if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
> > > +     if (!desc || IS_ERR(desc))
> >
> > IS_ERR_OR_NULL()
> 
> Ah, good point. It's a small nit though so I'll fix it when applying
> barring some major objections for the rest.
> 
> > >               return -EINVAL;

thinking more about it, shouldn't we return an actual error to the caller which
is in desc?

     if (!desc)
               return -EINVAL;
     if (IS_ERR(desc))
	return PTR_ERR(desc);

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 19/24] gpio: remove unnecessary checks from gpiod_to_chip()

[Bartosz Golaszewski]

On Thu, Feb 8, 2024 at 8:24 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Feb 08, 2024 at 08:17:14PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Feb 8, 2024 at 6:39 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Feb 08, 2024 at 10:59:15AM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > > > -     if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
> > > > +     if (!desc || IS_ERR(desc))
> > >
> > > IS_ERR_OR_NULL()
> >
> > Ah, good point. It's a small nit though so I'll fix it when applying
> > barring some major objections for the rest.
> >
> > > >               return -EINVAL;
>
> thinking more about it, shouldn't we return an actual error to the caller which
> is in desc?
>
>      if (!desc)
>                return -EINVAL;
>      if (IS_ERR(desc))
>         return PTR_ERR(desc);
>
> ?

Hmm... maybe but that's out of the scope of this series.

Bart

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3 19/24] gpio: remove unnecessary checks from gpiod_to_chip()

[Andy Shevchenko]

On Thu, Feb 08, 2024 at 08:34:56PM +0100, Bartosz Golaszewski wrote:
> On Thu, Feb 8, 2024 at 8:24 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Feb 08, 2024 at 08:17:14PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Feb 8, 2024 at 6:39 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Thu, Feb 08, 2024 at 10:59:15AM +0100, Bartosz Golaszewski wrote:

...

> > > > > -     if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
> > > > > +     if (!desc || IS_ERR(desc))
> > > >
> > > > IS_ERR_OR_NULL()
> > >
> > > Ah, good point. It's a small nit though so I'll fix it when applying
> > > barring some major objections for the rest.
> > >
> > > > >               return -EINVAL;
> >
> > thinking more about it, shouldn't we return an actual error to the caller which
> > is in desc?
> >
> >      if (!desc)
> >                return -EINVAL;
> >      if (IS_ERR(desc))
> >         return PTR_ERR(desc);
> >
> > ?
> 
> Hmm... maybe but that's out of the scope of this series.

Yeah, but just think about it.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 20/24] gpio: add the can_sleep flag to struct gpio_device

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Duplicating the can_sleep value in GPIO device will allow us to not
needlessly dereference the chip pointer in several places and reduce the
number of SRCU read-only critical sections.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 11 ++++++-----
 drivers/gpio/gpiolib.h |  3 +++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 140a44dec0be..6b696087c7f5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -901,6 +901,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	}
 
 	gdev->ngpio = gc->ngpio;
+	gdev->can_sleep = gc->can_sleep;
 
 	scoped_guard(mutex, &gpio_devices_lock) {
 		/*
@@ -3072,7 +3073,7 @@ int gpiod_get_raw_value(const struct gpio_desc *desc)
 {
 	VALIDATE_DESC(desc);
 	/* Should be using gpiod_get_raw_value_cansleep() */
-	WARN_ON(desc->gdev->chip->can_sleep);
+	WARN_ON(desc->gdev->can_sleep);
 	return gpiod_get_raw_value_commit(desc);
 }
 EXPORT_SYMBOL_GPL(gpiod_get_raw_value);
@@ -3093,7 +3094,7 @@ int gpiod_get_value(const struct gpio_desc *desc)
 
 	VALIDATE_DESC(desc);
 	/* Should be using gpiod_get_value_cansleep() */
-	WARN_ON(desc->gdev->chip->can_sleep);
+	WARN_ON(desc->gdev->can_sleep);
 
 	value = gpiod_get_raw_value_commit(desc);
 	if (value < 0)
@@ -3366,7 +3367,7 @@ void gpiod_set_raw_value(struct gpio_desc *desc, int value)
 {
 	VALIDATE_DESC_VOID(desc);
 	/* Should be using gpiod_set_raw_value_cansleep() */
-	WARN_ON(desc->gdev->chip->can_sleep);
+	WARN_ON(desc->gdev->can_sleep);
 	gpiod_set_raw_value_commit(desc, value);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_raw_value);
@@ -3407,7 +3408,7 @@ void gpiod_set_value(struct gpio_desc *desc, int value)
 {
 	VALIDATE_DESC_VOID(desc);
 	/* Should be using gpiod_set_value_cansleep() */
-	WARN_ON(desc->gdev->chip->can_sleep);
+	WARN_ON(desc->gdev->can_sleep);
 	gpiod_set_value_nocheck(desc, value);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_value);
@@ -3471,7 +3472,7 @@ EXPORT_SYMBOL_GPL(gpiod_set_array_value);
 int gpiod_cansleep(const struct gpio_desc *desc)
 {
 	VALIDATE_DESC(desc);
-	return desc->gdev->chip->can_sleep;
+	return desc->gdev->can_sleep;
 }
 EXPORT_SYMBOL_GPL(gpiod_cansleep);
 
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 9b7afe87f1bd..43ff4931e2c3 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -34,6 +34,8 @@
  * @descs: array of ngpio descriptors.
  * @ngpio: the number of GPIO lines on this GPIO device, equal to the size
  * of the @descs array.
+ * @can_sleep: indicate whether the GPIO chip driver's callbacks can sleep
+ * implying that they cannot be used from atomic context
  * @base: GPIO base in the DEPRECATED global Linux GPIO numberspace, assigned
  * at device creation time.
  * @label: a descriptive name for the GPIO device, such as the part number
@@ -64,6 +66,7 @@ struct gpio_device {
 	struct gpio_desc	*descs;
 	int			base;
 	u16			ngpio;
+	bool			can_sleep;
 	const char		*label;
 	void			*data;
 	struct list_head        list;
-- 
2.40.1

[PATCH v3 21/24] gpio: add SRCU infrastructure to struct gpio_device

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add the SRCU struct to GPIO device. It will be used to serialize access
to the GPIO chip pointer. Initialize and clean it up where applicable.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 13 ++++++++++---
 drivers/gpio/gpiolib.h |  2 ++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6b696087c7f5..9b1907f3e400 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -680,6 +680,7 @@ static void gpiodev_release(struct device *dev)
 	ida_free(&gpio_ida, gdev->id);
 	kfree_const(gdev->label);
 	kfree(gdev->descs);
+	cleanup_srcu_struct(&gdev->srcu);
 	kfree(gdev);
 }
 
@@ -948,6 +949,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
 	init_rwsem(&gdev->sem);
 
+	ret = init_srcu_struct(&gdev->srcu);
+	if (ret)
+		goto err_remove_from_list;
+
 #ifdef CONFIG_PINCTRL
 	INIT_LIST_HEAD(&gdev->pin_ranges);
 #endif
@@ -955,15 +960,15 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	if (gc->names) {
 		ret = gpiochip_set_desc_names(gc);
 		if (ret)
-			goto err_remove_from_list;
+			goto err_cleanup_gdev_srcu;
 	}
 	ret = gpiochip_set_names(gc);
 	if (ret)
-		goto err_remove_from_list;
+		goto err_cleanup_gdev_srcu;
 
 	ret = gpiochip_init_valid_mask(gc);
 	if (ret)
-		goto err_remove_from_list;
+		goto err_cleanup_gdev_srcu;
 
 	ret = of_gpiochip_add(gc);
 	if (ret)
@@ -1043,6 +1048,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		gpio_device_put(gdev);
 		goto err_print_message;
 	}
+err_cleanup_gdev_srcu:
+	cleanup_srcu_struct(&gdev->srcu);
 err_remove_from_list:
 	scoped_guard(mutex, &gpio_devices_lock)
 		list_del_rcu(&gdev->list);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 43ff4931e2c3..35d71e30c546 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -49,6 +49,7 @@
  * @sem: protects the structure from a NULL-pointer dereference of @chip by
  *       user-space operations when the device gets unregistered during
  *       a hot-unplug event
+ * @srcu: protects the pointer to the underlying GPIO chip
  * @pin_ranges: range of pins served by the GPIO driver
  *
  * This state container holds most of the runtime variable data
@@ -73,6 +74,7 @@ struct gpio_device {
 	struct blocking_notifier_head line_state_notifier;
 	struct blocking_notifier_head device_notifier;
 	struct rw_semaphore	sem;
+	struct srcu_struct	srcu;
 
 #ifdef CONFIG_PINCTRL
 	/*
-- 
2.40.1

[PATCH v3 22/24] gpio: protect the pointer to gpio_chip in gpio_device with SRCU

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Ensure we cannot crash if the GPIO device gets unregistered (and the
chip pointer set to NULL) during any of the API calls.

To that end: wait for all users of gdev->chip to exit their read-only
SRCU critical sections in gpiochip_remove().

For brevity: add a guard class which can be instantiated at the top of
every function requiring read-only access to the chip pointer and use it
in all API calls taking a GPIO descriptor as argument. In places where
we only deal with the GPIO device - use regular guard() helpers and
rcu_dereference() for chip access. Do the same in API calls taking a
const pointer to gpio_desc.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c  |  63 +++++----
 drivers/gpio/gpiolib-sysfs.c |  57 +++++---
 drivers/gpio/gpiolib.c       | 257 +++++++++++++++++++++++------------
 drivers/gpio/gpiolib.h       |  22 ++-
 4 files changed, 271 insertions(+), 128 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index e993c6a7215a..ccdeed013f6b 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -205,9 +205,9 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd,
 	unsigned int i;
 	int ret;
 
-	guard(rwsem_read)(&lh->gdev->sem);
+	guard(srcu)(&lh->gdev->srcu);
 
-	if (!lh->gdev->chip)
+	if (!rcu_dereference(lh->gdev->chip))
 		return -ENODEV;
 
 	switch (cmd) {
@@ -1520,9 +1520,9 @@ static long linereq_ioctl(struct file *file, unsigned int cmd,
 	struct linereq *lr = file->private_data;
 	void __user *ip = (void __user *)arg;
 
-	guard(rwsem_read)(&lr->gdev->sem);
+	guard(srcu)(&lr->gdev->srcu);
 
-	if (!lr->gdev->chip)
+	if (!rcu_dereference(lr->gdev->chip))
 		return -ENODEV;
 
 	switch (cmd) {
@@ -1551,9 +1551,9 @@ static __poll_t linereq_poll(struct file *file,
 	struct linereq *lr = file->private_data;
 	__poll_t events = 0;
 
-	guard(rwsem_read)(&lr->gdev->sem);
+	guard(srcu)(&lr->gdev->srcu);
 
-	if (!lr->gdev->chip)
+	if (!rcu_dereference(lr->gdev->chip))
 		return EPOLLHUP | EPOLLERR;
 
 	poll_wait(file, &lr->wait, wait);
@@ -1573,9 +1573,9 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
 	ssize_t bytes_read = 0;
 	int ret;
 
-	guard(rwsem_read)(&lr->gdev->sem);
+	guard(srcu)(&lr->gdev->srcu);
 
-	if (!lr->gdev->chip)
+	if (!rcu_dereference(lr->gdev->chip))
 		return -ENODEV;
 
 	if (count < sizeof(le))
@@ -1874,9 +1874,9 @@ static __poll_t lineevent_poll(struct file *file,
 	struct lineevent_state *le = file->private_data;
 	__poll_t events = 0;
 
-	guard(rwsem_read)(&le->gdev->sem);
+	guard(srcu)(&le->gdev->srcu);
 
-	if (!le->gdev->chip)
+	if (!rcu_dereference(le->gdev->chip))
 		return EPOLLHUP | EPOLLERR;
 
 	poll_wait(file, &le->wait, wait);
@@ -1912,9 +1912,9 @@ static ssize_t lineevent_read(struct file *file, char __user *buf,
 	ssize_t ge_size;
 	int ret;
 
-	guard(rwsem_read)(&le->gdev->sem);
+	guard(srcu)(&le->gdev->srcu);
 
-	if (!le->gdev->chip)
+	if (!rcu_dereference(le->gdev->chip))
 		return -ENODEV;
 
 	/*
@@ -1995,9 +1995,9 @@ static long lineevent_ioctl(struct file *file, unsigned int cmd,
 	void __user *ip = (void __user *)arg;
 	struct gpiohandle_data ghd;
 
-	guard(rwsem_read)(&le->gdev->sem);
+	guard(srcu)(&le->gdev->srcu);
 
-	if (!le->gdev->chip)
+	if (!rcu_dereference(le->gdev->chip))
 		return -ENODEV;
 
 	/*
@@ -2295,10 +2295,13 @@ static void gpio_v2_line_info_changed_to_v1(
 static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 				  struct gpio_v2_line_info *info)
 {
-	struct gpio_chip *gc = desc->gdev->chip;
 	unsigned long dflags;
 	const char *label;
 
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return;
+
 	memset(info, 0, sizeof(*info));
 	info->offset = gpio_chip_hwgpio(desc);
 
@@ -2331,8 +2334,8 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	    test_bit(FLAG_USED_AS_IRQ, &dflags) ||
 	    test_bit(FLAG_EXPORT, &dflags) ||
 	    test_bit(FLAG_SYSFS, &dflags) ||
-	    !gpiochip_line_is_valid(gc, info->offset) ||
-	    !pinctrl_gpio_can_use_line(gc, info->offset))
+	    !gpiochip_line_is_valid(guard.gc, info->offset) ||
+	    !pinctrl_gpio_can_use_line(guard.gc, info->offset))
 		info->flags |= GPIO_V2_LINE_FLAG_USED;
 
 	if (test_bit(FLAG_IS_OUT, &dflags))
@@ -2505,10 +2508,10 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct gpio_device *gdev = cdev->gdev;
 	void __user *ip = (void __user *)arg;
 
-	guard(rwsem_read)(&gdev->sem);
+	guard(srcu)(&gdev->srcu);
 
 	/* We fail any subsequent ioctl():s when the chip is gone */
-	if (!gdev->chip)
+	if (!rcu_dereference(gdev->chip))
 		return -ENODEV;
 
 	/* Fill in the struct and pass to userspace */
@@ -2591,9 +2594,9 @@ static __poll_t lineinfo_watch_poll(struct file *file,
 	struct gpio_chardev_data *cdev = file->private_data;
 	__poll_t events = 0;
 
-	guard(rwsem_read)(&cdev->gdev->sem);
+	guard(srcu)(&cdev->gdev->srcu);
 
-	if (!cdev->gdev->chip)
+	if (!rcu_dereference(cdev->gdev->chip))
 		return EPOLLHUP | EPOLLERR;
 
 	poll_wait(file, &cdev->wait, pollt);
@@ -2614,9 +2617,9 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
 	int ret;
 	size_t event_size;
 
-	guard(rwsem_read)(&cdev->gdev->sem);
+	guard(srcu)(&cdev->gdev->srcu);
 
-	if (!cdev->gdev->chip)
+	if (!rcu_dereference(cdev->gdev->chip))
 		return -ENODEV;
 
 #ifndef CONFIG_GPIO_CDEV_V1
@@ -2691,10 +2694,10 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
 	struct gpio_chardev_data *cdev;
 	int ret = -ENOMEM;
 
-	guard(rwsem_read)(&gdev->sem);
+	guard(srcu)(&gdev->srcu);
 
 	/* Fail on open if the backing gpiochip is gone */
-	if (!gdev->chip)
+	if (!rcu_dereference(gdev->chip))
 		return -ENODEV;
 
 	cdev = kzalloc(sizeof(*cdev), GFP_KERNEL);
@@ -2781,6 +2784,7 @@ static const struct file_operations gpio_fileops = {
 
 int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
 {
+	struct gpio_chip *gc;
 	int ret;
 
 	cdev_init(&gdev->chrdev, &gpio_fileops);
@@ -2791,8 +2795,13 @@ int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
 	if (ret)
 		return ret;
 
-	chip_dbg(gdev->chip, "added GPIO chardev (%d:%d)\n",
-		 MAJOR(devt), gdev->id);
+	guard(srcu)(&gdev->srcu);
+
+	gc = rcu_dereference(gdev->chip);
+	if (!gc)
+		return -ENODEV;
+
+	chip_dbg(gc, "added GPIO chardev (%d:%d)\n", MAJOR(devt), gdev->id);
 
 	return 0;
 }
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 2e3edf41e853..9f7b5e20fc25 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -171,6 +171,10 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
 	unsigned long irq_flags;
 	int ret;
 
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
+
 	data->irq = gpiod_to_irq(desc);
 	if (data->irq < 0)
 		return -EIO;
@@ -195,7 +199,7 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
 	 *        Remove this redundant call (along with the corresponding
 	 *        unlock) when those drivers have been fixed.
 	 */
-	ret = gpiochip_lock_as_irq(desc->gdev->chip, gpio_chip_hwgpio(desc));
+	ret = gpiochip_lock_as_irq(guard.gc, gpio_chip_hwgpio(desc));
 	if (ret < 0)
 		goto err_put_kn;
 
@@ -209,7 +213,7 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
 	return 0;
 
 err_unlock:
-	gpiochip_unlock_as_irq(desc->gdev->chip, gpio_chip_hwgpio(desc));
+	gpiochip_unlock_as_irq(guard.gc, gpio_chip_hwgpio(desc));
 err_put_kn:
 	sysfs_put(data->value_kn);
 
@@ -225,9 +229,13 @@ static void gpio_sysfs_free_irq(struct device *dev)
 	struct gpiod_data *data = dev_get_drvdata(dev);
 	struct gpio_desc *desc = data->desc;
 
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return;
+
 	data->irq_flags = 0;
 	free_irq(data->irq, data);
-	gpiochip_unlock_as_irq(desc->gdev->chip, gpio_chip_hwgpio(desc));
+	gpiochip_unlock_as_irq(guard.gc, gpio_chip_hwgpio(desc));
 	sysfs_put(data->value_kn);
 }
 
@@ -444,13 +452,12 @@ static ssize_t export_store(const struct class *class,
 				const char *buf, size_t len)
 {
 	struct gpio_desc *desc;
-	struct gpio_chip *gc;
 	int status, offset;
 	long gpio;
 
 	status = kstrtol(buf, 0, &gpio);
-	if (status < 0)
-		goto done;
+	if (status)
+		return status;
 
 	desc = gpio_to_desc(gpio);
 	/* reject invalid GPIOs */
@@ -458,9 +465,13 @@ static ssize_t export_store(const struct class *class,
 		pr_warn("%s: invalid GPIO %ld\n", __func__, gpio);
 		return -EINVAL;
 	}
-	gc = desc->gdev->chip;
+
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
+
 	offset = gpio_chip_hwgpio(desc);
-	if (!gpiochip_line_is_valid(gc, offset)) {
+	if (!gpiochip_line_is_valid(guard.gc, offset)) {
 		pr_warn("%s: GPIO %ld masked\n", __func__, gpio);
 		return -EINVAL;
 	}
@@ -563,7 +574,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	const char *ioname = NULL;
 	struct gpio_device *gdev;
 	struct gpiod_data *data;
-	struct gpio_chip *chip;
 	struct device *dev;
 	int status, offset;
 
@@ -578,16 +588,19 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 		return -EINVAL;
 	}
 
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
+
 	if (!test_and_set_bit(FLAG_EXPORT, &desc->flags))
 		return -EPERM;
 
 	gdev = desc->gdev;
-	chip = gdev->chip;
 
 	mutex_lock(&sysfs_lock);
 
 	/* check if chip is being removed */
-	if (!chip || !gdev->mockdev) {
+	if (!gdev->mockdev) {
 		status = -ENODEV;
 		goto err_unlock;
 	}
@@ -606,14 +619,14 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 
 	data->desc = desc;
 	mutex_init(&data->mutex);
-	if (chip->direction_input && chip->direction_output)
+	if (guard.gc->direction_input && guard.gc->direction_output)
 		data->direction_can_change = direction_may_change;
 	else
 		data->direction_can_change = false;
 
 	offset = gpio_chip_hwgpio(desc);
-	if (chip->names && chip->names[offset])
-		ioname = chip->names[offset];
+	if (guard.gc->names && guard.gc->names[offset])
+		ioname = guard.gc->names[offset];
 
 	dev = device_create_with_groups(&gpio_class, &gdev->dev,
 					MKDEV(0, 0), data, gpio_groups,
@@ -728,7 +741,7 @@ EXPORT_SYMBOL_GPL(gpiod_unexport);
 
 int gpiochip_sysfs_register(struct gpio_device *gdev)
 {
-	struct gpio_chip *chip = gdev->chip;
+	struct gpio_chip *chip;
 	struct device *parent;
 	struct device *dev;
 
@@ -741,6 +754,12 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
 	if (!class_is_registered(&gpio_class))
 		return 0;
 
+	guard(srcu)(&gdev->srcu);
+
+	chip = rcu_dereference(gdev->chip);
+	if (!chip)
+		return -ENODEV;
+
 	/*
 	 * For sysfs backward compatibility we need to preserve this
 	 * preferred parenting to the gpio_chip parent field, if set.
@@ -767,7 +786,7 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
 void gpiochip_sysfs_unregister(struct gpio_device *gdev)
 {
 	struct gpio_desc *desc;
-	struct gpio_chip *chip = gdev->chip;
+	struct gpio_chip *chip;
 
 	scoped_guard(mutex, &sysfs_lock) {
 		if (!gdev->mockdev)
@@ -779,6 +798,12 @@ void gpiochip_sysfs_unregister(struct gpio_device *gdev)
 		gdev->mockdev = NULL;
 	}
 
+	guard(srcu)(&gdev->srcu);
+
+	chip = rcu_dereference(gdev->chip);
+	if (chip)
+		return;
+
 	/* unregister gpiod class devices owned by sysfs */
 	for_each_gpio_desc_with_flag(chip, desc, FLAG_SYSFS) {
 		gpiod_unexport(desc);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9b1907f3e400..a14717a3e222 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -216,7 +216,7 @@ struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
 {
 	if (!desc)
 		return NULL;
-	return desc->gdev->chip;
+	return rcu_dereference(desc->gdev->chip);
 }
 EXPORT_SYMBOL_GPL(gpiod_to_chip);
 
@@ -285,7 +285,7 @@ EXPORT_SYMBOL(gpio_device_get_label);
  */
 struct gpio_chip *gpio_device_get_chip(struct gpio_device *gdev)
 {
-	return gdev->chip;
+	return rcu_dereference(gdev->chip);
 }
 EXPORT_SYMBOL_GPL(gpio_device_get_chip);
 
@@ -325,12 +325,21 @@ static int gpiochip_find_base_unlocked(int ngpio)
  */
 int gpiod_get_direction(struct gpio_desc *desc)
 {
-	struct gpio_chip *gc;
 	unsigned long flags;
 	unsigned int offset;
 	int ret;
 
-	gc = gpiod_to_chip(desc);
+	/*
+	 * We cannot use VALIDATE_DESC() as we must not return 0 for a NULL
+	 * descriptor like we usually do.
+	 */
+	if (!desc || IS_ERR(desc))
+		return -EINVAL;
+
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
+
 	offset = gpio_chip_hwgpio(desc);
 	flags = READ_ONCE(desc->flags);
 
@@ -342,10 +351,10 @@ int gpiod_get_direction(struct gpio_desc *desc)
 	    test_bit(FLAG_IS_OUT, &flags))
 		return 0;
 
-	if (!gc->get_direction)
+	if (!guard.gc->get_direction)
 		return -ENOTSUPP;
 
-	ret = gc->get_direction(gc, offset);
+	ret = guard.gc->get_direction(guard.gc, offset);
 	if (ret < 0)
 		return ret;
 
@@ -421,6 +430,7 @@ static struct gpio_desc *gpio_name_to_desc(const char * const name)
 {
 	struct gpio_device *gdev;
 	struct gpio_desc *desc;
+	struct gpio_chip *gc;
 
 	if (!name)
 		return NULL;
@@ -429,7 +439,13 @@ static struct gpio_desc *gpio_name_to_desc(const char * const name)
 
 	list_for_each_entry_srcu(gdev, &gpio_devices, list,
 				 srcu_read_lock_held(&gpio_devices_srcu)) {
-		for_each_gpio_desc(gdev->chip, desc) {
+		guard(srcu)(&gdev->srcu);
+
+		gc = rcu_dereference(gdev->chip);
+		if (!gc)
+			continue;
+
+		for_each_gpio_desc(gc, desc) {
 			if (desc->name && !strcmp(desc->name, name))
 				return desc;
 		}
@@ -853,7 +869,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	gdev->dev.type = &gpio_dev_type;
 	gdev->dev.bus = &gpio_bus_type;
 	gdev->dev.parent = gc->parent;
-	gdev->chip = gc;
+	rcu_assign_pointer(gdev->chip, gc);
 
 	gc->gpiodev = gdev;
 	gpiochip_set_data(gc, data);
@@ -1097,7 +1113,8 @@ void gpiochip_remove(struct gpio_chip *gc)
 	synchronize_srcu(&gpio_devices_srcu);
 
 	/* Numb the device, cancelling all outstanding operations */
-	gdev->chip = NULL;
+	rcu_assign_pointer(gdev->chip, NULL);
+	synchronize_srcu(&gdev->srcu);
 	gpiochip_irqchip_remove(gc);
 	acpi_gpiochip_remove(gc);
 	of_gpiochip_remove(gc);
@@ -1156,6 +1173,7 @@ struct gpio_device *gpio_device_find(void *data,
 						  void *data))
 {
 	struct gpio_device *gdev;
+	struct gpio_chip *gc;
 
 	/*
 	 * Not yet but in the future the spinlock below will become a mutex.
@@ -1166,8 +1184,13 @@ struct gpio_device *gpio_device_find(void *data,
 
 	guard(srcu)(&gpio_devices_srcu);
 
-	list_for_each_entry(gdev, &gpio_devices, list) {
-		if (gdev->chip && match(gdev->chip, data))
+	list_for_each_entry_srcu(gdev, &gpio_devices, list,
+				 srcu_read_lock_held(&gpio_devices_srcu)) {
+		guard(srcu)(&gdev->srcu);
+
+		gc = rcu_dereference(gdev->chip);
+
+		if (gc && match(gc, data))
 			return gpio_device_get(gdev);
 	}
 
@@ -2214,10 +2237,13 @@ EXPORT_SYMBOL_GPL(gpiochip_remove_pin_ranges);
  */
 static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 {
-	struct gpio_chip *gc = desc->gdev->chip;
 	unsigned int offset;
 	int ret;
 
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
+
 	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags))
 		return -EBUSY;
 
@@ -2231,17 +2257,17 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
 	 */
 
-	if (gc->request) {
+	if (guard.gc->request) {
 		offset = gpio_chip_hwgpio(desc);
-		if (gpiochip_line_is_valid(gc, offset))
-			ret = gc->request(gc, offset);
+		if (gpiochip_line_is_valid(guard.gc, offset))
+			ret = guard.gc->request(guard.gc, offset);
 		else
 			ret = -EINVAL;
 		if (ret)
 			goto out_clear_bit;
 	}
 
-	if (gc->get_direction)
+	if (guard.gc->get_direction)
 		gpiod_get_direction(desc);
 
 	ret = desc_set_label(desc, label ? : "?");
@@ -2308,18 +2334,18 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
 
 static bool gpiod_free_commit(struct gpio_desc *desc)
 {
-	struct gpio_chip *gc;
 	unsigned long flags;
 	bool ret = false;
 
 	might_sleep();
 
-	gc = desc->gdev->chip;
+	CLASS(gpio_chip_guard, guard)(desc);
+
 	flags = READ_ONCE(desc->flags);
 
-	if (gc && test_bit(FLAG_REQUESTED, &flags)) {
-		if (gc->free)
-			gc->free(gc, gpio_chip_hwgpio(desc));
+	if (guard.gc && test_bit(FLAG_REQUESTED, &flags)) {
+		if (guard.gc->free)
+			guard.gc->free(guard.gc, gpio_chip_hwgpio(desc));
 
 		clear_bit(FLAG_ACTIVE_LOW, &flags);
 		clear_bit(FLAG_REQUESTED, &flags);
@@ -2476,11 +2502,14 @@ static int gpio_set_config_with_argument(struct gpio_desc *desc,
 					 enum pin_config_param mode,
 					 u32 argument)
 {
-	struct gpio_chip *gc = desc->gdev->chip;
 	unsigned long config;
 
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
+
 	config = pinconf_to_config_packed(mode, argument);
-	return gpio_do_set_config(gc, gpio_chip_hwgpio(desc), config);
+	return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
 }
 
 static int gpio_set_config_with_argument_optional(struct gpio_desc *desc,
@@ -2570,18 +2599,20 @@ int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
  */
 int gpiod_direction_input(struct gpio_desc *desc)
 {
-	struct gpio_chip *gc;
 	int ret = 0;
 
 	VALIDATE_DESC(desc);
-	gc = desc->gdev->chip;
+
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
 
 	/*
 	 * It is legal to have no .get() and .direction_input() specified if
 	 * the chip is output-only, but you can't specify .direction_input()
 	 * and not support the .get() operation, that doesn't make sense.
 	 */
-	if (!gc->get && gc->direction_input) {
+	if (!guard.gc->get && guard.gc->direction_input) {
 		gpiod_warn(desc,
 			   "%s: missing get() but have direction_input()\n",
 			   __func__);
@@ -2594,10 +2625,12 @@ int gpiod_direction_input(struct gpio_desc *desc)
 	 * direction (if .get_direction() is supported) else we silently
 	 * assume we are in input mode after this.
 	 */
-	if (gc->direction_input) {
-		ret = gc->direction_input(gc, gpio_chip_hwgpio(desc));
-	} else if (gc->get_direction &&
-		  (gc->get_direction(gc, gpio_chip_hwgpio(desc)) != 1)) {
+	if (guard.gc->direction_input) {
+		ret = guard.gc->direction_input(guard.gc,
+						gpio_chip_hwgpio(desc));
+	} else if (guard.gc->get_direction &&
+		  (guard.gc->get_direction(guard.gc,
+					   gpio_chip_hwgpio(desc)) != 1)) {
 		gpiod_warn(desc,
 			   "%s: missing direction_input() operation and line is output\n",
 			   __func__);
@@ -2616,28 +2649,31 @@ EXPORT_SYMBOL_GPL(gpiod_direction_input);
 
 static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 {
-	struct gpio_chip *gc = desc->gdev->chip;
-	int val = !!value;
-	int ret = 0;
+	int val = !!value, ret = 0;
+
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
 
 	/*
 	 * It's OK not to specify .direction_output() if the gpiochip is
 	 * output-only, but if there is then not even a .set() operation it
 	 * is pretty tricky to drive the output line.
 	 */
-	if (!gc->set && !gc->direction_output) {
+	if (!guard.gc->set && !guard.gc->direction_output) {
 		gpiod_warn(desc,
 			   "%s: missing set() and direction_output() operations\n",
 			   __func__);
 		return -EIO;
 	}
 
-	if (gc->direction_output) {
-		ret = gc->direction_output(gc, gpio_chip_hwgpio(desc), val);
+	if (guard.gc->direction_output) {
+		ret = guard.gc->direction_output(guard.gc,
+						 gpio_chip_hwgpio(desc), val);
 	} else {
 		/* Check that we are in output mode if we can */
-		if (gc->get_direction &&
-		    gc->get_direction(gc, gpio_chip_hwgpio(desc))) {
+		if (guard.gc->get_direction &&
+		    guard.gc->get_direction(guard.gc, gpio_chip_hwgpio(desc))) {
 			gpiod_warn(desc,
 				"%s: missing direction_output() operation\n",
 				__func__);
@@ -2647,7 +2683,7 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 		 * If we can't actively set the direction, we are some
 		 * output-only chip, so just drive the output as desired.
 		 */
-		gc->set(gc, gpio_chip_hwgpio(desc), val);
+		guard.gc->set(guard.gc, gpio_chip_hwgpio(desc), val);
 	}
 
 	if (!ret)
@@ -2763,17 +2799,20 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output);
 int gpiod_enable_hw_timestamp_ns(struct gpio_desc *desc, unsigned long flags)
 {
 	int ret = 0;
-	struct gpio_chip *gc;
 
 	VALIDATE_DESC(desc);
 
-	gc = desc->gdev->chip;
-	if (!gc->en_hw_timestamp) {
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
+
+	if (!guard.gc->en_hw_timestamp) {
 		gpiod_warn(desc, "%s: hw ts not supported\n", __func__);
 		return -ENOTSUPP;
 	}
 
-	ret = gc->en_hw_timestamp(gc, gpio_chip_hwgpio(desc), flags);
+	ret = guard.gc->en_hw_timestamp(guard.gc,
+					gpio_chip_hwgpio(desc), flags);
 	if (ret)
 		gpiod_warn(desc, "%s: hw ts request failed\n", __func__);
 
@@ -2792,17 +2831,20 @@ EXPORT_SYMBOL_GPL(gpiod_enable_hw_timestamp_ns);
 int gpiod_disable_hw_timestamp_ns(struct gpio_desc *desc, unsigned long flags)
 {
 	int ret = 0;
-	struct gpio_chip *gc;
 
 	VALIDATE_DESC(desc);
 
-	gc = desc->gdev->chip;
-	if (!gc->dis_hw_timestamp) {
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
+
+	if (!guard.gc->dis_hw_timestamp) {
 		gpiod_warn(desc, "%s: hw ts not supported\n", __func__);
 		return -ENOTSUPP;
 	}
 
-	ret = gc->dis_hw_timestamp(gc, gpio_chip_hwgpio(desc), flags);
+	ret = guard.gc->dis_hw_timestamp(guard.gc, gpio_chip_hwgpio(desc),
+					 flags);
 	if (ret)
 		gpiod_warn(desc, "%s: hw ts release failed\n", __func__);
 
@@ -2821,12 +2863,13 @@ EXPORT_SYMBOL_GPL(gpiod_disable_hw_timestamp_ns);
  */
 int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
 {
-	struct gpio_chip *gc;
-
 	VALIDATE_DESC(desc);
-	gc = desc->gdev->chip;
 
-	return gpio_do_set_config(gc, gpio_chip_hwgpio(desc), config);
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
+
+	return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_config);
 
@@ -2924,10 +2967,19 @@ static int gpio_chip_get_value(struct gpio_chip *gc, const struct gpio_desc *des
 
 static int gpiod_get_raw_value_commit(const struct gpio_desc *desc)
 {
+	struct gpio_device *gdev;
 	struct gpio_chip *gc;
 	int value;
 
-	gc = desc->gdev->chip;
+	/* FIXME Unable to use gpio_chip_guard due to const desc. */
+	gdev = desc->gdev;
+
+	guard(srcu)(&gdev->srcu);
+
+	gc = rcu_dereference(gdev->chip);
+	if (!gc)
+		return -ENODEV;
+
 	value = gpio_chip_get_value(gc, desc);
 	value = value < 0 ? value : !!value;
 	trace_gpio_value(desc_to_gpio(desc), 1, value);
@@ -2953,6 +3005,14 @@ static int gpio_chip_get_multiple(struct gpio_chip *gc,
 	return -EIO;
 }
 
+/* The 'other' chip must be protected with its GPIO device's SRCU. */
+static bool gpio_device_chip_cmp(struct gpio_device *gdev, struct gpio_chip *gc)
+{
+	guard(srcu)(&gdev->srcu);
+
+	return gc == rcu_dereference(gdev->chip);
+}
+
 int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 				  unsigned int array_size,
 				  struct gpio_desc **desc_array,
@@ -2990,33 +3050,36 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 	}
 
 	while (i < array_size) {
-		struct gpio_chip *gc = desc_array[i]->gdev->chip;
 		DECLARE_BITMAP(fastpath_mask, FASTPATH_NGPIO);
 		DECLARE_BITMAP(fastpath_bits, FASTPATH_NGPIO);
 		unsigned long *mask, *bits;
 		int first, j;
 
-		if (likely(gc->ngpio <= FASTPATH_NGPIO)) {
+		CLASS(gpio_chip_guard, guard)(desc_array[i]);
+		if (!guard.gc)
+			return -ENODEV;
+
+		if (likely(guard.gc->ngpio <= FASTPATH_NGPIO)) {
 			mask = fastpath_mask;
 			bits = fastpath_bits;
 		} else {
 			gfp_t flags = can_sleep ? GFP_KERNEL : GFP_ATOMIC;
 
-			mask = bitmap_alloc(gc->ngpio, flags);
+			mask = bitmap_alloc(guard.gc->ngpio, flags);
 			if (!mask)
 				return -ENOMEM;
 
-			bits = bitmap_alloc(gc->ngpio, flags);
+			bits = bitmap_alloc(guard.gc->ngpio, flags);
 			if (!bits) {
 				bitmap_free(mask);
 				return -ENOMEM;
 			}
 		}
 
-		bitmap_zero(mask, gc->ngpio);
+		bitmap_zero(mask, guard.gc->ngpio);
 
 		if (!can_sleep)
-			WARN_ON(gc->can_sleep);
+			WARN_ON(guard.gc->can_sleep);
 
 		/* collect all inputs belonging to the same chip */
 		first = i;
@@ -3031,9 +3094,9 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 				i = find_next_zero_bit(array_info->get_mask,
 						       array_size, i);
 		} while ((i < array_size) &&
-			 (desc_array[i]->gdev->chip == gc));
+			 gpio_device_chip_cmp(desc_array[i]->gdev, guard.gc));
 
-		ret = gpio_chip_get_multiple(gc, mask, bits);
+		ret = gpio_chip_get_multiple(guard.gc, mask, bits);
 		if (ret) {
 			if (mask != fastpath_mask)
 				bitmap_free(mask);
@@ -3174,14 +3237,16 @@ EXPORT_SYMBOL_GPL(gpiod_get_array_value);
  */
 static void gpio_set_open_drain_value_commit(struct gpio_desc *desc, bool value)
 {
-	int ret = 0;
-	struct gpio_chip *gc = desc->gdev->chip;
-	int offset = gpio_chip_hwgpio(desc);
+	int ret = 0, offset = gpio_chip_hwgpio(desc);
+
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return;
 
 	if (value) {
-		ret = gc->direction_input(gc, offset);
+		ret = guard.gc->direction_input(guard.gc, offset);
 	} else {
-		ret = gc->direction_output(gc, offset, 0);
+		ret = guard.gc->direction_output(guard.gc, offset, 0);
 		if (!ret)
 			set_bit(FLAG_IS_OUT, &desc->flags);
 	}
@@ -3199,16 +3264,18 @@ static void gpio_set_open_drain_value_commit(struct gpio_desc *desc, bool value)
  */
 static void gpio_set_open_source_value_commit(struct gpio_desc *desc, bool value)
 {
-	int ret = 0;
-	struct gpio_chip *gc = desc->gdev->chip;
-	int offset = gpio_chip_hwgpio(desc);
+	int ret = 0, offset = gpio_chip_hwgpio(desc);
+
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return;
 
 	if (value) {
-		ret = gc->direction_output(gc, offset, 1);
+		ret = guard.gc->direction_output(guard.gc, offset, 1);
 		if (!ret)
 			set_bit(FLAG_IS_OUT, &desc->flags);
 	} else {
-		ret = gc->direction_input(gc, offset);
+		ret = guard.gc->direction_input(guard.gc, offset);
 	}
 	trace_gpio_direction(desc_to_gpio(desc), !value, ret);
 	if (ret < 0)
@@ -3219,11 +3286,12 @@ static void gpio_set_open_source_value_commit(struct gpio_desc *desc, bool value
 
 static void gpiod_set_raw_value_commit(struct gpio_desc *desc, bool value)
 {
-	struct gpio_chip *gc;
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return;
 
-	gc = desc->gdev->chip;
 	trace_gpio_value(desc_to_gpio(desc), 0, value);
-	gc->set(gc, gpio_chip_hwgpio(desc), value);
+	guard.gc->set(guard.gc, gpio_chip_hwgpio(desc), value);
 }
 
 /*
@@ -3284,33 +3352,36 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 	}
 
 	while (i < array_size) {
-		struct gpio_chip *gc = desc_array[i]->gdev->chip;
 		DECLARE_BITMAP(fastpath_mask, FASTPATH_NGPIO);
 		DECLARE_BITMAP(fastpath_bits, FASTPATH_NGPIO);
 		unsigned long *mask, *bits;
 		int count = 0;
 
-		if (likely(gc->ngpio <= FASTPATH_NGPIO)) {
+		CLASS(gpio_chip_guard, guard)(desc_array[i]);
+		if (!guard.gc)
+			return -ENODEV;
+
+		if (likely(guard.gc->ngpio <= FASTPATH_NGPIO)) {
 			mask = fastpath_mask;
 			bits = fastpath_bits;
 		} else {
 			gfp_t flags = can_sleep ? GFP_KERNEL : GFP_ATOMIC;
 
-			mask = bitmap_alloc(gc->ngpio, flags);
+			mask = bitmap_alloc(guard.gc->ngpio, flags);
 			if (!mask)
 				return -ENOMEM;
 
-			bits = bitmap_alloc(gc->ngpio, flags);
+			bits = bitmap_alloc(guard.gc->ngpio, flags);
 			if (!bits) {
 				bitmap_free(mask);
 				return -ENOMEM;
 			}
 		}
 
-		bitmap_zero(mask, gc->ngpio);
+		bitmap_zero(mask, guard.gc->ngpio);
 
 		if (!can_sleep)
-			WARN_ON(gc->can_sleep);
+			WARN_ON(guard.gc->can_sleep);
 
 		do {
 			struct gpio_desc *desc = desc_array[i];
@@ -3346,10 +3417,10 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 				i = find_next_zero_bit(array_info->set_mask,
 						       array_size, i);
 		} while ((i < array_size) &&
-			 (desc_array[i]->gdev->chip == gc));
+			 gpio_device_chip_cmp(desc_array[i]->gdev, guard.gc));
 		/* push collected bits to outputs */
 		if (count != 0)
-			gpio_chip_set_multiple(gc, mask, bits);
+			gpio_chip_set_multiple(guard.gc, mask, bits);
 
 		if (mask != fastpath_mask)
 			bitmap_free(mask);
@@ -3505,6 +3576,7 @@ EXPORT_SYMBOL_GPL(gpiod_set_consumer_name);
  */
 int gpiod_to_irq(const struct gpio_desc *desc)
 {
+	struct gpio_device *gdev;
 	struct gpio_chip *gc;
 	int offset;
 
@@ -3516,7 +3588,13 @@ int gpiod_to_irq(const struct gpio_desc *desc)
 	if (!desc || IS_ERR(desc))
 		return -EINVAL;
 
-	gc = desc->gdev->chip;
+	gdev = desc->gdev;
+	/* FIXME Cannot use gpio_chip_guard due to const desc. */
+	guard(srcu)(&gdev->srcu);
+	gc = rcu_dereference(gdev->chip);
+	if (!gc)
+		return -ENODEV;
+
 	offset = gpio_chip_hwgpio(desc);
 	if (gc->to_irq) {
 		int retirq = gc->to_irq(gc, offset);
@@ -4696,12 +4774,20 @@ core_initcall(gpiolib_dev_init);
 
 static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
 {
-	struct gpio_chip *gc = gdev->chip;
 	bool active_low, is_irq, is_out;
 	unsigned int gpio = gdev->base;
 	struct gpio_desc *desc;
+	struct gpio_chip *gc;
 	int value;
 
+	guard(srcu)(&gdev->srcu);
+
+	gc = rcu_dereference(gdev->chip);
+	if (!gc) {
+		seq_puts(s, "Underlying GPIO chip is gone\n");
+		return;
+	}
+
 	for_each_gpio_desc(gc, desc) {
 		guard(srcu)(&desc->srcu);
 		if (test_bit(FLAG_REQUESTED, &desc->flags)) {
@@ -4776,9 +4862,12 @@ static int gpiolib_seq_show(struct seq_file *s, void *v)
 {
 	struct gpiolib_seq_priv *priv = s->private;
 	struct gpio_device *gdev = v;
-	struct gpio_chip *gc = gdev->chip;
+	struct gpio_chip *gc;
 	struct device *parent;
 
+	guard(srcu)(&gdev->srcu);
+
+	gc = rcu_dereference(gdev->chip);
 	if (!gc) {
 		seq_printf(s, "%s%s: (dangling chip)",
 			   priv->newline ? "\n" : "",
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 35d71e30c546..b3810f7d286a 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -63,7 +63,7 @@ struct gpio_device {
 	int			id;
 	struct device		*mockdev;
 	struct module		*owner;
-	struct gpio_chip	*chip;
+	struct gpio_chip __rcu	*chip;
 	struct gpio_desc	*descs;
 	int			base;
 	u16			ngpio;
@@ -193,6 +193,26 @@ struct gpio_desc {
 
 #define gpiod_not_found(desc)		(IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
 
+struct gpio_chip_guard {
+	struct gpio_device *gdev;
+	struct gpio_chip *gc;
+	int idx;
+};
+
+DEFINE_CLASS(gpio_chip_guard,
+	     struct gpio_chip_guard,
+	     srcu_read_unlock(&_T.gdev->srcu, _T.idx),
+	     ({
+		struct gpio_chip_guard _guard;
+
+		_guard.gdev = desc->gdev;
+		_guard.idx = srcu_read_lock(&_guard.gdev->srcu);
+		_guard.gc = rcu_dereference(_guard.gdev->chip);
+
+		_guard;
+	     }),
+	     struct gpio_desc *desc)
+
 int gpiod_request(struct gpio_desc *desc, const char *label);
 void gpiod_free(struct gpio_desc *desc);
 
-- 
2.40.1

Re: [PATCH v3 22/24] gpio: protect the pointer to gpio_chip in gpio_device with SRCU

[kernel test robot]

Hello,

kernel test robot noticed "WARNING:suspicious_RCU_usage" on:

commit: c21131f83abc1f7227e7a6d5311e1df68bfa44e0 ("[PATCH v3 22/24] gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
url: https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/gpio-protect-the-list-of-GPIO-devices-with-SRCU/20240208-180822
base: https://git.kernel.org/cgit/linux/kernel/git/brgl/linux.git gpio/for-next
patch link: https://lore.kernel.org/all/20240208095920.8035-23-brgl@bgdev.pl/
patch subject: [PATCH v3 22/24] gpio: protect the pointer to gpio_chip in gpio_device with SRCU

in testcase: boot

compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G

(please refer to attached dmesg/kmsg for entire log/backtrace)


+-----------------------------------------------------------------+------------+------------+
|                                                                 | a3dfc11062 | c21131f83a |
+-----------------------------------------------------------------+------------+------------+
| drivers/gpio/gpiolib.c:#suspicious_rcu_dereference_check()usage | 0          | 8          |
| drivers/gpio/gpiolib.h:#suspicious_rcu_dereference_check()usage | 0          | 8          |
+-----------------------------------------------------------------+------------+------------+


If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202402122234.d85cca9b-lkp@intel.com



[   76.432519][    T1] gpiochip_find_base_unlocked: found new base at 512
[   76.434591][    T1]
[   76.435240][    T1] =============================
[   76.436545][    T1] WARNING: suspicious RCU usage
[   76.437813][    T1] 6.8.0-rc1-00050-gc21131f83abc #1 Tainted: G                 N
[   76.439873][    T1] -----------------------------
[   76.441158][    T1] drivers/gpio/gpiolib.c:219 suspicious rcu_dereference_check() usage!
[   76.443364][    T1]
[   76.443364][    T1] other info that might help us debug this:
[   76.443364][    T1]
[   76.446059][    T1]
[   76.446059][    T1] rcu_scheduler_active = 2, debug_locks = 1
[   76.448217][    T1] 1 lock held by swapper/1:
[ 76.449412][ T1] #0: ffff88816954f0f0 (&dev->mutex){....}-{3:3}, at: __driver_attach (drivers/base/dd.c:1216) 
[   76.451938][    T1]
[   76.451938][    T1] stack backtrace:
[   76.453486][    T1] CPU: 0 PID: 1 Comm: swapper Tainted: G                 N 6.8.0-rc1-00050-gc21131f83abc #1
[   76.456114][    T1] Call Trace:
[   76.456936][    T1]  <TASK>
[ 76.457682][ T1] dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1)) 
[ 76.458833][ T1] lockdep_rcu_suspicious (include/linux/context_tracking.h:153 kernel/locking/lockdep.c:6713) 
[ 76.460205][ T1] gpiod_to_chip (drivers/gpio/gpiolib.c:219 (discriminator 9)) 
[ 76.461346][ T1] gpiod_hog (drivers/gpio/gpiolib.h:243 drivers/gpio/gpiolib.c:4502) 
[ 76.462400][ T1] ? of_find_property (drivers/of/base.c:223) 
[ 76.463671][ T1] of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:799) 
[ 76.464933][ T1] ? __pfx_of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:785) 
[ 76.466378][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360) 
[ 76.467894][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423) 
[ 76.469220][ T1] ? _raw_spin_unlock_irqrestore (arch/x86/include/asm/preempt.h:103 include/linux/spinlock_api_smp.h:152 kernel/locking/spinlock.c:194) 
[ 76.470786][ T1] of_gpiochip_add (drivers/gpio/gpiolib-of.c:828 drivers/gpio/gpiolib-of.c:1143) 
[ 76.472060][ T1] ? fwnode_property_read_int_array (drivers/base/property.c:268 (discriminator 5)) 
[ 76.473692][ T1] gpiochip_add_data_with_key (drivers/gpio/gpiolib.c:989) 
[ 76.475271][ T1] ? kasan_save_track (arch/x86/include/asm/current.h:42 mm/kasan/common.c:60 mm/kasan/common.c:70) 
[ 76.476567][ T1] unittest_gpio_probe (drivers/of/unittest.c:1886) 
[ 76.477928][ T1] platform_probe (drivers/base/platform.c:1404) 
[ 76.479162][ T1] really_probe (drivers/base/dd.c:579 drivers/base/dd.c:658) 
[ 76.480403][ T1] __driver_probe_device (drivers/base/dd.c:800) 
[ 76.481791][ T1] driver_probe_device (drivers/base/dd.c:830) 
[ 76.483097][ T1] __driver_attach (drivers/base/dd.c:1217) 
[ 76.484388][ T1] ? __pfx___driver_attach (drivers/base/dd.c:1157) 
[ 76.485805][ T1] bus_for_each_dev (drivers/base/bus.c:367) 
[ 76.487037][ T1] ? lockdep_init_map_type (kernel/locking/lockdep.c:4892) 
[ 76.488477][ T1] ? __pfx_bus_for_each_dev (drivers/base/bus.c:356) 
[ 76.489897][ T1] ? bus_add_driver (drivers/base/bus.c:672) 
[ 76.491195][ T1] bus_add_driver (drivers/base/bus.c:674) 
[ 76.492463][ T1] driver_register (drivers/base/driver.c:246) 
[ 76.493723][ T1] of_unittest_overlay_gpio (drivers/of/unittest.c:1969 (discriminator 4)) 
[ 76.495167][ T1] of_unittest_overlay (drivers/of/unittest.c:2189 drivers/of/unittest.c:3217) 
[ 76.496478][ T1] ? __pfx_of_unittest_overlay (drivers/of/unittest.c:3155) 
[ 76.497886][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360) 
[ 76.499410][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423) 
[ 76.500744][ T1] of_unittest (drivers/of/unittest.c:4129) 
[ 76.501862][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080) 
[ 76.503098][ T1] ? add_device_randomness (drivers/char/random.c:918) 
[ 76.504492][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080) 
[ 76.505807][ T1] do_one_initcall (init/main.c:1236) 
[ 76.507054][ T1] ? __pfx_do_one_initcall (init/main.c:1227) 
[ 76.508517][ T1] do_initcalls (init/main.c:1297 init/main.c:1314) 
[ 76.509731][ T1] kernel_init_freeable (init/main.c:1555) 
[ 76.511106][ T1] ? __pfx_kernel_init (init/main.c:1433) 
[ 76.512435][ T1] kernel_init (init/main.c:1443) 
[ 76.513566][ T1] ? _raw_spin_unlock_irq (arch/x86/include/asm/preempt.h:103 include/linux/spinlock_api_smp.h:160 kernel/locking/spinlock.c:202) 
[ 76.514947][ T1] ret_from_fork (arch/x86/kernel/process.c:153) 
[ 76.516125][ T1] ? __pfx_kernel_init (init/main.c:1433) 
[ 76.517440][ T1] ret_from_fork_asm (arch/x86/entry/entry_64.S:250) 
[   76.518731][    T1]  </TASK>
[   76.519758][    T1]
[   76.520477][    T1] =============================
[   76.521774][    T1] WARNING: suspicious RCU usage
[   76.523076][    T1] 6.8.0-rc1-00050-gc21131f83abc #1 Tainted: G                 N
[   76.525108][    T1] -----------------------------
[   76.526429][    T1] drivers/gpio/gpiolib.h:210 suspicious rcu_dereference_check() usage!
[   76.528621][    T1]
[   76.528621][    T1] other info that might help us debug this:
[   76.528621][    T1]
[   76.531350][    T1]
[   76.531350][    T1] rcu_scheduler_active = 2, debug_locks = 1
[   76.533414][    T1] 2 locks held by swapper/1:
[ 76.534616][ T1] #0: ffff88816954f0f0 (&dev->mutex){....}-{3:3}, at: __driver_attach (drivers/base/dd.c:1216) 
[ 76.537073][ T1] #1: ffff888163afb6d0 (&gdev->srcu){.+.+}-{0:0}, at: gpiod_request_commit (include/linux/srcu.h:116 include/linux/srcu.h:215 drivers/gpio/gpiolib.h:202 drivers/gpio/gpiolib.c:2243) 
[   76.539703][    T1]
[   76.539703][    T1] stack backtrace:
[   76.541276][    T1] CPU: 0 PID: 1 Comm: swapper Tainted: G                 N 6.8.0-rc1-00050-gc21131f83abc #1
[   76.543890][    T1] Call Trace:
[   76.544767][    T1]  <TASK>
[ 76.545549][ T1] dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1)) 
[ 76.546740][ T1] lockdep_rcu_suspicious (include/linux/context_tracking.h:153 kernel/locking/lockdep.c:6713) 
[ 76.548196][ T1] gpiod_request_commit (drivers/gpio/gpiolib.h:202 drivers/gpio/gpiolib.c:2243) 
[ 76.549584][ T1] ? dump_stack_lvl (lib/dump_stack.c:108) 
[ 76.550829][ T1] gpiochip_request_own_desc (drivers/gpio/gpiolib.c:2454) 
[ 76.552354][ T1] gpiod_hog (drivers/gpio/gpiolib.c:4504) 
[ 76.553556][ T1] of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:799) 
[ 76.554948][ T1] ? __pfx_of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:785) 
[ 76.556536][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360) 
[ 76.558176][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423) 
[ 76.559633][ T1] ? _raw_spin_unlock_irqrestore (arch/x86/include/asm/preempt.h:103 include/linux/spinlock_api_smp.h:152 kernel/locking/spinlock.c:194) 
[ 76.561252][ T1] of_gpiochip_add (drivers/gpio/gpiolib-of.c:828 drivers/gpio/gpiolib-of.c:1143) 
[ 76.562584][ T1] ? fwnode_property_read_int_array (drivers/base/property.c:268 (discriminator 5)) 
[ 76.564302][ T1] gpiochip_add_data_with_key (drivers/gpio/gpiolib.c:989) 
[ 76.565974][ T1] ? kasan_save_track (arch/x86/include/asm/current.h:42 mm/kasan/common.c:60 mm/kasan/common.c:70) 
[ 76.567328][ T1] unittest_gpio_probe (drivers/of/unittest.c:1886) 
[ 76.568780][ T1] platform_probe (drivers/base/platform.c:1404) 
[ 76.570065][ T1] really_probe (drivers/base/dd.c:579 drivers/base/dd.c:658) 
[ 76.571309][ T1] __driver_probe_device (drivers/base/dd.c:800) 
[ 76.572801][ T1] driver_probe_device (drivers/base/dd.c:830) 
[ 76.574175][ T1] __driver_attach (drivers/base/dd.c:1217) 
[ 76.575513][ T1] ? __pfx___driver_attach (drivers/base/dd.c:1157) 
[ 76.576994][ T1] bus_for_each_dev (drivers/base/bus.c:367) 
[ 76.578331][ T1] ? lockdep_init_map_type (kernel/locking/lockdep.c:4892) 
[ 76.579877][ T1] ? __pfx_bus_for_each_dev (drivers/base/bus.c:356) 
[ 76.581368][ T1] ? bus_add_driver (drivers/base/bus.c:672) 
[ 76.582799][ T1] bus_add_driver (drivers/base/bus.c:674) 
[ 76.584128][ T1] driver_register (drivers/base/driver.c:246) 
[ 76.585452][ T1] of_unittest_overlay_gpio (drivers/of/unittest.c:1969 (discriminator 4)) 
[ 76.586969][ T1] of_unittest_overlay (drivers/of/unittest.c:2189 drivers/of/unittest.c:3217) 
[ 76.588400][ T1] ? __pfx_of_unittest_overlay (drivers/of/unittest.c:3155) 
[ 76.589958][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360) 
[ 76.591624][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423) 
[ 76.593071][ T1] of_unittest (drivers/of/unittest.c:4129) 
[ 76.594323][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080) 
[ 76.595700][ T1] ? add_device_randomness (drivers/char/random.c:918) 
[ 76.597164][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080) 
[ 76.598482][ T1] do_one_initcall (init/main.c:1236) 
[ 76.599728][ T1] ? __pfx_do_one_initcall (init/main.c:1227) 
[ 76.601163][ T1] do_initcalls (init/main.c:1297 init/main.c:1314) 
[ 76.602389][ T1] kernel_init_freeable (init/main.c:1555) 
[ 76.603810][ T1] ? __pfx_kernel_init (init/main.c:1433) 
[ 76.605101][ T1] kernel_init (init/main.c:1443) 
[ 76.606235][ T1] ? _raw_spin_unlock_irq (arch/x86/include/asm/preempt.h:103 include/linux/spinlock_api_smp.h:160 kernel/locking/spinlock.c:202) 
[ 76.607627][ T1] ret_from_fork (arch/x86/kernel/process.c:153) 
[ 76.608797][ T1] ? __pfx_kernel_init (init/main.c:1433) 
[ 76.610112][ T1] ret_from_fork_asm (arch/x86/entry/entry_64.S:250) 
[   76.611412][    T1]  </TASK>
[   76.612591][    T1] general protection fault, probably for non-canonical address 0xdffffc000000002f: 0000 [#1] PREEMPT KASAN PTI
[   76.615654][    T1] KASAN: null-ptr-deref in range [0x0000000000000178-0x000000000000017f]
[   76.617847][    T1] CPU: 0 PID: 1 Comm: swapper Tainted: G                 N 6.8.0-rc1-00050-gc21131f83abc #1
[ 76.620463][ T1] RIP: 0010:check_init_srcu_struct (kernel/rcu/srcutree.c:408) 
[ 76.622072][ T1] Code: 53 48 89 fb 80 3c 02 00 0f 85 fe 00 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 38 48 8d bd 78 01 00 00 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 ce 00 00 00 48 8b 85 78 01 00 00 a8 03 75 0b 5b
All code
========
   0:	53                   	push   %rbx
   1:	48 89 fb             	mov    %rdi,%rbx
   4:	80 3c 02 00          	cmpb   $0x0,(%rdx,%rax,1)
   8:	0f 85 fe 00 00 00    	jne    0x10c
   e:	48 b8 00 00 00 00 00 	movabs $0xdffffc0000000000,%rax
  15:	fc ff df 
  18:	48 8b 6b 38          	mov    0x38(%rbx),%rbp
  1c:	48 8d bd 78 01 00 00 	lea    0x178(%rbp),%rdi
  23:	48 89 fa             	mov    %rdi,%rdx
  26:	48 c1 ea 03          	shr    $0x3,%rdx
  2a:*	80 3c 02 00          	cmpb   $0x0,(%rdx,%rax,1)		<-- trapping instruction
  2e:	0f 85 ce 00 00 00    	jne    0x102
  34:	48 8b 85 78 01 00 00 	mov    0x178(%rbp),%rax
  3b:	a8 03                	test   $0x3,%al
  3d:	75 0b                	jne    0x4a
  3f:	5b                   	pop    %rbx

Code starting with the faulting instruction
===========================================
   0:	80 3c 02 00          	cmpb   $0x0,(%rdx,%rax,1)
   4:	0f 85 ce 00 00 00    	jne    0xd8
   a:	48 8b 85 78 01 00 00 	mov    0x178(%rbp),%rax
  11:	a8 03                	test   $0x3,%al
  13:	75 0b                	jne    0x20
  15:	5b                   	pop    %rbx
[   76.627183][    T1] RSP: 0018:ffff888103a6f718 EFLAGS: 00010202
[   76.628803][    T1] RAX: dffffc0000000000 RBX: ffff88810ee660f8 RCX: 0000000000000000
[   76.630879][    T1] RDX: 000000000000002f RSI: ffff88816976b000 RDI: 0000000000000178
[   76.632960][    T1] RBP: 0000000000000000 R08: 692d422d656e696c R09: 007475706e692d42
[   76.635045][    T1] R10: ffff888103a6f750 R11: ffffffff810b3aef R12: ffff88810ee66130
[   76.641151][    T1] R13: ffff888163afb6c0 R14: 0000000000000000 R15: ffff888163afb6d0
[   76.643224][    T1] FS:  0000000000000000(0000) GS:ffffffff84cd1000(0000) knlGS:0000000000000000
[   76.645563][    T1] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   76.647278][    T1] CR2: 00007fab8f4456f4 CR3: 0000000004cac000 CR4: 00000000000406b0
[   76.649355][    T1] Call Trace:
[   76.650221][    T1]  <TASK>
[ 76.651020][ T1] ? die_addr (arch/x86/kernel/dumpstack.c:421 arch/x86/kernel/dumpstack.c:460) 
[ 76.652132][ T1] ? exc_general_protection (arch/x86/kernel/traps.c:701 arch/x86/kernel/traps.c:643) 
[ 76.653604][ T1] ? asm_exc_general_protection (arch/x86/include/asm/idtentry.h:564) 
[ 76.655133][ T1] ? ret_from_fork (arch/x86/kernel/process.c:153) 
[ 76.656355][ T1] ? check_init_srcu_struct (kernel/rcu/srcutree.c:408) 
[ 76.657792][ T1] synchronize_srcu (kernel/rcu/srcutree.c:1167 kernel/rcu/srcutree.c:1458) 
[ 76.659048][ T1] gpiod_request_commit (drivers/gpio/gpiolib.c:127 drivers/gpio/gpiolib.c:2273) 
[ 76.660430][ T1] gpiochip_request_own_desc (drivers/gpio/gpiolib.c:2454) 
[ 76.661898][ T1] gpiod_hog (drivers/gpio/gpiolib.c:4504) 
[ 76.663000][ T1] of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:799) 
[ 76.664334][ T1] ? __pfx_of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:785) 
[ 76.665830][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360) 
[ 76.667387][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423) 
[ 76.668763][ T1] ? _raw_spin_unlock_irqrestore (arch/x86/include/asm/preempt.h:103 include/linux/spinlock_api_smp.h:152 kernel/locking/spinlock.c:194) 
[ 76.670279][ T1] of_gpiochip_add (drivers/gpio/gpiolib-of.c:828 drivers/gpio/gpiolib-of.c:1143) 
[ 76.671550][ T1] ? fwnode_property_read_int_array (drivers/base/property.c:268 (discriminator 5)) 
[ 76.673171][ T1] gpiochip_add_data_with_key (drivers/gpio/gpiolib.c:989) 
[ 76.674714][ T1] ? kasan_save_track (arch/x86/include/asm/current.h:42 mm/kasan/common.c:60 mm/kasan/common.c:70) 
[ 76.676002][ T1] unittest_gpio_probe (drivers/of/unittest.c:1886) 
[ 76.677340][ T1] platform_probe (drivers/base/platform.c:1404) 
[ 76.678544][ T1] really_probe (drivers/base/dd.c:579 drivers/base/dd.c:658) 
[ 76.679745][ T1] __driver_probe_device (drivers/base/dd.c:800) 
[ 76.681126][ T1] driver_probe_device (drivers/base/dd.c:830) 
[ 76.682411][ T1] __driver_attach (drivers/base/dd.c:1217) 
[ 76.683679][ T1] ? __pfx___driver_attach (drivers/base/dd.c:1157) 
[ 76.685074][ T1] bus_for_each_dev (drivers/base/bus.c:367) 
[ 76.686340][ T1] ? lockdep_init_map_type (kernel/locking/lockdep.c:4892) 
[ 76.687774][ T1] ? __pfx_bus_for_each_dev (drivers/base/bus.c:356) 
[ 76.689176][ T1] ? bus_add_driver (drivers/base/bus.c:672) 
[ 76.690452][ T1] bus_add_driver (drivers/base/bus.c:674) 
[ 76.691691][ T1] driver_register (drivers/base/driver.c:246) 
[ 76.692949][ T1] of_unittest_overlay_gpio (drivers/of/unittest.c:1969 (discriminator 4)) 
[ 76.694377][ T1] of_unittest_overlay (drivers/of/unittest.c:2189 drivers/of/unittest.c:3217) 
[ 76.695724][ T1] ? __pfx_of_unittest_overlay (drivers/of/unittest.c:3155) 
[ 76.697198][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360) 
[ 76.698764][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423) 
[ 76.700151][ T1] of_unittest (drivers/of/unittest.c:4129) 
[ 76.701323][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080) 
[ 76.702622][ T1] ? add_device_randomness (drivers/char/random.c:918) 
[ 76.704050][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080) 
[ 76.705347][ T1] do_one_initcall (init/main.c:1236) 
[ 76.706572][ T1] ? __pfx_do_one_initcall (init/main.c:1227) 
[ 76.708227][ T1] do_initcalls (init/main.c:1297 init/main.c:1314) 


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240212/202402122234.d85cca9b-lkp@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 22/24] gpio: protect the pointer to gpio_chip in gpio_device with SRCU

[Bartosz Golaszewski]

On Mon, Feb 12, 2024 at 4:11 PM kernel test robot <oliver.sang@intel.com> wrote:
>
>
>
> Hello,
>
> kernel test robot noticed "WARNING:suspicious_RCU_usage" on:
>
> commit: c21131f83abc1f7227e7a6d5311e1df68bfa44e0 ("[PATCH v3 22/24] gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
> url: https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/gpio-protect-the-list-of-GPIO-devices-with-SRCU/20240208-180822
> base: https://git.kernel.org/cgit/linux/kernel/git/brgl/linux.git gpio/for-next
> patch link: https://lore.kernel.org/all/20240208095920.8035-23-brgl@bgdev.pl/
> patch subject: [PATCH v3 22/24] gpio: protect the pointer to gpio_chip in gpio_device with SRCU
>
> in testcase: boot
>
> compiler: gcc-12
> test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
>
> (please refer to attached dmesg/kmsg for entire log/backtrace)
>
>
> +-----------------------------------------------------------------+------------+------------+
> |                                                                 | a3dfc11062 | c21131f83a |
> +-----------------------------------------------------------------+------------+------------+
> | drivers/gpio/gpiolib.c:#suspicious_rcu_dereference_check()usage | 0          | 8          |
> | drivers/gpio/gpiolib.h:#suspicious_rcu_dereference_check()usage | 0          | 8          |
> +-----------------------------------------------------------------+------------+------------+
>
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <oliver.sang@intel.com>
> | Closes: https://lore.kernel.org/oe-lkp/202402122234.d85cca9b-lkp@intel.com
>
>
>
> [   76.432519][    T1] gpiochip_find_base_unlocked: found new base at 512
> [   76.434591][    T1]
> [   76.435240][    T1] =============================
> [   76.436545][    T1] WARNING: suspicious RCU usage
> [   76.437813][    T1] 6.8.0-rc1-00050-gc21131f83abc #1 Tainted: G                 N
> [   76.439873][    T1] -----------------------------
> [   76.441158][    T1] drivers/gpio/gpiolib.c:219 suspicious rcu_dereference_check() usage!
> [   76.443364][    T1]
> [   76.443364][    T1] other info that might help us debug this:
> [   76.443364][    T1]
> [   76.446059][    T1]
> [   76.446059][    T1] rcu_scheduler_active = 2, debug_locks = 1
> [   76.448217][    T1] 1 lock held by swapper/1:
> [ 76.449412][ T1] #0: ffff88816954f0f0 (&dev->mutex){....}-{3:3}, at: __driver_attach (drivers/base/dd.c:1216)
> [   76.451938][    T1]
> [   76.451938][    T1] stack backtrace:
> [   76.453486][    T1] CPU: 0 PID: 1 Comm: swapper Tainted: G                 N 6.8.0-rc1-00050-gc21131f83abc #1
> [   76.456114][    T1] Call Trace:
> [   76.456936][    T1]  <TASK>
> [ 76.457682][ T1] dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1))
> [ 76.458833][ T1] lockdep_rcu_suspicious (include/linux/context_tracking.h:153 kernel/locking/lockdep.c:6713)
> [ 76.460205][ T1] gpiod_to_chip (drivers/gpio/gpiolib.c:219 (discriminator 9))
> [ 76.461346][ T1] gpiod_hog (drivers/gpio/gpiolib.h:243 drivers/gpio/gpiolib.c:4502)

Ah, gpiod_hog() is not taking the SRCU read lock as it should. I'll send a fix.

Bart

[snip]

Re: [PATCH v3 22/24] gpio: protect the pointer to gpio_chip in gpio_device with SRCU

[Bartosz Golaszewski]

On Mon, Feb 12, 2024 at 4:11 PM kernel test robot <oliver.sang@intel.com> wrote:
>
>
>
> Hello,
>
> kernel test robot noticed "WARNING:suspicious_RCU_usage" on:
>
> commit: c21131f83abc1f7227e7a6d5311e1df68bfa44e0 ("[PATCH v3 22/24] gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
> url: https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/gpio-protect-the-list-of-GPIO-devices-with-SRCU/20240208-180822
> base: https://git.kernel.org/cgit/linux/kernel/git/brgl/linux.git gpio/for-next
> patch link: https://lore.kernel.org/all/20240208095920.8035-23-brgl@bgdev.pl/
> patch subject: [PATCH v3 22/24] gpio: protect the pointer to gpio_chip in gpio_device with SRCU
>
> in testcase: boot
>
> compiler: gcc-12
> test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
>
> (please refer to attached dmesg/kmsg for entire log/backtrace)
>
>
> +-----------------------------------------------------------------+------------+------------+
> |                                                                 | a3dfc11062 | c21131f83a |
> +-----------------------------------------------------------------+------------+------------+
> | drivers/gpio/gpiolib.c:#suspicious_rcu_dereference_check()usage | 0          | 8          |
> | drivers/gpio/gpiolib.h:#suspicious_rcu_dereference_check()usage | 0          | 8          |
> +-----------------------------------------------------------------+------------+------------+
>
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <oliver.sang@intel.com>
> | Closes: https://lore.kernel.org/oe-lkp/202402122234.d85cca9b-lkp@intel.com
>
>
>
> [   76.432519][    T1] gpiochip_find_base_unlocked: found new base at 512
> [   76.434591][    T1]
> [   76.435240][    T1] =============================
> [   76.436545][    T1] WARNING: suspicious RCU usage
> [   76.437813][    T1] 6.8.0-rc1-00050-gc21131f83abc #1 Tainted: G                 N
> [   76.439873][    T1] -----------------------------
> [   76.441158][    T1] drivers/gpio/gpiolib.c:219 suspicious rcu_dereference_check() usage!
> [   76.443364][    T1]
> [   76.443364][    T1] other info that might help us debug this:
> [   76.443364][    T1]
> [   76.446059][    T1]
> [   76.446059][    T1] rcu_scheduler_active = 2, debug_locks = 1
> [   76.448217][    T1] 1 lock held by swapper/1:
> [ 76.449412][ T1] #0: ffff88816954f0f0 (&dev->mutex){....}-{3:3}, at: __driver_attach (drivers/base/dd.c:1216)
> [   76.451938][    T1]
> [   76.451938][    T1] stack backtrace:
> [   76.453486][    T1] CPU: 0 PID: 1 Comm: swapper Tainted: G                 N 6.8.0-rc1-00050-gc21131f83abc #1
> [   76.456114][    T1] Call Trace:
> [   76.456936][    T1]  <TASK>
> [ 76.457682][ T1] dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1))
> [ 76.458833][ T1] lockdep_rcu_suspicious (include/linux/context_tracking.h:153 kernel/locking/lockdep.c:6713)
> [ 76.460205][ T1] gpiod_to_chip (drivers/gpio/gpiolib.c:219 (discriminator 9))
> [ 76.461346][ T1] gpiod_hog (drivers/gpio/gpiolib.h:243 drivers/gpio/gpiolib.c:4502)
> [ 76.462400][ T1] ? of_find_property (drivers/of/base.c:223)
> [ 76.463671][ T1] of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:799)
> [ 76.464933][ T1] ? __pfx_of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:785)
> [ 76.466378][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360)
> [ 76.467894][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423)
> [ 76.469220][ T1] ? _raw_spin_unlock_irqrestore (arch/x86/include/asm/preempt.h:103 include/linux/spinlock_api_smp.h:152 kernel/locking/spinlock.c:194)
> [ 76.470786][ T1] of_gpiochip_add (drivers/gpio/gpiolib-of.c:828 drivers/gpio/gpiolib-of.c:1143)
> [ 76.472060][ T1] ? fwnode_property_read_int_array (drivers/base/property.c:268 (discriminator 5))
> [ 76.473692][ T1] gpiochip_add_data_with_key (drivers/gpio/gpiolib.c:989)
> [ 76.475271][ T1] ? kasan_save_track (arch/x86/include/asm/current.h:42 mm/kasan/common.c:60 mm/kasan/common.c:70)
> [ 76.476567][ T1] unittest_gpio_probe (drivers/of/unittest.c:1886)
> [ 76.477928][ T1] platform_probe (drivers/base/platform.c:1404)
> [ 76.479162][ T1] really_probe (drivers/base/dd.c:579 drivers/base/dd.c:658)
> [ 76.480403][ T1] __driver_probe_device (drivers/base/dd.c:800)
> [ 76.481791][ T1] driver_probe_device (drivers/base/dd.c:830)
> [ 76.483097][ T1] __driver_attach (drivers/base/dd.c:1217)
> [ 76.484388][ T1] ? __pfx___driver_attach (drivers/base/dd.c:1157)
> [ 76.485805][ T1] bus_for_each_dev (drivers/base/bus.c:367)
> [ 76.487037][ T1] ? lockdep_init_map_type (kernel/locking/lockdep.c:4892)
> [ 76.488477][ T1] ? __pfx_bus_for_each_dev (drivers/base/bus.c:356)
> [ 76.489897][ T1] ? bus_add_driver (drivers/base/bus.c:672)
> [ 76.491195][ T1] bus_add_driver (drivers/base/bus.c:674)
> [ 76.492463][ T1] driver_register (drivers/base/driver.c:246)
> [ 76.493723][ T1] of_unittest_overlay_gpio (drivers/of/unittest.c:1969 (discriminator 4))
> [ 76.495167][ T1] of_unittest_overlay (drivers/of/unittest.c:2189 drivers/of/unittest.c:3217)
> [ 76.496478][ T1] ? __pfx_of_unittest_overlay (drivers/of/unittest.c:3155)
> [ 76.497886][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360)
> [ 76.499410][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423)
> [ 76.500744][ T1] of_unittest (drivers/of/unittest.c:4129)
> [ 76.501862][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080)
> [ 76.503098][ T1] ? add_device_randomness (drivers/char/random.c:918)
> [ 76.504492][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080)
> [ 76.505807][ T1] do_one_initcall (init/main.c:1236)
> [ 76.507054][ T1] ? __pfx_do_one_initcall (init/main.c:1227)
> [ 76.508517][ T1] do_initcalls (init/main.c:1297 init/main.c:1314)
> [ 76.509731][ T1] kernel_init_freeable (init/main.c:1555)
> [ 76.511106][ T1] ? __pfx_kernel_init (init/main.c:1433)
> [ 76.512435][ T1] kernel_init (init/main.c:1443)
> [ 76.513566][ T1] ? _raw_spin_unlock_irq (arch/x86/include/asm/preempt.h:103 include/linux/spinlock_api_smp.h:160 kernel/locking/spinlock.c:202)
> [ 76.514947][ T1] ret_from_fork (arch/x86/kernel/process.c:153)
> [ 76.516125][ T1] ? __pfx_kernel_init (init/main.c:1433)
> [ 76.517440][ T1] ret_from_fork_asm (arch/x86/entry/entry_64.S:250)
> [   76.518731][    T1]  </TASK>
> [   76.519758][    T1]
> [   76.520477][    T1] =============================
> [   76.521774][    T1] WARNING: suspicious RCU usage
> [   76.523076][    T1] 6.8.0-rc1-00050-gc21131f83abc #1 Tainted: G                 N
> [   76.525108][    T1] -----------------------------
> [   76.526429][    T1] drivers/gpio/gpiolib.h:210 suspicious rcu_dereference_check() usage!
> [   76.528621][    T1]
> [   76.528621][    T1] other info that might help us debug this:
> [   76.528621][    T1]
> [   76.531350][    T1]
> [   76.531350][    T1] rcu_scheduler_active = 2, debug_locks = 1
> [   76.533414][    T1] 2 locks held by swapper/1:
> [ 76.534616][ T1] #0: ffff88816954f0f0 (&dev->mutex){....}-{3:3}, at: __driver_attach (drivers/base/dd.c:1216)
> [ 76.537073][ T1] #1: ffff888163afb6d0 (&gdev->srcu){.+.+}-{0:0}, at: gpiod_request_commit (include/linux/srcu.h:116 include/linux/srcu.h:215 drivers/gpio/gpiolib.h:202 drivers/gpio/gpiolib.c:2243)
> [   76.539703][    T1]
> [   76.539703][    T1] stack backtrace:
> [   76.541276][    T1] CPU: 0 PID: 1 Comm: swapper Tainted: G                 N 6.8.0-rc1-00050-gc21131f83abc #1
> [   76.543890][    T1] Call Trace:
> [   76.544767][    T1]  <TASK>
> [ 76.545549][ T1] dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1))
> [ 76.546740][ T1] lockdep_rcu_suspicious (include/linux/context_tracking.h:153 kernel/locking/lockdep.c:6713)
> [ 76.548196][ T1] gpiod_request_commit (drivers/gpio/gpiolib.h:202 drivers/gpio/gpiolib.c:2243)
> [ 76.549584][ T1] ? dump_stack_lvl (lib/dump_stack.c:108)
> [ 76.550829][ T1] gpiochip_request_own_desc (drivers/gpio/gpiolib.c:2454)
> [ 76.552354][ T1] gpiod_hog (drivers/gpio/gpiolib.c:4504)
> [ 76.553556][ T1] of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:799)
> [ 76.554948][ T1] ? __pfx_of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:785)
> [ 76.556536][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360)
> [ 76.558176][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423)
> [ 76.559633][ T1] ? _raw_spin_unlock_irqrestore (arch/x86/include/asm/preempt.h:103 include/linux/spinlock_api_smp.h:152 kernel/locking/spinlock.c:194)
> [ 76.561252][ T1] of_gpiochip_add (drivers/gpio/gpiolib-of.c:828 drivers/gpio/gpiolib-of.c:1143)
> [ 76.562584][ T1] ? fwnode_property_read_int_array (drivers/base/property.c:268 (discriminator 5))
> [ 76.564302][ T1] gpiochip_add_data_with_key (drivers/gpio/gpiolib.c:989)
> [ 76.565974][ T1] ? kasan_save_track (arch/x86/include/asm/current.h:42 mm/kasan/common.c:60 mm/kasan/common.c:70)
> [ 76.567328][ T1] unittest_gpio_probe (drivers/of/unittest.c:1886)
> [ 76.568780][ T1] platform_probe (drivers/base/platform.c:1404)
> [ 76.570065][ T1] really_probe (drivers/base/dd.c:579 drivers/base/dd.c:658)
> [ 76.571309][ T1] __driver_probe_device (drivers/base/dd.c:800)
> [ 76.572801][ T1] driver_probe_device (drivers/base/dd.c:830)
> [ 76.574175][ T1] __driver_attach (drivers/base/dd.c:1217)
> [ 76.575513][ T1] ? __pfx___driver_attach (drivers/base/dd.c:1157)
> [ 76.576994][ T1] bus_for_each_dev (drivers/base/bus.c:367)
> [ 76.578331][ T1] ? lockdep_init_map_type (kernel/locking/lockdep.c:4892)
> [ 76.579877][ T1] ? __pfx_bus_for_each_dev (drivers/base/bus.c:356)
> [ 76.581368][ T1] ? bus_add_driver (drivers/base/bus.c:672)
> [ 76.582799][ T1] bus_add_driver (drivers/base/bus.c:674)
> [ 76.584128][ T1] driver_register (drivers/base/driver.c:246)
> [ 76.585452][ T1] of_unittest_overlay_gpio (drivers/of/unittest.c:1969 (discriminator 4))
> [ 76.586969][ T1] of_unittest_overlay (drivers/of/unittest.c:2189 drivers/of/unittest.c:3217)
> [ 76.588400][ T1] ? __pfx_of_unittest_overlay (drivers/of/unittest.c:3155)
> [ 76.589958][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360)
> [ 76.591624][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423)
> [ 76.593071][ T1] of_unittest (drivers/of/unittest.c:4129)
> [ 76.594323][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080)
> [ 76.595700][ T1] ? add_device_randomness (drivers/char/random.c:918)
> [ 76.597164][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080)
> [ 76.598482][ T1] do_one_initcall (init/main.c:1236)
> [ 76.599728][ T1] ? __pfx_do_one_initcall (init/main.c:1227)
> [ 76.601163][ T1] do_initcalls (init/main.c:1297 init/main.c:1314)
> [ 76.602389][ T1] kernel_init_freeable (init/main.c:1555)
> [ 76.603810][ T1] ? __pfx_kernel_init (init/main.c:1433)
> [ 76.605101][ T1] kernel_init (init/main.c:1443)
> [ 76.606235][ T1] ? _raw_spin_unlock_irq (arch/x86/include/asm/preempt.h:103 include/linux/spinlock_api_smp.h:160 kernel/locking/spinlock.c:202)
> [ 76.607627][ T1] ret_from_fork (arch/x86/kernel/process.c:153)
> [ 76.608797][ T1] ? __pfx_kernel_init (init/main.c:1433)
> [ 76.610112][ T1] ret_from_fork_asm (arch/x86/entry/entry_64.S:250)
> [   76.611412][    T1]  </TASK>
> [   76.612591][    T1] general protection fault, probably for non-canonical address 0xdffffc000000002f: 0000 [#1] PREEMPT KASAN PTI
> [   76.615654][    T1] KASAN: null-ptr-deref in range [0x0000000000000178-0x000000000000017f]
> [   76.617847][    T1] CPU: 0 PID: 1 Comm: swapper Tainted: G                 N 6.8.0-rc1-00050-gc21131f83abc #1
> [ 76.620463][ T1] RIP: 0010:check_init_srcu_struct (kernel/rcu/srcutree.c:408)
> [ 76.622072][ T1] Code: 53 48 89 fb 80 3c 02 00 0f 85 fe 00 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 38 48 8d bd 78 01 00 00 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 ce 00 00 00 48 8b 85 78 01 00 00 a8 03 75 0b 5b
> All code
> ========
>    0:   53                      push   %rbx
>    1:   48 89 fb                mov    %rdi,%rbx
>    4:   80 3c 02 00             cmpb   $0x0,(%rdx,%rax,1)
>    8:   0f 85 fe 00 00 00       jne    0x10c
>    e:   48 b8 00 00 00 00 00    movabs $0xdffffc0000000000,%rax
>   15:   fc ff df
>   18:   48 8b 6b 38             mov    0x38(%rbx),%rbp
>   1c:   48 8d bd 78 01 00 00    lea    0x178(%rbp),%rdi
>   23:   48 89 fa                mov    %rdi,%rdx
>   26:   48 c1 ea 03             shr    $0x3,%rdx
>   2a:*  80 3c 02 00             cmpb   $0x0,(%rdx,%rax,1)               <-- trapping instruction
>   2e:   0f 85 ce 00 00 00       jne    0x102
>   34:   48 8b 85 78 01 00 00    mov    0x178(%rbp),%rax
>   3b:   a8 03                   test   $0x3,%al
>   3d:   75 0b                   jne    0x4a
>   3f:   5b                      pop    %rbx
>
> Code starting with the faulting instruction
> ===========================================
>    0:   80 3c 02 00             cmpb   $0x0,(%rdx,%rax,1)
>    4:   0f 85 ce 00 00 00       jne    0xd8
>    a:   48 8b 85 78 01 00 00    mov    0x178(%rbp),%rax
>   11:   a8 03                   test   $0x3,%al
>   13:   75 0b                   jne    0x20
>   15:   5b                      pop    %rbx
> [   76.627183][    T1] RSP: 0018:ffff888103a6f718 EFLAGS: 00010202
> [   76.628803][    T1] RAX: dffffc0000000000 RBX: ffff88810ee660f8 RCX: 0000000000000000
> [   76.630879][    T1] RDX: 000000000000002f RSI: ffff88816976b000 RDI: 0000000000000178
> [   76.632960][    T1] RBP: 0000000000000000 R08: 692d422d656e696c R09: 007475706e692d42
> [   76.635045][    T1] R10: ffff888103a6f750 R11: ffffffff810b3aef R12: ffff88810ee66130
> [   76.641151][    T1] R13: ffff888163afb6c0 R14: 0000000000000000 R15: ffff888163afb6d0
> [   76.643224][    T1] FS:  0000000000000000(0000) GS:ffffffff84cd1000(0000) knlGS:0000000000000000
> [   76.645563][    T1] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   76.647278][    T1] CR2: 00007fab8f4456f4 CR3: 0000000004cac000 CR4: 00000000000406b0
> [   76.649355][    T1] Call Trace:
> [   76.650221][    T1]  <TASK>
> [ 76.651020][ T1] ? die_addr (arch/x86/kernel/dumpstack.c:421 arch/x86/kernel/dumpstack.c:460)
> [ 76.652132][ T1] ? exc_general_protection (arch/x86/kernel/traps.c:701 arch/x86/kernel/traps.c:643)
> [ 76.653604][ T1] ? asm_exc_general_protection (arch/x86/include/asm/idtentry.h:564)
> [ 76.655133][ T1] ? ret_from_fork (arch/x86/kernel/process.c:153)
> [ 76.656355][ T1] ? check_init_srcu_struct (kernel/rcu/srcutree.c:408)
> [ 76.657792][ T1] synchronize_srcu (kernel/rcu/srcutree.c:1167 kernel/rcu/srcutree.c:1458)
> [ 76.659048][ T1] gpiod_request_commit (drivers/gpio/gpiolib.c:127 drivers/gpio/gpiolib.c:2273)
> [ 76.660430][ T1] gpiochip_request_own_desc (drivers/gpio/gpiolib.c:2454)
> [ 76.661898][ T1] gpiod_hog (drivers/gpio/gpiolib.c:4504)
> [ 76.663000][ T1] of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:799)
> [ 76.664334][ T1] ? __pfx_of_gpiochip_add_hog (drivers/gpio/gpiolib-of.c:785)
> [ 76.665830][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360)
> [ 76.667387][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423)
> [ 76.668763][ T1] ? _raw_spin_unlock_irqrestore (arch/x86/include/asm/preempt.h:103 include/linux/spinlock_api_smp.h:152 kernel/locking/spinlock.c:194)
> [ 76.670279][ T1] of_gpiochip_add (drivers/gpio/gpiolib-of.c:828 drivers/gpio/gpiolib-of.c:1143)
> [ 76.671550][ T1] ? fwnode_property_read_int_array (drivers/base/property.c:268 (discriminator 5))
> [ 76.673171][ T1] gpiochip_add_data_with_key (drivers/gpio/gpiolib.c:989)
> [ 76.674714][ T1] ? kasan_save_track (arch/x86/include/asm/current.h:42 mm/kasan/common.c:60 mm/kasan/common.c:70)
> [ 76.676002][ T1] unittest_gpio_probe (drivers/of/unittest.c:1886)
> [ 76.677340][ T1] platform_probe (drivers/base/platform.c:1404)
> [ 76.678544][ T1] really_probe (drivers/base/dd.c:579 drivers/base/dd.c:658)
> [ 76.679745][ T1] __driver_probe_device (drivers/base/dd.c:800)
> [ 76.681126][ T1] driver_probe_device (drivers/base/dd.c:830)
> [ 76.682411][ T1] __driver_attach (drivers/base/dd.c:1217)
> [ 76.683679][ T1] ? __pfx___driver_attach (drivers/base/dd.c:1157)
> [ 76.685074][ T1] bus_for_each_dev (drivers/base/bus.c:367)
> [ 76.686340][ T1] ? lockdep_init_map_type (kernel/locking/lockdep.c:4892)
> [ 76.687774][ T1] ? __pfx_bus_for_each_dev (drivers/base/bus.c:356)
> [ 76.689176][ T1] ? bus_add_driver (drivers/base/bus.c:672)
> [ 76.690452][ T1] bus_add_driver (drivers/base/bus.c:674)
> [ 76.691691][ T1] driver_register (drivers/base/driver.c:246)
> [ 76.692949][ T1] of_unittest_overlay_gpio (drivers/of/unittest.c:1969 (discriminator 4))
> [ 76.694377][ T1] of_unittest_overlay (drivers/of/unittest.c:2189 drivers/of/unittest.c:3217)
> [ 76.695724][ T1] ? __pfx_of_unittest_overlay (drivers/of/unittest.c:3155)
> [ 76.697198][ T1] ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:4360)
> [ 76.698764][ T1] ? lockdep_hardirqs_on (kernel/locking/lockdep.c:4423)
> [ 76.700151][ T1] of_unittest (drivers/of/unittest.c:4129)
> [ 76.701323][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080)
> [ 76.702622][ T1] ? add_device_randomness (drivers/char/random.c:918)
> [ 76.704050][ T1] ? __pfx_of_unittest (drivers/of/unittest.c:4080)
> [ 76.705347][ T1] do_one_initcall (init/main.c:1236)
> [ 76.706572][ T1] ? __pfx_do_one_initcall (init/main.c:1227)
> [ 76.708227][ T1] do_initcalls (init/main.c:1297 init/main.c:1314)
>
>
> The kernel config and materials to reproduce are available at:
> https://download.01.org/0day-ci/archive/20240212/202402122234.d85cca9b-lkp@intel.com
>
>
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

Paul,

Could you help me out here? It seems that lockdep complains (with
"suspicious RCU usage") whenever an RCU-protected pointer is passed to
rcu_dereference() but is not actually dereferenced later - in which
case switching to rcu_access_pointer() helps. But in the case of the
of_unittests() it also emits the same warning for
gpiod_direction_input() where gdev->chip is fetched with
rcu_dereference() using CLASS(gpio_chip_guard) and later actually
dereferenced by calling guard.gc->...

Any hints as to what I'm doing wrong?

Thanks,
Bartosz

Re: [PATCH v3 22/24] gpio: protect the pointer to gpio_chip in gpio_device with SRCU

[Bartosz Golaszewski]

On Mon, Feb 12, 2024 at 10:20 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>

[snip]

> >
> >
> > [   76.432519][    T1] gpiochip_find_base_unlocked: found new base at 512
> > [   76.434591][    T1]
> > [   76.435240][    T1] =============================
> > [   76.436545][    T1] WARNING: suspicious RCU usage
> > [   76.437813][    T1] 6.8.0-rc1-00050-gc21131f83abc #1 Tainted: G                 N
> > [   76.439873][    T1] -----------------------------
> > [   76.441158][    T1] drivers/gpio/gpiolib.c:219 suspicious rcu_dereference_check() usage!
> > [   76.443364][    T1]

[snip]

>
> Paul,
>
> Could you help me out here? It seems that lockdep complains (with
> "suspicious RCU usage") whenever an RCU-protected pointer is passed to
> rcu_dereference() but is not actually dereferenced later - in which
> case switching to rcu_access_pointer() helps. But in the case of the
> of_unittests() it also emits the same warning for
> gpiod_direction_input() where gdev->chip is fetched with
> rcu_dereference() using CLASS(gpio_chip_guard) and later actually
> dereferenced by calling guard.gc->...
>
> Any hints as to what I'm doing wrong?
>
> Thanks,
> Bartosz

Seems like these can be silenced with rcu_dereference_protected() so
I'll use it for now.

Bart

[PATCH v3 23/24] gpio: remove the RW semaphore from the GPIO device

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

With all accesses to gdev->chip being protected with SRCU, we can now
remove the RW-semaphore specific to the character device which
fullfilled the same role up to this point.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 1 -
 drivers/gpio/gpiolib.c      | 4 ----
 drivers/gpio/gpiolib.h      | 5 -----
 3 files changed, 10 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index ccdeed013f6b..9323b357df43 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -24,7 +24,6 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/poll.h>
 #include <linux/rbtree.h>
-#include <linux/rwsem.h>
 #include <linux/seq_file.h>
 #include <linux/spinlock.h>
 #include <linux/timekeeping.h>
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a14717a3e222..97829f0c8487 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -963,7 +963,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
-	init_rwsem(&gdev->sem);
 
 	ret = init_srcu_struct(&gdev->srcu);
 	if (ret)
@@ -1102,8 +1101,6 @@ void gpiochip_remove(struct gpio_chip *gc)
 	struct gpio_device *gdev = gc->gpiodev;
 	unsigned int i;
 
-	down_write(&gdev->sem);
-
 	/* FIXME: should the legacy sysfs handling be moved to gpio_device? */
 	gpiochip_sysfs_unregister(gdev);
 	gpiochip_free_hogs(gc);
@@ -1142,7 +1139,6 @@ void gpiochip_remove(struct gpio_chip *gc)
 	 * gone.
 	 */
 	gcdev_unregister(gdev);
-	up_write(&gdev->sem);
 	gpio_device_put(gdev);
 }
 EXPORT_SYMBOL_GPL(gpiochip_remove);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index b3810f7d286a..07443d26cbca 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -16,7 +16,6 @@
 #include <linux/gpio/driver.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
-#include <linux/rwsem.h>
 #include <linux/srcu.h>
 
 #define GPIOCHIP_NAME	"gpiochip"
@@ -46,9 +45,6 @@
  *                       requested, released or reconfigured
  * @device_notifier: used to notify character device wait queues about the GPIO
  *                   device being unregistered
- * @sem: protects the structure from a NULL-pointer dereference of @chip by
- *       user-space operations when the device gets unregistered during
- *       a hot-unplug event
  * @srcu: protects the pointer to the underlying GPIO chip
  * @pin_ranges: range of pins served by the GPIO driver
  *
@@ -73,7 +69,6 @@ struct gpio_device {
 	struct list_head        list;
 	struct blocking_notifier_head line_state_notifier;
 	struct blocking_notifier_head device_notifier;
-	struct rw_semaphore	sem;
 	struct srcu_struct	srcu;
 
 #ifdef CONFIG_PINCTRL
-- 
2.40.1

Re: [PATCH v3 23/24] gpio: remove the RW semaphore from the GPIO device

[Kent Gibson]

On Thu, Feb 08, 2024 at 10:59:19AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> With all accesses to gdev->chip being protected with SRCU, we can now
> remove the RW-semaphore specific to the character device which
> fullfilled the same role up to this point.
>

fulfilled

> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpiolib-cdev.c | 1 -
>  drivers/gpio/gpiolib.c      | 4 ----
>  drivers/gpio/gpiolib.h      | 5 -----
>  3 files changed, 10 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index ccdeed013f6b..9323b357df43 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -24,7 +24,6 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/poll.h>
>  #include <linux/rbtree.h>
> -#include <linux/rwsem.h>
>  #include <linux/seq_file.h>
>  #include <linux/spinlock.h>
>  #include <linux/timekeeping.h>

Shouldn't this be part of the rwsem -> srcu switch in the previous
patch?

Other than those nits, FWIW the series looks good to me.

Cheers,
Kent.

Re: [PATCH v3 23/24] gpio: remove the RW semaphore from the GPIO device

[Bartosz Golaszewski]

On Sat, Feb 10, 2024 at 6:37 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Feb 08, 2024 at 10:59:19AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > With all accesses to gdev->chip being protected with SRCU, we can now
> > remove the RW-semaphore specific to the character device which
> > fullfilled the same role up to this point.
> >
>
> fulfilled
>
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> >  drivers/gpio/gpiolib-cdev.c | 1 -
> >  drivers/gpio/gpiolib.c      | 4 ----
> >  drivers/gpio/gpiolib.h      | 5 -----
> >  3 files changed, 10 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index ccdeed013f6b..9323b357df43 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -24,7 +24,6 @@
> >  #include <linux/pinctrl/consumer.h>
> >  #include <linux/poll.h>
> >  #include <linux/rbtree.h>
> > -#include <linux/rwsem.h>
> >  #include <linux/seq_file.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/timekeeping.h>
>
> Shouldn't this be part of the rwsem -> srcu switch in the previous
> patch?
>

That other patch was already huge. I figured this should be separate.

Bart

> Other than those nits, FWIW the series looks good to me.
>
> Cheers,
> Kent.

Re: [PATCH v3 23/24] gpio: remove the RW semaphore from the GPIO device

[Kent Gibson]

On Mon, Feb 12, 2024 at 10:53:07AM +0100, Bartosz Golaszewski wrote:
> On Sat, Feb 10, 2024 at 6:37 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Feb 08, 2024 at 10:59:19AM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > With all accesses to gdev->chip being protected with SRCU, we can now
> > > remove the RW-semaphore specific to the character device which
> > > fullfilled the same role up to this point.
> > >
> >
> > fulfilled
> >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > ---
> > >  drivers/gpio/gpiolib-cdev.c | 1 -
> > >  drivers/gpio/gpiolib.c      | 4 ----
> > >  drivers/gpio/gpiolib.h      | 5 -----
> > >  3 files changed, 10 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > > index ccdeed013f6b..9323b357df43 100644
> > > --- a/drivers/gpio/gpiolib-cdev.c
> > > +++ b/drivers/gpio/gpiolib-cdev.c
> > > @@ -24,7 +24,6 @@
> > >  #include <linux/pinctrl/consumer.h>
> > >  #include <linux/poll.h>
> > >  #include <linux/rbtree.h>
> > > -#include <linux/rwsem.h>
> > >  #include <linux/seq_file.h>
> > >  #include <linux/spinlock.h>
> > >  #include <linux/timekeeping.h>
> >
> > Shouldn't this be part of the rwsem -> srcu switch in the previous
> > patch?
> >
>
> That other patch was already huge. I figured this should be separate.
>

To be clear, I mean just this header removal, not the whole patch.

Cheers,
Kent.

Re: [PATCH v3 23/24] gpio: remove the RW semaphore from the GPIO device

[Bartosz Golaszewski]

On Mon, 12 Feb 2024 at 10:57, Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, Feb 12, 2024 at 10:53:07AM +0100, Bartosz Golaszewski wrote:
> > On Sat, Feb 10, 2024 at 6:37 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Thu, Feb 08, 2024 at 10:59:19AM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > With all accesses to gdev->chip being protected with SRCU, we can now
> > > > remove the RW-semaphore specific to the character device which
> > > > fullfilled the same role up to this point.
> > > >
> > >
> > > fulfilled
> > >
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > > ---
> > > >  drivers/gpio/gpiolib-cdev.c | 1 -
> > > >  drivers/gpio/gpiolib.c      | 4 ----
> > > >  drivers/gpio/gpiolib.h      | 5 -----
> > > >  3 files changed, 10 deletions(-)
> > > >
> > > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > > > index ccdeed013f6b..9323b357df43 100644
> > > > --- a/drivers/gpio/gpiolib-cdev.c
> > > > +++ b/drivers/gpio/gpiolib-cdev.c
> > > > @@ -24,7 +24,6 @@
> > > >  #include <linux/pinctrl/consumer.h>
> > > >  #include <linux/poll.h>
> > > >  #include <linux/rbtree.h>
> > > > -#include <linux/rwsem.h>
> > > >  #include <linux/seq_file.h>
> > > >  #include <linux/spinlock.h>
> > > >  #include <linux/timekeeping.h>
> > >
> > > Shouldn't this be part of the rwsem -> srcu switch in the previous
> > > patch?
> > >
> >
> > That other patch was already huge. I figured this should be separate.
> >
>
> To be clear, I mean just this header removal, not the whole patch.
>
> Cheers,
> Kent.

Ah, then it makes sense indeed. I'll fix it in tree.

Bart

[PATCH v3 24/24] gpio: mark unsafe gpio_chip manipulators as deprecated

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We still have some functions that return the address of the GPIO chip
associated with the GPIO device. This is dangerous and the users should
find a better solution. Let's add appropriate comments to the kernel
docs.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 97829f0c8487..9f8cf064002d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -211,6 +211,11 @@ EXPORT_SYMBOL_GPL(desc_to_gpio);
 /**
  * gpiod_to_chip - Return the GPIO chip to which a GPIO descriptor belongs
  * @desc:	descriptor to return the chip of
+ *
+ * *DEPRECATED*
+ * This function is unsafe and should not be used. Using the chip address
+ * without taking the SRCU read lock may result in dereferencing a dangling
+ * pointer.
  */
 struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
 {
@@ -275,6 +280,7 @@ EXPORT_SYMBOL(gpio_device_get_label);
  * Returns:
  * Address of the GPIO chip backing this device.
  *
+ * *DEPRECATED*
  * Until we can get rid of all non-driver users of struct gpio_chip, we must
  * provide a way of retrieving the pointer to it from struct gpio_device. This
  * is *NOT* safe as the GPIO API is considered to be hot-unpluggable and the
-- 
2.40.1

Re: [PATCH v3 00/24] gpio: rework locking and object life-time control

[Andy Shevchenko]

On Thu, Feb 08, 2024 at 10:58:56AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> This is a big rework of locking in GPIOLIB. The current serialization is
> pretty much useless. There is one big spinlock (gpio_lock) that "protects"
> both the GPIO device list, GPIO descriptor access and who knows what else.
> 
> I'm putting "protects" in quotes as in several places the lock is
> taken, released whenever a sleeping function is called and re-taken
> without regards for the "protected" state that may have changed.
> 
> First a little background on what we're dealing with in GPIOLIB. We have
> consumer API functions that can be called from any context explicitly
> (get/set value, set direction) as well as many others which will get
> called in atomic context implicitly (e.g. set config called in certain
> situations from gpiod_direction_output()).
> 
> On the other side: we have GPIO provider drivers whose callbacks may or
> may not sleep depending on the underlying protocol.
> 
> This makes any attempts at serialization quite complex. We typically
> cannot use sleeping locks - we may be called from atomic - but we also
> often cannot use spinlocks - provider callbacks may sleep. Moreover: we
> have close ties with the interrupt and pinctrl subsystems, often either
> calling into them or getting called from them. They use their own locking
> schemes which are at odds with ours (pinctrl uses mutexes, the interrupt
> subsystem can call GPIO helpers with spinlock taken).
> 
> There is also another significant issue: the GPIO device object contains
> a pointer to gpio_chip which is the implementation of the GPIO provider.
> This object can be removed at any point - as GPIOLIB officially supports
> hotplugging with all the dynamic expanders that we provide drivers for -
> and leave the GPIO API callbacks with a suddenly NULL pointer. This is
> a problem that allowed user-space processes to easily crash the kernel
> until we patched it with a read-write semaphore in the user-space facing
> code (but the problem still exists for in-kernel users). This was
> recognized before as evidenced by the implementation of validate_desc()
> but without proper serialization, simple checking for a NULL pointer is
> pointless and we do need a generic solution for that issue as well.
> 
> If we want to get it right - the more lockless we go, the better. This is
> why SRCU seems to be the right candidate for the mechanism to use. In fact
> it's the only mechanism we can use our read-only critical sections to be
> called from atomic and protecc contexts as well as call driver callbacks
> that may sleep (for the latter case).
> 
> We're going to use it in three places: to protect the global list of GPIO
> devices, to ensure consistency when dereferencing the chip pointer in GPIO
> device struct and finally to ensure that users can access GPIO descriptors
> and always see a consistent state.
> 
> We do NOT serialize all API callbacks. This means that provider callbacks
> may be called simultaneously and GPIO drivers need to provide their own
> locking if needed. This is on purpose. First: we only support exclusive
> GPIO usage* so there's no risk of two drivers getting in each other's way
> over the same GPIO. Second: with this series, we ensure enough consistency
> to limit the chance of drivers or user-space users crashing the kernel.
> With additional improvements in handling the flags field in GPIO
> descriptors there's very little to gain, while bitbanging drivers may care
> about the increased performance of going lockless.
> 
> This series brings in one somewhat significant functional change for
> in-kernel users, namely: GPIO API calls, for which the underlying GPIO
> chip is gone, will no longer return 0 and emit a log message but instead
> will return -ENODEV.
> 
> I know this is a lot of code to go through but the more eyes we get on it
> the better.
> 
> Thanks,
> Bartosz
> 
> * - This is not technically true. We do provide the
> GPIOD_FLAGS_BIT_NONEXCLUSIVE flag. However this is just another piece of
> technical debt. This is a hack provided for a single use-case in the
> regulator framework which got out of control and is now used in many
> places that should have never touched it. It's utterly broken and doesn't
> even provide any contract as to what a "shared GPIO" is. I would argue
> that it's the next thing we should address by providing "reference counted
> GPIO enable", not just a flag allowing to request the same GPIO twice
> and then allow two drivers to fight over who toggles it as is the case
> now. For now, let's just treat users of GPIOD_FLAGS_BIT_NONEXCLUSIVE like
> they're consciously and deliberately choosing to risk undefined behavior.

LGTM, but I haven't done thorough review, hence, FWIW,
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 00/24] gpio: rework locking and object life-time control

[Bartosz Golaszewski]

On Thu, Feb 8, 2024 at 10:59 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> This is a big rework of locking in GPIOLIB. The current serialization is
> pretty much useless. There is one big spinlock (gpio_lock) that "protects"
> both the GPIO device list, GPIO descriptor access and who knows what else.
>
> I'm putting "protects" in quotes as in several places the lock is
> taken, released whenever a sleeping function is called and re-taken
> without regards for the "protected" state that may have changed.
>
> First a little background on what we're dealing with in GPIOLIB. We have
> consumer API functions that can be called from any context explicitly
> (get/set value, set direction) as well as many others which will get
> called in atomic context implicitly (e.g. set config called in certain
> situations from gpiod_direction_output()).
>
> On the other side: we have GPIO provider drivers whose callbacks may or
> may not sleep depending on the underlying protocol.
>
> This makes any attempts at serialization quite complex. We typically
> cannot use sleeping locks - we may be called from atomic - but we also
> often cannot use spinlocks - provider callbacks may sleep. Moreover: we
> have close ties with the interrupt and pinctrl subsystems, often either
> calling into them or getting called from them. They use their own locking
> schemes which are at odds with ours (pinctrl uses mutexes, the interrupt
> subsystem can call GPIO helpers with spinlock taken).
>
> There is also another significant issue: the GPIO device object contains
> a pointer to gpio_chip which is the implementation of the GPIO provider.
> This object can be removed at any point - as GPIOLIB officially supports
> hotplugging with all the dynamic expanders that we provide drivers for -
> and leave the GPIO API callbacks with a suddenly NULL pointer. This is
> a problem that allowed user-space processes to easily crash the kernel
> until we patched it with a read-write semaphore in the user-space facing
> code (but the problem still exists for in-kernel users). This was
> recognized before as evidenced by the implementation of validate_desc()
> but without proper serialization, simple checking for a NULL pointer is
> pointless and we do need a generic solution for that issue as well.
>
> If we want to get it right - the more lockless we go, the better. This is
> why SRCU seems to be the right candidate for the mechanism to use. In fact
> it's the only mechanism we can use our read-only critical sections to be
> called from atomic and protecc contexts as well as call driver callbacks
> that may sleep (for the latter case).
>
> We're going to use it in three places: to protect the global list of GPIO
> devices, to ensure consistency when dereferencing the chip pointer in GPIO
> device struct and finally to ensure that users can access GPIO descriptors
> and always see a consistent state.
>
> We do NOT serialize all API callbacks. This means that provider callbacks
> may be called simultaneously and GPIO drivers need to provide their own
> locking if needed. This is on purpose. First: we only support exclusive
> GPIO usage* so there's no risk of two drivers getting in each other's way
> over the same GPIO. Second: with this series, we ensure enough consistency
> to limit the chance of drivers or user-space users crashing the kernel.
> With additional improvements in handling the flags field in GPIO
> descriptors there's very little to gain, while bitbanging drivers may care
> about the increased performance of going lockless.
>
> This series brings in one somewhat significant functional change for
> in-kernel users, namely: GPIO API calls, for which the underlying GPIO
> chip is gone, will no longer return 0 and emit a log message but instead
> will return -ENODEV.
>
> I know this is a lot of code to go through but the more eyes we get on it
> the better.
>
> Thanks,
> Bartosz
>
> * - This is not technically true. We do provide the
> GPIOD_FLAGS_BIT_NONEXCLUSIVE flag. However this is just another piece of
> technical debt. This is a hack provided for a single use-case in the
> regulator framework which got out of control and is now used in many
> places that should have never touched it. It's utterly broken and doesn't
> even provide any contract as to what a "shared GPIO" is. I would argue
> that it's the next thing we should address by providing "reference counted
> GPIO enable", not just a flag allowing to request the same GPIO twice
> and then allow two drivers to fight over who toggles it as is the case
> now. For now, let's just treat users of GPIOD_FLAGS_BIT_NONEXCLUSIVE like
> they're consciously and deliberately choosing to risk undefined behavior.
>
> v2 -> v3:
> - fix SRCU cleanup in error path
> - add a comment explaining the use of gpio_device_find() in sysfs code
> - don't needlessly dereference gdev->chip in sysfs code
> - don't return 1 (INPUT) for NULL descriptors from gpiod_get_direction(),
>   rather: return -EINVAL
> - fix debugfs code: take the SRCU read lock in .start() and release it in
>   .stop() callbacks for seq file instead of taking it locally which
>   doesn't protect the entire seq printout
> - move the removal of the GPIO device from the list before setting the
>   chip pointer to NULL
>
> v1 -> v2:
> - fix jumping over variable initialization in sysfs code
> - fix RCU-related sparse warnings
> - fix a smatch complaint about uninitialized variables (even though it's
>   a false positive coming from the fact that scoped_guard() is implemented
>   as a for loop
> - fix a potential NULL-pointer dereference in debugfs callbacks
> - improve commit messages
>
> Bartosz Golaszewski (24):
>   gpio: protect the list of GPIO devices with SRCU
>   gpio: of: assign and read the hog pointer atomically
>   gpio: remove unused logging helpers
>   gpio: provide and use gpiod_get_label()
>   gpio: don't set label from irq helpers
>   gpio: add SRCU infrastructure to struct gpio_desc
>   gpio: protect the descriptor label with SRCU
>   gpio: sysfs: use gpio_device_find() to iterate over existing devices
>   gpio: remove gpio_lock
>   gpio: reinforce desc->flags handling
>   gpio: remove unneeded code from gpio_device_get_desc()
>   gpio: sysfs: extend the critical section for unregistering sysfs
>     devices
>   gpio: sysfs: pass the GPIO device - not chip - to sysfs callbacks
>   gpio: cdev: replace gpiochip_get_desc() with gpio_device_get_desc()
>   gpio: cdev: don't access gdev->chip if it's not needed
>   gpio: sysfs: don't access gdev->chip if it's not needed
>   gpio: don't dereference gdev->chip in gpiochip_setup_dev()
>   gpio: reduce the functionality of validate_desc()
>   gpio: remove unnecessary checks from gpiod_to_chip()
>   gpio: add the can_sleep flag to struct gpio_device
>   gpio: add SRCU infrastructure to struct gpio_device
>   gpio: protect the pointer to gpio_chip in gpio_device with SRCU
>   gpio: remove the RW semaphore from the GPIO device
>   gpio: mark unsafe gpio_chip manipulators as deprecated
>
>  drivers/gpio/gpiolib-cdev.c  |  92 ++--
>  drivers/gpio/gpiolib-of.c    |   4 +-
>  drivers/gpio/gpiolib-sysfs.c | 151 ++++---
>  drivers/gpio/gpiolib.c       | 791 +++++++++++++++++++----------------
>  drivers/gpio/gpiolib.h       |  86 ++--
>  5 files changed, 635 insertions(+), 489 deletions(-)
>
> --
> 2.40.1
>

Applied the series. I'll let it spend some time in next and let's fix
any remaining issues in tree.

Bart

Re: [PATCH v3 00/24] gpio: rework locking and object life-time control

[Marek Szyprowski]

On 08.02.2024 10:58, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> This is a big rework of locking in GPIOLIB. The current serialization is
> pretty much useless. There is one big spinlock (gpio_lock) that "protects"
> both the GPIO device list, GPIO descriptor access and who knows what else.
>
> I'm putting "protects" in quotes as in several places the lock is
> taken, released whenever a sleeping function is called and re-taken
> without regards for the "protected" state that may have changed.
>
> First a little background on what we're dealing with in GPIOLIB. We have
> consumer API functions that can be called from any context explicitly
> (get/set value, set direction) as well as many others which will get
> called in atomic context implicitly (e.g. set config called in certain
> situations from gpiod_direction_output()).
>
> On the other side: we have GPIO provider drivers whose callbacks may or
> may not sleep depending on the underlying protocol.
>
> This makes any attempts at serialization quite complex. We typically
> cannot use sleeping locks - we may be called from atomic - but we also
> often cannot use spinlocks - provider callbacks may sleep. Moreover: we
> have close ties with the interrupt and pinctrl subsystems, often either
> calling into them or getting called from them. They use their own locking
> schemes which are at odds with ours (pinctrl uses mutexes, the interrupt
> subsystem can call GPIO helpers with spinlock taken).
>
> There is also another significant issue: the GPIO device object contains
> a pointer to gpio_chip which is the implementation of the GPIO provider.
> This object can be removed at any point - as GPIOLIB officially supports
> hotplugging with all the dynamic expanders that we provide drivers for -
> and leave the GPIO API callbacks with a suddenly NULL pointer. This is
> a problem that allowed user-space processes to easily crash the kernel
> until we patched it with a read-write semaphore in the user-space facing
> code (but the problem still exists for in-kernel users). This was
> recognized before as evidenced by the implementation of validate_desc()
> but without proper serialization, simple checking for a NULL pointer is
> pointless and we do need a generic solution for that issue as well.
>
> If we want to get it right - the more lockless we go, the better. This is
> why SRCU seems to be the right candidate for the mechanism to use. In fact
> it's the only mechanism we can use our read-only critical sections to be
> called from atomic and protecc contexts as well as call driver callbacks
> that may sleep (for the latter case).
>
> We're going to use it in three places: to protect the global list of GPIO
> devices, to ensure consistency when dereferencing the chip pointer in GPIO
> device struct and finally to ensure that users can access GPIO descriptors
> and always see a consistent state.
>
> We do NOT serialize all API callbacks. This means that provider callbacks
> may be called simultaneously and GPIO drivers need to provide their own
> locking if needed. This is on purpose. First: we only support exclusive
> GPIO usage* so there's no risk of two drivers getting in each other's way
> over the same GPIO. Second: with this series, we ensure enough consistency
> to limit the chance of drivers or user-space users crashing the kernel.
> With additional improvements in handling the flags field in GPIO
> descriptors there's very little to gain, while bitbanging drivers may care
> about the increased performance of going lockless.
>
> This series brings in one somewhat significant functional change for
> in-kernel users, namely: GPIO API calls, for which the underlying GPIO
> chip is gone, will no longer return 0 and emit a log message but instead
> will return -ENODEV.
>
> I know this is a lot of code to go through but the more eyes we get on it
> the better.

I've noticed that this patchset landed in today's linux-next. It causes 
a lots of warning during boot on my test boards when LOCKDEP is enabled 
in kernel configs. Do you want me to report all of them? Some can be 
easily reproduced even with QEMU's virt ARM and ARM64 machines.

Here are some examples captured on ARM 32bit Exynos4412-based OdroidU3 
board:

=============================
WARNING: suspicious RCU usage
6.8.0-rc4-next-20240213 #8017 Not tainted
-----------------------------
drivers/gpio/gpiolib-cdev.c:2799 suspicious rcu_dereference_check() usage!

other info that might help us debug this:


rcu_scheduler_active = 2, debug_locks = 1
2 locks held by swapper/0/1:
  #0: c1f9608c (&dev->mutex){....}-{3:3}, at: __device_attach+0x2c/0x1fc
  #1: c1f973c0 (&gdev->srcu){.+.+}-{0:0}, at: 
gpiolib_cdev_register+0x5c/0x1c4

stack backtrace:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.8.0-rc4-next-20240213 #8017
Hardware name: Samsung Exynos (Flattened Device Tree)
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x68/0x88
  dump_stack_lvl from lockdep_rcu_suspicious+0x150/0x1c0
  lockdep_rcu_suspicious from gpiolib_cdev_register+0x180/0x1c4
  gpiolib_cdev_register from gpiochip_setup_dev+0x44/0xb0
  gpiochip_setup_dev from gpiochip_add_data_with_key+0x9b4/0xab8
  gpiochip_add_data_with_key from devm_gpiochip_add_data_with_key+0x20/0x5c
  devm_gpiochip_add_data_with_key from samsung_pinctrl_probe+0x938/0xb18
  samsung_pinctrl_probe from platform_probe+0x5c/0xb8
  platform_probe from really_probe+0xe0/0x400
  really_probe from __driver_probe_device+0x9c/0x1f4
  __driver_probe_device from driver_probe_device+0x30/0xc0
  driver_probe_device from __device_attach_driver+0xa8/0x120
  __device_attach_driver from bus_for_each_drv+0x80/0xcc
  bus_for_each_drv from __device_attach+0xac/0x1fc
  __device_attach from bus_probe_device+0x8c/0x90
  bus_probe_device from device_add+0x5d4/0x7fc
  device_add from of_platform_device_create_pdata+0x94/0xc4
  of_platform_device_create_pdata from of_platform_bus_create+0x1f8/0x4c0
  of_platform_bus_create from of_platform_bus_create+0x268/0x4c0
  of_platform_bus_create from of_platform_populate+0x80/0x110
  of_platform_populate from of_platform_default_populate_init+0xd4/0xec
  of_platform_default_populate_init from do_one_initcall+0x64/0x2fc
  do_one_initcall from kernel_init_freeable+0x1c4/0x228
  kernel_init_freeable from kernel_init+0x1c/0x12c
  kernel_init from ret_from_fork+0x14/0x28
Exception stack(0xf0845fb0 to 0xf0845ff8)
...

=============================
WARNING: suspicious RCU usage
iommu: Default domain type: Translated
6.8.0-rc4-next-20240213 #8017 Not tainted
iommu: DMA domain TLB invalidation policy: strict mode
-----------------------------
drivers/gpio/gpiolib.c:1193 suspicious rcu_dereference_check() usage!

other info that might help us debug this:


rcu_scheduler_active = 2, debug_locks = 1
6 locks held by kworker/u16:1/36:
  #0: c1ced8b4 ((wq_completion)async){+.+.}-{0:0}, at: 
process_one_work+0x14c/0x690
  #1: f0905f20 ((work_completion)(&entry->work)){+.+.}-{0:0}, at: 
process_one_work+0x14c/0x690
  #2: c24e488c (&dev->mutex){....}-{3:3}, at: 
__driver_attach_async_helper+0x38/0xec
  #3: c13f4c5c (gpio_devices_srcu){.+.+}-{0:0}, at: 
gpiod_find_and_request+0x0/0x578
  #4: c13f4c5c (gpio_devices_srcu){.+.+}-{0:0}, at: 
gpio_device_find+0x0/0x2d8
  #5: c1f973c0 (&gdev->srcu){.+.+}-{0:0}, at: gpio_device_find+0xf0/0x2d8

stack backtrace:
CPU: 0 PID: 36 Comm: kworker/u16:1 Not tainted 6.8.0-rc4-next-20240213 #8017
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: async async_run_entry_fn
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x68/0x88
  dump_stack_lvl from lockdep_rcu_suspicious+0x150/0x1c0
  lockdep_rcu_suspicious from gpio_device_find+0x190/0x2d8
  gpio_device_find from of_get_named_gpiod_flags+0xa4/0x318
  of_get_named_gpiod_flags from of_find_gpio+0x80/0x168
  of_find_gpio from gpiod_find_and_request+0x15c/0x578
  gpiod_find_and_request from gpiod_get_optional+0x54/0x90
  gpiod_get_optional from reg_fixed_voltage_probe+0x200/0x400
  reg_fixed_voltage_probe from platform_probe+0x5c/0xb8
  platform_probe from really_probe+0xe0/0x400
  really_probe from __driver_probe_device+0x9c/0x1f4
  __driver_probe_device from driver_probe_device+0x30/0xc0
  driver_probe_device from __driver_attach_async_helper+0x54/0xec
  __driver_attach_async_helper from async_run_entry_fn+0x40/0x154
  async_run_entry_fn from process_one_work+0x224/0x690
  process_one_work from worker_thread+0x1a0/0x3f4
  worker_thread from kthread+0x104/0x138
  kthread from ret_from_fork+0x14/0x28
Exception stack(0xf0905fb0 to 0xf0905ff8)
...

=============================
WARNING: suspicious RCU usage
6.8.0-rc4-next-20240213 #8017 Not tainted
-----------------------------
drivers/gpio/gpiolib.c:112 suspicious rcu_dereference_check() usage!

other info that might help us debug this:


rcu_scheduler_active = 2, debug_locks = 1
2 locks held by swapper/0/1:
  #0: c24e548c (&dev->mutex){....}-{3:3}, at: __driver_attach+0x118/0x1d4
  #1: c1db1118 (&desc->srcu){.+.+}-{0:0}, at: 
gpiod_configure_flags+0x188/0x460

stack backtrace:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.8.0-rc4-next-20240213 #8017
Hardware name: Samsung Exynos (Flattened Device Tree)
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x68/0x88
  dump_stack_lvl from lockdep_rcu_suspicious+0x150/0x1c0
  lockdep_rcu_suspicious from gpiod_get_label+0xc4/0xcc
  gpiod_get_label from gpiod_configure_flags+0x214/0x460
  gpiod_configure_flags from gpiod_find_and_request+0x214/0x578
  gpiod_find_and_request from fwnode_gpiod_get_index+0x34/0x3c
  fwnode_gpiod_get_index from devm_fwnode_gpiod_get_index+0x60/0x9c
  devm_fwnode_gpiod_get_index from gpio_led_probe+0x300/0x3c0
  gpio_led_probe from platform_probe+0x5c/0xb8
  platform_probe from really_probe+0xe0/0x400
  really_probe from __driver_probe_device+0x9c/0x1f4
  __driver_probe_device from driver_probe_device+0x30/0xc0
  driver_probe_device from __driver_attach+0x124/0x1d4
  __driver_attach from bus_for_each_dev+0x6c/0xb4
  bus_for_each_dev from bus_add_driver+0xe0/0x200
  bus_add_driver from driver_register+0x7c/0x118
  driver_register from do_one_initcall+0x64/0x2fc
  do_one_initcall from kernel_init_freeable+0x1c4/0x228
  kernel_init_freeable from kernel_init+0x1c/0x12c
  kernel_init from ret_from_fork+0x14/0x28
Exception stack(0xf0845fb0 to 0xf0845ff8)
...

=============================
WARNING: suspicious RCU usage
6.8.0-rc4-next-20240213 #8017 Not tainted
-----------------------------
drivers/gpio/gpiolib.c:3596 suspicious rcu_dereference_check() usage!

other info that might help us debug this:


rcu_scheduler_active = 2, debug_locks = 1
4 locks held by kworker/u16:1/36:
  #0: c1ced8b4 ((wq_completion)async){+.+.}-{0:0}, at: 
process_one_work+0x14c/0x690
  #1: f0905f20 ((work_completion)(&entry->work)){+.+.}-{0:0}, at: 
process_one_work+0x14c/0x690
  #2: c1d2708c (&dev->mutex){....}-{3:3}, at: 
__driver_attach_async_helper+0x38/0xec
  #3: c1fc9fc0 (&gdev->srcu){.+.?}-{0:0}, at: gpiod_to_irq+0x18/0x194

stack backtrace:
CPU: 1 PID: 36 Comm: kworker/u16:1 Not tainted 6.8.0-rc4-next-20240213 #8017
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: async async_run_entry_fn
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x68/0x88
  dump_stack_lvl from lockdep_rcu_suspicious+0x150/0x1c0
  lockdep_rcu_suspicious from gpiod_to_irq+0x144/0x194
  gpiod_to_irq from mmc_gpiod_request_cd_irq+0xb4/0xd0
  mmc_gpiod_request_cd_irq from mmc_start_host+0x48/0x9c
  mmc_start_host from mmc_add_host+0x7c/0xb0
  mmc_add_host from __sdhci_add_host+0x1b4/0x304
  __sdhci_add_host from sdhci_add_host+0x24/0x38
  sdhci_add_host from sdhci_s3c_probe+0x484/0x554
  sdhci_s3c_probe from platform_probe+0x5c/0xb8
  platform_probe from really_probe+0xe0/0x400
  really_probe from __driver_probe_device+0x9c/0x1f4
  __driver_probe_device from driver_probe_device+0x30/0xc0
  driver_probe_device from __driver_attach_async_helper+0x54/0xec
  __driver_attach_async_helper from async_run_entry_fn+0x40/0x154
  async_run_entry_fn from process_one_work+0x224/0x690
  process_one_work from worker_thread+0x1a0/0x3f4
  worker_thread from kthread+0x104/0x138
  kthread from ret_from_fork+0x14/0x28
Exception stack(0xf0905fb0 to 0xf0905ff8)
...

=============================
WARNING: suspicious RCU usage
6.8.0-rc4-next-20240213 #8017 Not tainted
-----------------------------
drivers/gpio/gpiolib.c:2981 suspicious rcu_dereference_check() usage!

other info that might help us debug this:


rcu_scheduler_active = 2, debug_locks = 1
3 locks held by kworker/1:0/22:
  #0: c1c06cb4 ((wq_completion)events_freezable){+.+.}-{0:0}, at: 
process_one_work+0x14c/0x690
  #1: f08cdf20 ((work_completion)(&(&host->detect)->work)){+.+.}-{0:0}, 
at: process_one_work+0x14c/0x690
  #2: c1fc9fc0 (&gdev->srcu){.+.?}-{0:0}, at: 
gpiod_get_raw_value_commit+0x0/0x1b4

stack backtrace:
CPU: 1 PID: 22 Comm: kworker/1:0 Not tainted 6.8.0-rc4-next-20240213 #8017
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: events_freezable mmc_rescan
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x68/0x88
  dump_stack_lvl from lockdep_rcu_suspicious+0x150/0x1c0
  lockdep_rcu_suspicious from gpiod_get_raw_value_commit+0x164/0x1b4
  gpiod_get_raw_value_commit from gpiod_get_value+0x2c/0x94
  gpiod_get_value from sdhci_get_cd+0xc/0x68
  sdhci_get_cd from mmc_rescan+0x124/0x3a8
  mmc_rescan from process_one_work+0x224/0x690
  process_one_work from worker_thread+0x1a0/0x3f4
  worker_thread from kthread+0x104/0x138
  kthread from ret_from_fork+0x14/0x28
Exception stack(0xf08cdfb0 to 0xf08cdff8)
...


> Thanks,
> Bartosz
>
> * - This is not technically true. We do provide the
> GPIOD_FLAGS_BIT_NONEXCLUSIVE flag. However this is just another piece of
> technical debt. This is a hack provided for a single use-case in the
> regulator framework which got out of control and is now used in many
> places that should have never touched it. It's utterly broken and doesn't
> even provide any contract as to what a "shared GPIO" is. I would argue
> that it's the next thing we should address by providing "reference counted
> GPIO enable", not just a flag allowing to request the same GPIO twice
> and then allow two drivers to fight over who toggles it as is the case
> now. For now, let's just treat users of GPIOD_FLAGS_BIT_NONEXCLUSIVE like
> they're consciously and deliberately choosing to risk undefined behavior.
>
> v2 -> v3:
> - fix SRCU cleanup in error path
> - add a comment explaining the use of gpio_device_find() in sysfs code
> - don't needlessly dereference gdev->chip in sysfs code
> - don't return 1 (INPUT) for NULL descriptors from gpiod_get_direction(),
>    rather: return -EINVAL
> - fix debugfs code: take the SRCU read lock in .start() and release it in
>    .stop() callbacks for seq file instead of taking it locally which
>    doesn't protect the entire seq printout
> - move the removal of the GPIO device from the list before setting the
>    chip pointer to NULL
>
> v1 -> v2:
> - fix jumping over variable initialization in sysfs code
> - fix RCU-related sparse warnings
> - fix a smatch complaint about uninitialized variables (even though it's
>    a false positive coming from the fact that scoped_guard() is implemented
>    as a for loop
> - fix a potential NULL-pointer dereference in debugfs callbacks
> - improve commit messages
>
> Bartosz Golaszewski (24):
>    gpio: protect the list of GPIO devices with SRCU
>    gpio: of: assign and read the hog pointer atomically
>    gpio: remove unused logging helpers
>    gpio: provide and use gpiod_get_label()
>    gpio: don't set label from irq helpers
>    gpio: add SRCU infrastructure to struct gpio_desc
>    gpio: protect the descriptor label with SRCU
>    gpio: sysfs: use gpio_device_find() to iterate over existing devices
>    gpio: remove gpio_lock
>    gpio: reinforce desc->flags handling
>    gpio: remove unneeded code from gpio_device_get_desc()
>    gpio: sysfs: extend the critical section for unregistering sysfs
>      devices
>    gpio: sysfs: pass the GPIO device - not chip - to sysfs callbacks
>    gpio: cdev: replace gpiochip_get_desc() with gpio_device_get_desc()
>    gpio: cdev: don't access gdev->chip if it's not needed
>    gpio: sysfs: don't access gdev->chip if it's not needed
>    gpio: don't dereference gdev->chip in gpiochip_setup_dev()
>    gpio: reduce the functionality of validate_desc()
>    gpio: remove unnecessary checks from gpiod_to_chip()
>    gpio: add the can_sleep flag to struct gpio_device
>    gpio: add SRCU infrastructure to struct gpio_device
>    gpio: protect the pointer to gpio_chip in gpio_device with SRCU
>    gpio: remove the RW semaphore from the GPIO device
>    gpio: mark unsafe gpio_chip manipulators as deprecated
>
>   drivers/gpio/gpiolib-cdev.c  |  92 ++--
>   drivers/gpio/gpiolib-of.c    |   4 +-
>   drivers/gpio/gpiolib-sysfs.c | 151 ++++---
>   drivers/gpio/gpiolib.c       | 791 +++++++++++++++++++----------------
>   drivers/gpio/gpiolib.h       |  86 ++--
>   5 files changed, 635 insertions(+), 489 deletions(-)
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 00/24] gpio: rework locking and object life-time control

[Bartosz Golaszewski]

On Tue, Feb 13, 2024 at 1:05 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 08.02.2024 10:58, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > This is a big rework of locking in GPIOLIB. The current serialization is
> > pretty much useless. There is one big spinlock (gpio_lock) that "protects"
> > both the GPIO device list, GPIO descriptor access and who knows what else.
> >
> > I'm putting "protects" in quotes as in several places the lock is
> > taken, released whenever a sleeping function is called and re-taken
> > without regards for the "protected" state that may have changed.
> >
> > First a little background on what we're dealing with in GPIOLIB. We have
> > consumer API functions that can be called from any context explicitly
> > (get/set value, set direction) as well as many others which will get
> > called in atomic context implicitly (e.g. set config called in certain
> > situations from gpiod_direction_output()).
> >
> > On the other side: we have GPIO provider drivers whose callbacks may or
> > may not sleep depending on the underlying protocol.
> >
> > This makes any attempts at serialization quite complex. We typically
> > cannot use sleeping locks - we may be called from atomic - but we also
> > often cannot use spinlocks - provider callbacks may sleep. Moreover: we
> > have close ties with the interrupt and pinctrl subsystems, often either
> > calling into them or getting called from them. They use their own locking
> > schemes which are at odds with ours (pinctrl uses mutexes, the interrupt
> > subsystem can call GPIO helpers with spinlock taken).
> >
> > There is also another significant issue: the GPIO device object contains
> > a pointer to gpio_chip which is the implementation of the GPIO provider.
> > This object can be removed at any point - as GPIOLIB officially supports
> > hotplugging with all the dynamic expanders that we provide drivers for -
> > and leave the GPIO API callbacks with a suddenly NULL pointer. This is
> > a problem that allowed user-space processes to easily crash the kernel
> > until we patched it with a read-write semaphore in the user-space facing
> > code (but the problem still exists for in-kernel users). This was
> > recognized before as evidenced by the implementation of validate_desc()
> > but without proper serialization, simple checking for a NULL pointer is
> > pointless and we do need a generic solution for that issue as well.
> >
> > If we want to get it right - the more lockless we go, the better. This is
> > why SRCU seems to be the right candidate for the mechanism to use. In fact
> > it's the only mechanism we can use our read-only critical sections to be
> > called from atomic and protecc contexts as well as call driver callbacks
> > that may sleep (for the latter case).
> >
> > We're going to use it in three places: to protect the global list of GPIO
> > devices, to ensure consistency when dereferencing the chip pointer in GPIO
> > device struct and finally to ensure that users can access GPIO descriptors
> > and always see a consistent state.
> >
> > We do NOT serialize all API callbacks. This means that provider callbacks
> > may be called simultaneously and GPIO drivers need to provide their own
> > locking if needed. This is on purpose. First: we only support exclusive
> > GPIO usage* so there's no risk of two drivers getting in each other's way
> > over the same GPIO. Second: with this series, we ensure enough consistency
> > to limit the chance of drivers or user-space users crashing the kernel.
> > With additional improvements in handling the flags field in GPIO
> > descriptors there's very little to gain, while bitbanging drivers may care
> > about the increased performance of going lockless.
> >
> > This series brings in one somewhat significant functional change for
> > in-kernel users, namely: GPIO API calls, for which the underlying GPIO
> > chip is gone, will no longer return 0 and emit a log message but instead
> > will return -ENODEV.
> >
> > I know this is a lot of code to go through but the more eyes we get on it
> > the better.
>
> I've noticed that this patchset landed in today's linux-next. It causes
> a lots of warning during boot on my test boards when LOCKDEP is enabled
> in kernel configs. Do you want me to report all of them? Some can be
> easily reproduced even with QEMU's virt ARM and ARM64 machines.
>

Marek,

Thanks for the report. I've already sent out patches that fix these
problems. Sorry for this.

Bartosz

[snip]

Re: [PATCH v3 00/24] gpio: rework locking and object life-time control

[Jiawen Wu]

Hi Bartosz Golaszewski,

I ran into a kernel crash problem when I pull the latest net-next.git, and
finally it was found that is caused by this patch series merged.

The kernel crashed because I got gpio=0 when I called irq_find_mapping()
and then struct irq_data *d=null, as my driver describes:

	int gpio = irq_find_mapping(gc->irq.domain, hwirq);
	struct irq_data *d = irq_get_irq_data(gpio);

	txgbe_gpio_irq_ack(d);

The deeper positioning is this line in __irq_resolve_mapping().

	data = rcu_dereference(domain->revmap[hwirq]);
		
So, is it the addition of SRCU infrastructure that causes this issue?


Thanks,
Jiawen

Re: [PATCH v3 00/24] gpio: rework locking and object life-time control

[Andy Shevchenko]

On Fri, Apr 19, 2024 at 03:03:54PM +0800, Jiawen Wu wrote:
> Hi Bartosz Golaszewski,
> 
> I ran into a kernel crash problem when I pull the latest net-next.git, and
> finally it was found that is caused by this patch series merged.

Can you bisect further, i.e. which patch (now a commit message) is the culprit?

> The kernel crashed because I got gpio=0 when I called irq_find_mapping()
> and then struct irq_data *d=null, as my driver describes:
> 
> 	int gpio = irq_find_mapping(gc->irq.domain, hwirq);
> 	struct irq_data *d = irq_get_irq_data(gpio);
> 
> 	txgbe_gpio_irq_ack(d);
> 
> The deeper positioning is this line in __irq_resolve_mapping().
> 
> 	data = rcu_dereference(domain->revmap[hwirq]);
> 		
> So, is it the addition of SRCU infrastructure that causes this issue?

Do you have a full traceback / Oops message to share?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 00/24] gpio: rework locking and object life-time control

[Bartosz Golaszewski]

On Fri, 19 Apr 2024 at 09:04, Jiawen Wu <jiawenwu@trustnetic.com> wrote:
>
> Hi Bartosz Golaszewski,
>
> I ran into a kernel crash problem when I pull the latest net-next.git, and
> finally it was found that is caused by this patch series merged.
>
> The kernel crashed because I got gpio=0 when I called irq_find_mapping()
> and then struct irq_data *d=null, as my driver describes:
>
>         int gpio = irq_find_mapping(gc->irq.domain, hwirq);
>         struct irq_data *d = irq_get_irq_data(gpio);
>
>         txgbe_gpio_irq_ack(d);
>
> The deeper positioning is this line in __irq_resolve_mapping().
>
>         data = rcu_dereference(domain->revmap[hwirq]);
>
> So, is it the addition of SRCU infrastructure that causes this issue?
>

This is irq-specific RCU that I did not add in the GPIO series. Please
provide us with more information. Bisect to the exact commit causing
the issue and post the kernel log (we don't know what kind of crash
you trigger and what the stack trace is).

Bart

RE: [PATCH v3 00/24] gpio: rework locking and object life-time control

[Jiawen Wu]

On Sat, April 20, 2024 5:29 AM, Bartosz Golaszewski wrote:
> On Fri, 19 Apr 2024 at 09:04, Jiawen Wu <jiawenwu@trustnetic.com> wrote:
> >
> > Hi Bartosz Golaszewski,
> >
> > I ran into a kernel crash problem when I pull the latest net-next.git, and
> > finally it was found that is caused by this patch series merged.
> >
> > The kernel crashed because I got gpio=0 when I called irq_find_mapping()
> > and then struct irq_data *d=null, as my driver describes:
> >
> >         int gpio = irq_find_mapping(gc->irq.domain, hwirq);
> >         struct irq_data *d = irq_get_irq_data(gpio);
> >
> >         txgbe_gpio_irq_ack(d);
> >
> > The deeper positioning is this line in __irq_resolve_mapping().
> >
> >         data = rcu_dereference(domain->revmap[hwirq]);
> >
> > So, is it the addition of SRCU infrastructure that causes this issue?
> >
> 
> This is irq-specific RCU that I did not add in the GPIO series. Please
> provide us with more information. Bisect to the exact commit causing
> the issue and post the kernel log (we don't know what kind of crash
> you trigger and what the stack trace is).
> 
> Bart
> 

Hi Bartosz & Andy,

Thanks for your replies.

I'm sorry for the misunderstanding, and glad this patch doesn't cause any
problems. I thought the issue was in this patch because of my mistake.
It's actually caused by other patches. :)