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

[Dan Carpenter]

Hello Cristian Marussi,

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/


    2706 
    2707                 if (IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT) && info->dbg) {
    2708                         int id, num_chans = 0;
    2709                         struct scmi_chan_info *cinfo;
    2710                         u8 channels[SCMI_MAX_CHANNELS] = {};
    2711                         DECLARE_BITMAP(protos, SCMI_MAX_CHANNELS) = {};
    2712 
    2713                         /* Enumerate all channels to collect their ids */
    2714                         idr_for_each_entry(&info->tx_idr, cinfo, id) {
    2715                                 /*
    2716                                  * Cannot happen, but be defensive.
    2717                                  * Zero as num_chans is ok, warn and carry on.
    2718                                  */
    2719                                 if (num_chans >= SCMI_MAX_CHANNELS || !cinfo) {
    2720                                         dev_warn(dev,
    2721                                                  "SCMI RAW - Error enumerating channels\n");
    2722                                         break;
    2723                                 }
    2724 
    2725                                 if (!test_bit(cinfo->id, protos)) {
    2726                                         channels[num_chans++] = cinfo->id;
    2727                                         set_bit(cinfo->id, protos);
    2728                                 }
    2729                         }
    2730 
    2731                         info->raw = scmi_raw_mode_init(handle,
--> 2732                                                        info->dbg->top_dentry,
                                                                ^^^^^^^^^^^
Error pointer dereference.


    2733                                                        info->id,
    2734                                                        channels, num_chans,
    2735                                                        info->desc,
    2736                                                        info->tx_minfo.max_msg);
    2737                         if (IS_ERR(info->raw)) {
    2738                                 dev_err(dev, "Failed to initialize SCMI RAW Mode !\n");
    2739 
    2740                                 ret = PTR_ERR(info->raw);
    2741                         }
    2742 
    2743                         if (!IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT_COEX)) {
    2744                                 if (!ret)
    2745                                         return ret;
    2746                                 /*
    2747                                  * Bail out if we failed to init raw mode and
    2748                                  * RAW_MODE_SUPPORT_COEX was not configured.
    2749                                  */
    2750                                 goto clear_dev_req_notifier;
    2751                         }
    2752 

regards,
dan carpenter