Re: Re: Re: [PATCH v13 5/5] clocksource: dw_apb_timer: Use reset array API to handle all resets

[Xuyang Dong]

> 
> On Mo, 2026-07-27 at 17:26 +0800, Xuyang Dong wrote:
> > > 
> > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > - [High] The patch dynamically allocates an exclusive reset control array using the unmanaged `of_` API but fails to free it, permanently locking shared APB resets and deadlocking other devices.
> > > --
> > > 
> > > --- Patch [5]: [PATCH v13 5/5] clocksource: dw_apb_timer: Use reset array API to handle all resets ---
> > > commit 015418d722b66a454000fd7a9c2606bea3f7e18b
> > > Author: Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>
> > > 
> > > clocksource: dw_apb_timer: Use reset array API to handle all resets
> > > 
> > > This patch updates the timer initialization to fetch and pulse all reset lines
> > > specified in the device tree by switching to the reset array API.
> > > 
> > > > Replace it with devm_reset_control_array_get_optional_exclusive() to
> > > > fetch and control all reset lines listed in the device tree, ensuring
> > > > every line is properly asserted and deasserted rather than only the first.
> > > 
> > > [Severity: Low]
> > > This isn't a bug, but the commit message mentions using the managed
> > > devm_reset_control_array_get_optional_exclusive function, while the code
> > > below actually uses the unmanaged of_reset_control_array_get_optional_exclusive
> > > function.
> > > 
> > > > diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
> > > > --- a/drivers/clocksource/dw_apb_timer_of.c
> > > > +++ b/drivers/clocksource/dw_apb_timer_of.c
> > > [ ... ]
> > > > @@ -29,9 +29,11 @@ static int __init timer_get_base_and_rate(struct device_node *np,
> > > >  	/*
> > > >  	 * Reset the timer if the reset control is available, wiping
> > > > -	 * out the state the firmware may have left it
> > > > +	 * out the state the firmware may have left it.
> > > > +	 * Use array variant to handle multiple resets (e.g., timer
> > > > +	 * and APB interface resets) if specified in the device tree.
> > > >  	 */
> > > > -	rstc = of_reset_control_get(np, NULL);
> > > > +	rstc = of_reset_control_array_get_optional_exclusive(np);
> > > >  	if (!IS_ERR(rstc)) {
> > > >  		reset_control_assert(rstc);
> > > >  		reset_control_deassert(rstc);
> > > >  	}
> > > 
> > > [Severity: High]
> > > Does this code leak the exclusive reset control array?
> > > 
> > > Since of_reset_control_array_get_optional_exclusive is used instead of the
> > > managed devm variant, the returned rstc needs to be manually freed.
> > > 
> > > Because this API acquires exclusive references to the resets (which the
> > > comment notes might include shared APB interface resets), exiting
> 
> Is this true? Do multiple timers share the same bus reset?
> 

Hi Philipp,

Thanks for your comments and time.
I think each timer might have its own reset signal(s).

> > > timer_get_base_and_rate without calling reset_control_put(rstc) leaves
> > > these resets permanently locked.
> > > 
> > > Could this prevent other devices on the bus from acquiring their shared
> > > resets during probe by returning -EBUSY?
> > 
> > Hi Sashiko,
> > 
> > Will change devm_reset_control_array_get_optional_exclusive() to
> > of_reset_control_array_get_optional_exclusive() in commit message.
> > 
> > Add reset_control_put(rstc) after deassert. The next version will be 
> > updated with the following changes. 
> > 
> > -	rstc = of_reset_control_get(np, NULL);
> > +	rstc = of_reset_control_array_get_optional_exclusive(np);
> >  	if (!IS_ERR(rstc)) {
> >  		reset_control_assert(rstc);
> >  		reset_control_deassert(rstc);
> > +		reset_control_put(rstc);
> > +		rstc = NULL;
> >  	}
> > 
> > Do you think this change is correct?
> 
> It does allows multiple timers to repeatedly assert/deassert the same
> bus reset. Is this what you need?
> 

We do not need to repeatedly assert and deassert the same bus reset, 
because we do not want other timers to affect the current timer when 
they assert the reset.

> Semantically, it is wrong, though. By releasing the reset controls, the
> driver states that it doesn't care about the state of the reset lines
> anymore.
> 

Will drop "reset_control_put(rstc);" and "rstc = NULL;".
Keep only the content below:

-	rstc = of_reset_control_get(np, NULL);
+	rstc = of_reset_control_array_get_optional_exclusive(np);
 	if (!IS_ERR(rstc)) {
 		reset_control_assert(rstc);
 		reset_control_deassert(rstc);
 	}

Do you think this change is correct?

Best regards,
Xuyang Dong