[v5] soc/fsl/qe: fix err handling of ucc_of_parse_tdm
diff mbox series

Message ID 1546446223-17184-1-git-send-email-peng.hao2@zte.com.cn
State Superseded
Delegated to: David Miller
Headers show
Series
  • [v5] soc/fsl/qe: fix err handling of ucc_of_parse_tdm
Related show

Commit Message

Peng Hao Jan. 2, 2019, 4:23 p.m. UTC
From: Wen Yang <wen.yang99@zte.com.cn>

Currently there are some issues with the ucc_of_parse_tdm function:
1, a possible null pointer dereference in ucc_of_parse_tdm,
detected by the semantic patch deref_null.cocci,
with the following warning:
drivers/soc/fsl/qe/qe_tdm.c:177:21-24: ERROR: pdev is NULL but dereferenced.
2, dev gets modified, so in any case that devm_iounmap() will fail
even when the new pdev is valid, because the iomap was done with a
 different pdev.
3, there is no driver bind with the "fsl,t1040-qe-si" or
"fsl,t1040-qe-siram" device. So allocating resources using devm_*()
with these devices won't provide a cleanup path for these resources
when the caller fails.

This patch fixes them.

Suggested-by: Li Yang <leoyang.li@nxp.com>
Suggested-by: Christophe LEROY <christophe.leroy@c-s.fr>
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Reviewed-by: Peng Hao <peng.hao2@zte.com.cn>
CC: Julia Lawall <julia.lawall@lip6.fr>
CC: Zhao Qiang <qiang.zhao@nxp.com>
CC: David S. Miller <davem@davemloft.net>
CC: netdev@vger.kernel.org
CC: linuxppc-dev@lists.ozlabs.org
CC: linux-kernel@vger.kernel.org
---
 drivers/net/wan/fsl_ucc_hdlc.c | 62 +++++++++++++++++++++++++++++++++++++++++-
 drivers/soc/fsl/qe/qe_tdm.c    | 55 -------------------------------------
 2 files changed, 61 insertions(+), 56 deletions(-)

Comments

Christophe Leroy Jan. 2, 2019, 8:14 a.m. UTC | #1
Peng Hao <peng.hao2@zte.com.cn> a écrit :

> From: Wen Yang <wen.yang99@zte.com.cn>
>
> Currently there are some issues with the ucc_of_parse_tdm function:
> 1, a possible null pointer dereference in ucc_of_parse_tdm,
> detected by the semantic patch deref_null.cocci,
> with the following warning:
> drivers/soc/fsl/qe/qe_tdm.c:177:21-24: ERROR: pdev is NULL but dereferenced.
> 2, dev gets modified, so in any case that devm_iounmap() will fail
> even when the new pdev is valid, because the iomap was done with a
>  different pdev.
> 3, there is no driver bind with the "fsl,t1040-qe-si" or
> "fsl,t1040-qe-siram" device. So allocating resources using devm_*()
> with these devices won't provide a cleanup path for these resources
> when the caller fails.
>
> This patch fixes them.
>
> Suggested-by: Li Yang <leoyang.li@nxp.com>
> Suggested-by: Christophe LEROY <christophe.leroy@c-s.fr>
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> Reviewed-by: Peng Hao <peng.hao2@zte.com.cn>
> CC: Julia Lawall <julia.lawall@lip6.fr>
> CC: Zhao Qiang <qiang.zhao@nxp.com>
> CC: David S. Miller <davem@davemloft.net>
> CC: netdev@vger.kernel.org
> CC: linuxppc-dev@lists.ozlabs.org
> CC: linux-kernel@vger.kernel.org
> ---

In order to ease review, could add the list of changes between each  
version of the patch ? Usually we do it at this place so that it is  
available for reviewers but not part of the commit text.

Christophe

