[PATCH v3] reset: core: Serialize concurrent deassert/reset of shared resets

[shengjiu . wang]

From: Shengjiu Wang <shengjiu.wang@xxxxxxx>

reset_control_deassert() and reset_control_reset() are documented to
guarantee that the reset line has been deasserted, respectively that a
reset pulse has been issued, by the time they return.

For a shared reset only the first caller runs the hardware op; the other
callers just bump the refcount and return. When the op can sleep this
breaks the guarantee: a second caller can observe the bumped refcount and
return before the first caller's op has finished, and then touch hardware
that is still in reset.

Add a reset_controller_dev::can_sleep flag that a provider sets when its
reset ops may sleep. When it is set, serialize the shared refcount update
and the hardware op in reset_control_reset() and reset_control_deassert()
under a new per-control mutex, taken inside the SRCU read-side critical
section. Providers that leave the flag clear keep the existing lockless
path, so callers in atomic context are unaffected.

Set can_sleep in the reset-gpio driver, which drives the line with
gpiod_set_value_cansleep() and may therefore sleep. Other providers whose
reset ops may sleep still need to opt in.

Fixes: 0b52297f2288 ("reset: Add support for shared reset controls")
Suggested-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
Assisted-by: Anthropic:claude-opus-4-8
Signed-off-by: Shengjiu Wang <shengjiu.wang@xxxxxxx>
Link: https://lore.kernel.org/all/aszDZlJ0KahNwgZO@xxxxxxxxxx/
---
Changes in v3:
- Drop the reset_control_reset_atomic()/reset_control_deassert_atomic()
  variants and their reset.h declarations/stubs; gate the mutex on
  rcdev->can_sleep instead, so non-sleeping controllers keep the lockless
  path and need no new API.
- Use a dedicated rstc->shared_lock taken inside the SRCU read-side section
  instead of reusing rstc->lock before SRCU.
- Set can_sleep in reset-gpio; no other provider is converted.
- Drop the lockdep_assert_not_held() additions and the reset.rst / kerneldoc
  text about sleeping-vs-atomic variants.

