Re: [PATCH v5 1/6] clk: correct clk_div_mask() return value for width == 32

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] clk_div_mask(width) generates undefined behavior and corrupts registers when width is 0.
- [Medium] Using clk_div_mask(0) with a compile-time constant breaks the build.
- [Low] The commit message incorrectly claims GENMASK provides compile-time bounds checking for variables.
- [Low] The new macro evaluates the `width` argument multiple times, creating a risk for side-effects.
--

commit 73197243e5c552cead36bc9d0e0d0747fab0a7ab
Author: Junhui Liu <junhui.liu@pigmoral.tech>

clk: correct clk_div_mask() return value for width == 32

This commit replaces the custom clk_div_mask macro with GENMASK. This change
aims to fix undefined behavior when the width is 32, preventing incorrect mask
generation.

> Fix this by using the standard GENMASK() macro. This safely resolves the
> undefined behavior on both 32-bit and 64-bit architectures, while also
> benefiting from the built-in compile-time type and bounds checking
> provided by the GENMASK() macro.

This isn't a bug, but the commit message claims GENMASK provides compile-time
bounds checking.

Since width is almost exclusively used as a runtime variable in the clock
subsystem, the compiler bypasses the bounds check which relies on
__is_constexpr. Is this description accurate for the common usage of this
macro?

> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index b01a38fef8cf2..b986cc054d827 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h

[ ... ]

> @@ -714,7 +715,7 @@ struct clk_divider {
>  	spinlock_t	*lock;
>  };
>  
> -#define clk_div_mask(width)	((1 << (width)) - 1)
> +#define clk_div_mask(width)	GENMASK((width) - 1, 0)

If a clock driver uses a dummy divider or an edge case where width is 0, this
evaluates to GENMASK(-1, 0).

Internally, GENMASK executes a shift of ~0UL by BITS_PER_LONG, which is
undefined behavior in C. On architectures like ARM and x86, this results in
a shift by 0, yielding a mask of all 1s (0xFFFFFFFF) instead of the previous
safe mask of 0.

Could this inadvertently clear adjacent clock controls like gates and muxes
when applying the mask in drivers/clk/clk-divider.c?

val &= ~(clk_div_mask(divider->width) << divider->shift);

Additionally, if width is passed as a compile-time constant of 0,
GENMASK(-1, 0) will trigger the GENMASK_INPUT_CHECK because the high bit is
less than the low bit. Does this introduce a build breakage for drivers that
statically evaluate clk_div_mask(0)?

Also, wrapping the parameter in GENMASK causes the width argument to be
evaluated multiple times. Will this cause unexpected behavior if a driver
invokes the macro with an expression containing side-effects?

>  #define to_clk_divider(_hw) container_of(_hw, struct clk_divider, hw)
>  
>  #define CLK_DIVIDER_ONE_BASED		BIT(0)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-dr1v90-cru-v5-0-34f3021aab51@pigmoral.tech?part=1