Re: [bug report] clk: scmi: Allocate CLK operations dynamically

[Cristian Marussi <cristian.marussi@arm.com>]

On Tue, Feb 20, 2024 at 01:13:21PM +0300, Dan Carpenter wrote:
> Hello Cristian Marussi,
> 
Hi,

> The patch 11f5b1a48a70: "clk: scmi: Allocate CLK operations
> dynamically" from Feb 14, 2024 (linux-next), leads to the following
> Smatch static checker warning:
> 
> 	drivers/clk/clk-scmi.c:362 scmi_clocks_probe()
> 	error: uninitialized symbol 'atomic_threshold'.
> 
> drivers/clk/clk-scmi.c
>     307 static int scmi_clocks_probe(struct scmi_device *sdev)
>     308 {
>     309         int idx, count, err;
>     310         unsigned int atomic_threshold;
>     311         bool is_atomic;
>     312         struct clk_hw **hws;
>     313         struct clk_hw_onecell_data *clk_data;
>     314         struct device *dev = &sdev->dev;
>     315         struct device_node *np = dev->of_node;
>     316         const struct scmi_handle *handle = sdev->handle;
>     317         struct scmi_protocol_handle *ph;
>     318 
>     319         if (!handle)
>     320                 return -ENODEV;
>     321 
>     322         scmi_proto_clk_ops =
>     323                 handle->devm_protocol_get(sdev, SCMI_PROTOCOL_CLOCK, &ph);
>     324         if (IS_ERR(scmi_proto_clk_ops))
>     325                 return PTR_ERR(scmi_proto_clk_ops);
>     326 
>     327         count = scmi_proto_clk_ops->count_get(ph);
>     328         if (count < 0) {
>     329                 dev_err(dev, "%pOFn: invalid clock output count\n", np);
>     330                 return -EINVAL;
>     331         }
>     332 
>     333         clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, count),
>     334                                 GFP_KERNEL);
>     335         if (!clk_data)
>     336                 return -ENOMEM;
>     337 
>     338         clk_data->num = count;
>     339         hws = clk_data->hws;
>     340 
>     341         is_atomic = handle->is_transport_atomic(handle, &atomic_threshold);
> 
> atomic_threshold is not initialized when is_atomic is false.
>

yes

>     342 
>     343         for (idx = 0; idx < count; idx++) {
>     344                 struct scmi_clk *sclk;
>     345                 const struct clk_ops *scmi_ops;
>     346 
>     347                 sclk = devm_kzalloc(dev, sizeof(*sclk), GFP_KERNEL);
>     348                 if (!sclk)
>     349                         return -ENOMEM;
>     350 
>     351                 sclk->info = scmi_proto_clk_ops->info_get(ph, idx);
>     352                 if (!sclk->info) {
>     353                         dev_dbg(dev, "invalid clock info for idx %d\n", idx);
>     354                         devm_kfree(dev, sclk);
>     355                         continue;
>     356                 }
>     357 
>     358                 sclk->id = idx;
>     359                 sclk->ph = ph;
>     360                 sclk->dev = dev;
>     361 
> --> 362                 scmi_ops = scmi_clk_ops_alloc(sclk, is_atomic, atomic_threshold);
>                                                             ^^^^^^^^^^ ^^^^^^^^^^^^^^^^
> Here we're passing uninitialized data, but it's only used when
> is_atomic is true.  This is okay if scmi_clk_ops_alloc() is inlined,
> but it is considered a bug when it's not inlined.
>

Ok, I will add an explicit initialization to mute smatch but why is it
this considered a bug only when not inlined, because it cannot verify
that it is indeed not used when uninitialized ?

Thanks,
Cristian

>     363                 if (!scmi_ops)
>     364                         return -ENOMEM;
>     365 
>     366                 /* Initialize clock parent data. */
>     367                 if (sclk->info->num_parents > 0) {
>     368                         sclk->parent_data = devm_kcalloc(dev, sclk->info->num_parents,
>     369                                                          sizeof(*sclk->parent_data), GFP_KERNEL);
>     370                         if (!sclk->parent_data)
>     371                                 return -ENOMEM;
>     372 
>     373                         for (int i = 0; i < sclk->info->num_parents; i++) {
>     374                                 sclk->parent_data[i].index = sclk->info->parents[i];
>     375                                 sclk->parent_data[i].hw = hws[sclk->info->parents[i]];
>     376                         }
>     377                 }
>     378 
>     379                 err = scmi_clk_ops_init(dev, sclk, scmi_ops);
>     380                 if (err) {
>     381                         dev_err(dev, "failed to register clock %d\n", idx);
>     382                         devm_kfree(dev, sclk->parent_data);
>     383                         devm_kfree(dev, scmi_ops);
>     384                         devm_kfree(dev, sclk);
>     385                         hws[idx] = NULL;
>     386                 } else {
>     387                         dev_dbg(dev, "Registered clock:%s%s\n",
>     388                                 sclk->info->name,
>     389                                 scmi_ops->enable ? " (atomic ops)" : "");
>     390                         hws[idx] = &sclk->hw;
>     391                 }
>     392         }
>     393 
>     394         return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>     395                                            clk_data);
>     396 }
> 
> regards,
> dan carpenter