[PATCH 0/1] clk: nuvoton: ma35d1-divider: use direct formula instead of divider table

[PATCH 0/1] clk: nuvoton: ma35d1-divider: use direct formula instead of divider table

[Joey Lu]

The MA35D1 ADC clock divider register implements a simple closed-form
relation, rate = parent_rate / (2 * (N + 1)), but the driver models it
by building a clk_div_table with one entry per possible divider value
(up to 2^width entries) and feeding it through the generic
divider_recalc_rate()/divider_determine_rate()/divider_get_val() helpers.
Replace the table with direct arithmetic in recalc_rate()/determine_rate()/
set_rate(), and drop the unused mask_bit mechanism, whose only call site
passed a bitmask instead of a bit index, causing an out-of-range BIT()
shift.

Joey Lu (1):
  clk: nuvoton: ma35d1-divider: use direct formula instead of divider
    table

 drivers/clk/nuvoton/clk-ma35d1-divider.c | 73 +++++++++++-------------
 drivers/clk/nuvoton/clk-ma35d1.c         |  2 +-
 drivers/clk/nuvoton/clk-ma35d1.h         |  2 +-
 3 files changed, 36 insertions(+), 41 deletions(-)

-- 
2.43.0

[PATCH 1/1] clk: nuvoton: ma35d1-divider: use direct formula instead of divider table

[Joey Lu]

The ADC clock divider register implements a simple closed-form relation,
rate = parent_rate / (2 * (N + 1)), but it was previously modeled by
building a clk_div_table with one entry per possible divider value (up
to 2^width entries) and feeding it through the generic
divider_recalc_rate()/divider_determine_rate()/divider_get_val() helpers.
This is unnecessarily expensive (a large table allocation and a linear
scan on every rate lookup) for what is just a closed-form expression.

Also drop the mask_bit/mask mechanism: the previous code unconditionally
OR'd an arbitrary bit into the divider register on every set_rate() call.
The one existing caller passed mask_bit as a bitmask value (0x1ffff)
rather than a bit index, which BIT() then shifted by an out-of-range
amount, undefined behavior. Since the divider field write already clears
and rewrites only its own bits, no extra forced bit is required, so the
parameter is removed entirely.

Compute the divisor directly in recalc_rate()/determine_rate()/set_rate()
instead, with explicit range clamping and a rate == 0 guard.

Fixes: 691521a367cf ("clk: nuvoton: Add clock driver for ma35d1 clock controller")
Signed-off-by: Joey Lu <a0987203069@gmail.com>
---
 drivers/clk/nuvoton/clk-ma35d1-divider.c | 73 +++++++++++-------------
 drivers/clk/nuvoton/clk-ma35d1.c         |  2 +-
 drivers/clk/nuvoton/clk-ma35d1.h         |  2 +-
 3 files changed, 36 insertions(+), 41 deletions(-)

diff --git a/drivers/clk/nuvoton/clk-ma35d1-divider.c b/drivers/clk/nuvoton/clk-ma35d1-divider.c
index e992e7c30341..03fcf87caae1 100644
--- a/drivers/clk/nuvoton/clk-ma35d1-divider.c
+++ b/drivers/clk/nuvoton/clk-ma35d1-divider.c
@@ -16,8 +16,6 @@ struct ma35d1_adc_clk_div {
 	void __iomem *reg;
 	u8 shift;
 	u8 width;
-	u32 mask;
-	const struct clk_div_table *table;
 	/* protects concurrent access to clock divider registers */
 	spinlock_t *lock;
 };
@@ -29,44 +27,60 @@ static inline struct ma35d1_adc_clk_div *to_ma35d1_adc_clk_div(struct clk_hw *_h
 
 static unsigned long ma35d1_clkdiv_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 {
-	unsigned int val;
 	struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw);
+	unsigned int val;
 
 	val = readl_relaxed(dclk->reg) >> dclk->shift;
 	val &= clk_div_mask(dclk->width);
-	val += 1;
-	return divider_recalc_rate(hw, parent_rate, val, dclk->table,
-				   CLK_DIVIDER_ROUND_CLOSEST, dclk->width);
+
+	return DIV_ROUND_CLOSEST_ULL((u64)parent_rate, 2 * (val + 1));
 }
 
 static int ma35d1_clkdiv_determine_rate(struct clk_hw *hw,
 					struct clk_rate_request *req)
 {
 	struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw);
+	unsigned int val;
+
+	if (!req->rate)
+		return -EINVAL;
 
-	return divider_determine_rate(hw, req, dclk->table, dclk->width,
-				      CLK_DIVIDER_ROUND_CLOSEST);
+	val = DIV_ROUND_UP(req->best_parent_rate, 2 * req->rate);
+	if (val == 0)
+		val = 1;
+	if (val > (unsigned int)clk_div_mask(dclk->width) + 1)
+		val = clk_div_mask(dclk->width) + 1;
+
+	req->rate = DIV_ROUND_CLOSEST_ULL((u64)req->best_parent_rate, 2 * val);
+
+	return 0;
 }
 
 static int ma35d1_clkdiv_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate)
 {
-	int value;
-	unsigned long flags = 0;
-	u32 data;
 	struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw);
