Re: [bug report] firmware: arm_scmi: Call Raw mode hooks from the core stack

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

On Tue, Jan 17, 2023 at 01:29:13PM +0300, Dan Carpenter wrote:
> Hello Cristian Marussi,
> 
Hi Dan,

> The patch ffb07d58dcba: "firmware: arm_scmi: Call Raw mode hooks from
> the core stack" from Jan 13, 2023, leads to the following Smatch
> static checker warning:
> 
> 	drivers/firmware/arm_scmi/driver.c:2732 scmi_probe()
> 	error: 'info->dbg' dereferencing possible ERR_PTR()
> 
> drivers/firmware/arm_scmi/driver.c
>     2630 static int scmi_probe(struct platform_device *pdev)
>     2631 {
>     2632         int ret;
>     2633         struct scmi_handle *handle;
>     2634         const struct scmi_desc *desc;
>     2635         struct scmi_info *info;
>     2636         struct device *dev = &pdev->dev;
>     2637         struct device_node *child, *np = dev->of_node;
>     2638 
>     2639         desc = of_device_get_match_data(dev);
>     2640         if (!desc)
>     2641                 return -EINVAL;
>     2642 
>     2643         info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>     2644         if (!info)
>     2645                 return -ENOMEM;
>     2646 
>     2647         info->id = ida_alloc_min(&scmi_id, 0, GFP_KERNEL);
>     2648         if (info->id < 0)
>     2649                 return info->id;
>     2650 
>     2651         info->dev = dev;
>     2652         info->desc = desc;
>     2653         info->bus_nb.notifier_call = scmi_bus_notifier;
>     2654         info->dev_req_nb.notifier_call = scmi_device_request_notifier;
>     2655         INIT_LIST_HEAD(&info->node);
>     2656         idr_init(&info->protocols);
>     2657         mutex_init(&info->protocols_mtx);
>     2658         idr_init(&info->active_protocols);
>     2659         mutex_init(&info->devreq_mtx);
>     2660 
>     2661         platform_set_drvdata(pdev, info);
>     2662         idr_init(&info->tx_idr);
>     2663         idr_init(&info->rx_idr);
>     2664 
>     2665         handle = &info->handle;
>     2666         handle->dev = info->dev;
>     2667         handle->version = &info->version;
>     2668         handle->devm_protocol_acquire = scmi_devm_protocol_acquire;
>     2669         handle->devm_protocol_get = scmi_devm_protocol_get;
>     2670         handle->devm_protocol_put = scmi_devm_protocol_put;
>     2671 
>     2672         /* System wide atomic threshold for atomic ops .. if any */
>     2673         if (!of_property_read_u32(np, "atomic-threshold-us",
>     2674                                   &info->atomic_threshold))
>     2675                 dev_info(dev,
>     2676                          "SCMI System wide atomic threshold set to %d us\n",
>     2677                          info->atomic_threshold);
>     2678         handle->is_transport_atomic = scmi_is_transport_atomic;
>     2679 
>     2680         if (desc->ops->link_supplier) {
>     2681                 ret = desc->ops->link_supplier(dev);
>     2682                 if (ret)
>     2683                         goto clear_ida;
>     2684         }
>     2685 
>     2686         /* Setup all channels described in the DT at first */
>     2687         ret = scmi_channels_setup(info);
>     2688         if (ret)
>     2689                 goto clear_ida;
>     2690 
>     2691         ret = bus_register_notifier(&scmi_bus_type, &info->bus_nb);
>     2692         if (ret)
>     2693                 goto clear_txrx_setup;
>     2694 
>     2695         ret = blocking_notifier_chain_register(&scmi_requested_devices_nh,
>     2696                                                &info->dev_req_nb);
>     2697         if (ret)
>     2698                 goto clear_bus_notifier;
>     2699 
>     2700         ret = scmi_xfer_info_init(info);
>     2701         if (ret)
>     2702                 goto clear_dev_req_notifier;
>     2703 
>     2704         if (scmi_top_dentry) {
>     2705                 info->dbg = scmi_debugfs_common_setup(info);
> 
> The scmi_debugfs_common_setup() has messed up returns.
> 
> It returns both NULL and error pointers for errors.  It checks debugfs
> functions and the returns for those are generally supposed to be
> ignored.  I have written a blog about mixing error pointers and NULL
> which also explains this historical/psychology based reason why debugfs
> does not follow the normal pattern:
> 
> https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/
>

Right. I'll fix dropping all unneded debugfs checks and repost as a
reply patch so that Sudeep can re-submit on next quickly.

I'm still puzzled why I regularly do not catch these errors on my smatch
setup running kchecker which does only reports me a bunch of errors on
core bitops code. (I have recently pulled updated smatch branch...)

Thanks,
Cristian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel