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

[Xuyang Dong]

> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Exclusive acquisition of a shared bus reset blocks other devices on the bus from probing and may inadvertently reset the entire bus.
> --
> 
> --- Patch [5]: [PATCH v14 5/5] clocksource: dw_apb_timer: Use reset array API to handle all resets ---
> commit 66ef3e54cc1daa673ec2f6b658e0a17fb7f358ab
> Author: Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>
> 
> clocksource: dw_apb_timer: Use reset array API to handle all resets
> 
> This commit updates the timer initialization to fetch and trigger all
> reset lines listed in the device tree using the array variant of the
> reset API. It intentionally holds exclusive references to these lines
> to keep them deasserted while the timer runs.
> 
> > 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,14 @@ 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 the array variant to fetch and trigger all reset lines
> > +	 * (e.g., timer and APB interface resets) together, without
> > +	 * ordering requirements.
> > +	 * The exclusive references are intentionally held to keep the
> > +	 * resets deasserted while the timer is running.
> >  	 */
> > -	rstc = of_reset_control_get(np, NULL);
> > +	rstc = of_reset_control_array_get_optional_exclusive(np);
> 
> [Severity: High]
> If any of these reset lines are shared bus resets, does acquiring them
> exclusively lock out other devices that share the same reset line?
> 

Hi Sashiko,

These reset lines should be exclusively owned by each timer.

> The commit message mentions APB interface resets, which are typically shared
> across multiple peripherals. If this driver holds a shared reset line
> exclusively, subsequent reset_control_get() calls from other drivers sharing
> the bus will fail with -EBUSY, preventing them from probing.
> 
> Furthermore, unconditionally asserting a shared bus reset here in
> timer_get_base_and_rate() might inadvertently reset other peripherals on the
> same bus that are already initialized. Does the timer driver need to use a
> shared reset API to avoid disrupting other devices on the bus?
> 

If the resets are exclusive, do you think using the exclusive reset API 
is better?

Best regards,
Xuyang Dong

> >  	if (!IS_ERR(rstc)) {
> >  		reset_control_assert(rstc);
> >  		reset_control_deassert(rstc);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260730111425.1680-1-dongxuyang@xxxxxxxxxxxxxxxxxx?part=5