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

[Philipp Zabel]

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?

> > 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?

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.

regards
Philipp