>  drivers/net/wan/fsl_ucc_hdlc.c | 62  
> +++++++++++++++++++++++++++++++++++++++++-
>  drivers/soc/fsl/qe/qe_tdm.c    | 55 -------------------------------------
>  2 files changed, 61 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 839fa77..f30a040 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -1057,6 +1057,54 @@ static const struct net_device_ops uhdlc_ops = {
>  	.ndo_tx_timeout	= uhdlc_tx_timeout,
>  };
>
> +static int hdlc_map_iomem(char *name, int init_flag, void __iomem **ptr)
> +{
> +	struct device_node *np;
> +	struct platform_device *pdev;
> +	struct resource *res;
> +	static int siram_init_flag;
> +	int ret = 0;
> +
> +	np = of_find_compatible_node(NULL, NULL, name);
> +	if (!np)
> +		return -EINVAL;
> +
> +	pdev = of_find_device_by_node(np);
> +	if (!pdev) {
> +		pr_err("%pOFn: failed to lookup pdev\n", np);
> +		of_node_put(np);
> +		return -EINVAL;
> +	}
> +
> +	of_node_put(np);
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		ret = -EINVAL;
> +		goto error_put_device;
> +	}
> +	*ptr = ioremap(res->start, resource_size(res));
> +	if (!*ptr) {
> +		ret = -ENOMEM;
> +		goto error_put_device;
> +	}
> +
> +	/* We've remapped the addresses, and we don't need the device any
> +	 * more, so we should release it.
> +	 */
> +	put_device(&pdev->dev);
> +
> +	if (init_flag && siram_init_flag == 0) {
> +		memset_io(*ptr, 0, resource_size(res));
> +		siram_init_flag = 1;
> +	}
> +	return  0;
> +
> +error_put_device:
> +	put_device(&pdev->dev);
> +
> +	return ret;
> +}
> +
>  static int ucc_hdlc_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -1151,6 +1199,15 @@ static int ucc_hdlc_probe(struct  
> platform_device *pdev)
>  		ret = ucc_of_parse_tdm(np, utdm, ut_info);
>  		if (ret)
>  			goto free_utdm;
> +
> +		ret = hdlc_map_iomem("fsl,t1040-qe-si", 0,
> +				     (void __iomem **)&utdm->si_regs);
> +		if (ret)
> +			goto free_utdm;
> +		ret = hdlc_map_iomem("fsl,t1040-qe-siram", 1,
> +				     (void __iomem **)&utdm->siram);
> +		if (ret)
> +			goto unmap_si_regs;
>  	}
>
>  	if (of_property_read_u16(np, "fsl,hmask", &uhdlc_priv->hmask))
> @@ -1159,7 +1216,7 @@ static int ucc_hdlc_probe(struct platform_device *pdev)
>  	ret = uhdlc_init(uhdlc_priv);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to init uhdlc\n");
> -		goto free_utdm;
> +		goto undo_uhdlc_init;
>  	}
>
>  	dev = alloc_hdlcdev(uhdlc_priv);
> @@ -1188,6 +1245,9 @@ static int ucc_hdlc_probe(struct platform_device *pdev)
>  free_dev:
>  	free_netdev(dev);
>  undo_uhdlc_init:
> +	iounmap(utdm->siram);
> +unmap_si_regs:
> +	iounmap(utdm->si_regs);
>  free_utdm:
>  	if (uhdlc_priv->tsa)
>  		kfree(utdm);
> diff --git a/drivers/soc/fsl/qe/qe_tdm.c b/drivers/soc/fsl/qe/qe_tdm.c
> index f78c346..76480df 100644
> --- a/drivers/soc/fsl/qe/qe_tdm.c
> +++ b/drivers/soc/fsl/qe/qe_tdm.c
> @@ -44,10 +44,6 @@ int ucc_of_parse_tdm(struct device_node *np,  
> struct ucc_tdm *utdm,
>  	const char *sprop;
>  	int ret = 0;
>  	u32 val;
> -	struct resource *res;
> -	struct device_node *np2;
> -	static int siram_init_flag;
> -	struct platform_device *pdev;
>
>  	sprop = of_get_property(np, "fsl,rx-sync-clock", NULL);
>  	if (sprop) {
> @@ -124,57 +120,6 @@ int ucc_of_parse_tdm(struct device_node *np,  
> struct ucc_tdm *utdm,
>  	utdm->siram_entry_id = val;
>
>  	set_si_param(utdm, ut_info);
> -
> -	np2 = of_find_compatible_node(NULL, NULL, "fsl,t1040-qe-si");
> -	if (!np2)
> -		return -EINVAL;
> -
> -	pdev = of_find_device_by_node(np2);
> -	if (!pdev) {
> -		pr_err("%pOFn: failed to lookup pdev\n", np2);
> -		of_node_put(np2);
> -		return -EINVAL;
> -	}
> -
> -	of_node_put(np2);
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	utdm->si_regs = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(utdm->si_regs)) {
> -		ret = PTR_ERR(utdm->si_regs);
> -		goto err_miss_siram_property;
> -	}
> -
> -	np2 = of_find_compatible_node(NULL, NULL, "fsl,t1040-qe-siram");
> -	if (!np2) {
> -		ret = -EINVAL;
> -		goto err_miss_siram_property;
> -	}
> -
> -	pdev = of_find_device_by_node(np2);
> -	if (!pdev) {
> -		ret = -EINVAL;
> -		pr_err("%pOFn: failed to lookup pdev\n", np2);
> -		of_node_put(np2);
> -		goto err_miss_siram_property;
> -	}
> -
> -	of_node_put(np2);
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	utdm->siram = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(utdm->siram)) {
> -		ret = PTR_ERR(utdm->siram);
> -		goto err_miss_siram_property;
> -	}
> -
> -	if (siram_init_flag == 0) {
> -		memset_io(utdm->siram, 0,  resource_size(res));
> -		siram_init_flag = 1;
> -	}
> -
> -	return ret;
> -
> -err_miss_siram_property:
> -	devm_iounmap(&pdev->dev, utdm->si_regs);
>  	return ret;
>  }
>  EXPORT_SYMBOL(ucc_of_parse_tdm);
> --
> 2.9.5

