Re: [PATCH v3 03/12] clk: qcom: clk-regmap-divider: Support CLK_DIVIDER_* flags

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Uninitialized stack variable used in clock calculations if regmap_read() fails.
- [High] Divide-by-zero kernel panic in divider_ro_determine_rate() exposed by CLK_DIVIDER_ONE_BASED.
--

--- Patch [3]: [PATCH v3 03/12] clk: qcom: clk-regmap-divider: Support CLK_DIVIDER_* flags ---
commit dbde27a9bd18456c99d0f970183ac77f2661a8eb
Author: Luo Jie <[email protected]>

clk: qcom: clk-regmap-divider: Support CLK_DIVIDER_* flags

Add a flags field to struct clk_regmap_div and OR it into the flag
passed to divider determine/recalc rate functions. This allows qcom
drivers to opt into behavior variants such as CLK_DIVIDER_ONE_BASED.

> diff --git a/drivers/clk/qcom/clk-regmap-divider.c b/drivers/clk/qcom/clk-regmap-divider.c
> index 672e82caf2050..6e12aebf14b6a 100644
> --- a/drivers/clk/qcom/clk-regmap-divider.c
> +++ b/drivers/clk/qcom/clk-regmap-divider.c
> @@ -27,7 +27,8 @@ static int div_ro_determine_rate(struct clk_hw *hw,
>  	val &= BIT(divider->width) - 1;

[Severity: High]
This is a pre-existing issue, but does this code need to check the return
value of regmap_read() that populates 'val' right before this line?

If regmap_read() fails (for example, due to a bus error or the power domain
being off), 'val' remains uninitialized on the stack.

Will proceeding to shift and mask this uninitialized stack memory lead to
unpredictable clock frequencies being passed to the core framework?

>  
>  	return divider_ro_determine_rate(hw, req, NULL, divider->width,
> -					 CLK_DIVIDER_ROUND_CLOSEST, val);
> +					 CLK_DIVIDER_ROUND_CLOSEST | divider->flags,
> +					 val);

[Severity: High]
This isn't a bug introduced by this patch, but could this addition of
divider->flags trigger a divide-by-zero panic in the core clock framework?

If a QCOM driver opts into CLK_DIVIDER_ONE_BASED, and the hardware register
reads 0 (or is uninitialized due to a regmap_read failure), _get_div() will
return 0.

Unlike divider_recalc_rate() which checks for a zero divisor,
divider_ro_determine_rate() in drivers/clk/clk-divider.c lacks this check:

drivers/clk/clk-divider.c:divider_ro_determine_rate() {
    ...
    div = _get_div(table, val, flags, width);
    ...
    req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);
    return 0;
}

Will passing CLK_DIVIDER_ONE_BASED through divider->flags cause a
divide-by-zero exception here when div is 0?

>  }

[ ... ]

> @@ -65,7 +68,8 @@ static unsigned long div_recalc_rate(struct clk_hw *hw,
>  	div &= BIT(divider->width) - 1;

[Severity: High]
This is also a pre-existing issue, but similar to div_ro_determine_rate(),
does the return value of regmap_read() need to be checked here before
manipulating 'div'?

If regmap_read() returns an error code, 'div' will contain uninitialized
stack data which is then used in hardware clock frequency calculations.

>  
>  	return divider_recalc_rate(hw, parent_rate, div, NULL,
> -				   CLK_DIVIDER_ROUND_CLOSEST, divider->width);
> +				   CLK_DIVIDER_ROUND_CLOSEST | divider->flags,
> +				   divider->width);
>  }

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