[PATCH] reset: use a shared SRCU domain for reset controls

[PATCH] reset: use a shared SRCU domain for reset controls

[Steven Price]

Commit 78ebbff6d1a0 ("reset: handle removing supplier before consumers")
added a dynamically initialized srcu_struct to every reset_control and
cleaned it up again when the handle was dropped.

That breaks early boot users which acquire and release reset handles
before workqueues are online. On rk3288 this shows up during
rockchip_smp_prepare_cpus(), where pmu_set_power_domain() gets a reset
control for a CPU core and then drops it again before SMP bring-up has
finished. cleanup_srcu_struct() then tries to flush delayed SRCU work
and hits the WARN_ON(!wq_online) path, which can leave the machine
hanging before the serial console appears.

Keep the supplier-removal protection, but move it to a single shared
static SRCU domain for the reset core. That preserves the rcdev lifetime
protection needed for supplier unregister without requiring per-handle
init_srcu_struct()/cleanup_srcu_struct() on normal get/put paths.

Fixes: 78ebbff6d1a0 ("reset: handle removing supplier before consumers")
---
 drivers/reset/core.c | 48 ++++++++++++++++++--------------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 38e189d04d09..ab4989084824 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -29,6 +29,11 @@
 
 static DEFINE_MUTEX(reset_list_mutex);
 static LIST_HEAD(reset_controller_list);
+/*
+ * Use one shared SRCU domain so reset handles don't need per-instance
+ * init/cleanup on early boot paths.
+ */
+DEFINE_STATIC_SRCU(reset_control_srcu);
 
 /* Protects reset_gpio_lookup_list */
 static DEFINE_MUTEX(reset_gpio_lookup_mutex);
@@ -39,7 +44,6 @@ static DEFINE_IDA(reset_gpio_ida);
  * struct reset_control - a reset control
  * @rcdev: a pointer to the reset controller device
  *         this reset control belongs to
- * @srcu: protects the rcdev pointer from removal during consumer access
  * @list: list entry for the rcdev's reset controller list
  * @id: ID of the reset controller in the reset
  *      controller device
@@ -55,7 +59,6 @@ static DEFINE_IDA(reset_gpio_ida);
  */
 struct reset_control {
 	struct reset_controller_dev __rcu *rcdev;
-	struct srcu_struct srcu;
 	struct list_head list;
 	unsigned int id;
 	struct kref refcnt;
@@ -188,7 +191,7 @@ void reset_controller_unregister(struct reset_controller_dev *rcdev)
 		 */
 		list_for_each_entry_safe(rstc, pos, &rcdev->reset_control_head, list) {
 			rcu_assign_pointer(rstc->rcdev, NULL);
-			synchronize_srcu(&rstc->srcu);
+			synchronize_srcu(&reset_control_srcu);
 			reset_controller_remove(rcdev, rstc);
 		}
 	}
@@ -382,9 +385,9 @@ int reset_control_reset(struct reset_control *rstc)
 	if (reset_control_is_array(rstc))
 		return reset_control_array_reset(rstc_to_array(rstc));
 
-	guard(srcu)(&rstc->srcu);
+	guard(srcu)(&reset_control_srcu);
 
-	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
+	rcdev = srcu_dereference(rstc->rcdev, &reset_control_srcu);
 	if (!rcdev)
 		return -ENODEV;
 
@@ -503,9 +506,9 @@ int reset_control_assert(struct reset_control *rstc)
 	if (reset_control_is_array(rstc))
 		return reset_control_array_assert(rstc_to_array(rstc));
 
-	guard(srcu)(&rstc->srcu);
+	guard(srcu)(&reset_control_srcu);
 
-	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
+	rcdev = srcu_dereference(rstc->rcdev, &reset_control_srcu);
 	if (!rcdev)
 		return -ENODEV;
 
@@ -599,9 +602,9 @@ int reset_control_deassert(struct reset_control *rstc)
 	if (reset_control_is_array(rstc))
 		return reset_control_array_deassert(rstc_to_array(rstc));
 
-	guard(srcu)(&rstc->srcu);
+	guard(srcu)(&reset_control_srcu);
 
-	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
+	rcdev = srcu_dereference(rstc->rcdev, &reset_control_srcu);
 	if (!rcdev)
 		return -ENODEV;
 
@@ -679,9 +682,9 @@ int reset_control_status(struct reset_control *rstc)
 	if (WARN_ON(IS_ERR(rstc)) || reset_control_is_array(rstc))
 		return -EINVAL;
 
-	guard(srcu)(&rstc->srcu);
+	guard(srcu)(&reset_control_srcu);
 
-	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
+	rcdev = srcu_dereference(rstc->rcdev, &reset_control_srcu);
 	if (!rcdev)
 		return -ENODEV;
 
@@ -731,9 +734,9 @@ int reset_control_acquire(struct reset_control *rstc)
 	if (rstc->acquired)
 		return 0;
 
-	guard(srcu)(&rstc->srcu);
+	guard(srcu)(&reset_control_srcu);
 
-	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
+	rcdev = srcu_dereference(rstc->rcdev, &reset_control_srcu);
 	if (!rcdev)
 		return -ENODEV;
 