Changes in v2:
- Serialize only the shared may-sleep path under rstc->lock (taken before
  SRCU to match reset_control_acquire()'s order); exclusive path stays
  lockless, and reset_control_assert() is left unchanged.
- Add lockless reset_control_reset_atomic()/reset_control_deassert_atomic()
  for atomic callers; gate the mutex on can_sleep so no shared consumer is
  regressed and so it works on all configs.
- Factor the body into __reset_control_*()/__reset_control_*_locked()
  helpers, and lockdep_assert_not_held() at each lock site to catch a
  controller op re-entering the API on the same control.
- Declare the new variants and stubs in reset.h, and document the sleeping
  vs atomic variants in the kerneldoc and reset.rst.

 drivers/reset/core.c             | 125 +++++++++++++++++++++----------
 drivers/reset/reset-gpio.c       |   1 +
 include/linux/reset-controller.h |   3 +
 3 files changed, 89 insertions(+), 40 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 38e189d04d09..dd6e47163f6c 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -52,6 +52,9 @@ static DEFINE_IDA(reset_gpio_ida);
  *                   only used for shared resets, which means that the value
  *                   will be either 0 or 1.
  * @lock: serializes the internals of reset_control_acquire()
+ * @shared_lock: serializes the shared refcount update and the hardware op for
+ *               controllers whose ops may sleep (rcdev->can_sleep); taken only
+ *               inside the SRCU read-side critical section
  */
 struct reset_control {
 	struct reset_controller_dev __rcu *rcdev;
@@ -65,6 +68,8 @@ struct reset_control {
 	atomic_t deassert_count;
 	atomic_t triggered_count;
 	struct mutex lock;
+	/* serializes the shared refcount update with its hardware op */
+	struct mutex shared_lock;
 };
 
 /**
@@ -355,6 +360,32 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
 	return rstc->array;
 }
 
+static int __reset_control_reset(struct reset_control *rstc,
+				 struct reset_controller_dev *rcdev)
+{
+	int ret;
+
+	if (!rcdev->ops->reset)
+		return -ENOTSUPP;
+
+	if (rstc->shared) {
+		if (WARN_ON(atomic_read(&rstc->deassert_count) != 0))
+			return -EINVAL;
+
+		if (atomic_inc_return(&rstc->triggered_count) != 1)
+			return 0;
+	} else {
+		if (!rstc->acquired)
+			return -EPERM;
+	}
+
+	ret = rcdev->ops->reset(rcdev, rstc->id);
+	if (rstc->shared && ret)
+		atomic_dec(&rstc->triggered_count);
+
+	return ret;
+}
+
 /**
  * reset_control_reset - reset the controlled device
  * @rstc: reset controller
@@ -362,6 +393,9 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
  * On a shared reset line the actual reset pulse is only triggered once for the
  * lifetime of the reset_control instance: for all but the first caller this is
  * a no-op.
+ *
+ * After calling this function, the reset is guaranteed to have seen a reset
+ * pulse at any point in the past, not necessarily during the function call.
  * Consumers must not use reset_control_(de)assert on shared reset lines when
  * reset_control_reset has been used.
  *
@@ -371,7 +405,6 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
 int reset_control_reset(struct reset_control *rstc)
 {
 	struct reset_controller_dev *rcdev;
-	int ret;
 
 	if (!rstc)
 		return 0;
@@ -388,25 +421,18 @@ int reset_control_reset(struct reset_control *rstc)
 	if (!rcdev)
 		return -ENODEV;
 
-	if (!rcdev->ops->reset)
-		return -ENOTSUPP;
-
-	if (rstc->shared) {
-		if (WARN_ON(atomic_read(&rstc->deassert_count) != 0))
-			return -EINVAL;
-
-		if (atomic_inc_return(&rstc->triggered_count) != 1)
-			return 0;
-	} else {
-		if (!rstc->acquired)
-			return -EPERM;
+	/*
+	 * For a shared reset only the first caller runs the hardware op while
+	 * concurrent callers bump the refcount and return. If the op may sleep
+	 * (rcdev->can_sleep), serialize the refcount update and the op so a
+	 * concurrent caller cannot return before the op has finished.
+	 */
+	if (rstc->shared && rcdev->can_sleep) {
+		guard(mutex)(&rstc->shared_lock);
+		return __reset_control_reset(rstc, rcdev);
 	}
 
-	ret = rcdev->ops->reset(rcdev, rstc->id);
-	if (rstc->shared && ret)
-		atomic_dec(&rstc->triggered_count);
-
-	return ret;
+	return __reset_control_reset(rstc, rcdev);
 }
 EXPORT_SYMBOL_GPL(reset_control_reset);
 
@@ -575,6 +601,36 @@ int reset_control_bulk_assert(int num_rstcs,
 }
 EXPORT_SYMBOL_GPL(reset_control_bulk_assert);
 
+static int __reset_control_deassert(struct reset_control *rstc,
+				    struct reset_controller_dev *rcdev)
+{
+	if (rstc->shared) {
+		if (WARN_ON(atomic_read(&rstc->triggered_count) != 0))
+			return -EINVAL;
+
+		if (atomic_inc_return(&rstc->deassert_count) != 1)
+			return 0;
+	} else {
+		if (!rstc->acquired) {
+			WARN(1, "reset %s (ID: %u) is not acquired\n",
+			     rcdev_name(rcdev), rstc->id);
+			return -EPERM;
+		}
+	}
+
+	/*
+	 * If the reset controller does not implement .deassert(), we assume
+	 * that it handles self-deasserting reset lines via .reset(). In that
+	 * case, the reset lines are deasserted by default. If that is not the
+	 * case, the reset controller driver should implement .deassert() and
+	 * return -ENOTSUPP.
+	 */
+	if (!rcdev->ops->deassert)
+		return 0;
+
+	return rcdev->ops->deassert(rcdev, rstc->id);
+}
+
 /**
  * reset_control_deassert - deasserts the reset line
  * @rstc: reset controller
@@ -605,31 +661,18 @@ int reset_control_deassert(struct reset_control *rstc)
 	if (!rcdev)
 		return -ENODEV;
 
-	if (rstc->shared) {
-		if (WARN_ON(atomic_read(&rstc->triggered_count) != 0))
-			return -EINVAL;
-
-		if (atomic_inc_return(&rstc->deassert_count) != 1)
-			return 0;
-	} else {
-		if (!rstc->acquired) {
-			WARN(1, "reset %s (ID: %u) is not acquired\n",
-			     rcdev_name(rcdev), rstc->id);
-			return -EPERM;
-		}
-	}
-
 	/*
-	 * If the reset controller does not implement .deassert(), we assume
-	 * that it handles self-deasserting reset lines via .reset(). In that
-	 * case, the reset lines are deasserted by default. If that is not the
-	 * case, the reset controller driver should implement .deassert() and
-	 * return -ENOTSUPP.
+	 * For a shared reset only the first caller runs the hardware op while
+	 * concurrent callers bump the refcount and return. If the op may sleep
+	 * (rcdev->can_sleep), serialize the refcount update and the op so a
+	 * concurrent caller cannot return before the op has finished.
 	 */
-	if (!rcdev->ops->deassert)
-		return 0;
+	if (rstc->shared && rcdev->can_sleep) {
+		guard(mutex)(&rstc->shared_lock);
+		return __reset_control_deassert(rstc, rcdev);
+	}
 
-	return rcdev->ops->deassert(rcdev, rstc->id);
+	return __reset_control_deassert(rstc, rcdev);
 }
 EXPORT_SYMBOL_GPL(reset_control_deassert);
 
