Re: [RESEND PATCH v5] soc/fsl/qe: fix err handling of ucc_of_parse_tdm

2019-01-04 Thread David Miller
From: Peng Hao 
Date: Thu, 3 Jan 2019 01:09:53 +0800

> From: Wen Yang 
> 
> 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 
> Suggested-by: Christophe LEROY 
> Signed-off-by: Wen Yang 
> Reviewed-by: Peng Hao 

Applied, thanks.


[RESEND PATCH v5] soc/fsl/qe: fix err handling of ucc_of_parse_tdm

2019-01-02 Thread Peng Hao
From: Wen Yang 

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 
Suggested-by: Christophe LEROY 
Signed-off-by: Wen Yang 
Reviewed-by: Peng Hao 
CC: Julia Lawall 
CC: Zhao Qiang 
CC: David S. Miller 
CC: net...@vger.kernel.org
CC: linuxppc-dev@lists.ozlabs.org
CC: linux-ker...@vger.kernel.org
---
v5->v4:
- "of_find_device_by_node()" takes a reference to the underlying device 
structure,
   we should release it.

v4->v3:
- This assignment to 'res' doesn't do what you think it does. The caller never
  sees the value you compute.

v3->v2:
- 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.

v2->v1:
- 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.
  
 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(