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

[Heiko Stuebner <heiko@sntech.de>]

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);
> > 
> 
>