diff mbox series

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

Message ID 20220706083433.303135-1-windhl@126.com (mailing list archive)
State New, archived
Headers show
Series [v2] soc: brcmstb: pm-arm: Fix refcount leak and __iomem leak bugs | expand

Commit Message

Liang He July 6, 2022, 8:34 a.m. UTC 
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@126.com>
---
 changelog:

 v2: fix __iomem leak bug advised by Arnd
 v1: fix refcount leak bug

 v1 link: https://lore.kernel.org/all/20220705112815.282835-1-windhl@126.com/

 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 48 +++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 10 deletions(-)

Comments

Arnd Bergmann July 6, 2022, 2:16 p.m. UTC | #1 
On Wed, Jul 6, 2022 at 10:34 AM Liang He <windhl@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@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
Liang He July 6, 2022, 2:30 p.m. UTC | #2 
At 2022-07-06 22:16:31, "Arnd Bergmann" <arnd@arndb.de> wrote:
>On Wed, Jul 6, 2022 at 10:34 AM Liang He <windhl@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@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
Arnd Bergmann July 6, 2022, 2:51 p.m. UTC | #3 
On Wed, Jul 6, 2022 at 4:30 PM Liang He <windhl@126.com> wrote:
> >
> >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?
>

Yes, exactly.

        Arnd
diff mbox series

Patch

diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
index 70ad0f3dce28..425b851fd770 100644
--- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
+++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
@@ -684,13 +684,14 @@  static int brcmstb_pm_probe(struct platform_device *pdev)
 	const struct of_device_id *of_id = NULL;
 	struct device_node *dn;
 	void __iomem *base;
-	int ret, i;
+	int ret, i, j, s;
 
 	/* AON ctrl registers */
 	base = brcmstb_ioremap_match(aon_ctrl_dt_ids, 0, NULL);
 	if (IS_ERR(base)) {
 		pr_err("error mapping AON_CTRL\n");
-		return PTR_ERR(base);
+		ret = PTR_ERR(base);
+		goto aon_err;
 	}
 	ctrl.aon_ctrl_base = base;
 
@@ -700,8 +701,10 @@  static int brcmstb_pm_probe(struct platform_device *pdev)
 		/* Assume standard offset */
 		ctrl.aon_sram = ctrl.aon_ctrl_base +
 				     AON_CTRL_SYSTEM_DATA_RAM_OFS;
+		s = 0;
 	} else {
 		ctrl.aon_sram = base;
+		s = 1;
 	}
 
 	writel_relaxed(0, ctrl.aon_sram + AON_REG_PANIC);
@@ -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;
 	/* DDR SHIM-PHY registers */
 	for_each_matching_node(dn, ddr_shimphy_dt_ids) {
 		i = ctrl.num_memc;
 		if (i >= MAX_NUM_MEMC) {
+			of_node_put(dn);
 			pr_warn("too many MEMCs (max %d)\n", MAX_NUM_MEMC);
 			break;
 		}
 
 		base = of_io_request_and_map(dn, 0, dn->full_name);
 		if (IS_ERR(base)) {
+			of_node_put(dn);
 			if (!ctrl.support_warm_boot)
 				break;
 
 			pr_err("error mapping DDR SHIMPHY %d\n", i);
-			return PTR_ERR(base);
+			ret = PTR_ERR(base);
+			goto ddr_shimphy_err;
 		}
 		ctrl.memcs[i].ddr_shimphy_base = base;
 		ctrl.num_memc++;
@@ -752,14 +760,18 @@  static int brcmstb_pm_probe(struct platform_device *pdev)
 	for_each_matching_node(dn, brcmstb_memc_of_match) {
 		base = of_iomap(dn, 0);
 		if (!base) {
+			of_node_put(dn);
 			pr_err("error mapping DDR Sequencer %d\n", i);
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto brcmstb_memc_err;
 		}
 
 		of_id = of_match_node(brcmstb_memc_of_match, dn);
 		if (!of_id) {
 			iounmap(base);
-			return -EINVAL;
+			of_node_put(dn);
+			ret = -EINVAL;
+			goto brcmstb_memc_err;
 		}
 
 		ddr_seq_data = of_id->data;
@@ -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);
 	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);
@@ -816,6 +831,19 @@  static int brcmstb_pm_probe(struct platform_device *pdev)
 
 	pr_warn("PM: initialization failed with code %d\n", ret);
 
+brcmstb_memc_err:
+	for (i--; i >= 0; i--)
+		iounmap(ctrl.memcs[i].ddr_ctrl);
+ddr_shimphy_err:
+	for (i = j; i < ctrl.num_memc; i++)
+		iounmap(ctrl.memcs[i].ddr_shimphy_base);
+
+	iounmap(ctrl.memcs[0].ddr_phy_base);
+ddr_phy_err:
+	iounmap(ctrl.aon_ctrl_base);
+	if (s)
+		iounmap(ctrl.aon_sram);
+aon_err:
 	return ret;
 }