Re:Re: [PATCH] firmware: arm_scpi: Fix error handle when scpi probe failed

[huhai]

>On Fri, Jul 01, 2022 at 06:04:53PM +0800, huhai wrote:
>> 
>> scpi_info is a global variable， we must set it to NULL when it is not valid
>
>How about not assigning the global variable until the end of the probe ?

>Something like below:>
>Regards,
>Sudeep
>
>-->8
>
>diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
>index ddf0b9ff9e15..5463501735ff 100644
>--- i/drivers/firmware/arm_scpi.c
>+++ w/drivers/firmware/arm_scpi.c
>@@ -913,13 +913,14 @@ static int scpi_probe(struct platform_device *pdev)
>        struct resource res;
>        struct device *dev = &pdev->dev;
>        struct device_node *np = dev->of_node;
>+       struct scpi_drvinfo *scpi_drvinfo;
>
>-       scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
>-       if (!scpi_info)
>+       scpi_drvinfo = devm_kzalloc(dev, sizeof(*scpi_drvinfo), GFP_KERNEL);
>+       if (!scpi_drvinfo)
>                return -ENOMEM;
>
>        if (of_match_device(legacy_scpi_of_match, &pdev->dev))
>-               scpi_info->is_legacy = true;
>+               scpi_drvinfo->is_legacy = true;
>
>        count = of_count_phandle_with_args(np, "mboxes", "#mbox-cells");
>        if (count < 0) {
>@@ -927,19 +928,19 @@ static int scpi_probe(struct platform_device *pdev)
>                return -ENODEV;
>        }
>
>-       scpi_info->channels = devm_kcalloc(dev, count, sizeof(struct scpi_chan),
>-                                          GFP_KERNEL);
>-       if (!scpi_info->channels)
>+       scpi_drvinfo->channels =
>+               devm_kcalloc(dev, count, sizeof(struct scpi_chan), GFP_KERNEL);
>+       if (!scpi_drvinfo->channels)
>                return -ENOMEM;
>
>-       ret = devm_add_action(dev, scpi_free_channels, scpi_info);
>+       ret = devm_add_action(dev, scpi_free_channels, scpi_drvinfo);
>        if (ret)
>                return ret;
>
>-       for (; scpi_info->num_chans < count; scpi_info->num_chans++) {
>+       for (; scpi_drvinfo->num_chans < count; scpi_drvinfo->num_chans++) {
>                resource_size_t size;
>-               int idx = scpi_info->num_chans;
>-               struct scpi_chan *pchan = scpi_info->channels + idx;
>+               int idx = scpi_drvinfo->num_chans;
>+               struct scpi_chan *pchan = scpi_drvinfo->channels + idx;
>                struct mbox_client *cl = &pchan->cl;
>                struct device_node *shmem = of_parse_phandle(np, "shmem", idx);
>
>@@ -986,43 +987,44 @@ static int scpi_probe(struct platform_device *pdev)
>                return ret;
>        }
>
>-       scpi_info->commands = scpi_std_commands;
>+       scpi_drvinfo->commands = scpi_std_commands;
>
>-       platform_set_drvdata(pdev, scpi_info);
>+       platform_set_drvdata(pdev, scpi_drvinfo);
>
>-       if (scpi_info->is_legacy) {
>+       if (scpi_drvinfo->is_legacy) {
>                /* Replace with legacy variants */
>                scpi_ops.clk_set_val = legacy_scpi_clk_set_val;
>-               scpi_info->commands = scpi_legacy_commands;
>+               scpi_drvinfo->commands = scpi_legacy_commands;
>
>                /* Fill priority bitmap */
>                for (idx = 0; idx < ARRAY_SIZE(legacy_hpriority_cmds); idx++)
>                        set_bit(legacy_hpriority_cmds[idx],
>-                               scpi_info->cmd_priority);
>+                               scpi_drvinfo->cmd_priority);
>        }
>
>-       ret = scpi_init_versions(scpi_info);
>+       ret = scpi_init_versions(scpi_drvinfo);
>        if (ret) {
>                dev_err(dev, "incorrect or no SCP firmware found\n");
>                return ret;
>        }
>
>-       if (scpi_info->is_legacy && !scpi_info->protocol_version &&
>-           !scpi_info->firmware_version)
>+       if (scpi_drvinfo->is_legacy && !scpi_drvinfo->protocol_version &&
>+           !scpi_drvinfo->firmware_version)
>                dev_info(dev, "SCP Protocol legacy pre-1.0 firmware\n");
>        else
>                dev_info(dev, "SCP Protocol %lu.%lu Firmware %lu.%lu.%lu version\n",
>                         FIELD_GET(PROTO_REV_MAJOR_MASK,
>-                                  scpi_info->protocol_version),
>+                                  scpi_drvinfo->protocol_version),
>                         FIELD_GET(PROTO_REV_MINOR_MASK,
>-                                  scpi_info->protocol_version),
>+                                  scpi_drvinfo->protocol_version),
>                         FIELD_GET(FW_REV_MAJOR_MASK,
>-                                  scpi_info->firmware_version),
>+                                  scpi_drvinfo->firmware_version),
>                         FIELD_GET(FW_REV_MINOR_MASK,
>-                                  scpi_info->firmware_version),
>+                                  scpi_drvinfo->firmware_version),
>                         FIELD_GET(FW_REV_PATCH_MASK,
>-                                  scpi_info->firmware_version));
>-       scpi_info->scpi_ops = &scpi_ops;
>+                                  scpi_drvinfo->firmware_version));
>+       scpi_drvinfo->scpi_ops = &scpi_ops;

>+       scpi_info = scpi_drvinfo;

Yes, I think this patch will work well until it runs here.

>

>        return devm_of_platform_populate(dev);

we should not return devm_of_platform_populate() directly, because devm_of_platform_populate()
may fails,  if it fails,  the scpi_info will pointing to free-ed memory.

as you said before:
-       return devm_of_platform_populate(dev);
+       devm_of_platform_populate(dev);
+
+       return ret;
will work well.

thanks

> }