@@ -831,7 +834,6 @@ __reset_control_get_internal(struct reset_controller_dev *rcdev,
 	bool shared = flags & RESET_CONTROL_FLAGS_BIT_SHARED;
 	bool acquired = flags & RESET_CONTROL_FLAGS_BIT_ACQUIRED;
 	struct reset_control *rstc;
-	int ret;
 
 	lockdep_assert_held(&rcdev->lock);
 
@@ -862,14 +864,7 @@ __reset_control_get_internal(struct reset_controller_dev *rcdev,
 	if (!rstc)
 		return ERR_PTR(-ENOMEM);
 
-	ret = init_srcu_struct(&rstc->srcu);
-	if (ret) {
-		kfree(rstc);
-		return ERR_PTR(ret);
-	}
-
 	if (!try_module_get(rcdev->owner)) {
-		cleanup_srcu_struct(&rstc->srcu);
 		kfree(rstc);
 		return ERR_PTR(-ENODEV);
 	}
@@ -892,7 +887,7 @@ static void __reset_control_release(struct kref *kref)
 						  refcnt);
 	struct reset_controller_dev *rcdev;
 
-	lockdep_assert_held(&rstc->srcu);
+	lockdep_assert_held(&reset_control_srcu);
 
 	rcdev = rcu_replace_pointer(rstc->rcdev, NULL, true);
 	if (rcdev) {
@@ -911,8 +906,8 @@ static void reset_control_put_internal(struct reset_control *rstc)
 	if (IS_ERR_OR_NULL(rstc))
 		return;
 
-	scoped_guard(srcu, &rstc->srcu) {
-		rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
+	scoped_guard(srcu, &reset_control_srcu) {
+		rcdev = srcu_dereference(rstc->rcdev, &reset_control_srcu);
 		if (!rcdev)
 			/* Already released. */
 			return;
@@ -921,11 +916,8 @@ static void reset_control_put_internal(struct reset_control *rstc)
 		ret = kref_put(&rstc->refcnt, __reset_control_release);
 	}
 
-	if (ret) {
-		synchronize_srcu(&rstc->srcu);
-		cleanup_srcu_struct(&rstc->srcu);
+	if (ret)
 		kfree(rstc);
-	}
 }
 
 static void reset_gpio_aux_device_release(struct device *dev)
-- 
2.39.5

Re: [PATCH] reset: use a shared SRCU domain for reset controls

[Steven Price]

On 17/04/2026 16:48, Steven Price wrote:
> Commit 78ebbff6d1a0 ("reset: handle removing supplier before consumers")
> added a dynamically initialized srcu_struct to every reset_control and
> cleaned it up again when the handle was dropped.
> 
> That breaks early boot users which acquire and release reset handles
> before workqueues are online. On rk3288 this shows up during
> rockchip_smp_prepare_cpus(), where pmu_set_power_domain() gets a reset
> control for a CPU core and then drops it again before SMP bring-up has
> finished. cleanup_srcu_struct() then tries to flush delayed SRCU work
> and hits the WARN_ON(!wq_online) path, which can leave the machine
> hanging before the serial console appears.
> 
> Keep the supplier-removal protection, but move it to a single shared
> static SRCU domain for the reset core. That preserves the rcdev lifetime
> protection needed for supplier unregister without requiring per-handle
> init_srcu_struct()/cleanup_srcu_struct() on normal get/put paths.
> 
> Fixes: 78ebbff6d1a0 ("reset: handle removing supplier before consumers")

Whoops, I forgot by SoB:

Signed-off-by: Steven Price <steven.price@arm.com>

Note I'm not very familiar with this region of code, so let me know if
I'm fixing this in the wrong way. This is a regression for the RK3288
Firefly board I'm using so I'm keen to get some form of fix in.

Thanks,
Steve

> ---
>  drivers/reset/core.c | 48 ++++++++++++++++++--------------------------
>  1 file changed, 20 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 38e189d04d09..ab4989084824 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -29,6 +29,11 @@
>  
>  static DEFINE_MUTEX(reset_list_mutex);
>  static LIST_HEAD(reset_controller_list);
> +/*
> + * Use one shared SRCU domain so reset handles don't need per-instance
> + * init/cleanup on early boot paths.
> + */
> +DEFINE_STATIC_SRCU(reset_control_srcu);
>  
>  /* Protects reset_gpio_lookup_list */
>  static DEFINE_MUTEX(reset_gpio_lookup_mutex);
> @@ -39,7 +44,6 @@ static DEFINE_IDA(reset_gpio_ida);
>   * struct reset_control - a reset control
>   * @rcdev: a pointer to the reset controller device
>   *         this reset control belongs to
> - * @srcu: protects the rcdev pointer from removal during consumer access
>   * @list: list entry for the rcdev's reset controller list
>   * @id: ID of the reset controller in the reset
>   *      controller device
> @@ -55,7 +59,6 @@ static DEFINE_IDA(reset_gpio_ida);
>   */
>  struct reset_control {
>  	struct reset_controller_dev __rcu *rcdev;
> -	struct srcu_struct srcu;
>  	struct list_head list;
>  	unsigned int id;
>  	struct kref refcnt;
> @@ -188,7 +191,7 @@ void reset_controller_unregister(struct reset_controller_dev *rcdev)
>  		 */
>  		list_for_each_entry_safe(rstc, pos, &rcdev->reset_control_head, list) {
>  			rcu_assign_pointer(rstc->rcdev, NULL);
> -			synchronize_srcu(&rstc->srcu);
> +			synchronize_srcu(&reset_control_srcu);
>  			reset_controller_remove(rcdev, rstc);
>  		}
>  	}
> @@ -382,9 +385,9 @@ int reset_control_reset(struct reset_control *rstc)
>  	if (reset_control_is_array(rstc))
>  		return reset_control_array_reset(rstc_to_array(rstc));
>  
> -	guard(srcu)(&rstc->srcu);
> +	guard(srcu)(&reset_control_srcu);
>  
> -	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> +	rcdev = srcu_dereference(rstc->rcdev, &reset_control_srcu);
>  	if (!rcdev)
>  		return -ENODEV;
>  
> @@ -503,9 +506,9 @@ int reset_control_assert(struct reset_control *rstc)
>  	if (reset_control_is_array(rstc))
>  		return reset_control_array_assert(rstc_to_array(rstc));
>  
> -	guard(srcu)(&rstc->srcu);
> +	guard(srcu)(&reset_control_srcu);
>  
> -	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> +	rcdev = srcu_dereference(rstc->rcdev, &reset_control_srcu);
>  	if (!rcdev)
>  		return -ENODEV;
>  
> @@ -599,9 +602,9 @@ int reset_control_deassert(struct reset_control *rstc)
>  	if (reset_control_is_array(rstc))
>  		return reset_control_array_deassert(rstc_to_array(rstc));
>  
> -	guard(srcu)(&rstc->srcu);
> +	guard(srcu)(&reset_control_srcu);
>  
> -	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> +	rcdev = srcu_dereference(rstc->rcdev, &reset_control_srcu);
>  	if (!rcdev)
>  		return -ENODEV;
>  
> @@ -679,9 +682,9 @@ int reset_control_status(struct reset_control *rstc)
>  	if (WARN_ON(IS_ERR(rstc)) || reset_control_is_array(rstc))
>  		return -EINVAL;
>  
> -	guard(srcu)(&rstc->srcu);
> +	guard(srcu)(&reset_control_srcu);
>  
> -	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> +	rcdev = srcu_dereference(rstc->rcdev, &reset_control_srcu);
>  	if (!rcdev)
>  		return -ENODEV;
>  
> @@ -731,9 +734,9 @@ int reset_control_acquire(struct reset_control *rstc)
>  	if (rstc->acquired)
>  		return 0;
>  
> -	guard(srcu)(&rstc->srcu);
> +	guard(srcu)(&reset_control_srcu);
>  
> -	rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> +	rcdev = srcu_dereference(rstc->rcdev, &reset_control_srcu);
>  	if (!rcdev)
>  		return -ENODEV;
>  
> @@ -831,7 +834,6 @@ __reset_control_get_internal(struct reset_controller_dev *rcdev,
>  	bool shared = flags & RESET_CONTROL_FLAGS_BIT_SHARED;
>  	bool acquired = flags & RESET_CONTROL_FLAGS_BIT_ACQUIRED;
>  	struct reset_control *rstc;
> -	int ret;
>  
>  	lockdep_assert_held(&rcdev->lock);
>  
> @@ -862,14 +864,7 @@ __reset_control_get_internal(struct reset_controller_dev *rcdev,
>  	if (!rstc)
>  		return ERR_PTR(-ENOMEM);
>  
> -	ret = init_srcu_struct(&rstc->srcu);
> -	if (ret) {
> -		kfree(rstc);
> -		return ERR_PTR(ret);
> -	}
> -
>  	if (!try_module_get(rcdev->owner)) {
> -		cleanup_srcu_struct(&rstc->srcu);
>  		kfree(rstc);
>  		return ERR_PTR(-ENODEV);
>  	}
> @@ -892,7 +887,7 @@ static void __reset_control_release(struct kref *kref)
>  						  refcnt);
>  	struct reset_controller_dev *rcdev;
>  
> -	lockdep_assert_held(&rstc->srcu);
> +	lockdep_assert_held(&reset_control_srcu);
>  
>  	rcdev = rcu_replace_pointer(rstc->rcdev, NULL, true);
>  	if (rcdev) {
> @@ -911,8 +906,8 @@ static void reset_control_put_internal(struct reset_control *rstc)
>  	if (IS_ERR_OR_NULL(rstc))
>  		return;
>  
> -	scoped_guard(srcu, &rstc->srcu) {
> -		rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
> +	scoped_guard(srcu, &reset_control_srcu) {
> +		rcdev = srcu_dereference(rstc->rcdev, &reset_control_srcu);
>  		if (!rcdev)
>  			/* Already released. */
>  			return;
> @@ -921,11 +916,8 @@ static void reset_control_put_internal(struct reset_control *rstc)
>  		ret = kref_put(&rstc->refcnt, __reset_control_release);
>  	}
>  
> -	if (ret) {
> -		synchronize_srcu(&rstc->srcu);
> -		cleanup_srcu_struct(&rstc->srcu);
> +	if (ret)
>  		kfree(rstc);
> -	}
>  }
>  
>  static void reset_gpio_aux_device_release(struct device *dev)

Re: [PATCH] reset: use a shared SRCU domain for reset controls

[Philipp Zabel]

Hi Steven,

On Fr, 2026-04-17 at 16:48 +0100, Steven Price wrote:
> Commit 78ebbff6d1a0 ("reset: handle removing supplier before consumers")
> added a dynamically initialized srcu_struct to every reset_control and
> cleaned it up again when the handle was dropped.
> 
> That breaks early boot users which acquire and release reset handles
> before workqueues are online. On rk3288 this shows up during
> rockchip_smp_prepare_cpus(), where pmu_set_power_domain() gets a reset
> control for a CPU core and then drops it again before SMP bring-up has
> finished.

Can the reset_control_put() call be dropped from pmu_set_power_domain()
to fix the problem?

Putting the reset control should mean that the driver doesn't care
about the state of the reset line anymore, but the platsmp code very
much expects the reset line to stay deasserted after enabling a CPU.
Acquiring reset controls in rockchip_smp_prepare_cpus() once and never
giving them up via reset_control_put() seems like a correct fix,
regardless of whether this patch is applied or not.

It looks like the meson platsmp suffers from the same issue.

> cleanup_srcu_struct() then tries to flush delayed SRCU work
> and hits the WARN_ON(!wq_online) path, which can leave the machine
> hanging before the serial console appears.
> 
> Keep the supplier-removal protection, but move it to a single shared
> static SRCU domain for the reset core. That preserves the rcdev lifetime
> protection needed for supplier unregister without requiring per-handle
> init_srcu_struct()/cleanup_srcu_struct() on normal get/put paths.

I'd prefer to document the workqueue requirement and keep the SRCU
domain per reset_control, if possible.

regards
Philipp

Re: [PATCH] reset: use a shared SRCU domain for reset controls

[Steven Price]

+Heiko for the Rockchip questions.

On 23/04/2026 11:27, Philipp Zabel wrote:
> Hi Steven,
> 
> On Fr, 2026-04-17 at 16:48 +0100, Steven Price wrote:
>> Commit 78ebbff6d1a0 ("reset: handle removing supplier before consumers")
>> added a dynamically initialized srcu_struct to every reset_control and
>> cleaned it up again when the handle was dropped.
>>
>> That breaks early boot users which acquire and release reset handles
>> before workqueues are online. On rk3288 this shows up during
>> rockchip_smp_prepare_cpus(), where pmu_set_power_domain() gets a reset
>> control for a CPU core and then drops it again before SMP bring-up has
>> finished.
> 
> Can the reset_control_put() call be dropped from pmu_set_power_domain()
> to fix the problem?

I'm not that familiar with the code, so I'm not sure.

Just dropping that call causes a WARN_ON() bringing the secondary CPUs
on (because the call to rockchip_get_core_reset() expects to have
exclusive access to the reset). Switching to a shared reset then his a
WARN_ON() in reset_control_assert because deassert_count == 0. I could
keep digging blindly but I'm not really sure how this code is meant to work.

Hopefully Heiko might be able to shed some more light on this?

> Putting the reset control should mean that the driver doesn't care
> about the state of the reset line anymore, but the platsmp code very
> much expects the reset line to stay deasserted after enabling a CPU.
> Acquiring reset controls in rockchip_smp_prepare_cpus() once and never
> giving them up via reset_control_put() seems like a correct fix,
> regardless of whether this patch is applied or not.
> 
> It looks like the meson platsmp suffers from the same issue.

This is why I did the fix in the reset code - how many other platforms
might have similar issues? But obviously if these platforms are buggy
then they should be fixed.

My interest is keeping the devboard working so I can keep testing
Panfrost on it.

Thanks,
Steve

>> cleanup_srcu_struct() then tries to flush delayed SRCU work
>> and hits the WARN_ON(!wq_online) path, which can leave the machine
>> hanging before the serial console appears.
>>
>> Keep the supplier-removal protection, but move it to a single shared
>> static SRCU domain for the reset core. That preserves the rcdev lifetime
>> protection needed for supplier unregister without requiring per-handle
>> init_srcu_struct()/cleanup_srcu_struct() on normal get/put paths.
> 
> I'd prefer to document the workqueue requirement and keep the SRCU
> domain per reset_control, if possible.
> 
> regards
> Philipp

Re: [PATCH] reset: use a shared SRCU domain for reset controls

[Philipp Zabel]

Hi Steven, Heiko,

On Do, 2026-04-23 at 13:45 +0100, Steven Price wrote:
> +Heiko for the Rockchip questions.
> 
> On 23/04/2026 11:27, Philipp Zabel wrote:
> > Hi Steven,
> > 
> > On Fr, 2026-04-17 at 16:48 +0100, Steven Price wrote:
> > > Commit 78ebbff6d1a0 ("reset: handle removing supplier before consumers")
> > > added a dynamically initialized srcu_struct to every reset_control and
> > > cleaned it up again when the handle was dropped.
> > > 
> > > That breaks early boot users which acquire and release reset handles
> > > before workqueues are online. On rk3288 this shows up during
> > > rockchip_smp_prepare_cpus(), where pmu_set_power_domain() gets a reset
> > > control for a CPU core and then drops it again before SMP bring-up has
> > > finished.
> > 
> > Can the reset_control_put() call be dropped from pmu_set_power_domain()
> > to fix the problem?
> 
> I'm not that familiar with the code, so I'm not sure.
> 
> Just dropping that call causes a WARN_ON() bringing the secondary CPUs
> on (because the call to rockchip_get_core_reset() expects to have
> exclusive access to the reset).

Yes, caused by re-requesting the same reset again. I was thinking of
storing the controls in an array in rockchip_smp_prepare_cpus() and
reusing them in pmu_set_power_domain(), see the patch below.

> Switching to a shared reset then his a
> WARN_ON() in reset_control_assert because deassert_count == 0. I could
> keep digging blindly but I'm not really sure how this code is meant to work.

Shared reset is not the right mechanism here, that would be for
multiple drivers/devices sharing the same reset line.

> Hopefully Heiko might be able to shed some more light on this?
>
> > Putting the reset control should mean that the driver doesn't care
> > about the state of the reset line anymore, but the platsmp code very
> > much expects the reset line to stay deasserted after enabling a CPU.
> > Acquiring reset controls in rockchip_smp_prepare_cpus() once and never
> > giving them up via reset_control_put() seems like a correct fix,
> > regardless of whether this patch is applied or not.
> > 
> > It looks like the meson platsmp suffers from the same issue.
> 
> This is why I did the fix in the reset code -

And I'm grateful, because that made sashiko.dev point out an ABBA
deadlock opportunity in the existing code.

> how many other platforms
> might have similar issues? But obviously if these platforms are buggy
> then they should be fixed.

Agreed.

> My interest is keeping the devboard working so I can keep testing
> Panfrost on it.

Does this patch work?


----------8<----------
From: Philipp Zabel <p.zabel@pengutronix.de>
Subject: [PATCH] ARM: rockchip: keep reset control around

Do not put the reset control, retain exclusive control over it.
After turning on a CPU, the corresponding reset line must stay
deasserted.

This also avoids calling reset_control_put() before workqueues
are operational.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 arch/arm/mach-rockchip/platsmp.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c
index f432d22bfed8..c28816fffce8 100644
--- a/arch/arm/mach-rockchip/platsmp.c
+++ b/arch/arm/mach-rockchip/platsmp.c
@@ -34,6 +34,7 @@ static int ncores;
 
 static struct regmap *pmu;
 static int has_pmu = true;
+static struct reset_control *cpu_rstc[4];
 
 static int pmu_power_domain_is_on(int pd)
 {
@@ -64,9 +65,11 @@ static struct reset_control *rockchip_get_core_reset(int cpu)
 static int pmu_set_power_domain(int pd, bool on)
 {
 	u32 val = (on) ? 0 : BIT(pd);
-	struct reset_control *rstc = rockchip_get_core_reset(pd);
+	struct reset_control *rstc;
 	int ret;
 
+	rstc = pd < ARRAY_SIZE(cpu_rstc) ? cpu_rstc[pd] : ERR_PTR(-EINVAL);
+
 	if (IS_ERR(rstc) && read_cpuid_part() != ARM_CPU_PART_CORTEX_A9) {
 		pr_err("%s: could not get reset control for core %d\n",
 		       __func__, pd);
@@ -100,11 +103,8 @@ static int pmu_set_power_domain(int pd, bool on)
 		}
 	}
 
-	if (!IS_ERR(rstc)) {
-		if (on)
-			reset_control_deassert(rstc);
-		reset_control_put(rstc);
-	}
+	if (!IS_ERR(rstc) && on)
+		reset_control_deassert(rstc);
 
 	return 0;
 }
@@ -312,6 +312,9 @@ static void __init rockchip_smp_prepare_cpus(unsigned int max_cpus)
 		ncores = ((l2ctlr >> 24) & 0x3) + 1;
 	}
 
+	for (i = 0; i < ncores; i++)
+		cpu_rstc[i] = rockchip_get_core_reset(i);
+
 	/* Make sure that all cores except the first are really off */
 	for (i = 1; i < ncores; i++)
 		pmu_set_power_domain(0 + i, false);
-- 
2.47.3
---------->8----------

regards
Philipp

Re: [PATCH] reset: use a shared SRCU domain for reset controls

[Steven Price]

On 23/04/2026 15:15, Philipp Zabel wrote:
> Hi Steven, Heiko,
> 
> On Do, 2026-04-23 at 13:45 +0100, Steven Price wrote:
>> +Heiko for the Rockchip questions.
>>
>> On 23/04/2026 11:27, Philipp Zabel wrote:
>>> Hi Steven,
>>>
>>> On Fr, 2026-04-17 at 16:48 +0100, Steven Price wrote:
>>>> Commit 78ebbff6d1a0 ("reset: handle removing supplier before consumers")
>>>> added a dynamically initialized srcu_struct to every reset_control and
>>>> cleaned it up again when the handle was dropped.
>>>>
>>>> That breaks early boot users which acquire and release reset handles
>>>> before workqueues are online. On rk3288 this shows up during
>>>> rockchip_smp_prepare_cpus(), where pmu_set_power_domain() gets a reset
>>>> control for a CPU core and then drops it again before SMP bring-up has
>>>> finished.
>>>
>>> Can the reset_control_put() call be dropped from pmu_set_power_domain()
>>> to fix the problem?
>>
>> I'm not that familiar with the code, so I'm not sure.
>>
>> Just dropping that call causes a WARN_ON() bringing the secondary CPUs
>> on (because the call to rockchip_get_core_reset() expects to have
>> exclusive access to the reset).
> 
> Yes, caused by re-requesting the same reset again. I was thinking of
> storing the controls in an array in rockchip_smp_prepare_cpus() and
> reusing them in pmu_set_power_domain(), see the patch below.
> 
>> Switching to a shared reset then his a
>> WARN_ON() in reset_control_assert because deassert_count == 0. I could
>> keep digging blindly but I'm not really sure how this code is meant to work.
> 
> Shared reset is not the right mechanism here, that would be for
> multiple drivers/devices sharing the same reset line.

That makes sense - I hadn't really looked into whether this was a single
reset for all CPUs or somehow one shared between them all. Thinking
about it more I can't see how a shared reset would have worked ;)

>> Hopefully Heiko might be able to shed some more light on this?
>>
>>> Putting the reset control should mean that the driver doesn't care
>>> about the state of the reset line anymore, but the platsmp code very
>>> much expects the reset line to stay deasserted after enabling a CPU.
>>> Acquiring reset controls in rockchip_smp_prepare_cpus() once and never
>>> giving them up via reset_control_put() seems like a correct fix,
>>> regardless of whether this patch is applied or not.
>>>
>>> It looks like the meson platsmp suffers from the same issue.
>>
>> This is why I did the fix in the reset code -
> 
> And I'm grateful, because that made sashiko.dev point out an ABBA
> deadlock opportunity in the existing code.

Cool.

>> how many other platforms
>> might have similar issues? But obviously if these platforms are buggy
>> then they should be fixed.
> 
> Agreed.
> 
>> My interest is keeping the devboard working so I can keep testing
>> Panfrost on it.
> 
> Does this patch work?

Indeed it does - thanks. Feel free to add my T-b when posting:

Tested-by: Steven Price <steven.price@arm.com>

Thanks,
Steve

> 
> ----------8<----------
> From: Philipp Zabel <p.zabel@pengutronix.de>
> Subject: [PATCH] ARM: rockchip: keep reset control around
> 
> Do not put the reset control, retain exclusive control over it.
> After turning on a CPU, the corresponding reset line must stay
> deasserted.
> 
> This also avoids calling reset_control_put() before workqueues
> are operational.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  arch/arm/mach-rockchip/platsmp.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c
> index f432d22bfed8..c28816fffce8 100644
> --- a/arch/arm/mach-rockchip/platsmp.c
> +++ b/arch/arm/mach-rockchip/platsmp.c
> @@ -34,6 +34,7 @@ static int ncores;
>  
>  static struct regmap *pmu;
>  static int has_pmu = true;
> +static struct reset_control *cpu_rstc[4];
>  
>  static int pmu_power_domain_is_on(int pd)
>  {
> @@ -64,9 +65,11 @@ static struct reset_control *rockchip_get_core_reset(int cpu)
>  static int pmu_set_power_domain(int pd, bool on)
>  {
>  	u32 val = (on) ? 0 : BIT(pd);
> -	struct reset_control *rstc = rockchip_get_core_reset(pd);
> +	struct reset_control *rstc;
>  	int ret;
>  
> +	rstc = pd < ARRAY_SIZE(cpu_rstc) ? cpu_rstc[pd] : ERR_PTR(-EINVAL);
> +
>  	if (IS_ERR(rstc) && read_cpuid_part() != ARM_CPU_PART_CORTEX_A9) {
>  		pr_err("%s: could not get reset control for core %d\n",
>  		       __func__, pd);
> @@ -100,11 +103,8 @@ static int pmu_set_power_domain(int pd, bool on)
>  		}
>  	}
>  
> -	if (!IS_ERR(rstc)) {
> -		if (on)
> -			reset_control_deassert(rstc);
> -		reset_control_put(rstc);
> -	}
> +	if (!IS_ERR(rstc) && on)
> +		reset_control_deassert(rstc);
>  
>  	return 0;
>  }
> @@ -312,6 +312,9 @@ static void __init rockchip_smp_prepare_cpus(unsigned int max_cpus)
>  		ncores = ((l2ctlr >> 24) & 0x3) + 1;
>  	}
>  
> +	for (i = 0; i < ncores; i++)
> +		cpu_rstc[i] = rockchip_get_core_reset(i);
> +
>  	/* Make sure that all cores except the first are really off */
>  	for (i = 1; i < ncores; i++)
>  		pmu_set_power_domain(0 + i, false);

Re: [PATCH] reset: use a shared SRCU domain for reset controls

[Steven Price]

Gentle ping (see below)

On 24/04/2026 10:04, Steven Price wrote:
> On 23/04/2026 15:15, Philipp Zabel wrote:
>> Hi Steven, Heiko,
>>
>> On Do, 2026-04-23 at 13:45 +0100, Steven Price wrote:
>>> +Heiko for the Rockchip questions.
>>>
>>> On 23/04/2026 11:27, Philipp Zabel wrote:
>>>> Hi Steven,
>>>>
>>>> On Fr, 2026-04-17 at 16:48 +0100, Steven Price wrote:
>>>>> Commit 78ebbff6d1a0 ("reset: handle removing supplier before consumers")
>>>>> added a dynamically initialized srcu_struct to every reset_control and
>>>>> cleaned it up again when the handle was dropped.
>>>>>
>>>>> That breaks early boot users which acquire and release reset handles
>>>>> before workqueues are online. On rk3288 this shows up during
>>>>> rockchip_smp_prepare_cpus(), where pmu_set_power_domain() gets a reset
>>>>> control for a CPU core and then drops it again before SMP bring-up has
>>>>> finished.
>>>>
>>>> Can the reset_control_put() call be dropped from pmu_set_power_domain()
>>>> to fix the problem?
>>>
>>> I'm not that familiar with the code, so I'm not sure.
>>>
>>> Just dropping that call causes a WARN_ON() bringing the secondary CPUs
>>> on (because the call to rockchip_get_core_reset() expects to have
>>> exclusive access to the reset).
>>
>> Yes, caused by re-requesting the same reset again. I was thinking of
>> storing the controls in an array in rockchip_smp_prepare_cpus() and
>> reusing them in pmu_set_power_domain(), see the patch below.
>>
>>> Switching to a shared reset then his a
>>> WARN_ON() in reset_control_assert because deassert_count == 0. I could
>>> keep digging blindly but I'm not really sure how this code is meant to work.
>>
>> Shared reset is not the right mechanism here, that would be for
>> multiple drivers/devices sharing the same reset line.
> 
> That makes sense - I hadn't really looked into whether this was a single
> reset for all CPUs or somehow one shared between them all. Thinking
> about it more I can't see how a shared reset would have worked ;)
> 
>>> Hopefully Heiko might be able to shed some more light on this?
>>>
>>>> Putting the reset control should mean that the driver doesn't care
>>>> about the state of the reset line anymore, but the platsmp code very
>>>> much expects the reset line to stay deasserted after enabling a CPU.
>>>> Acquiring reset controls in rockchip_smp_prepare_cpus() once and never
>>>> giving them up via reset_control_put() seems like a correct fix,
>>>> regardless of whether this patch is applied or not.
>>>>
>>>> It looks like the meson platsmp suffers from the same issue.
>>>
>>> This is why I did the fix in the reset code -
>>
>> And I'm grateful, because that made sashiko.dev point out an ABBA
>> deadlock opportunity in the existing code.
> 
> Cool.
> 
>>> how many other platforms
>>> might have similar issues? But obviously if these platforms are buggy
>>> then they should be fixed.
>>
>> Agreed.
>>
>>> My interest is keeping the devboard working so I can keep testing
>>> Panfrost on it.
>>
>> Does this patch work?
> 
> Indeed it does - thanks. Feel free to add my T-b when posting:
> 
> Tested-by: Steven Price <steven.price@arm.com>

Can we pick the below patch up please? Let me know if there's anything I
can do to move this along.

Thanks,
Steve

> 
> Thanks,
> Steve
> 
>>
>> ----------8<----------
>> From: Philipp Zabel <p.zabel@pengutronix.de>
>> Subject: [PATCH] ARM: rockchip: keep reset control around
>>
>> Do not put the reset control, retain exclusive control over it.
>> After turning on a CPU, the corresponding reset line must stay
>> deasserted.
>>
>> This also avoids calling reset_control_put() before workqueues
>> are operational.
>>
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> ---
>>  arch/arm/mach-rockchip/platsmp.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c
>> index f432d22bfed8..c28816fffce8 100644
>> --- a/arch/arm/mach-rockchip/platsmp.c
>> +++ b/arch/arm/mach-rockchip/platsmp.c
>> @@ -34,6 +34,7 @@ static int ncores;
>>  
>>  static struct regmap *pmu;
>>  static int has_pmu = true;
>> +static struct reset_control *cpu_rstc[4];
>>  
>>  static int pmu_power_domain_is_on(int pd)
>>  {
>> @@ -64,9 +65,11 @@ static struct reset_control *rockchip_get_core_reset(int cpu)
>>  static int pmu_set_power_domain(int pd, bool on)
>>  {
>>  	u32 val = (on) ? 0 : BIT(pd);
>> -	struct reset_control *rstc = rockchip_get_core_reset(pd);
>> +	struct reset_control *rstc;
>>  	int ret;
>>  
>> +	rstc = pd < ARRAY_SIZE(cpu_rstc) ? cpu_rstc[pd] : ERR_PTR(-EINVAL);
>> +
>>  	if (IS_ERR(rstc) && read_cpuid_part() != ARM_CPU_PART_CORTEX_A9) {
>>  		pr_err("%s: could not get reset control for core %d\n",
>>  		       __func__, pd);
>> @@ -100,11 +103,8 @@ static int pmu_set_power_domain(int pd, bool on)
>>  		}
>>  	}
>>  
>> -	if (!IS_ERR(rstc)) {
>> -		if (on)
>> -			reset_control_deassert(rstc);
>> -		reset_control_put(rstc);
>> -	}
>> +	if (!IS_ERR(rstc) && on)
>> +		reset_control_deassert(rstc);
>>  
>>  	return 0;
>>  }
>> @@ -312,6 +312,9 @@ static void __init rockchip_smp_prepare_cpus(unsigned int max_cpus)
>>  		ncores = ((l2ctlr >> 24) & 0x3) + 1;
>>  	}
>>  
>> +	for (i = 0; i < ncores; i++)
>> +		cpu_rstc[i] = rockchip_get_core_reset(i);
>> +
>>  	/* Make sure that all cores except the first are really off */
>>  	for (i = 1; i < ncores; i++)
>>  		pmu_set_power_domain(0 + i, false);
> 

Re: [PATCH] reset: use a shared SRCU domain for reset controls

[Heiko Stuebner]

Hi,

Am Donnerstag, 14. Mai 2026, 11:07:29 Mitteleuropäische Sommerzeit schrieb Steven Price:
> Gentle ping (see below)
> 
> On 24/04/2026 10:04, Steven Price wrote:
> > On 23/04/2026 15:15, Philipp Zabel wrote:
> >> Hi Steven, Heiko,
> >>
> >> On Do, 2026-04-23 at 13:45 +0100, Steven Price wrote:
> >>> +Heiko for the Rockchip questions.
> >>>
> >>> On 23/04/2026 11:27, Philipp Zabel wrote:
> >>>> Hi Steven,
> >>>>
> >>>> On Fr, 2026-04-17 at 16:48 +0100, Steven Price wrote:
> >>>>> Commit 78ebbff6d1a0 ("reset: handle removing supplier before consumers")
> >>>>> added a dynamically initialized srcu_struct to every reset_control and
> >>>>> cleaned it up again when the handle was dropped.
> >>>>>
> >>>>> That breaks early boot users which acquire and release reset handles
> >>>>> before workqueues are online. On rk3288 this shows up during
> >>>>> rockchip_smp_prepare_cpus(), where pmu_set_power_domain() gets a reset
> >>>>> control for a CPU core and then drops it again before SMP bring-up has
> >>>>> finished.
> >>>>
> >>>> Can the reset_control_put() call be dropped from pmu_set_power_domain()
> >>>> to fix the problem?
> >>>
> >>> I'm not that familiar with the code, so I'm not sure.
> >>>
> >>> Just dropping that call causes a WARN_ON() bringing the secondary CPUs
> >>> on (because the call to rockchip_get_core_reset() expects to have
> >>> exclusive access to the reset).
> >>
> >> Yes, caused by re-requesting the same reset again. I was thinking of
> >> storing the controls in an array in rockchip_smp_prepare_cpus() and
> >> reusing them in pmu_set_power_domain(), see the patch below.
> >>
> >>> Switching to a shared reset then his a
> >>> WARN_ON() in reset_control_assert because deassert_count == 0. I could
> >>> keep digging blindly but I'm not really sure how this code is meant to work.
> >>
> >> Shared reset is not the right mechanism here, that would be for
> >> multiple drivers/devices sharing the same reset line.
> > 
> > That makes sense - I hadn't really looked into whether this was a single
> > reset for all CPUs or somehow one shared between them all. Thinking
> > about it more I can't see how a shared reset would have worked ;)
> > 
> >>> Hopefully Heiko might be able to shed some more light on this?
> >>>
> >>>> Putting the reset control should mean that the driver doesn't care
> >>>> about the state of the reset line anymore, but the platsmp code very
> >>>> much expects the reset line to stay deasserted after enabling a CPU.
> >>>> Acquiring reset controls in rockchip_smp_prepare_cpus() once and never
> >>>> giving them up via reset_control_put() seems like a correct fix,
> >>>> regardless of whether this patch is applied or not.
> >>>>
> >>>> It looks like the meson platsmp suffers from the same issue.
> >>>
> >>> This is why I did the fix in the reset code -
> >>
> >> And I'm grateful, because that made sashiko.dev point out an ABBA
> >> deadlock opportunity in the existing code.
> > 
> > Cool.
> > 
> >>> how many other platforms
> >>> might have similar issues? But obviously if these platforms are buggy
> >>> then they should be fixed.
> >>
> >> Agreed.
> >>
> >>> My interest is keeping the devboard working so I can keep testing
> >>> Panfrost on it.
> >>
> >> Does this patch work?
> > 
> > Indeed it does - thanks. Feel free to add my T-b when posting:
> > 
> > Tested-by: Steven Price <steven.price@arm.com>
> 
> Can we pick the below patch up please? Let me know if there's anything I
> can do to move this along.

I completely missed this series, until now and until I fell onto that issue
while testing on the rk3288.

I tested the patch and send it out to the lists for a loop [0],
(and just realized after the fact I forgot Bartosz, sorry)

before I plan to apply it in the next das for the current rc kernels.


Heiko


[0] http://lore.kernel.org/all/20260521210915.2331176-1-heiko@sntech.de


> >> ----------8<----------
> >> From: Philipp Zabel <p.zabel@pengutronix.de>
> >> Subject: [PATCH] ARM: rockchip: keep reset control around
> >>
> >> Do not put the reset control, retain exclusive control over it.
> >> After turning on a CPU, the corresponding reset line must stay
> >> deasserted.
> >>
> >> This also avoids calling reset_control_put() before workqueues
> >> are operational.
> >>
> >> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >> ---
> >>  arch/arm/mach-rockchip/platsmp.c | 15 +++++++++------
> >>  1 file changed, 9 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c
> >> index f432d22bfed8..c28816fffce8 100644
> >> --- a/arch/arm/mach-rockchip/platsmp.c
> >> +++ b/arch/arm/mach-rockchip/platsmp.c
> >> @@ -34,6 +34,7 @@ static int ncores;
> >>  
> >>  static struct regmap *pmu;
> >>  static int has_pmu = true;
> >> +static struct reset_control *cpu_rstc[4];
> >>  
> >>  static int pmu_power_domain_is_on(int pd)
> >>  {
> >> @@ -64,9 +65,11 @@ static struct reset_control *rockchip_get_core_reset(int cpu)
> >>  static int pmu_set_power_domain(int pd, bool on)
> >>  {
> >>  	u32 val = (on) ? 0 : BIT(pd);
> >> -	struct reset_control *rstc = rockchip_get_core_reset(pd);
> >> +	struct reset_control *rstc;
> >>  	int ret;
> >>  
> >> +	rstc = pd < ARRAY_SIZE(cpu_rstc) ? cpu_rstc[pd] : ERR_PTR(-EINVAL);
> >> +
> >>  	if (IS_ERR(rstc) && read_cpuid_part() != ARM_CPU_PART_CORTEX_A9) {
> >>  		pr_err("%s: could not get reset control for core %d\n",
> >>  		       __func__, pd);
> >> @@ -100,11 +103,8 @@ static int pmu_set_power_domain(int pd, bool on)
> >>  		}
> >>  	}
> >>  
> >> -	if (!IS_ERR(rstc)) {
> >> -		if (on)
> >> -			reset_control_deassert(rstc);
> >> -		reset_control_put(rstc);
> >> -	}
> >> +	if (!IS_ERR(rstc) && on)
> >> +		reset_control_deassert(rstc);
> >>  
> >>  	return 0;
> >>  }
> >> @@ -312,6 +312,9 @@ static void __init rockchip_smp_prepare_cpus(unsigned int max_cpus)
> >>  		ncores = ((l2ctlr >> 24) & 0x3) + 1;
> >>  	}
> >>  
> >> +	for (i = 0; i < ncores; i++)
> >> +		cpu_rstc[i] = rockchip_get_core_reset(i);
> >> +
> >>  	/* Make sure that all cores except the first are really off */
> >>  	for (i = 1; i < ncores; i++)
> >>  		pmu_set_power_domain(0 + i, false);
> > 
> 
>