[bug report] clk: visconti: Add support common clock driver and reset driver

[bug report] clk: visconti: Add support common clock driver and reset driver

[Dan Carpenter]

Hello Nobuhiro Iwamatsu,

The patch b4cbe606dc36: "clk: visconti: Add support common clock
driver and reset driver" from Oct 25, 2021, leads to the following
Smatch static checker warning:

	drivers/clk/visconti/clkc.c:150 visconti_clk_register_gates()
	warn: always true condition '(clks[i]->rs_id >= 0) => (0-255 >= 0)'

drivers/clk/visconti/clkc.c
    124 int visconti_clk_register_gates(struct visconti_clk_provider *ctx,
    125                                 const struct visconti_clk_gate_table *clks,
    126                                 int num_gate,
    127                                 const struct visconti_reset_data *reset,
    128                                 spinlock_t *lock)
    129 {
    130         struct device *dev = ctx->dev;
    131         int i;
    132 
    133         for (i = 0; i < num_gate; i++) {
    134                 const char *parent_div_name = clks[i].parent_data[0].name;
    135                 struct clk_parent_data *pdata;
    136                 u32 rson_offset, rsoff_offset;
    137                 struct clk_hw *gate_clk;
    138                 struct clk_hw *div_clk;
    139                 char *dev_name;
    140                 u8 rs_idx;
    141 
    142                 pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
    143                 if (!pdata)
    144                         return -ENOMEM;
    145 
    146                 dev_name = devm_kasprintf(dev, GFP_KERNEL, "%s_div", clks[i].name);
    147                 if (!dev_name)
    148                         return -ENOMEM;
    149 
--> 150                 if (clks[i].rs_id >= 0) {

This is a u8.  But it does get set to -1 for PISYSTEM in clkc-tmpv770x.c

> drivers/clk/visconti/clkc-tmpv770x.c
>   175                /* PISYSTEM */
>   176                { TMPV770X_CLK_WRCK, "wrck",
>   177                        clks_parent_data, ARRAY_SIZE(clks_parent_data),
>   178                        0, 0x68, 0x168, 9, 32,
>   179                        -1, }, /* No reset */

    151                         rson_offset = reset[clks[i].rs_id].rson_offset;
    152                         rsoff_offset = reset[clks[i].rs_id].rsoff_offset;
    153                         rs_idx = reset[clks[i].rs_id].rs_idx;
    154                 } else {
    155                         rson_offset = rsoff_offset = rs_idx = -1;

All these variables are unsigned but they're set to -1.  clks[i].rs_idx
is also unsigned.

    156                 }
    157 
    158                 div_clk = devm_clk_hw_register_fixed_factor(dev,
    159                                                             dev_name,
    160                                                             parent_div_name,
    161                                                             0, 1,
    162                                                             clks[i].div);
    163                 if (IS_ERR(div_clk))
    164                         return PTR_ERR(div_clk);
    165 
    166                 gate_clk = visconti_clk_register_gate(dev,
    167                                                       clks[i].name,
    168                                                       dev_name,
    169                                                       ctx->regmap,
    170                                                       &clks[i],
    171                                                       rson_offset,
    172                                                       rsoff_offset,
    173                                                       rs_idx,
    174                                                       lock);
    175                 if (IS_ERR(gate_clk)) {
    176                         dev_err(dev, "%s: failed to register clock %s\n",
    177                                 __func__, clks[i].name);
    178                         return PTR_ERR(gate_clk);
    179                 }
    180 
    181                 ctx->clk_data.hws[clks[i].id] = gate_clk;
    182         }
    183 
    184         return 0;
    185 }

regards,
dan carpenter

Re: [bug report] clk: visconti: Add support common clock driver and reset driver

[Stephen Boyd]

Quoting Dan Carpenter (2022-01-11 00:14:35)
> Hello Nobuhiro Iwamatsu,
> 
> The patch b4cbe606dc36: "clk: visconti: Add support common clock
> driver and reset driver" from Oct 25, 2021, leads to the following
> Smatch static checker warning:
> 
>         drivers/clk/visconti/clkc.c:150 visconti_clk_register_gates()
>         warn: always true condition '(clks[i]->rs_id >= 0) => (0-255 >= 0)'

Is there any patch to fix this?

[bug report] clk: visconti: Add support common clock driver and reset driver

[Dan Carpenter]

Hello Kernel Hardenning developers,

