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

[Arnd Bergmann]

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.

> @@ -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.

>         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