Re: [PATCH v4 4/5] clk: renesas: Extract RZ/V2H PLL calculation helpers into shared library

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The new library file `rzv2h-cpg-lib.c` uses 64-bit math macros and functions but fails to explicitly include `<linux/math64.h>`.

Pre-existing issues:
- [High] Incorrect signed/unsigned promotion on 32-bit architectures causes frequency calculation to erroneously increase instead of decrease when the K component is negative.
--

commit 70a8fa3652af11a63021ff758a114c890594d96c
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

clk: renesas: Extract RZ/V2H PLL calculation helpers into shared library

This patch extracts the RZ/V2H PLL and divider parameter calculation
helpers into a new shared library, introducing the CLK_RZV2H_CPG_LIB
Kconfig symbol. It adds rzv2h-cpg-lib.c to host the algorithms and
updates the header to expose compatibility aliases for existing users.

> diff --git a/drivers/clk/renesas/rzv2h-cpg-lib.c b/drivers/clk/renesas/rzv2h-cpg-lib.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/renesas/rzv2h-cpg-lib.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0
[ ... ]
> +#include <linux/clk/renesas.h>
> +#include <linux/export.h>
> +#include <linux/math.h>
> +#include <linux/types.h>
> +#include <linux/units.h>

[Severity: Low]
Does this new file need to explicitly include linux/math64.h?

It heavily relies on 64-bit math operations like mul_u32_u32(), div_s64(),
DIV_S64_ROUND_CLOSEST(), and DIV_U64_ROUND_CLOSEST(), which are defined in
linux/math64.h. The original implementation in rzv2h-cpg.c included it,
but it appears to be missing here, which could lead to implicit function
declaration errors on certain architectures.

[ ... ]
> +bool rzv2h_cpg_get_pll_pars(const struct rzv2h_pll_limits *limits,
> +			    struct rzv2h_pll_pars *pars, u64 freq_millihz)
> +{
[ ... ]
> +				/* PLL_M component of (output * 65536 * PLL_P) */
> +				output = mul_u32_u32(p.m * 65536, input_fref);
> +				/* PLL_K component of (output * 65536 * PLL_P) */
> +				output += p.k * input_fref;

[Severity: High]
This isn't a bug introduced by this patch, but could this integer promotion
cause incorrect frequency calculations on 32-bit architectures?

The variable p.k is declared as an s16 (signed) and input_fref is an
unsigned long. On 32-bit systems, unsigned long is a 32-bit unsigned
integer. When p.k is negative, does the C integer promotion rule cause it
to be cast to a 32-bit unsigned integer before multiplication, resulting
in a massive positive number wrapping around?

If so, when added to the u64 output variable, it would be zero-extended
rather than sign-extended, effectively increasing the frequency massively
rather than subtracting the proportional frequency.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618181949.3036280-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=4