[v1] clk: hisilicon: clkdivider-hi6220: use kzalloc_flex

[PATCH] clk: hisilicon: clkdivider-hi6220: use kzalloc_flex

Posted by Rosen Penev 6 days, 14 hours ago

Combine allocations using kzalloc_flex and a flexible array member.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/clk/hisilicon/clkdivider-hi6220.c | 24 ++++++++---------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/hisilicon/clkdivider-hi6220.c b/drivers/clk/hisilicon/clkdivider-hi6220.c
index 1787ecefe601..14d72fd716c2 100644
--- a/drivers/clk/hisilicon/clkdivider-hi6220.c
+++ b/drivers/clk/hisilicon/clkdivider-hi6220.c
@@ -35,8 +35,8 @@ struct hi6220_clk_divider {
 	u8		shift;
 	u8		width;
 	u32		mask;
-	const struct clk_div_table *table;
 	spinlock_t	*lock;
+	struct clk_div_table table[];
 };
 
 #define to_hi6220_clk_divider(_hw)	\
@@ -108,24 +108,19 @@ struct clk *hi6220_register_clkdiv(struct device *dev, const char *name,
 	u32 max_div, min_div;
 	int i;
 
-	/* allocate the divider */
-	div = kzalloc_obj(*div);
-	if (!div)
-		return ERR_PTR(-ENOMEM);
-
 	/* Init the divider table */
 	max_div = div_mask(width) + 1;
 	min_div = 1;
 
-	table = kzalloc_objs(*table, max_div + 1);
-	if (!table) {
-		kfree(div);
+	/* allocate the divider */
+	div = kzalloc_flex(*div, table, max_div + 1);
+	if (!div)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	for (i = 0; i < max_div; i++) {
-		table[i].div = min_div + i;
-		table[i].val = table[i].div - 1;
+		table = &div->table[i];
+		table->div = min_div + i;
+		table->val = table[i].div - 1;
 	}
 
 	init.name = name;
@@ -141,14 +136,11 @@ struct clk *hi6220_register_clkdiv(struct device *dev, const char *name,
 	div->mask = mask_bit ? BIT(mask_bit) : 0;
 	div->lock = lock;
 	div->hw.init = &init;
-	div->table = table;
 
 	/* register the clock */
 	clk = clk_register(dev, &div->hw);
-	if (IS_ERR(clk)) {
-		kfree(table);
+	if (IS_ERR(clk))
 		kfree(div);
-	}
 
 	return clk;
 }
-- 
2.53.0

Re: [PATCH] clk: hisilicon: clkdivider-hi6220: use kzalloc_flex

Posted by Brian Masney 3 days, 3 hours ago

Hi Rosen,

On Thu, Mar 26, 2026 at 07:40:32PM -0700, Rosen Penev wrote:
> Combine allocations using kzalloc_flex and a flexible array member.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/clk/hisilicon/clkdivider-hi6220.c | 24 ++++++++---------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/clk/hisilicon/clkdivider-hi6220.c b/drivers/clk/hisilicon/clkdivider-hi6220.c
> index 1787ecefe601..14d72fd716c2 100644
> --- a/drivers/clk/hisilicon/clkdivider-hi6220.c
> +++ b/drivers/clk/hisilicon/clkdivider-hi6220.c
> @@ -35,8 +35,8 @@ struct hi6220_clk_divider {
>  	u8		shift;
>  	u8		width;
>  	u32		mask;
> -	const struct clk_div_table *table;
>  	spinlock_t	*lock;
> +	struct clk_div_table table[];
>  };
>  
>  #define to_hi6220_clk_divider(_hw)	\
> @@ -108,24 +108,19 @@ struct clk *hi6220_register_clkdiv(struct device *dev, const char *name,
>  	u32 max_div, min_div;
>  	int i;
>  
> -	/* allocate the divider */
> -	div = kzalloc_obj(*div);
> -	if (!div)
> -		return ERR_PTR(-ENOMEM);
> -
>  	/* Init the divider table */
>  	max_div = div_mask(width) + 1;
>  	min_div = 1;
>  
> -	table = kzalloc_objs(*table, max_div + 1);
> -	if (!table) {
> -		kfree(div);
> +	/* allocate the divider */
> +	div = kzalloc_flex(*div, table, max_div + 1);
> +	if (!div)
>  		return ERR_PTR(-ENOMEM);
> -	}
>  
>  	for (i = 0; i < max_div; i++) {
> -		table[i].div = min_div + i;
> -		table[i].val = table[i].div - 1;
> +		table = &div->table[i];
> +		table->div = min_div + i;
> +		table->val = table[i].div - 1;
                             ^^^^^^^^^^^^^^^^^
Sashiko has a question:
https://sashiko.dev/#/patchset/20260326042317.122536-1-rosenp%40gmail.com

    Does this code read out-of-bounds and write incorrect values to the table?
    
    Because table is reassigned to point to &div->table[i], accessing table[i].div
    in the following line effectively evaluates to div->table[2 * i].div.
    
    For any i > 0, this reads uninitialized memory from further down the array,
    and when 2 * i >= max_div + 1, it reads beyond the newly allocated flex array.
    Could we use table->val = table->div - 1 instead?

Brian

>  	}
>  
>  	init.name = name;
> @@ -141,14 +136,11 @@ struct clk *hi6220_register_clkdiv(struct device *dev, const char *name,
>  	div->mask = mask_bit ? BIT(mask_bit) : 0;
>  	div->lock = lock;
>  	div->hw.init = &init;
> -	div->table = table;
>  
>  	/* register the clock */
>  	clk = clk_register(dev, &div->hw);
> -	if (IS_ERR(clk)) {
> -		kfree(table);
> +	if (IS_ERR(clk))
>  		kfree(div);
> -	}
>  
>  	return clk;
>  }
> -- 
> 2.53.0
>

Re: [PATCH] clk: hisilicon: clkdivider-hi6220: use kzalloc_flex

Posted by Rosen Penev 2 days, 20 hours ago

On Mon, Mar 30, 2026 at 6:20 AM Brian Masney <bmasney@redhat.com> wrote:
>
> Hi Rosen,
>
> On Thu, Mar 26, 2026 at 07:40:32PM -0700, Rosen Penev wrote:
> > Combine allocations using kzalloc_flex and a flexible array member.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/clk/hisilicon/clkdivider-hi6220.c | 24 ++++++++---------------
> >  1 file changed, 8 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/clk/hisilicon/clkdivider-hi6220.c b/drivers/clk/hisilicon/clkdivider-hi6220.c
> > index 1787ecefe601..14d72fd716c2 100644
> > --- a/drivers/clk/hisilicon/clkdivider-hi6220.c
> > +++ b/drivers/clk/hisilicon/clkdivider-hi6220.c
> > @@ -35,8 +35,8 @@ struct hi6220_clk_divider {
> >       u8              shift;
> >       u8              width;
> >       u32             mask;
> > -     const struct clk_div_table *table;
> >       spinlock_t      *lock;
> > +     struct clk_div_table table[];
> >  };
> >
> >  #define to_hi6220_clk_divider(_hw)   \
> > @@ -108,24 +108,19 @@ struct clk *hi6220_register_clkdiv(struct device *dev, const char *name,
> >       u32 max_div, min_div;
> >       int i;
> >
> > -     /* allocate the divider */
> > -     div = kzalloc_obj(*div);
> > -     if (!div)
> > -             return ERR_PTR(-ENOMEM);
> > -
> >       /* Init the divider table */
> >       max_div = div_mask(width) + 1;
> >       min_div = 1;
> >
> > -     table = kzalloc_objs(*table, max_div + 1);
> > -     if (!table) {
> > -             kfree(div);
> > +     /* allocate the divider */
> > +     div = kzalloc_flex(*div, table, max_div + 1);
> > +     if (!div)
> >               return ERR_PTR(-ENOMEM);
> > -     }
> >
> >       for (i = 0; i < max_div; i++) {
> > -             table[i].div = min_div + i;
> > -             table[i].val = table[i].div - 1;
> > +             table = &div->table[i];
> > +             table->div = min_div + i;
> > +             table->val = table[i].div - 1;
>                              ^^^^^^^^^^^^^^^^^
> Sashiko has a question:
> https://sashiko.dev/#/patchset/20260326042317.122536-1-rosenp%40gmail.com
>
>     Does this code read out-of-bounds and write incorrect values to the table?
>
>     Because table is reassigned to point to &div->table[i], accessing table[i].div
>     in the following line effectively evaluates to div->table[2 * i].div.
>
>     For any i > 0, this reads uninitialized memory from further down the array,
>     and when 2 * i >= max_div + 1, it reads beyond the newly allocated flex array.
>     Could we use table->val = table->div - 1 instead?
yep. left that out accidentally.
>
> Brian
>
> >       }
> >
> >       init.name = name;
> > @@ -141,14 +136,11 @@ struct clk *hi6220_register_clkdiv(struct device *dev, const char *name,
> >       div->mask = mask_bit ? BIT(mask_bit) : 0;
> >       div->lock = lock;
> >       div->hw.init = &init;
> > -     div->table = table;
> >
> >       /* register the clock */
> >       clk = clk_register(dev, &div->hw);
> > -     if (IS_ERR(clk)) {
> > -             kfree(table);
> > +     if (IS_ERR(clk))
> >               kfree(div);
> > -     }
> >
> >       return clk;
> >  }
> > --
> > 2.53.0
> >
>