@@ -879,6 +922,7 @@ __reset_control_get_internal(struct reset_controller_dev *rcdev,
 	rstc->id = index;
 	kref_init(&rstc->refcnt);
 	mutex_init(&rstc->lock);
+	mutex_init(&rstc->shared_lock);
 	rstc->acquired = acquired;
 	rstc->shared = shared;
 	get_device(rcdev->dev);
@@ -901,6 +945,7 @@ static void __reset_control_release(struct kref *kref)
 	}
 
 	mutex_destroy(&rstc->lock);
+	mutex_destroy(&rstc->shared_lock);
 }
 
 static void reset_control_put_internal(struct reset_control *rstc)
diff --git a/drivers/reset/reset-gpio.c b/drivers/reset/reset-gpio.c
index bf95296a512e..b4a198792107 100644
--- a/drivers/reset/reset-gpio.c
+++ b/drivers/reset/reset-gpio.c
@@ -67,6 +67,7 @@ static int reset_gpio_probe(struct auxiliary_device *adev,
 				     "Could not get reset gpios\n");
 
 	priv->rc.ops = &reset_gpio_ops;
+	priv->rc.can_sleep = true;
 	priv->rc.owner = THIS_MODULE;
 	priv->rc.dev = dev;
 
diff --git a/include/linux/reset-controller.h b/include/linux/reset-controller.h
index 52a5a4e81f18..7318c07b80e1 100644
--- a/include/linux/reset-controller.h
+++ b/include/linux/reset-controller.h
@@ -47,6 +47,8 @@ struct of_phandle_args;
  *                id as given to the reset control ops, defaults to
  *                :c:func:`fwnode_reset_simple_xlate`
  * @nr_resets: number of reset controls in this reset controller device
+ * @can_sleep: set by the provider if its reset ops may sleep; makes the core
+ *             serialize the shared reset and deassert paths under a mutex
  * @lock: protects the reset control list from concurrent access
  */
 struct reset_controller_dev {
@@ -64,6 +66,7 @@ struct reset_controller_dev {
 	int (*fwnode_xlate)(struct reset_controller_dev *rcdev,
 			    const struct fwnode_reference_args *reset_spec);
 	unsigned int nr_resets;
+	bool can_sleep;
 	struct mutex lock;
 };
 
-- 
2.34.1