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

> 
> 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 <[email protected]>
> 
> 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
> 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?

Best regards,
Xuyang Dong

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5