Messages in this thread | | | Date | Fri, 1 Jul 2022 22:38:09 +0800 (CST) | From | huhai <> | Subject | Re:Re: [PATCH] firmware: arm_scpi: Fix error handle when scpi probe failed |
| |
>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
> }
| |