Patch
diff mbox series

diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 839fa77..f30a040 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -1057,6 +1057,54 @@  static const struct net_device_ops uhdlc_ops = {
 	.ndo_tx_timeout	= uhdlc_tx_timeout,
 };
 
+static int hdlc_map_iomem(char *name, int init_flag, void __iomem **ptr)
+{
+	struct device_node *np;
+	struct platform_device *pdev;
+	struct resource *res;
+	static int siram_init_flag;
+	int ret = 0;
+
+	np = of_find_compatible_node(NULL, NULL, name);
+	if (!np)
+		return -EINVAL;
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev) {
+		pr_err("%pOFn: failed to lookup pdev\n", np);
+		of_node_put(np);
+		return -EINVAL;
+	}
+
+	of_node_put(np);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		ret = -EINVAL;
+		goto error_put_device;
+	}
+	*ptr = ioremap(res->start, resource_size(res));
+	if (!*ptr) {
+		ret = -ENOMEM;
+		goto error_put_device;
+	}
+
+	/* We've remapped the addresses, and we don't need the device any
+	 * more, so we should release it.
+	 */
+	put_device(&pdev->dev);
+
+	if (init_flag && siram_init_flag == 0) {
+		memset_io(*ptr, 0, resource_size(res));
+		siram_init_flag = 1;
+	}
+	return  0;
+
+error_put_device:
+	put_device(&pdev->dev);
+
+	return ret;
+}
+
 static int ucc_hdlc_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -1151,6 +1199,15 @@  static int ucc_hdlc_probe(struct platform_device *pdev)
 		ret = ucc_of_parse_tdm(np, utdm, ut_info);
 		if (ret)
 			goto free_utdm;
