[PATCH v2] soc: brcmstb: pm-arm: Fix refcount leak and __iomem leak bugs
Liang He
windhl at 126.com
Wed Jul 6 07:30:54 PDT 2022
At 2022-07-06 22:16:31, "Arnd Bergmann" <arnd at arndb.de> wrote:
>On Wed, Jul 6, 2022 at 10:34 AM Liang He <windhl at 126.com> wrote:
>>
>> In brcmstb_pm_probe(), there are two kinds of leak bugs:
>>
>> (1) we need to add of_node_put() when for_each__matching_node() breaks
>> (2) we need to add iounmap() for each iomap in fail path
>>
>> Fixes: 0b741b8234c8 ("soc: bcm: brcmstb: Add support for S2/S3/S5 suspend states (ARM)")
>> Signed-off-by: Liang He <windhl at 126.com>
>> ---
>> changelog:
>>
>> v2: fix __iomem leak bug advised by Arnd
>
>This looks mostly ok, but some of it is not very readable.
>
>> @@ -711,7 +714,8 @@ static int brcmstb_pm_probe(struct platform_device *pdev)
>> (const void **)&ddr_phy_data);
>> if (IS_ERR(base)) {
>> pr_err("error mapping DDR PHY\n");
>> - return PTR_ERR(base);
>> + ret = PTR_ERR(base);
>> + goto ddr_phy_err;
>> }
>> ctrl.support_warm_boot = ddr_phy_data->supports_warm_boot;
>> ctrl.pll_status_offset = ddr_phy_data->pll_status_offset;
>> @@ -727,21 +731,25 @@ static int brcmstb_pm_probe(struct platform_device *pdev)
>> */
>> ctrl.warm_boot_offset = ddr_phy_data->warm_boot_offset;
>>
>> + j = ctrl.num_memc;
>
>I think the driver is confusing because it only supports a single instance and
>stores the data in a global 'ctrl' structure. When you get into the probe
>function, the value here should always be '0'. This also means it won't
>work correctly when it runs across deferred probing (you can ignore that
>bug, but maybe someone else should address this).
>
>Removing the 'j' variable from the function would make this more readable.
>
Hi, Arnd,
Based on your explaination, removing 'j' also means following patch 'for (i = j; i < ctrl.num_memc; i++)' to release
'ctrl.memcs[i].ddr_shimphy_base' should start from i=0, right?
If yes, I will send a new version patch.
>> @@ -779,21 +791,24 @@ static int brcmstb_pm_probe(struct platform_device *pdev)
>> dn = of_find_matching_node(NULL, sram_dt_ids);
>> if (!dn) {
>> pr_err("SRAM not found\n");
>> - return -EINVAL;
>> + ret = -EINVAL;
>> + goto brcmstb_memc_err;
>> }
>>
>> ret = brcmstb_init_sram(dn);
>> of_node_put(dn);
>
>There is another ioremap inside of brcmstb_init_sram() that should be cleaned up
>in the error path if the kmalloc fails or dma_map_single fails.
>
Thanks, I will add 'iounmap(ctrl->ctrl.boot_sram)' in fail path.
>> if (ret) {
>> pr_err("error setting up SRAM for PM\n");
>> - return ret;
>> + goto brcmstb_memc_err;
>> }
>>
>> ctrl.pdev = pdev;
>>
>> ctrl.s3_params = kmalloc(sizeof(*ctrl.s3_params), GFP_KERNEL);
>> - if (!ctrl.s3_params)
>> - return -ENOMEM;
>> + if (!ctrl.s3_params) {
>> + ret = -ENOMEM;
>> + goto brcmstb_memc_err;
>> + }
>> ctrl.s3_params_pa = dma_map_single(&pdev->dev, ctrl.s3_params,
>> sizeof(*ctrl.s3_params),
>> DMA_TO_DEVICE);
>
> Arnd
Thanks all your help,
Liang
More information about the linux-arm-kernel
mailing list