[PATCH v2] soc: brcmstb: pm-arm: Fix refcount leak and __iomem leak bugs

[Liang He]

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