Commit b4cbe606dc36 ("clk: visconti: Add support common clock driver
and reset driver") from Oct 25, 2021 (linux-next), leads to the
question:

drivers/clk/visconti/clkc.c
    187 struct visconti_clk_provider *visconti_init_clk(struct device *dev,
    188                                                 struct regmap *regmap,
    189                                                 unsigned long nr_clks)
    190 {
    191         struct visconti_clk_provider *ctx;
    192         int i;
    193 
    194         ctx = devm_kzalloc(dev, struct_size(ctx, clk_data.hws, nr_clks), GFP_KERNEL);
    195         if (!ctx)
    196                 return ERR_PTR(-ENOMEM);
    197 
    198         for (i = 0; i < nr_clks; ++i)
--> 199                 ctx->clk_data.hws[i] = ERR_PTR(-ENOENT);
    200         ctx->clk_data.num = nr_clks;

ctx->clk_data.hws[] is __counted_by() ctx->clk_data.num.  Don't we have to
set the .num before we fill initialize the array?  Or does the checker
code allow us to access the array when the counted by variable is zero?
I seem to remember this used to be a common false positive with the counted
by checking.

    201 
    202         ctx->dev = dev;
    203         ctx->regmap = regmap;
    204 
    205         return ctx;
    206 }

regards,
dan carpenter

Re: [bug report] clk: visconti: Add support common clock driver and reset driver

[Nathan Chancellor]

Hey Dan,

On Tue, Dec 09, 2025 at 04:05:58PM +0300, Dan Carpenter wrote:
> Hello Kernel Hardenning developers,
> 
> Commit b4cbe606dc36 ("clk: visconti: Add support common clock driver
> and reset driver") from Oct 25, 2021 (linux-next), leads to the
> question:
> 
> drivers/clk/visconti/clkc.c
>     187 struct visconti_clk_provider *visconti_init_clk(struct device *dev,
>     188                                                 struct regmap *regmap,
>     189                                                 unsigned long nr_clks)
>     190 {
>     191         struct visconti_clk_provider *ctx;
>     192         int i;
>     193 
>     194         ctx = devm_kzalloc(dev, struct_size(ctx, clk_data.hws, nr_clks), GFP_KERNEL);
>     195         if (!ctx)
>     196                 return ERR_PTR(-ENOMEM);
>     197 
>     198         for (i = 0; i < nr_clks; ++i)
> --> 199                 ctx->clk_data.hws[i] = ERR_PTR(-ENOENT);
>     200         ctx->clk_data.num = nr_clks;
> 
> ctx->clk_data.hws[] is __counted_by() ctx->clk_data.num.  Don't we have to
> set the .num before we fill initialize the array?  Or does the checker

Yes, it looks like line 200 needs to be moved above line 198.

> code allow us to access the array when the counted by variable is zero?

Nope, I just had to fix an instance where num is one and hws[0] is being
accessed:

  https://lore.kernel.org/20251124-exynos-clkout-fix-ubsan-bounds-error-v1-1-224a5282514b@kernel.org/

I suspect there will eventually be a report on real hardware when
CONFIG_UBSAN_BOUNDS is enabled and GCC 15 is used to build the running
kernel, which is the first version that supports __counted_by(). How did
you find this? A new smatch check? It would be nice if the compilers
could flag this but I guess it is probably harder to do this in a
generic way.

Cheers,
Nathan

Re: [bug report] clk: visconti: Add support common clock driver and reset driver

[Dan Carpenter]

On Wed, Dec 10, 2025 at 04:36:11PM +0900, Nathan Chancellor wrote:
> Hey Dan,
> 
> On Tue, Dec 09, 2025 at 04:05:58PM +0300, Dan Carpenter wrote:
> > Hello Kernel Hardenning developers,
> > 
> > Commit b4cbe606dc36 ("clk: visconti: Add support common clock driver
> > and reset driver") from Oct 25, 2021 (linux-next), leads to the
> > question:
> > 
> > drivers/clk/visconti/clkc.c
> >     187 struct visconti_clk_provider *visconti_init_clk(struct device *dev,
> >     188                                                 struct regmap *regmap,
> >     189                                                 unsigned long nr_clks)
> >     190 {
> >     191         struct visconti_clk_provider *ctx;
> >     192         int i;
> >     193 
> >     194         ctx = devm_kzalloc(dev, struct_size(ctx, clk_data.hws, nr_clks), GFP_KERNEL);
> >     195         if (!ctx)
> >     196                 return ERR_PTR(-ENOMEM);
> >     197 
> >     198         for (i = 0; i < nr_clks; ++i)
> > --> 199                 ctx->clk_data.hws[i] = ERR_PTR(-ENOENT);
> >     200         ctx->clk_data.num = nr_clks;
> > 
> > ctx->clk_data.hws[] is __counted_by() ctx->clk_data.num.  Don't we have to
> > set the .num before we fill initialize the array?  Or does the checker
> 
> Yes, it looks like line 200 needs to be moved above line 198.
> 
> > code allow us to access the array when the counted by variable is zero?
> 
> Nope, I just had to fix an instance where num is one and hws[0] is being
> accessed:
> 
>   https://lore.kernel.org/20251124-exynos-clkout-fix-ubsan-bounds-error-v1-1-224a5282514b@kernel.org/
> 
> I suspect there will eventually be a report on real hardware when
> CONFIG_UBSAN_BOUNDS is enabled and GCC 15 is used to build the running
> kernel, which is the first version that supports __counted_by(). How did
> you find this? A new smatch check?

Yeah, but it was a bug in my check.  I need to add __counted_by()
support to Sparse.  If I had that, it would easy enough to add a
warning for places which access the array before setting the counted
by variable.

regards,
carpenter