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

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

[weiyj_lk]

From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>

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 <yongjun_wei@trendmicro.com.cn>
---
 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);

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

[David Miller]

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.

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

[weiyj_lk]

From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>

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 <yongjun_wei@trendmicro.com.cn>
---
 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);
 		}
 	}

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

[Sergei Shtylyov]

Hello.

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

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

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

[David Miller]

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

> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> 
> 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 <yongjun_wei@trendmicro.com.cn>

Applied, thanks.

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

[Salil Mehta]

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
> -----Original Message-----
> From: weiyj_lk@163.com [mailto:weiyj_lk@163.com]
> Sent: Tuesday, July 05, 2016 8:57 AM
> To: Zhuangyuzeng (Yisen); Salil Mehta; Yankejian (Hackim Yim)
> Cc: Wei Yongjun; netdev@vger.kernel.org
> Subject: [PATCH -next v2] net: hns: fix return value check in
> hns_dsaf_get_cfg()
> 
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> 
> 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 <yongjun_wei@trendmicro.com.cn>
> ---
>  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);
>  		}
>  	}
> 

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

[David Miller]

From: Salil Mehta <salil.mehta@huawei.com>
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.

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

[Salil Mehta]

> -----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 <salil.mehta@huawei.com>
> 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.
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.