+	unsigned long flags;
+	unsigned int val;
+	u32 data;
+
+	if (!rate)
+		return -EINVAL;
 
-	value = divider_get_val(rate, parent_rate, dclk->table,
-				dclk->width, CLK_DIVIDER_ROUND_CLOSEST);
+	val = DIV_ROUND_UP(parent_rate, 2 * rate);
+	if (val == 0)
+		val = 1;
+	if (val > (unsigned int)clk_div_mask(dclk->width) + 1)
+		val = clk_div_mask(dclk->width) + 1;
 
 	spin_lock_irqsave(dclk->lock, flags);
 
 	data = readl_relaxed(dclk->reg);
 	data &= ~(clk_div_mask(dclk->width) << dclk->shift);
-	data |= (value - 1) << dclk->shift;
-	data |= dclk->mask;
+	data |= (val - 1) << dclk->shift;
 	writel_relaxed(data, dclk->reg);
 
 	spin_unlock_irqrestore(dclk->lock, flags);
+
 	return 0;
 }
 
@@ -79,39 +93,21 @@ static const struct clk_ops ma35d1_adc_clkdiv_ops = {
 struct clk_hw *ma35d1_reg_adc_clkdiv(struct device *dev, const char *name,
 				     struct clk_hw *parent_hw, spinlock_t *lock,
 				     unsigned long flags, void __iomem *reg,
-				     u8 shift, u8 width, u32 mask_bit)
+				     u8 shift, u8 width)
 {
-	struct ma35d1_adc_clk_div *div;
-	struct clk_init_data init;
-	struct clk_div_table *table;
 	struct clk_parent_data pdata = { .index = 0 };
-	u32 max_div, min_div;
+	struct ma35d1_adc_clk_div *div;
+	struct clk_init_data init = {};
 	struct clk_hw *hw;
 	int ret;
-	int i;
 
 	div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
 	if (!div)
 		return ERR_PTR(-ENOMEM);
 
-	max_div = clk_div_mask(width) + 1;
-	min_div = 1;
-
-	table = devm_kcalloc(dev, max_div + 1, sizeof(*table), GFP_KERNEL);
-	if (!table)
-		return ERR_PTR(-ENOMEM);
-
-	for (i = 0; i < max_div; i++) {
-		table[i].val = min_div + i;
-		table[i].div = 2 * table[i].val;
-	}
-	table[max_div].val = 0;
-	table[max_div].div = 0;
-
-	memset(&init, 0, sizeof(init));
 	init.name = name;
 	init.ops = &ma35d1_adc_clkdiv_ops;
-	init.flags |= flags;
+	init.flags = flags;
 	pdata.hw = parent_hw;
 	init.parent_data = &pdata;
 	init.num_parents = 1;
@@ -119,15 +115,14 @@ struct clk_hw *ma35d1_reg_adc_clkdiv(struct device *dev, const char *name,
 	div->reg = reg;
 	div->shift = shift;
 	div->width = width;
-	div->mask = mask_bit ? BIT(mask_bit) : 0;
 	div->lock = lock;
 	div->hw.init = &init;
-	div->table = table;
 
 	hw = &div->hw;
 	ret = devm_clk_hw_register(dev, hw);
 	if (ret)
 		return ERR_PTR(ret);
+
 	return hw;
 }
 EXPORT_SYMBOL_GPL(ma35d1_reg_adc_clkdiv);
diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c
index 9f65d0623517..f91a8369a2bc 100644
--- a/drivers/clk/nuvoton/clk-ma35d1.c
+++ b/drivers/clk/nuvoton/clk-ma35d1.c
@@ -1017,7 +1017,7 @@ static int ma35d1_clocks_probe(struct platform_device *pdev)
 	hws[ADC_DIV] = ma35d1_reg_adc_clkdiv(dev, "adc_div", hws[PCLK0],
 					     &ma35d1_lock, 0,
 					     clk_base + REG_CLK_CLKDIV4,
-					     4, 17, 0x1ffff);
+					     4, 17);
 	hws[ADC_GATE] = ma35d1_clk_gate(dev, "adc_gate", "adc_div",
 					clk_base + REG_CLK_APBCLK2, 24);
 
diff --git a/drivers/clk/nuvoton/clk-ma35d1.h b/drivers/clk/nuvoton/clk-ma35d1.h
index 3adee440f60a..1fdcd4658b5f 100644
--- a/drivers/clk/nuvoton/clk-ma35d1.h
+++ b/drivers/clk/nuvoton/clk-ma35d1.h
@@ -13,6 +13,6 @@ struct clk_hw *ma35d1_reg_clk_pll(struct device *dev, u32 id, u8 u8mode, const c
 struct clk_hw *ma35d1_reg_adc_clkdiv(struct device *dev, const char *name,
 				     struct clk_hw *parent_hw, spinlock_t *lock,
 				     unsigned long flags, void __iomem *reg,
-				     u8 shift, u8 width, u32 mask_bit);
+				     u8 shift, u8 width);
 
 #endif /* __DRV_CLK_NUVOTON_MA35D1_H */