+
+		ret = hdlc_map_iomem("fsl,t1040-qe-si", 0,
+				     (void __iomem **)&utdm->si_regs);
+		if (ret)
+			goto free_utdm;
+		ret = hdlc_map_iomem("fsl,t1040-qe-siram", 1,
+				     (void __iomem **)&utdm->siram);
+		if (ret)
+			goto unmap_si_regs;
 	}
 
 	if (of_property_read_u16(np, "fsl,hmask", &uhdlc_priv->hmask))
@@ -1159,7 +1216,7 @@  static int ucc_hdlc_probe(struct platform_device *pdev)
 	ret = uhdlc_init(uhdlc_priv);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to init uhdlc\n");
-		goto free_utdm;
+		goto undo_uhdlc_init;
 	}
 
 	dev = alloc_hdlcdev(uhdlc_priv);
@@ -1188,6 +1245,9 @@  static int ucc_hdlc_probe(struct platform_device *pdev)
 free_dev:
 	free_netdev(dev);
 undo_uhdlc_init:
+	iounmap(utdm->siram);
+unmap_si_regs:
+	iounmap(utdm->si_regs);
 free_utdm:
 	if (uhdlc_priv->tsa)
 		kfree(utdm);
diff --git a/drivers/soc/fsl/qe/qe_tdm.c b/drivers/soc/fsl/qe/qe_tdm.c
index f78c346..76480df 100644
--- a/drivers/soc/fsl/qe/qe_tdm.c
+++ b/drivers/soc/fsl/qe/qe_tdm.c
@@ -44,10 +44,6 @@  int ucc_of_parse_tdm(struct device_node *np, struct ucc_tdm *utdm,
 	const char *sprop;
 	int ret = 0;
 	u32 val;
-	struct resource *res;
-	struct device_node *np2;
-	static int siram_init_flag;
-	struct platform_device *pdev;
 
 	sprop = of_get_property(np, "fsl,rx-sync-clock", NULL);
 	if (sprop) {
@@ -124,57 +120,6 @@  int ucc_of_parse_tdm(struct device_node *np, struct ucc_tdm *utdm,
 	utdm->siram_entry_id = val;
 
 	set_si_param(utdm, ut_info);
-
-	np2 = of_find_compatible_node(NULL, NULL, "fsl,t1040-qe-si");
-	if (!np2)
-		return -EINVAL;
-
-	pdev = of_find_device_by_node(np2);
-	if (!pdev) {
-		pr_err("%pOFn: failed to lookup pdev\n", np2);
-		of_node_put(np2);
-		return -EINVAL;
-	}
-
-	of_node_put(np2);
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	utdm->si_regs = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(utdm->si_regs)) {
-		ret = PTR_ERR(utdm->si_regs);
-		goto err_miss_siram_property;
-	}
-
-	np2 = of_find_compatible_node(NULL, NULL, "fsl,t1040-qe-siram");
-	if (!np2) {
-		ret = -EINVAL;
-		goto err_miss_siram_property;
-	}
-
-	pdev = of_find_device_by_node(np2);
-	if (!pdev) {
-		ret = -EINVAL;
-		pr_err("%pOFn: failed to lookup pdev\n", np2);
-		of_node_put(np2);
-		goto err_miss_siram_property;
-	}
-
-	of_node_put(np2);
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	utdm->siram = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(utdm->siram)) {
-		ret = PTR_ERR(utdm->siram);
-		goto err_miss_siram_property;
-	}
-
-	if (siram_init_flag == 0) {
-		memset_io(utdm->siram, 0,  resource_size(res));
-		siram_init_flag = 1;
-	}
-
-	return ret;
-
-err_miss_siram_property:
-	devm_iounmap(&pdev->dev, utdm->si_regs);
 	return ret;
 }
 EXPORT_SYMBOL(ucc_of_parse_tdm);