* [bug report] clk: scmi: Allocate CLK operations dynamically
@ 2024-02-20 10:13 Dan Carpenter
2024-02-20 11:09 ` Cristian Marussi
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2024-02-20 10:13 UTC (permalink / raw)
To: cristian.marussi; +Cc: linux-clk
Hello Cristian Marussi,
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.
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.
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
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [bug report] clk: scmi: Allocate CLK operations dynamically
2024-02-20 10:13 [bug report] clk: scmi: Allocate CLK operations dynamically Dan Carpenter
@ 2024-02-20 11:09 ` Cristian Marussi
0 siblings, 0 replies; 2+ messages in thread
From: Cristian Marussi @ 2024-02-20 11:09 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-clk
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-02-20 11:09 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-20 10:13 [bug report] clk: scmi: Allocate CLK operations dynamically Dan Carpenter
2024-02-20 11:09 ` Cristian Marussi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox