[PATCH -next] net: hns: fix return value check in hns_dsaf_get_cfg()

From: Wei Yongjun <redacted>

In case of error, function devm_ioremap_resource() returns ERR_PTR()
and never returns NULL. The NULL test in the return value check should
be replaced with IS_ERR().
Also removed unneeded error handling on the result of call to
platform_get_resource() and redundant dev_err call.

Signed-off-by: Wei Yongjun <redacted>
---
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 49 +++++-----------------
 1 file changed, 10 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
index 86ce28a..b09d56a 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c

@@ -107,68 +107,39 @@ int hns_dsaf_get_cfg(struct dsaf_device *dsaf_dev)
 		if (IS_ERR_OR_NULL(syscon)) {
 			res = platform_get_resource(pdev, IORESOURCE_MEM,
 						    res_idx++);
-			if (!res) {
-				dev_err(dsaf_dev->dev, "subctrl info is needed!\n");
-				return -ENOMEM;
-			}
-
 			dsaf_dev->sc_base = devm_ioremap_resource(&pdev->dev,
 								  res);
-			if (!dsaf_dev->sc_base) {
-				dev_err(dsaf_dev->dev, "subctrl can not map!\n");
-				return -ENOMEM;
-			}
+			if (IS_ERR(dsaf_dev->sc_base))
+				return PTR_ERR(dsaf_dev->sc_base);
 
 			res = platform_get_resource(pdev, IORESOURCE_MEM,
 						    res_idx++);
-			if (!res) {
-				dev_err(dsaf_dev->dev, "serdes-ctrl info is needed!\n");
-				return -ENOMEM;
-			}
-
 			dsaf_dev->sds_base = devm_ioremap_resource(&pdev->dev,
 								   res);
-			if (!dsaf_dev->sds_base) {
-				dev_err(dsaf_dev->dev, "serdes-ctrl can not map!\n");
-				return -ENOMEM;
-			}
+			if (IS_ERR(dsaf_dev->sds_base))
+				return PTR_ERR(dsaf_dev->sds_base);
 		} else {
 			dsaf_dev->sub_ctrl = syscon;
 		}
 	}
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ppe-base");
-	if (!res) {
+	if (!res)
 		res = platform_get_resource(pdev, IORESOURCE_MEM, res_idx++);
-		if (!res) {
-			dev_err(dsaf_dev->dev, "ppe-base info is needed!\n");
-			return -ENOMEM;
-		}
-	}
 	dsaf_dev->ppe_base = devm_ioremap_resource(&pdev->dev, res);
-	if (!dsaf_dev->ppe_base) {
-		dev_err(dsaf_dev->dev, "ppe-base resource can not map!\n");
-		return -ENOMEM;
-	}
+	if (IS_ERR(dsaf_dev->ppe_base))
+		return PTR_ERR(dsaf_dev->ppe_base);
 	dsaf_dev->ppe_paddr = res->start;
 
 	if (!HNS_DSAF_IS_DEBUG(dsaf_dev)) {
 		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 						   "dsaf-base");
-		if (!res) {
+		if (!res)
 			res = platform_get_resource(pdev, IORESOURCE_MEM,
 						    res_idx);
-			if (!res) {
-				dev_err(dsaf_dev->dev,
-					"dsaf-base info is needed!\n");
-				return -ENOMEM;
-			}
-		}
 		dsaf_dev->io_base = devm_ioremap_resource(&pdev->dev, res);
-		if (!dsaf_dev->io_base) {
-			dev_err(dsaf_dev->dev, "dsaf-base resource can not map!\n");
-			return -ENOMEM;
-		}
+		if (IS_ERR(dsaf_dev->io_base))
+			return PTR_ERR(dsaf_dev->io_base);
 	}
 
 	ret = device_property_read_u32(dsaf_dev->dev, "desc-num", &desc_num);

From: weiyj_lk@163.com
Date: Mon,  4 Jul 2016 15:00:58 +0000

 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ppe-base");
-	if (!res) {
+	if (!res)
 		res = platform_get_resource(pdev, IORESOURCE_MEM, res_idx++);
-		if (!res) {
-			dev_err(dsaf_dev->dev, "ppe-base info is needed!\n");
-			return -ENOMEM;
-		}
-	}

platform_get_resource() can return NULL, you can't just remove this check.

This is also a case of a patch trying to do many things at once, and that's
why bugs like this tend to slip in.

From: Wei Yongjun <redacted>

In case of error, function devm_ioremap_resource() returns ERR_PTR()
and never returns NULL. The NULL test in the return value check should
be replaced with IS_ERR().

Signed-off-by: Wei Yongjun <redacted>
---
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
index 86ce28a..2ef4277 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c

@@ -114,9 +114,9 @@ int hns_dsaf_get_cfg(struct dsaf_device *dsaf_dev)
 
 			dsaf_dev->sc_base = devm_ioremap_resource(&pdev->dev,
 								  res);
-			if (!dsaf_dev->sc_base) {
+			if (IS_ERR(dsaf_dev->sc_base)) {
 				dev_err(dsaf_dev->dev, "subctrl can not map!\n");
-				return -ENOMEM;
+				return PTR_ERR(dsaf_dev->sc_base);
 			}
 
 			res = platform_get_resource(pdev, IORESOURCE_MEM,

@@ -128,9 +128,9 @@ int hns_dsaf_get_cfg(struct dsaf_device *dsaf_dev)
 
 			dsaf_dev->sds_base = devm_ioremap_resource(&pdev->dev,
 								   res);
-			if (!dsaf_dev->sds_base) {
+			if (IS_ERR(dsaf_dev->sds_base)) {
 				dev_err(dsaf_dev->dev, "serdes-ctrl can not map!\n");
-				return -ENOMEM;
+				return PTR_ERR(dsaf_dev->sds_base);
 			}
 		} else {
 			dsaf_dev->sub_ctrl = syscon;

@@ -146,9 +146,9 @@ int hns_dsaf_get_cfg(struct dsaf_device *dsaf_dev)
 		}
 	}
 	dsaf_dev->ppe_base = devm_ioremap_resource(&pdev->dev, res);
-	if (!dsaf_dev->ppe_base) {
+	if (IS_ERR(dsaf_dev->ppe_base)) {
 		dev_err(dsaf_dev->dev, "ppe-base resource can not map!\n");
-		return -ENOMEM;
+		return PTR_ERR(dsaf_dev->ppe_base);
 	}
 	dsaf_dev->ppe_paddr = res->start;
 

@@ -165,9 +165,9 @@ int hns_dsaf_get_cfg(struct dsaf_device *dsaf_dev)
 			}
 		}
 		dsaf_dev->io_base = devm_ioremap_resource(&pdev->dev, res);
-		if (!dsaf_dev->io_base) {
+		if (IS_ERR(dsaf_dev->io_base)) {
 			dev_err(dsaf_dev->dev, "dsaf-base resource can not map!\n");
-			return -ENOMEM;
+			return PTR_ERR(dsaf_dev->io_base);
 		}
 	}

Hello.

On 7/5/2016 10:08 AM, David Miller wrote:

quoted

 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ppe-base");
-	if (!res) {
+	if (!res)
 		res = platform_get_resource(pdev, IORESOURCE_MEM, res_idx++);
-		if (!res) {
-			dev_err(dsaf_dev->dev, "ppe-base info is needed!\n");
-			return -ENOMEM;
-		}
-	}

platform_get_resource() can return NULL, you can't just remove this check.

    He can -- since devm_ioremap_resource() called right afterwards checks for 
NULL.

This is also a case of a patch trying to do many things at once, and that's
why bugs like this tend to slip in.

    Agreed.

MBR, Sergei

From: weiyj_lk@163.com
Date: Tue,  5 Jul 2016 07:56:52 +0000

From: Wei Yongjun <redacted>

In case of error, function devm_ioremap_resource() returns ERR_PTR()
and never returns NULL. The NULL test in the return value check should
be replaced with IS_ERR().

Signed-off-by: Wei Yongjun <redacted>

Applied, thanks.

Hello,
This patch will conflict with Doug Ledford's hns-roce's HNS driver.
This might lead to problems later during this merge window of 4.9.

Therefore, Please re-submit it later. The patch files it has are
Directly conflicting with RoCE patches:

[PATCH for-next 1/2] net: hns: Add support of ACPI to HNS driver RoCE Reset function 
drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.h
drivers/net/ethernet/hisilicon/hns/hns_dsaf_misc.c
drivers/net/ethernet/hisilicon/hns/hns_dsaf_misc.h
drivers/net/ethernet/hisilicon/hns/hns_dsaf_reg.h

we are in process of streamlining the internal process for the better
synchronization between HNS and RoCE/RDMA teams and would maintain
internal repo for such conflicting patches and which can be
git-pulled by David Miller and Doug Ledford.

Best regards
Salil

From: Salil Mehta <redacted>
Date: Mon, 5 Sep 2016 14:20:33 +0000

This patch will conflict with Doug Ledford's hns-roce's HNS driver.
This might lead to problems later during this merge window of 4.9.

You don't need to say this three times.

These changes will not be reverted, instead the conflicts will need
to simply be resolved during the merges just like any other conflict
that ever happens in our trees.

-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Monday, September 05, 2016 8:43 PM
To: Salil Mehta
Cc: weiyj_lk@163.com; Zhuangyuzeng (Yisen); Yankejian (Hackim Yim);
yongjun_wei@trendmicro.com.cn; netdev@vger.kernel.org
Subject: Re: [PATCH -next v2] net: hns: fix return value check in
hns_dsaf_get_cfg()

From: Salil Mehta <redacted>
Date: Mon, 5 Sep 2016 14:20:33 +0000

quoted

This patch will conflict with Doug Ledford's hns-roce's HNS driver.
This might lead to problems later during this merge window of 4.9.

You don't need to say this three times.

Sorry about this

These changes will not be reverted, instead the conflicts will need
to simply be resolved during the merges just like any other conflict
that ever happens in our trees.

Fine, got your point.