-- 
2.43.0

Re: [PATCH 1/1] clk: nuvoton: ma35d1-divider: use direct formula instead of divider table

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: nuvoton: rate mismatch and missing bounds check in determine_rate
- [High] clk: nuvoton: divide by zero due to integer overflow in rate multiplication
--

commit 434b92fcdb326fe48e43db4fafe0804d04aee387
Author: Joey Lu <a0987203069@gmail.com>

    clk: nuvoton: ma35d1-divider: use direct formula instead of divider table

This patch replaces the table-based clock divider with a direct formula in the
Nuvoton MA35D1 ADC clock divider driver. It drops the mask bit mechanism and
computes the divisor explicitly in the recalc, determine, and set rate
callbacks.

> diff --git a/drivers/clk/nuvoton/clk-ma35d1-divider.c b/drivers/clk/nuvoton/clk-ma35d1-divider.c
> index e992e7c303419..03fcf87caae18 100644
> --- a/drivers/clk/nuvoton/clk-ma35d1-divider.c
> +++ b/drivers/clk/nuvoton/clk-ma35d1-divider.c
[ ... ]
> @@ -29,44 +27,60 @@ static inline struct ma35d1_adc_clk_div *to_ma35d1_adc_clk_div(struct clk_hw *_h
[ ... ]
>  static int ma35d1_clkdiv_determine_rate(struct clk_hw *hw,
>  					struct clk_rate_request *req)
>  {
>  	struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw);
> +	unsigned int val;
> +
> +	if (!req->rate)
> +		return -EINVAL;
>  
> -	return divider_determine_rate(hw, req, dclk->table, dclk->width,
> -				      CLK_DIVIDER_ROUND_CLOSEST);
> +	val = DIV_ROUND_UP(req->best_parent_rate, 2 * req->rate);

[Severity: High]
Does this code divide by zero due to integer overflow in rate multiplication?

If a caller or untrusted user (via debugfs) requests a rate such that
2 * req->rate overflows a 32-bit or 64-bit unsigned integer (for example,
rate = 1UL << (BITS_PER_LONG - 1)), the denominator 2 * req->rate wraps to 0.

This would cause DIV_ROUND_UP(req->best_parent_rate, 0) to evaluate to a
division by zero panic.

[Severity: High]
Can this create a rate mismatch between ma35d1_clkdiv_determine_rate() and
ma35d1_clkdiv_set_rate()?

This calculates the divider using a strict ceiling instead of closest match,
and also fails to check rate boundaries against req->min_rate and
req->max_rate.

> +	if (val == 0)
> +		val = 1;
> +	if (val > (unsigned int)clk_div_mask(dclk->width) + 1)
> +		val = clk_div_mask(dclk->width) + 1;
> +
> +	req->rate = DIV_ROUND_CLOSEST_ULL((u64)req->best_parent_rate, 2 * val);

It then promises a closest-rounded integer rate to the clock framework here.
When ma35d1_clkdiv_set_rate() is called later, the recomputation will yield a
different divisor.

> +
> +	return 0;
>  }
>  
>  static int ma35d1_clkdiv_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate)
>  {
> -	int value;
> -	unsigned long flags = 0;
> -	u32 data;
>  	struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw);
> +	unsigned long flags;
> +	unsigned int val;
> +	u32 data;
> +
> +	if (!rate)
> +		return -EINVAL;
>  
> -	value = divider_get_val(rate, parent_rate, dclk->table,
> -				dclk->width, CLK_DIVIDER_ROUND_CLOSEST);
> +	val = DIV_ROUND_UP(parent_rate, 2 * rate);

[Severity: High]
Could this identical integer overflow cause division by zero here as well?

Also, because the rate was rounded in ma35d1_clkdiv_determine_rate(),
recalculating the divider using DIV_ROUND_UP(parent_rate, 2 * rate) can yield
a completely different divisor.

For example, with parent_rate=1000 and an original val=6,
ma35d1_clkdiv_determine_rate() promises rate=83.

Then ma35d1_clkdiv_set_rate() calculates DIV_ROUND_UP(1000, 166) = 7, applying
a divider of 7 instead of 6 (which results in an actual rate of 71 instead of
83).

> +	if (val == 0)
> +		val = 1;
> +	if (val > (unsigned int)clk_div_mask(dclk->width) + 1)
> +		val = clk_div_mask(dclk->width) + 1;
>  
>  	spin_lock_irqsave(dclk->lock, flags);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929072106.175202-1-a0987203069@gmail.com?part=1